Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 9 Feb 2022 16:32:04 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Stephane Eranian <eranian@google.com>
Cc:     linux-kernel@vger.kernel.org, kim.phillips@amd.com,
        acme@redhat.com, jolsa@redhat.com, songliubraving@fb.com
Subject: Re: [PATCH v6 06/12] perf/x86/amd: add AMD branch sampling period
 adjustment
Message-ID: <YgPedIWUiPIzF8OW@hirez.programming.kicks-ass.net>
References: <20220208211637.2221872-1-eranian@google.com>
 <20220208211637.2221872-7-eranian@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220208211637.2221872-7-eranian@google.com>
Precedence: bulk

On Tue, Feb 08, 2022 at 01:16:31PM -0800, Stephane Eranian wrote:
> Add code to adjust the sampling event period when used with the Branch
> Sampling feature (BRS). Given the depth of the BRS (16), the period is
> reduced by that depth such that in the best case scenario, BRS saturates at
> the desired sampling period. In practice, though, the processor may execute
> more branches. Given a desired period P and a depth D, the kernel programs
> the actual period at P - D. After P occurrences of the sampling event, the
> counter overflows. It then may take X branches (skid) before the NMI is
> caught and held by the hardware and BRS activates. Then, after D branches,
> BRS saturates and the NMI is delivered.  With no skid, the effective period
> would be (P - D) + D = P. In practice, however, it will likely be (P - D) +
> X + D. There is no way to eliminate X or predict X.
> 
> Signed-off-by: Stephane Eranian <eranian@google.com>
> ---
>  arch/x86/events/core.c       |  7 +++++++
>  arch/x86/events/perf_event.h | 12 ++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index c2a890caeb0a..ed285f640efe 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -1374,6 +1374,13 @@ int x86_perf_event_set_period(struct perf_event *event)
>  	    x86_pmu.set_topdown_event_period)
>  		return x86_pmu.set_topdown_event_period(event);
>  
> +	/*
> +	 * decrease period by the depth of the BRS feature to get
> +	 * the last N taken branches and approximate the desired period
> +	 */
> +	if (has_branch_stack(event))
> +		period = amd_brs_adjust_period(period);
> +
>  	/*
>  	 * If we are way outside a reasonable range then just skip forward:
>  	 */
> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> index 3485a4cf0241..25b037b571e4 100644
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -1263,6 +1263,14 @@ static inline bool amd_brs_active(void)
>  	return cpuc->brs_active;
>  }
>  
> +static inline s64 amd_brs_adjust_period(s64 period)
> +{
> +	if (period > x86_pmu.lbr_nr)
> +		return period - x86_pmu.lbr_nr;
> +
> +	return period;
> +}

This makes no sense to me without also enforcing that the event is in
fact that branch retired thing.