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:

quoted

On 6/5/23 19:13, Mark Rutland wrote:

quoted

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.

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

Sorry for flip-flopping on this.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel