Re: [PATCH V2 22/23] perf tools: Allow system-wide events to keep their own CPUs

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

On 12/05/22 21:53, Namhyung Kim wrote:
> On Thu, May 12, 2022 at 3:35 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 12/05/22 08:27, Namhyung Kim wrote:
>>> On Fri, May 6, 2022 at 5:27 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>>
>>>> Currently, user_requested_cpus supplants system-wide CPUs when the evlist
>>>> has_user_cpus. Change that so that system-wide events retain their own
>>>> CPUs and they are added to all_cpus.
>>>>
>>>> Acked-by: Ian Rogers <irogers@google.com>
>>>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>>>> ---
>>>>  tools/lib/perf/evlist.c | 11 +++++------
>>>>  1 file changed, 5 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
>>>> index 1c801f8da44f..9a6801b53274 100644
>>>> --- a/tools/lib/perf/evlist.c
>>>> +++ b/tools/lib/perf/evlist.c
>>>> @@ -40,12 +40,11 @@ static void __perf_evlist__propagate_maps(struct perf_evlist *evlist,
>>>>          * We already have cpus for evsel (via PMU sysfs) so
>>>>          * keep it, if there's no target cpu list defined.
>>>>          */
>>>> -       if (!evsel->own_cpus || evlist->has_user_cpus) {
>>>> -               perf_cpu_map__put(evsel->cpus);
>>>> -               evsel->cpus = perf_cpu_map__get(evlist->user_requested_cpus);
>>>> -       } else if (!evsel->system_wide &&
>>>> -                  !evsel->requires_cpu &&
>>>> -                  perf_cpu_map__empty(evlist->user_requested_cpus)) {
>>>> +       if (!evsel->own_cpus ||
>>>> +           (!evsel->system_wide && evlist->has_user_cpus) ||
>>>> +           (!evsel->system_wide &&
>>>> +            !evsel->requires_cpu &&
>>>> +            perf_cpu_map__empty(evlist->user_requested_cpus))) {
>>>
>>> This is getting hard to understand.  IIUC this propagation basically
>>> sets user requested cpus to evsel unless it has its own cpus, right?
>>
>> I put the conditional logic altogether because that is kernel style but
>> it does make it practically unreadable.
>>
>> If we start with the original logic:
>>
>>         if (!evsel->own_cpus || evlist->has_user_cpus) {
>>                 perf_cpu_map__put(evsel->cpus);
>>                 evsel->cpus = perf_cpu_map__get(evlist->user_requested_cpus);
>>         } else if (!evsel->system_wide && perf_cpu_map__empty(evlist->user_requested_cpus)) {
>>                 perf_cpu_map__put(evsel->cpus);
>>                 evsel->cpus = perf_cpu_map__get(evlist->user_requested_cpus);
>>         } else if (evsel->cpus != evsel->own_cpus) {
>>                 perf_cpu_map__put(evsel->cpus);
>>                 evsel->cpus = perf_cpu_map__get(evsel->own_cpus);
>>         }
>>
>> Then make it more readable, i.e. same functionality
>>
>>         struct perf_cpu_map *cpus;
>>
>>         if (!evsel->own_cpus || evlist->has_user_cpus)
>>                 cpus = evlist->user_requested_cpus;
>>         else if (!evsel->system_wide && perf_cpu_map__empty(evlist->user_requested_cpus))
>>                 cpus = evlist->user_requested_cpus;
>>         else
>>                 cpus = evsel->own_cpus;
>>
>>         if (evsel->cpus != cpus) {
>>                 perf_cpu_map__put(evsel->cpus);
>>                 evsel->cpus = perf_cpu_map__get(cpus);
>>         }
>>
>> Then separate out the conditions, i.e. still same functionality
>>
>>         if (!evsel->own_cpus)
>>                 cpus = evlist->user_requested_cpus;
>>         else if (evlist->has_user_cpus)
>>                 cpus = evlist->user_requested_cpus;
>>         else if (evsel->system_wide)
>>                 cpus = evsel->own_cpus;
>>         else if (perf_cpu_map__empty(evlist->user_requested_cpus)) /* per-thread */
>>                 cpus = evlist->user_requested_cpus;
>>         else
>>                 cpus = evsel->own_cpus;
>>
>> Then add the new requires_cpu flag:
>>
>>         if (!evsel->own_cpus)
>>                 cpus = evlist->user_requested_cpus;
>>         else if (evlist->has_user_cpus)
>>                 cpus = evlist->user_requested_cpus;
>>         else if (evsel->system_wide)
>>                 cpus = evsel->own_cpus;
>> -       else if (perf_cpu_map__empty(evlist->user_requested_cpus)) /* per-thread */
>> +       else if (!evsel->requres_cpu && perf_cpu_map__empty(evlist->user_requested_cpus)) /* per-thread */
>>                 cpus = evlist->user_requested_cpus;
>>         else
>>                 cpus = evsel->own_cpus;
>>
>> Then make system_wide keep own_cpus even if has_user_cpus:
>>
>>         if (!evsel->own_cpus)
>>                 cpus = evlist->user_requested_cpus;
>> +       else if (evsel->system_wide)
>> +               cpus = evsel->own_cpus;
>>         else if (evlist->has_user_cpus)
>>                 cpus = evlist->user_requested_cpus;
>> -       else if (evsel->system_wide)
>> -               cpus = evsel->own_cpus;
>>         else if (!evsel->requres_cpu && perf_cpu_map__empty(evlist->user_requested_cpus)) /* per-thread */
>>                 cpus = evlist->user_requested_cpus;
>>         else
>>                 cpus = evsel->own_cpus;
>>
>> Which leaves:
>>
>>         if (!evsel->own_cpus)
>>                 cpus = evlist->user_requested_cpus;
>>         else if (evsel->system_wide)
>>                 cpus = evsel->own_cpus;
>>         else if (evlist->has_user_cpus)
>>                 cpus = evlist->user_requested_cpus;
>>         else if (!evsel->requres_cpu && perf_cpu_map__empty(evlist->user_requested_cpus)) /* per-thread */
>>                 cpus = evlist->user_requested_cpus;
>>         else
>>                 cpus = evsel->own_cpus;
>>
>> And putting it back together:
>>
>>         if (!evsel->own_cpus ||
>>             (!evsel->system_wide && evlist->has_user_cpus) ||
>>             (!evsel->system_wide &&
>>              !evsel->requires_cpu &&
>>              perf_cpu_map__empty(evlist->user_requested_cpus))) {
>>                 cpus = evlist->user_requested_cpus;
>>         else
>>                 cpus = evsel->own_cpus;
>>
>> Perhaps I shouldn't put it together?
> 
> Cool, thanks a lot for explaining it in detail.
> I do not oppose your change but little worried about the
> complexity.  And I think we have some issues with uncore
> events already.

Yes it is a bit complicated because we are handling
many different use cases.

> 
> So do you have any idea where evsel->own_cpus
> doesn't propagate to evsel->cpus?

We let the user's list of CPUs override it i.e. the
evlist->has_user_cpus case.  Essentially we are expecting
the user to know what they are doing.

> 
> I think evsel->system_wide and evsel->requires_cpu
> can be replaced to check evsel->own_cpus instead.

Not at the moment because we let the user override
own_cpus.

> 
> Actually evlist->has_user_cpus is checked first so
> uncore events' own_cpus might not be used.

Yes

> 
> In my laptop, perf stat -a -A -e imc/data_reads/
> will use cpu 0 as it's listed in the pmu cpumask.
> But when I use -C1,2 it'll use the both cpus and
> returns the similar values each (so the sum is 2x).

We expect the user to understand the uncore PMU they
are using.  AFAICT an uncore PMU cpu mask with only
CPU 0 typically means a single PMU that counts events
that could be indrectly caused by any CPU.  When the
cpu mask has more than one CPU, it means a PMU for
each of a group of CPU's (e.g. a core or socket)

So in the example you gave above, there is only 1 PMU
and reading from any CPU will give it's value.

A user providing a list of CPUs for uncore events
is useful only in certain cases.  For example when
each core has an uncore PMU and you only want to get
values from one core.

> 
> I'm not sure if it's intended.  I expect it runs on
> cpu 0 or one of the given cpus.  Or it runs on both
> cpus and returns value in half so that the sum is
> the same as the original value (from a cpu).

I don't know if there is anything wrong with the way
we are handling uncore PMUs, except that I don't know
if it is documented anywhere.

> 
>>
>>>
>>> But the hybrid pmus make this complex.  Maybe we can move the
>>> logic in evlist__fix_hybrid_cpus() here and simplify it like below
>>>
>>> if (evsel->own_cpus) {
>>>    if (evsel->pmu->is_hybrid)
>>>       evsel->cpus = fixup_hybrid_cpus(evsel>own_cpus,
>>>                                       evlist->user_requested_cpus);  //?
>>>    else
>>>       evsel->cpus = evlist->own_cpus;  // put + get
>>> } else {
>>>    evsel->cpus = evlist->user_requested_cpus;  // put + get
>>> }
>>>
>>> Then we need to make sure evsel->pmu is set properly.
>>>
>>> What do you think?
>>
>> Hybrid handling looks complicated.  I would have to spend time
>> better understanding it.
>>
>> So, in the context of this patch set, I don't want to look at
>> issues with hybrid CPUs, except that there should be no change
>> to how they are handled.
> 
> Fair enough.  But I think we have to look at it again soon.
> 
> Thanks,
> Namhyung