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

On Fri, Jun 09, 2023 at 10:52:37AM +0530, Anshuman Khandual wrote:

[...]

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

quoted

quoted

+void armv8pmu_branch_read(struct pmu_hw_events *cpuc, struct perf_event *event)
+{
+	struct brbe_hw_attr *brbe_attr = (struct brbe_hw_attr *)cpuc->percpu_pmu->private;
+	u64 brbfcr, brbcr;
+	int idx, loop1_idx1, loop1_idx2, loop2_idx1, loop2_idx2, count;
+
+	brbcr = read_sysreg_s(SYS_BRBCR_EL1);
+	brbfcr = read_sysreg_s(SYS_BRBFCR_EL1);
+
+	/* Ensure pause on PMU interrupt is enabled */
+	WARN_ON_ONCE(!(brbcr & BRBCR_EL1_FZP));
+
+	/* Pause the buffer */
+	write_sysreg_s(brbfcr | BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1);
+	isb();
+
+	/* Determine the indices for each loop */
+	loop1_idx1 = BRBE_BANK0_IDX_MIN;
+	if (brbe_attr->brbe_nr <= BRBE_BANK_MAX_ENTRIES) {
+		loop1_idx2 = brbe_attr->brbe_nr - 1;
+		loop2_idx1 = BRBE_BANK1_IDX_MIN;
+		loop2_idx2 = BRBE_BANK0_IDX_MAX;
+	} else {
+		loop1_idx2 = BRBE_BANK0_IDX_MAX;
+		loop2_idx1 = BRBE_BANK1_IDX_MIN;
+		loop2_idx2 = brbe_attr->brbe_nr - 1;
+	}
+
+	/* Loop through bank 0 */
+	select_brbe_bank(BRBE_BANK_IDX_0);
+	for (idx = 0, count = loop1_idx1; count <= loop1_idx2; idx++, count++) {
+		if (!capture_branch_entry(cpuc, event, idx))
+			goto skip_bank_1;
+	}
+
+	/* Loop through bank 1 */
+	select_brbe_bank(BRBE_BANK_IDX_1);
+	for (count = loop2_idx1; count <= loop2_idx2; idx++, count++) {
+		if (!capture_branch_entry(cpuc, event, idx))
+			break;
+	}
+
+skip_bank_1:
+	cpuc->branches->branch_stack.nr = idx;
+	cpuc->branches->branch_stack.hw_idx = -1ULL;
+	process_branch_aborts(cpuc);
+
+	/* Unpause the buffer */
+	write_sysreg_s(brbfcr & ~BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1);
+	isb();
+	armv8pmu_branch_reset();
+}

The loop indicies are rather difficult to follow, and I think those can be made
quite a lot simpler if split out, e.g.

| int __armv8pmu_branch_read(struct pmu_hw_events *cpuc, struct perf_event *event)
| {
| 	struct brbe_hw_attr *brbe_attr = (struct brbe_hw_attr *)cpuc->percpu_pmu->private;
| 	int nr_hw_entries = brbe_attr->brbe_nr;
| 	int idx;

I guess idx needs an init to 0.

Yes, sorry, that should have been:

	int idx = 0;

quoted

| 
| 	select_brbe_bank(BRBE_BANK_IDX_0);
| 	while (idx < nr_hw_entries && idx < BRBE_BANK0_IDX_MAX) {
| 		if (!capture_branch_entry(cpuc, event, idx))
| 			return idx;
| 		idx++;
| 	}
| 
| 	select_brbe_bank(BRBE_BANK_IDX_1);
| 	while (idx < nr_hw_entries && idx < BRBE_BANK1_IDX_MAX) {
| 		if (!capture_branch_entry(cpuc, event, idx))
| 			return idx;
| 		idx++;
| 	}
| 
| 	return idx;
| }

These loops are better than the proposed one with indices, will update.

Great!

quoted

| 
| void armv8pmu_branch_read(struct pmu_hw_events *cpuc, struct perf_event *event)
| {
| 	u64 brbfcr, brbcr;
| 	int nr;
| 
| 	brbcr = read_sysreg_s(SYS_BRBCR_EL1);
| 	brbfcr = read_sysreg_s(SYS_BRBFCR_EL1);
| 
| 	/* Ensure pause on PMU interrupt is enabled */
| 	WARN_ON_ONCE(!(brbcr & BRBCR_EL1_FZP));
| 
| 	/* Pause the buffer */
| 	write_sysreg_s(brbfcr | BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1);
| 	isb();
| 
| 	nr = __armv8pmu_branch_read(cpus, event);
| 
| 	cpuc->branches->branch_stack.nr = nr;
| 	cpuc->branches->branch_stack.hw_idx = -1ULL;
| 	process_branch_aborts(cpuc);
| 
| 	/* Unpause the buffer */
| 	write_sysreg_s(brbfcr & ~BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1);
| 	isb();
| 	armv8pmu_branch_reset();
| }

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?

Thanks,
Mark.

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