Re: [PATCH V2 08/25] perf/x86: Hybrid PMU support for hardware cache event

[Liang, Kan]

On 3/11/2021 6:07 AM, Peter Zijlstra wrote:
> On Wed, Mar 10, 2021 at 08:37:44AM -0800, kan.liang@xxxxxxxxxxxxxxx wrote:
> > From: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> >
> > The hardware cache events are different among hybrid PMUs. Each hybrid
> > PMU should have its own hw cache event table.
> >
> > Reviewed-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> > Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> > ---
> >   arch/x86/events/core.c       | 11 +++++++++--
> >   arch/x86/events/perf_event.h |  9 +++++++++
> >   2 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> > index 039a851..1db4a67 100644
> > --- a/arch/x86/events/core.c
> > +++ b/arch/x86/events/core.c
> > @@ -352,6 +352,7 @@ set_ext_hw_attr(struct hw_perf_event *hwc, struct perf_event *event)
> >   {
> >   	struct perf_event_attr *attr = &event->attr;
> >   	unsigned int cache_type, cache_op, cache_result;
> > +	struct x86_hybrid_pmu *pmu = is_hybrid() ? hybrid_pmu(event->pmu) : NULL;
> >   	u64 config, val;
> >   
> >   	config = attr->config;
> > @@ -371,7 +372,10 @@ set_ext_hw_attr(struct hw_perf_event *hwc, struct perf_event *event)
> >   		return -EINVAL;
> >   	cache_result = array_index_nospec(cache_result, PERF_COUNT_HW_CACHE_RESULT_MAX);
> >   
> > -	val = hw_cache_event_ids[cache_type][cache_op][cache_result];
> > +	if (pmu)
> > +		val = pmu->hw_cache_event_ids[cache_type][cache_op][cache_result];
> > +	else
> > +		val = hw_cache_event_ids[cache_type][cache_op][cache_result];
> >   
> >   	if (val == 0)
> >   		return -ENOENT;
> > @@ -380,7 +384,10 @@ set_ext_hw_attr(struct hw_perf_event *hwc, struct perf_event *event)
> >   		return -EINVAL;
> >   
> >   	hwc->config |= val;
> > -	attr->config1 = hw_cache_extra_regs[cache_type][cache_op][cache_result];
> > +	if (pmu)
> > +		attr->config1 = pmu->hw_cache_extra_regs[cache_type][cache_op][cache_result];
> > +	else
> > +		attr->config1 = hw_cache_extra_regs[cache_type][cache_op][cache_result];
>
> Why not:
>
> 	attr->config1 = hybrid(event->pmu, hw_cache_extra_regs)[cache_type][cache_op][cache_result];
>
> ?

Because hw_cache_extra_regs is not part of the struct x86_pmu.


Thanks,
Kan