linux-kernel - Re: [PATCH 1/2] perf evlist: Keep topdown counters in weak group

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]

Date:   Thu, 5 May 2022 15:43:56 -0400
From:   "Liang, Kan" <kan.liang@...ux.intel.com>
To:     Ian Rogers <irogers@...gle.com>
Cc:     Caleb Biggers <caleb.biggers@...el.com>,
        Perry Taylor <perry.taylor@...el.com>,
        Kshipra Bopardikar <kshipra.bopardikar@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Ravi Bangoria <ravi.bangoria@....com>,
        Andi Kleen <ak@...ux.intel.com>,
        Haowen Bai <baihaowen@...zu.com>,
        Riccardo Mancini <rickyman7@...il.com>,
        Kim Phillips <kim.phillips@....com>,
        Madhavan Srinivasan <maddy@...ux.ibm.com>,
        Shunsuke Nakamura <nakamura.shun@...itsu.com>,
        Florian Fischer <florian.fischer@...q.space>,
        linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        Stephane Eranian <eranian@...gle.com>,
        Zhengjun Xing <zhengjun.xing@...ux.intel.com>
Subject: Re: [PATCH 1/2] perf evlist: Keep topdown counters in weak group



On 5/5/2022 2:31 PM, Ian Rogers wrote:
>>> So I think fixing all of these should be a follow up. I am working to
>>> get access to an Alderlake system, could we land this first?
>>>
>> I think we can use pmu_name to replace the "cpu" to fix the issue for
>> the hybrid platform. For a hybrid platform, the pmu_name is either
>> cpu_atom or cpu_core.
>>
>> Besides, the topdown events may have a PMU prefix, e.g.,
>> cpu_core/topdown-be-bound/. The strcasecmp may not work well for this case.
>>
>> How about the below patch?
>> If it's OK for you, could you please merge it into your V2 patch set?
>> I can do the test on a ADL system.
>>
>> diff --git a/tools/perf/arch/x86/util/evsel.c
>> b/tools/perf/arch/x86/util/evsel.c
>> index 40b171de2086..551ae2bab70e 100644
>> --- a/tools/perf/arch/x86/util/evsel.c
>> +++ b/tools/perf/arch/x86/util/evsel.c
>> @@ -33,11 +33,12 @@ void arch_evsel__fixup_new_cycles(struct
>> perf_event_attr *attr)
>>
>>    bool arch_evsel__must_be_in_group(const struct evsel *evsel)
>>    {
>> -       if ((evsel->pmu_name && strcmp(evsel->pmu_name, "cpu")) ||
>> -           !pmu_have_event("cpu", "slots"))
>> +       const char *pmu_name = evsel->pmu_name ? evsel->pmu_name : "cpu";
>> +
>> +       if (!pmu_have_event(pmu_name, "slots"))
>>                  return false;
> Hmm. The idea with this test is to see if the architecture supports
> topdown events before going further. There's a similar test in all the
> arch_evlist functions. I think with cpu_core this needs to become:
>

The case is a little bit different here. For the arch_evlist functions, 
the input is the evlist, not the specific evsel. So we have to check all 
the possible PMU names which are "cpu" and "cpu_core". Then we decide 
whether going further.

The input of the evsel__must_be_in_group() is the evsel. The PMU name is 
stored in the evsel->pmu_name. I don't think we need to check all the 
possible PMU names. Using evsel->pmu_name should be good enough.

> if (!pmu_have_event("cpu", "slots") && !pmu_have_event("cpu_core", "slots") )
> 
> But we should add a helper function for this. It is odd to have this
> change supporting Alderlake but the existing evlist work not. Perhaps
> we should just wait until Zhengjun's patches land.

Yes, a helper function is good for the arch_evlist functions. But I 
don't think this patch needs the helper function. Zhengjun's patches are 
to fix the other topdown issues on ADL. There is no dependency between 
this patch and zhengjun's patches.

Thanks,
Kan