Re: [PATCH] perf/x86/amd/lbr: Limit branch snapshots to valid entries

[Sandipan Das]

On 14-10-2026 14:31, Mi, Dapeng wrote:
> 
> On 10/13/2026 8:53 PM, Mi, Dapeng wrote:
>> On 10/13/2026 3:46 PM, Peter Zijlstra wrote:
>>> On Tue, Oct 13, 2026 at 12:14:58PM +0530, Sandipan Das wrote:
>>>> On 12-10-2026 21:00, Peter Zijlstra wrote:
>>>>> On Wed, Oct 07, 2026 at 11:40:02PM +0530, Sandipan Das wrote:
>>>>>> The branch snapshot callback used by bpf_get_branch_snapshot() copies
>>>>>> up to x86_pmu.lbr_nr entries from cpuc->lbr_entries[], even when the
>>>>>> last LBR read kept fewer branches. The entries past lbr_stack.nr are
>>>>>> invalid, but they end up reaching BPF programs.
>>>>>>
>>>>>> Limit copying to lbr_stack.nr entries and also reset lbr_stack.nr when
>>>>>> amd_pmu_lbr_read() skips reading the registers, so that a stale count
>>>>>> is never reused.
>>>>>>
>>>>>> Fixes: a4d18112e531 ("perf/x86/amd: Support capturing LBR from software events")
>>>>>> Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
>>>>>> Closes: https://lore.kernel.org/all/sashiko-outbox-163130@xxxxxxxxxx/
>>>>>> Signed-off-by: Sandipan Das <sandipan.das@xxxxxxx>
>>>>>> ---
>>>>>>  arch/x86/events/amd/core.c | 2 +-
>>>>>>  arch/x86/events/amd/lbr.c  | 4 +++-
>>>>>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
>>>>>> index 49b6b8fce566..bf631f60757c 100644
>>>>>> --- a/arch/x86/events/amd/core.c
>>>>>> +++ b/arch/x86/events/amd/core.c
>>>>>> @@ -944,7 +944,7 @@ static int amd_pmu_v2_snapshot_branch_stack(struct perf_branch_entry *entries, u
>>>>>>  	cpuc = this_cpu_ptr(&cpu_hw_events);
>>>>>>  
>>>>>>  	amd_pmu_lbr_read();
>>>>>> -	cnt = min(cnt, x86_pmu.lbr_nr);
>>>>>> +	cnt = min(cnt, cpuc->lbr_stack.nr);
>>>>>>  	memcpy(entries, cpuc->lbr_entries, sizeof(struct perf_branch_entry) * cnt);
>>>>>>  
>>>>>>  	amd_pmu_v2_enable_all(0);
>>>>>> diff --git a/arch/x86/events/amd/lbr.c b/arch/x86/events/amd/lbr.c
>>>>>> index 1e1b2f08cb51..a00d3850c4a8 100644
>>>>>> --- a/arch/x86/events/amd/lbr.c
>>>>>> +++ b/arch/x86/events/amd/lbr.c
>>>>>> @@ -165,8 +165,10 @@ void amd_pmu_lbr_read(void)
>>>>>>  	struct branch_entry entry;
>>>>>>  	int out = 0, idx, i;
>>>>>>  
>>>>>> -	if (!cpuc->lbr_users)
>>>>>> +	if (!cpuc->lbr_users) {
>>>>>> +		cpuc->lbr_stack.nr = 0;
>>>>>>  		return;
>>>>>> +	}
>>>>>>  
>>>>>>  	for (i = 0; i < x86_pmu.lbr_nr; i++) {
>>>>>>  		entry.from.full	= amd_pmu_lbr_get_from(i);
>>>>> You now seem to deviate from what the Intel code does. Please clarify.
>>>> Yes, it is a deviation. I thought of proposing the following but didn't
>>>> proceed because I was not sure about the if condition in the second hunk
>>>> which considers things other than cpuc->lbr_users for an early return.
>>> Dapeng, can you look into this? I would really like to keep behaviour
>>> consistent where possible.
>> Sure.
>>
>>
>>>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>>>> index 0a34d674df59..5dfb9e9156e8 100644
>>>> --- a/arch/x86/events/intel/core.c
>>>> +++ b/arch/x86/events/intel/core.c
>>>> @@ -2822,7 +2822,7 @@ __intel_pmu_snapshot_branch_stack(struct perf_branch_entry *entries,
>>>>         struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
>>>>
>>>>         intel_pmu_lbr_read();
>>>> -       cnt = min_t(unsigned int, cnt, x86_pmu.lbr_nr);
>>>> +       cnt = min_t(unsigned int, cnt, cpuc->lbr_stack.nr);
>>>>
>>>>         memcpy(entries, cpuc->lbr_entries, sizeof(struct perf_branch_entry) * cnt);
>>>>         intel_pmu_enable_all(0);
>>>> diff --git a/arch/x86/events/intel/lbr.c b/arch/x86/events/intel/lbr.c
>>>> index 0b446d8912e4..6b070490f461 100644
>>>> --- a/arch/x86/events/intel/lbr.c
>>>> +++ b/arch/x86/events/intel/lbr.c
>>>> @@ -1026,8 +1026,10 @@ void intel_pmu_lbr_read(void)
>>>>          * but this simple approach seems to work for now.
>>>>          */
>>>>         if (!cpuc->lbr_users || vlbr_exclude_host() ||
>>>> -           cpuc->lbr_users == cpuc->lbr_pebs_users)
>>>> +           cpuc->lbr_users == cpuc->lbr_pebs_users) {
>>>> +               cpuc->lbr_stack.nr = 0;
>> I checked the code, and it looks safe to clear cpuc->lbr_stack.nr here. For
>> PEBS events with the LBR group, the LBR stack is populated again for each
>> PEBS record, so this should not affect the LBR trace for PEBS events.
> 
> I test the changes on SPR for the 3 cases, pure LBR PEBS sampling, pure LBR
> non-PEBS sampling and the PEBS/non-PEBS mixed LBR sampling. No issue is
> found for the LBR stack sampling.
> 
> Sandipan, would you mind to post the fixes for Intel LBR with the AMD fixes
> together? Thanks.
> 

Will post the fixes together. Thanks for testing this.

> 
>>
>> But I’m not fully sure if it's intentional for the
>> bpf_get_branch_snapshot() helper. Clearing lbr_stack.nr to 0 prevents the
>> BPF helper from reading LBR data from PEBS although the LBR data could be
>> stale since it comes from the past PEBS record instead of the runtime
>> hardware LBR state. That seems reasonable and correct, but the side effect
>> is that the BPF helper can't read the LBR stack when all LBR events are
>> PEBS events. Not sure if it's intentional and expected. 
>>
>> I’d like to run some tests for this change tomorrow to confirm the
>> behavior. Thanks.
>>
>>
>>>>                 return;
>>>> +       }
>>>>
>>>>         x86_pmu.lbr_read(cpuc);
>>>>