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

[Arnaldo Carvalho de Melo]

Em Fri, May 13, 2022 at 07:48:40AM +0300, Adrian Hunter escreveu:
> On 12/05/22 21:53, Namhyung Kim wrote:
> > On Thu, May 12, 2022 at 3:35 AM Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
> >>
> >> On 12/05/22 08:27, Namhyung Kim wrote:
> >>> On Fri, May 6, 2022 at 5:27 AM Adrian Hunter <adrian.hunter@xxxxxxxxx> 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@xxxxxxxxxx>
> >>>> Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
> >>>> ---
> >>>>  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.

Good thing about this conversation is that it will result in
documentation :-)

Thank you guys for having it and detailing it so nicely.

- Arnaldo
 
> > 
> >>
> >>>
> >>> 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