| 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: <Yn+wJlzymeAaHHcI@kernel.org>
Date: Sat, 14 May 2022 10:35:34 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Adrian Hunter <adrian.hunter@...el.com>
Cc: Namhyung Kim <namhyung@...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
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@...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.
>
> 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
Powered by blists - more mailing lists