Re: [PATCH V7 08/14] perf/x86/intel: Generic support for hardware TopDown metrics

[Peter Zijlstra]

On Fri, Jul 24, 2020 at 03:10:52PM -0400, Liang, Kan wrote:

> > > > Per the SIBLING patch this then wants to be:
> > > > 
> > > >             if (!is_slots_event(leader))
> > > >                 return -EINVAL;
> > > > 
> > > >             event->event_caps |= PERF_EV_CAP_SIBLING.
> > > >             /*
> > > >              * Only once we have a METRICs sibling to we
> > > >              * need TopDown magic.
> > > >              */
> > > >             leader->hw.flags |= PERF_X86_EVENT_TOPDOWN;
> 
> Since we only set the flag for the SLOTS event now, the V7 patch will treat
> the metric events as normal events, which trigger an error.

Damn, that was a silly oversight on my part.

> I think we don't need the PERF_X86_EVENT_TOPDOWN flag anymore.
> If it's a non-sampling slots event, apply the special function.
> If it's a metric event, do nothing.

> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 0f3d01562ded..02dfee0b6615 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -73,10 +73,10 @@ u64 x86_perf_event_update(struct perf_event *event)
>  	u64 prev_raw_count, new_raw_count;
>  	u64 delta;
> 
> -	if (unlikely(!hwc->event_base))
> +	if (unlikely(!hwc->event_base || is_metric_event(event)))
>  		return 0;
> 
> -	if (unlikely(is_topdown_count(event)) && x86_pmu.update_topdown_event)
> +	if (unlikely(is_slots_count(event)) && x86_pmu.update_topdown_event)
>  		return x86_pmu.update_topdown_event(event);
> 
>  	/*
> @@ -1280,11 +1280,10 @@ int x86_perf_event_set_period(struct perf_event
> *event)
>  	s64 period = hwc->sample_period;
>  	int ret = 0, idx = hwc->idx;
> 
> -	if (unlikely(!hwc->event_base))
> +	if (unlikely(!hwc->event_base || is_metric_event(event)))
>  		return 0;
> 
> -	if (unlikely(is_topdown_count(event)) &&
> -	    x86_pmu.set_topdown_event_period)
> +	if (unlikely(is_slots_count(event)) && x86_pmu.set_topdown_event_period)
>  		return x86_pmu.set_topdown_event_period(event);
> 
>  	/*

This; I don't like that much, it adds even more conditions to fairly hot
code.

I was even considering adding a static_branch for
x86_pmu.intel_cap.perf_metrics.

Anyway, let me fix this.