Message-ID: <0c8da8b4-4f77-4329-9e63-721d05fd258d@linux.intel.com>
Date:   Mon, 9 May 2022 17:01:30 -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/9/2022 1:28 PM, Ian Rogers wrote:
> On Thu, May 5, 2022 at 12:44 PM Liang, Kan <kan.liang@...ux.intel.com> wrote:
>>
>>
>>
>> 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
> 
> TL;DR I think we can move forward with landing these patches to fix Icelake.

This patch doesn't work with the hybrid platform for sure. I can send 
you a fix for the hybrid part if you prefer this way? Then I guess you 
may append it as the patch 3 for V2.

Besides the hybrid thing, the patch set also has other two issues I 
mentioned in the previous reply.
- I don't think the strcasecmp() can handle the case like 
cpu/topdown-bad-spec/ or cpu/slots/. It should be an issue for both 
hybrid and non-hybrid platforms.
- It's better not to use non-architecture events, e.g., baclears.any, 
ARITH.DIVIDER_ACTIVE, even in the test case. The non-architecture events 
may be disappear in the future platforms. If so, you have to update the 
test case again for the future platforms.
IMHO, I don't think the patch set is ready.

> 
> For Alderlake/hybrid we have a problem. To determine what happens with
> grouping we need to know does the CPU have topdown events? This is a
> runtime question for doing perf_event_open and so an arch test and
> weak symbol are appropriate. For Icelake we are determining the
> presence of topdown events by looking at the special PMU cpu. For
> Alderlake the same information can be found by looking at the PMUs
> cpu_core and cpu_atom, but how to discover those PMU names?

The PMU name can be retrieved either from the event list or perf command.
For the non-hybrid, the PMU name is hard code to "cpu" for the core 
events. So users/event files don't need to specify the PMU name.
For the hybrid platform, a PMU name is required and stored in the 
evsel->pmu_name. If the evsel->pmu_name is NULL, we can assume that it's 
a non-hybrid PMU, CPU.

> It is
> already somewhat concerning that we've hard coded "cpu" and we don't
> want to have an ever growing list of PMU names.
> 
> We have similarly hard coded "cpu" in the topology code here:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/cputopo.c?h=tmp.perf/core#n18
> Is this unreasonable given cpu is already supposed to be ABI stable:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/Documentation/ABI/stable/sysfs-devices-system-cpu?h=tmp.perf/core

I don't think there is a stable ABI for a PMU name.
The PMU name may be changed generation by generation because of the 
different micro arch. We will try our best to keep it unchanged in X86 
but it's not guaranteed especially when the hybrid is introduced.


> 
> It is hard to say what the right hybrid fix is here. I should get a
> system I can poke shortly. I'd also like to compare what's in sysfs
> for Alderlake with ARM's big.little approach. I can imagine we need a
> function that returns a list of CPU like PMUs for probing. Ideally we
> could work this out from sysfs and use some stable ABI.
>

I don't think there is a standard PMU naming rule for all the ARCHs. For 
X86, it may be possible. You can assume that the name like "cpu" or 
"cpu_*" are for core PMUs. But for other ARCH e.g., ARM, AFAIK, they use 
a quite different naming rule.


Thanks,
Kan

Powered by blists - more mailing lists