Re: [PATCH v3 11/27] perf pmu: Support 'cycles' and 'branches' inside hybrid PMU

[Jin, Yao]

Hi Jiri,

On 4/9/2021 9:48 PM, Jiri Olsa wrote:
> On Mon, Mar 29, 2021 at 03:00:30PM +0800, Jin Yao wrote:
> > On hybrid platform, user may want to enable the hardware event
> > only on one PMU. So following syntax is supported:
> >
> > cpu_core/<hardware event>/
> > cpu_atom/<hardware event>/
> >
> >    # perf stat -e cpu_core/cpu-cycles/ -a -- sleep 1
> >
> >     Performance counter stats for 'system wide':
> >
> >             6,049,336      cpu_core/cpu-cycles/
> >
> >           1.003577042 seconds time elapsed
> >
> > It enables the event 'cpu-cycles' only on cpu_core pmu.
> >
> > But for 'cycles' and 'branches', the syntax doesn't work.
>
> because the alias is not there.. but there's:
>    cpu/cpu-cycles/
>    cpu/branch-instructions/
>
> doing the same thing..  what's wrong with that?
>
> I have a feeling we discussed this in the previous
> version.. did I give up? ;-)

Yes, we discussed this in previous threads. :)

Now I'm fine to keep the original behavior. Because the syntax 'cpu/cycles/' and 'cpu/branches/' are 
not supported by current perf.

> SNIP
>
> > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> > index beff29981101..72e5ae5e868e 100644
> > --- a/tools/perf/util/pmu.c
> > +++ b/tools/perf/util/pmu.c
> > @@ -916,6 +916,35 @@ static int pmu_max_precise(const char *name)
> >   	return max_precise;
> >   }
> >   
> > +static void perf_pmu__add_hybrid_aliases(struct list_head *head)
> > +{
> > +	static struct pmu_event pme_hybrid_fixup[] = {
> > +		{
> > +			.name = "cycles",
> > +			.event = "event=0x3c",
> > +		},
> > +		{
> > +			.name = "branches",
> > +			.event = "event=0xc4",
> > +		},
> > +		{
> > +			.name = 0,
> > +			.event = 0,
> > +		},
>
> if you really need to access these 2 events with special name,
> why not add it through the json.. let's not have yet another
> place that defines aliases ... also this should be model specific
> no?

Yes, defining in json is a good idea if we really need to support 'cpu/cycles/' and 'cpu/branches/'.

Anyway, I will drop this patch in next version.

Thanks
Jin Yao

> jirka