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
| ||
|
Date: Fri, 13 May 2022 09:42:51 -0700 From: Namhyung Kim <namhyung@...nel.org> To: Adrian Hunter <adrian.hunter@...el.com> Cc: "Liang, Kan" <kan.liang@...ux.intel.com>, 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> Subject: Re: [PATCH V2 22/23] perf tools: Allow system-wide events to keep their own CPUs On Fri, May 13, 2022 at 9:11 AM Adrian Hunter <adrian.hunter@...el.com> wrote: > > On 13/05/22 18:46, Liang, Kan wrote: > > > > > > On 5/13/2022 11:21 AM, Adrian Hunter wrote: > >> On 13/05/22 17:12, Liang, Kan wrote: > >>> > >>> > >>> On 5/13/2022 12:48 AM, Adrian Hunter wrote: > >>>> 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. > >>> > >>> Do we check whether the user's input is valid (match the PMU's cpumask) before the override? > >>> > >>> I think we know the PMU name. The cpumask of the PMU can be found in the sysfs. So we should have enough information for a sanity check. > >> > >> For the uncore PMU case, I am not sure what sanity is :-) > >> > > > > For a non-core PMU, e.g., uncore, cstate, power and etc. The cpumask is under the /sys/devices/<PMU>/cpumask. It shows the cpumask which kernel supports. If a end user request a different CPU other that the cpumask, I think it's better throw a waning. It should mitigate the confusion which Namhyung mentioned (uncore -C1,2). > > So you couldn't get uncore events unless you are also coincidentally wanting to trace CPU 0. > > I guess really the requrement is not to perf_event_open() an uncore PMU more than once? > To figure that out we'd need to be able map CPUs to uncore PMUs? We might just use evsel->own_cpus for uncore events and if the user-given cpu list contains other cpus it can show an warning. Thanks, Namhyung
Powered by blists - more mailing lists