Re: [PATCH V11 06/10] arm64/perf: Enable branch stack events via FEAT_BRBE

[Anshuman Khandual]

On 6/9/23 18:45, Mark Rutland wrote:
> On Fri, Jun 09, 2023 at 01:47:18PM +0100, Mark Rutland wrote:
>> On Fri, Jun 09, 2023 at 10:52:37AM +0530, Anshuman Khandual wrote:
>>> On 6/5/23 19:13, Mark Rutland wrote:
>>>> Looking at <linux/perf_event.h> I see:
>>>>
>>>> | /*
>>>> |  * branch stack layout:
>>>> |  *  nr: number of taken branches stored in entries[]
>>>> |  *  hw_idx: The low level index of raw branch records
>>>> |  *          for the most recent branch.
>>>> |  *          -1ULL means invalid/unknown.
>>>> |  *
>>>> |  * Note that nr can vary from sample to sample
>>>> |  * branches (to, from) are stored from most recent
>>>> |  * to least recent, i.e., entries[0] contains the most
>>>> |  * recent branch.
>>>> |  * The entries[] is an abstraction of raw branch records,
>>>> |  * which may not be stored in age order in HW, e.g. Intel LBR.
>>>> |  * The hw_idx is to expose the low level index of raw
>>>> |  * branch record for the most recent branch aka entries[0].
>>>> |  * The hw_idx index is between -1 (unknown) and max depth,
>>>> |  * which can be retrieved in /sys/devices/cpu/caps/branches.
>>>> |  * For the architectures whose raw branch records are
>>>> |  * already stored in age order, the hw_idx should be 0.
>>>> |  */
>>>> | struct perf_branch_stack {
>>>> |         __u64                           nr;  
>>>> |         __u64                           hw_idx;
>>>> |         struct perf_branch_entry        entries[];
>>>> | };
>>>>
>>>> ... which seems to indicate we should be setting hw_idx to 0, since IIUC our
>>>> records are in age order.
>>> Branch records are indeed in age order, sure will change hw_idx as 0. Earlier
>>> figured that there was no need for hw_idx and hence marked it as -1UL similar
>>> to other platforms like powerpc.
>>
>> That's fair enough; looking at power_pmu_bhrb_read() in
>> arch/powerpc/perf/core-book3s.c, I see a comment:
>>
>> 	Branches are read most recent first (ie. mfbhrb 0 is
>> 	the most recent branch).
>>
>> ... which suggests that should be 0 also, or that the documentation is wrong.
>>
>> Do you know how the perf tool consumes this?
> 
> 
> Thinking about this some more, if what this is saying is that if entries[0]
> must be strictly the last branch, and we've lost branches due to interrupt
> latency, then we clearly don't meet that requirement and must report -1ULL
> here.

'last branch' means relative to the captured records not in absolute terms.
Loosing records for interrupt latency too does not change the relative age
order for the set. Hence '0' might suggest valid relative not absolute age
order for the branch records set.

> 
> So while it'd be nice to figure this out, I'm happy using -1ULL, and would be a
> bit concerned using 0.

Sounds reasonable. If tools are not going to use this anyway, I guess there
is no point in suggesting that each record set has got valid age order with
subtle conditions involved.