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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fc8e6ede-af8b-4777-a003-585886eff897@linux.intel.com>
Date: Thu, 3 Apr 2025 15:25:50 -0400
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Arnaldo Carvalho de Melo <acme@...nel.org>,
 Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>,
 Leo Yan <leo.yan@....com>, Yoshihiro Furudera <fj5100bi@...itsu.com>,
 Weilin Wang <weilin.wang@...el.com>, Andi Kleen <ak@...ux.intel.com>,
 James Clark <james.clark@...aro.org>,
 Dominique Martinet <asmadeus@...ewreck.org>,
 Yicong Yang <yangyicong@...ilicon.com>, linux-perf-users@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/4] perf parse-events: Set is_pmu_core for legacy
 hardware events



On 2025-04-03 11:44 a.m., Ian Rogers wrote:
> On Thu, Apr 3, 2025 at 8:17 AM Liang, Kan <kan.liang@...ux.intel.com> wrote:
>>
>>
>>
>> On 2025-02-10 1:38 p.m., Ian Rogers wrote:
>>> Also set the CPU map to all online CPU maps. This is done so the
>>> behavior of legacy hardware and hardware cache events better matches
>>> that of sysfs and json events during
>>> __perf_evlist__propagate_maps. Fix missing cpumap put in "Synthesize
>>> attr update" test.
>>>
>>> Signed-off-by: Ian Rogers <irogers@...gle.com>
>>> ---
>>>  tools/perf/tests/event_update.c |  1 +
>>>  tools/perf/util/parse-events.c  | 37 ++++++++++++++++++++-------------
>>>  2 files changed, 24 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/tools/perf/tests/event_update.c b/tools/perf/tests/event_update.c
>>> index d6b4ce3ef4ee..9301fde11366 100644
>>> --- a/tools/perf/tests/event_update.c
>>> +++ b/tools/perf/tests/event_update.c
>>> @@ -109,6 +109,7 @@ static int test__event_update(struct test_suite *test __maybe_unused, int subtes
>>>       TEST_ASSERT_VAL("failed to synthesize attr update name",
>>>                       !perf_event__synthesize_event_update_name(&tmp.tool, evsel, process_event_name));
>>>
>>> +     perf_cpu_map__put(evsel->core.own_cpus);
>>>       evsel->core.own_cpus = perf_cpu_map__new("1,2,3");
>>>
>>>       TEST_ASSERT_VAL("failed to synthesize attr update cpus",


The above change should have been merged.
2c744f38da7a ("perf test: Fix leak in "Synthesize attr update" test").

It makes the patch failed to be applied to perf-tools-next.


>>> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
>>> index 6c36b98875bc..8cccf1e22cdf 100644
>>> --- a/tools/perf/util/parse-events.c
>>> +++ b/tools/perf/util/parse-events.c
>>> @@ -231,21 +231,30 @@ __add_event(struct list_head *list, int *idx,
>>>           struct perf_cpu_map *cpu_list, u64 alternate_hw_config)
>>>  {
>>>       struct evsel *evsel;
>>> -     struct perf_cpu_map *cpus = perf_cpu_map__is_empty(cpu_list) && pmu ? pmu->cpus : cpu_list;
>>> +     bool is_pmu_core;
>>> +     struct perf_cpu_map *cpus;
>>>
>>> -     cpus = perf_cpu_map__get(cpus);
>>> -     if (pmu)
>>> +     if (pmu) {
>>> +             is_pmu_core = pmu->is_core;
>>> +             cpus = perf_cpu_map__get(perf_cpu_map__is_empty(cpu_list) ? pmu->cpus : cpu_list);
>>>               perf_pmu__warn_invalid_formats(pmu);
>>> -
>>> -     if (pmu && (attr->type == PERF_TYPE_RAW || attr->type >= PERF_TYPE_MAX)) {
>>> -             perf_pmu__warn_invalid_config(pmu, attr->config, name,
>>> -                                           PERF_PMU_FORMAT_VALUE_CONFIG, "config");
>>> -             perf_pmu__warn_invalid_config(pmu, attr->config1, name,
>>> -                                           PERF_PMU_FORMAT_VALUE_CONFIG1, "config1");
>>> -             perf_pmu__warn_invalid_config(pmu, attr->config2, name,
>>> -                                           PERF_PMU_FORMAT_VALUE_CONFIG2, "config2");
>>> -             perf_pmu__warn_invalid_config(pmu, attr->config3, name,
>>> -                                           PERF_PMU_FORMAT_VALUE_CONFIG3, "config3");
>>> +             if (attr->type == PERF_TYPE_RAW || attr->type >= PERF_TYPE_MAX) {
>>> +                     perf_pmu__warn_invalid_config(pmu, attr->config, name,
>>> +                                             PERF_PMU_FORMAT_VALUE_CONFIG, "config");
>>> +                     perf_pmu__warn_invalid_config(pmu, attr->config1, name,
>>> +                                             PERF_PMU_FORMAT_VALUE_CONFIG1, "config1");
>>> +                     perf_pmu__warn_invalid_config(pmu, attr->config2, name,
>>> +                                             PERF_PMU_FORMAT_VALUE_CONFIG2, "config2");
>>> +                     perf_pmu__warn_invalid_config(pmu, attr->config3, name,
>>> +                                             PERF_PMU_FORMAT_VALUE_CONFIG3, "config3");
>>> +             }
>>> +     } else {
>>> +             is_pmu_core = (attr->type == PERF_TYPE_HARDWARE ||
>>> +                            attr->type == PERF_TYPE_HW_CACHE);
>>> +             if (perf_cpu_map__is_empty(cpu_list))
>>> +                     cpus = is_pmu_core ? perf_cpu_map__new_online_cpus() : NULL;
>>
>> All online CPUs? Is there a problem for hybrid?
> 
> For hybrid if you do cpu_atom/cycles/ then the PMU is found by
> perf_pmus__find here:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/parse-events.c?h=perf-tools-next#n1651
>
> The wildcard case (e.g. perf stat -e cycles ...) scans all the PMUs>
passing the respective pmu into __add_event:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/parse-events.c?h=perf-tools-next#n1382

Yes, the pmu should always be available for hybrid case.

> 
> For non-hybrid the PMU is given by perf_pmus__find_by_type which only
> returns a PMU if there is an exact match on the type, ie it won't
> return perf_pmus__find_core_pmu  when asked to lookup the PMU for a
> legacy type, evsel__find_pmu will do this though.
> 
> Anyway, the assumption in this legacy configuration no PMU case is
> that the event is heading to all CPUs, which is true for software and
> tracepoint. For hybrid we could get into this kind of problem is sysfs
> isn't mounted, but I think we're pretty generally broken in that case
> anyway.
> 
> Fwiw, I'm carrying these changes in google's tree and test it on my
> hybrid desktop:
> https://github.com/googleprodkernel/linux-perf

OK. I will do some tests then.

Thanks,
Kan

> 
> Thanks,
> Ian
> 
>> Thanks,
>> Kan> +          else
>>> +                     cpus = perf_cpu_map__get(cpu_list);
>>>       }
>>>       if (init_attr)
>>>               event_attr_init(attr);
>>> @@ -260,7 +269,7 @@ __add_event(struct list_head *list, int *idx,
>>>       evsel->core.cpus = cpus;
>>>       evsel->core.own_cpus = perf_cpu_map__get(cpus);
>>>       evsel->core.requires_cpu = pmu ? pmu->is_uncore : false;
>>> -     evsel->core.is_pmu_core = pmu ? pmu->is_core : false;
>>> +     evsel->core.is_pmu_core = is_pmu_core;
>>>       evsel->auto_merge_stats = auto_merge_stats;
>>>       evsel->pmu = pmu;
>>>       evsel->alternate_hw_config = alternate_hw_config;
>>
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ