Message-ID: <86b7143f-b757-a2d3-b837-686b420cf3c8@linux.intel.com>
Date:   Tue, 10 May 2022 15:24:38 -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/10/2022 12:58 PM, Ian Rogers wrote:
> On Mon, May 9, 2022 at 2:01 PM Liang, Kan <kan.liang@...ux.intel.com> wrote:
>>
>> 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.
> 
> So all the stated objections are that I'm checking cpu/slots/ for an
> indication of topdown support and this doesn't work for hybrid?

No, besides the hybrid issue, I also pointed out two non-hybrid issues 
for the patch set.

Have you tried this with the patch set on ICX?

./perf stat -e 
'{cpu/slots/,cpu/topdown-bad-spec/,cpu/topdown-be-bound/,cpu/topdown-fe-bound/,cpu/topdown-retiring/,branch-instructions,branch-misses,bus-cycles,cache-misses,cache-references,cpu-cycles,instructions,mem-loads,mem-stores,ref-cycles,cache-misses,cache-references}:W' 
-a sleep 1

I don't think it works.


The ARITH.DIVIDER_ACTIVE is already a deprecated event in SPR.
I don't think we want to update the test case for the platform after SPR.


> This
> is identical to the arch evlist code:
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/arch/x86/util/evlist.c?h=perf/core#n10
> both in the functions arch_evlist__add_default_attrs and
> arch_evlist__leader (one I wrote and one I didn't). So the patch set
> isn't ready because I haven't fixed alderlake, but alderlake already
> isn't working? And there is no example of how to make this work for
> alderlake. So basically your ask is that I bring up alderlake. I think
> this is stretching things for a patch fixing icelake. The value here
> is in fixing icelake and alderlake will have to be the next problem.
> 
>>>
>>> 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.
> 
> This doesn't make sense. Hybrid implies more than 1 CPU type, how can
> more than one be the same as 1 type to be used for the PMU? 

One event can only belongs to one PMU. It's saved in the evsel->pmu_name.
But it may be NULL on a non-hybrid machine, because there is only one 
CPU type for a non-hybrid machine. The old code may not set the variable.

> Again, you
> are asking I make all the alderlake logic work and I think that should
> be follow up.

I didn't ask you to make all the alderlake logic work.

What I asked is not to introduce more such kind problem, because we 
already have enough to fix.

I even offered you a fixed patch for the problem.

https://lore.kernel.org/all/f1c212e3-c1fd-4a2d-0dfe-bc913d4f4f36@linux.intel.com/T/#m22465c6d35be59fc853a7d86f5bd6025ce7e539b


> As shown above the arch evlist code also needs fixing as
> follow up.

Zhengjun is working on them. There is no dependency between this patch 
and that fix. Don't worry about it.


Thanks,
Kan

Powered by blists - more mailing lists