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

[Liang, Kan]

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