Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 28 Jul 2020 14:32:40 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     "Liang, Kan" <kan.liang@linux.intel.com>
Cc:     acme@redhat.com, mingo@kernel.org, linux-kernel@vger.kernel.org,
        jolsa@kernel.org, eranian@google.com,
        alexander.shishkin@linux.intel.com, ak@linux.intel.com,
        like.xu@linux.intel.com
Subject: Re: [PATCH V7 08/14] perf/x86/intel: Generic support for hardware
 TopDown metrics
Message-ID: <20200728123240.GJ10769@hirez.programming.kicks-ass.net>
References: <20200723171117.9918-1-kan.liang@linux.intel.com>
 <20200723171117.9918-9-kan.liang@linux.intel.com>
 <20200724131906.GW119549@hirez.programming.kicks-ass.net>
 <20200724152755.GK43129@hirez.programming.kicks-ass.net>
 <d7ae2272-52ea-c5a9-2937-9a51c544ade8@linux.intel.com>
 <ddeae082-c38d-a961-4d90-1fbc1c9f2726@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ddeae082-c38d-a961-4d90-1fbc1c9f2726@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.