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

[Mi, Dapeng]

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.


>
> 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);
>>>