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

[Ian Rogers]

On Mon, May 9, 2022 at 2:01 PM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
>
> On 5/9/2022 1:28 PM, Ian Rogers wrote:
> > On Thu, May 5, 2022 at 12:44 PM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> 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? 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? Again, you
are asking I make all the alderlake logic work and I think that should
be follow up. As shown above the arch evlist code also needs fixing as
follow up.

Thanks,
Ian

> > 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