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

["Liang, Kan" <kan.liang@linux.intel.com>]

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