| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAM9d7cgDCd2uUJbWcvqmCDGMoPc9kppx--_rcO2OVp_GarLJkg@mail.gmail.com>
Date: Thu, 12 May 2022 11:53:25 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Adrian Hunter <adrian.hunter@...el.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
Jiri Olsa <jolsa@...hat.com>, Ian Rogers <irogers@...gle.com>,
Alexey Bayduraev <alexey.v.bayduraev@...ux.intel.com>,
Leo Yan <leo.yan@...aro.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
Stephane Eranian <eranian@...gle.com>,
Kan Liang <kan.liang@...ux.intel.com>
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@...el.com> wrote:
>
> On 12/05/22 08:27, Namhyung Kim wrote:
> > On Fri, May 6, 2022 at 5:27 AM Adrian Hunter <adrian.hunter@...el.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@...gle.com>
> >> Signed-off-by: Adrian Hunter <adrian.hunter@...el.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
Powered by blists - more mailing lists