Messages in this thread | | | Date | Thu, 12 May 2022 13:34:47 +0300 | Subject | Re: [PATCH V2 22/23] perf tools: Allow system-wide events to keep their own CPUs | From | Adrian Hunter <> |
| |
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?
> > 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.
> > Thanks, > Namhyung > > >> perf_cpu_map__put(evsel->cpus); >> evsel->cpus = perf_cpu_map__get(evlist->user_requested_cpus); >> } else if (evsel->cpus != evsel->own_cpus) { >> -- >> 2.25.1 >>
| |