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

[Namhyung Kim <namhyung@kernel.org>]

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.

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

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

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

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).

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).

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