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

[Mi, Dapeng]

On 10/29/2026 6:29 PM, Peter Zijlstra wrote:
> 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@xxxxxxxxxx>
>> Closes: https://lore.kernel.org/all/sashiko-outbox-179823@xxxxxxxxxx/
>> Fixes: 425507fa5f32 ("perf/x86/intel/lbr: Optimize v4 LBR unfreezing")
>> Signed-off-by: Dapeng Mi <dapeng1.mi@xxxxxxxxxxxxxxx>
>> ---
>>  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?

Sure. I would investigate the history and check if it can be removed.