Messages in this thread | | | Date | Thu, 12 May 2022 14:40:00 +0300 | Subject | Re: [PATCH V2 21/23] libperf evsel: Add comments for booleans | From | Adrian Hunter <> |
| |
On 12/05/22 08:34, Ian Rogers wrote: > On Wed, May 11, 2022 at 12:03 AM Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> On 6/05/22 23:51, Ian Rogers wrote: >>> On Fri, May 6, 2022 at 5:26 AM Adrian Hunter <adrian.hunter@intel.com> wrote: >>>> >>>> Add comments for 'system_wide' and 'requires_cpu' booleans >>>> >>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com> >>>> --- >>>> tools/lib/perf/include/internal/evsel.h | 9 +++++++++ >>>> 1 file changed, 9 insertions(+) >>>> >>>> diff --git a/tools/lib/perf/include/internal/evsel.h b/tools/lib/perf/include/internal/evsel.h >>>> index 77fbb8b97e5c..cc8f1ba790bd 100644 >>>> --- a/tools/lib/perf/include/internal/evsel.h >>>> +++ b/tools/lib/perf/include/internal/evsel.h >>>> @@ -49,7 +49,16 @@ struct perf_evsel { >>>> >>>> /* parse modifier helper */ >>>> int nr_members; >>>> + /* >>>> + * system_wide is for events that need to be on every CPU, irrespective >>>> + * of user requested CPUs or threads. Map propagation will not override >>>> + * this events own_cpus, which will contribute to evlist all_cpus. >>> >>> So this muddies my understanding of cpus and own_cpus (sigh, again >>> undocumented). I believe cpus to be the set of CPUs (or any CPU, aka >>> dummy) that perf_event_open happens on. all_cpus in evlist is the >>> union of all the evsels cpus (ie not related to own_cpus as described >>> here). Own_cpus I believe to be what's computed at parse-event time >>> from sysfs. Is this a typo in the comment or quite likely my error? >> >> A 'system_wide' event is not created by the parser, so own_cpus is set >> by the code adding the event. > > I think I may be misreading the comment. Perhaps it should read: > > system_wide is for events that need to be on every CPU, irrespective > of user requested CPUs or threads. Map propagation will not override > this event's own_cpus and own_cpus don't contribute to the evlist > all_cpus.
For a system_wide evsel, evsel->cpus are set to own_cpus and the evsel->cpus contribute to all_cpus.
> > It would be nice if there were a comment on own_cpus to explain its > relationship to cpus and more broadly when it is used. Fwiw, the parse > time copy is made here: > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/parse-events.c?h=perf/core#n367 > > I worry that if own_cpus should be used in place of cpus then > iterators like evlist__for_each_cpu may need to handle this, and so > they'd be broken currently.
No, evsel->own_cpus may be used to determine evsel->cpus, but that is where their influence stops.
> > Thanks, > Ian > >>> >>> Thanks, >>> Ian >>> >>>> + */ >>>> bool system_wide; >>>> + /* >>>> + * Some events, for example uncore events, require a CPU. >>>> + * i.e. it cannot be the 'any CPU' value of -1. >>>> + */ >>>> bool requires_cpu; >>>> int idx; >>>> }; >>>> -- >>>> 2.25.1 >>>> >>
| |