Re: [Patch v5 09/18] perf/x86/intel: Keep LBR frozen throughout PMI handling

[Peter Zijlstra <peterz@infradead.org>]

On Thu, Oct 29, 2026 at 12:39:05PM +0800, Dapeng Mi wrote:
> For the platforms which has 4+ PMU version, streamline LBR freeze is
> supported. The PERF_GLOBAL_STATUS.LBR_FRZ bit is set to freeze LBR once
> a PMU counter overflow. However, the current PMI handler clears the LBR
> freeze bit too early, allowing branch operations that occur during PMI
> handling to be recorded into the LBR stack.
> 
> This can pollute or overwrite the intended branch snapshot, especially
> when bpf_get_branch_snapshot() is invoked from bpf_overflow_handler().
> In that case, branches recorded while processing the PMI are mixed into
> the original LBR data.
> 
> Defer unfreezing until after PMI handling completes so the LBR remains
> frozen for the entire PMI path and no branch activity from the handler
> is recorded.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/all/sashiko-outbox-179823@kernel.org/
> Fixes: 425507fa5f32 ("perf/x86/intel/lbr: Optimize v4 LBR unfreezing")
> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.intel.com>
> ---
>  arch/x86/events/intel/core.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 15dcf8b78de9..a67297148bb6 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c
> @@ -4062,7 +4062,13 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
>  	loops = 0;
>  again:
>  	intel_pmu_lbr_read();
> -	intel_pmu_ack_status(status);
> +
> +	/*
> +	 * Do not clear the LBR frozen status here; keep LBR frozen
> +	 * throughout PMI handling.
> +	 */
> +	intel_pmu_ack_status(status & ~GLOBAL_STATUS_LBRS_FROZEN);
> +
>  	if (++loops > 100) {
>  		static bool warned;
>  
> @@ -4081,10 +4087,13 @@ static int intel_pmu_handle_irq(struct pt_regs *regs)
>  	 * Repeat if there is more work to be done:
>  	 */
>  	status = intel_pmu_get_status();
> -	if (status)
> +	if (status & ~GLOBAL_STATUS_LBRS_FROZEN)
>  		goto again;
>  
>  done:
> +	if (status & GLOBAL_STATUS_LBRS_FROZEN)
> +		intel_pmu_ack_status(status & GLOBAL_STATUS_LBRS_FROZEN);
> +
>  	if (mid_ack)
>  		apic_write(APIC_LVTPC, APIC_DM_NMI);
>  	/* Only restore PMU state when it's active. See x86_pmu_disable(). */

Given the whole PMU is disabled per __intel_pmu_disable_all(), all that
can really happen is for more bits to be set, right? I mean, any counter
cleared on the first ack_status, will never be re-set, it already fired
and its disabled. All that can happen is that the disable is 'delayed'
and a counter can maybe fire a little bit after the disable; getting
missed on the first go.

Can we really get more than 1 extra loop there?

I'm sure this code has changed lots since we added that loop thing
there.

Anyway, if we can never get more than 1 extra loop, we can probably move
the ack to the end, without having to do a second.

I suppose this isn't something you have to do now, but keep it on the
todo list or something?