Re: [PATCH V2 13/23] perf evlist: Add evlist__add_dummy_on_all_cpus()

[Adrian Hunter <adrian.hunter@intel.com>]

On 6/05/22 16:47, Ian Rogers wrote:
> On Fri, May 6, 2022 at 5:26 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> Add evlist__add_dummy_on_all_cpus() to enable creating a system-wide dummy
>> event that sets up the system-wide maps before map propagation.
>>
>> For convenience, add evlist__add_aux_dummy() so that the logic can be used
>> whether or not the event needs to be system-wide.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>  tools/perf/util/evlist.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>  tools/perf/util/evlist.h |  5 +++++
>>  2 files changed, 45 insertions(+)
>>
>> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
>> index 78c47cbafbc2..c16bd4836314 100644
>> --- a/tools/perf/util/evlist.c
>> +++ b/tools/perf/util/evlist.c
>> @@ -264,6 +264,46 @@ int evlist__add_dummy(struct evlist *evlist)
>>         return 0;
>>  }
>>
>> +static void evlist__add_on_all_cpus(struct evlist *evlist, struct evsel *evsel)
>> +{
>> +       evsel->core.system_wide = true;
>> +
>> +       /* All CPUs */
>> +       perf_cpu_map__put(evsel->core.own_cpus);
>> +       evsel->core.own_cpus = perf_cpu_map__new(NULL);
>> +       perf_cpu_map__put(evsel->core.cpus);
>> +       evsel->core.cpus = perf_cpu_map__get(evsel->core.own_cpus);
>> +
>> +       /* No threads */
>> +       perf_thread_map__put(evsel->core.threads);
>> +       evsel->core.threads = perf_thread_map__new_dummy();
>> +
>> +       evlist__add(evlist, evsel);
>> +}
>> +
>> +struct evsel *evlist__add_aux_dummy(struct evlist *evlist, bool system_wide)
>> +{
>> +       struct evsel *evsel = evlist__dummy_event(evlist);
>> +
>> +       if (!evsel)
>> +               return NULL;
>> +
>> +       evsel->core.attr.exclude_kernel = 1;
>> +       evsel->core.attr.exclude_guest = 1;
>> +       evsel->core.attr.exclude_hv = 1;
>> +       evsel->core.attr.freq = 0;
>> +       evsel->core.attr.sample_period = 1;
>> +       evsel->no_aux_samples = true;
>> +       evsel->name = strdup("dummy:u");
>> +
>> +       if (system_wide)
>> +               evlist__add_on_all_cpus(evlist, evsel);
>> +       else
>> +               evlist__add(evlist, evsel);
>> +
>> +       return evsel;
>> +}
>> +
>>  static int evlist__add_attrs(struct evlist *evlist, struct perf_event_attr *attrs, size_t nr_attrs)
>>  {
>>         struct evsel *evsel, *n;
>> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
>> index 4062f5aebfc1..1bde9ccf4e7d 100644
>> --- a/tools/perf/util/evlist.h
>> +++ b/tools/perf/util/evlist.h
>> @@ -114,6 +114,11 @@ int arch_evlist__add_default_attrs(struct evlist *evlist);
>>  struct evsel *arch_evlist__leader(struct list_head *list);
>>
>>  int evlist__add_dummy(struct evlist *evlist);
>> +struct evsel *evlist__add_aux_dummy(struct evlist *evlist, bool system_wide);
>> +static inline struct evsel *evlist__add_dummy_on_all_cpus(struct evlist *evlist)
> 
> Sorry to be a language lawyer. What I hope to clean up with CPU maps is that:
> 
> empty == dummy == any CPU
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/lib/perf/cpumap.c?h=perf/core#n279
> 
> Given every CPU map should be empty or contain any CPU then it seems
> they all meet the definition of empty - so something is wrong.

Nothing is wrong.  I am not against clarifying things, but stop assuming
natural language has to mean anything exactly.  That is what computer
languages are for.

Sometimes more abstract language is used, precisely to stop people
making assumptions about the details.

> 
> The cpu map here is explicitly opened so that it gets all online CPUs:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/lib/perf/cpumap.c?h=perf/core#n174
> 
> From:
> https://github.com/torvalds/linux/blob/master/Documentation/admin-guide/cputopology.rst
> there are example topologies like:
> kernel_max: 31
>    offline: 2,4-31,32-63
>     online: 0-1,3
>   possible: 0-31
>    present: 0-31
> 
> all_cpus could mean the union of offline and online CPUs, possible
> CPUs or present CPUs. You are saying that in the perf code all_cpus
> should be the same as all online cpus as only those CPUs are valid
> with perf_event_open. That's true but offline CPUs can be made online.
> If that happens here then the dummy events will have a CPU map that
> rather than being for all CPUs will be for all online CPUs at the
> point it was opened. Having online in the function name I think
> captures the time dependent nature of this - but if you think that's
> too much could we add a comment?

If you ask me it does the exact opposite.  The function of the code
is to put the event on all CPUS without having to know the details
of: well actually perf doesn't automagically retain or restore events
across enabling or disabling CPUs so in fact we deal only in online
CPUs.

> too much could we add a comment? I'm trying to avoid a situation, like
> with the CPU map code, where all and online are interchangeable
> leading to the code being unnecessarily confusing unless you read
> every line.

It is normal to have to read the details of code, and, in my
experience at least, normal for the code not to work exactly the
way I'd imagined.

> 
> Thanks,
> Ian
> 
>> +{
>> +       return evlist__add_aux_dummy(evlist, true);
>> +}
>>
>>  int evlist__add_sb_event(struct evlist *evlist, struct perf_event_attr *attr,
>>                          evsel__sb_cb_t cb, void *data);
>> --
>> 2.25.1
>>