Messages in this thread | | | From | Namhyung Kim <> | Date | Thu, 12 May 2022 11:53:25 -0700 | Subject | Re: [PATCH V2 22/23] perf tools: Allow system-wide events to keep their own CPUs |
| |
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
| |