Re: [PATCH V5 6/7] arm64/perf: Add BRBE driver

[Anshuman Khandual]

On 11/7/22 11:55, Anshuman Khandual wrote:
> +void arm64_pmu_brbe_read(struct pmu_hw_events *cpuc, struct perf_event *event)
> +{
> +	u64 brbinf;
> +	int idx;
> +
> +	if (brbe_disabled(cpuc))
> +		return;
> +
> +	set_brbe_paused();
> +	for (idx = 0; idx < cpuc->brbe_nr; idx++) {
> +		select_brbe_bank_index(idx);
> +		brbinf = get_brbinf_reg(idx);
> +		/*
> +		 * There are no valid entries anymore on the buffer.
> +		 * Abort the branch record processing to save some
> +		 * cycles and also reduce the capture/process load
> +		 * for the user space as well.
> +		 */
> +		if (brbe_invalid(brbinf))
> +			break;
> +
> +		if (brbe_valid(brbinf)) {
> +			cpuc->branches->brbe_entries[idx].from =  get_brbsrc_reg(idx);
> +			cpuc->branches->brbe_entries[idx].to =  get_brbtgt_reg(idx);
> +		} else if (brbe_source(brbinf)) {
> +			cpuc->branches->brbe_entries[idx].from =  get_brbsrc_reg(idx);
> +			cpuc->branches->brbe_entries[idx].to = 0;
> +		} else if (brbe_target(brbinf)) {
> +			cpuc->branches->brbe_entries[idx].from = 0;
> +			cpuc->branches->brbe_entries[idx].to =  get_brbtgt_reg(idx);
> +		}
> +		capture_brbe_flags(cpuc, event, brbinf, idx);
> +	}
> +	cpuc->branches->brbe_stack.nr = idx;
> +	cpuc->branches->brbe_stack.hw_idx = -1ULL;
> +	process_branch_aborts(cpuc);
> +}

The following additional changes are required to ensure that BRBE remains "un-paused" after
processing the branch records inside PMU interrupt handler. Without this change, there will
PMU interrupts without valid branch records, reducing branch stack sample collection during
given workload execution. I will fold this into the BRBE driver.

diff --git a/drivers/perf/arm_pmu_brbe.c b/drivers/perf/arm_pmu_brbe.c
index ce1aa4171481..c8154ddd341d 100644
--- a/drivers/perf/arm_pmu_brbe.c
+++ b/drivers/perf/arm_pmu_brbe.c
@@ -429,6 +429,7 @@ void arm64_pmu_brbe_read(struct pmu_hw_events *cpuc, struct perf_event *event)
        cpuc->branches->brbe_stack.nr = idx;
        cpuc->branches->brbe_stack.hw_idx = -1ULL;
        process_branch_aborts(cpuc);
+       clr_brbe_paused();
 }
 
 void arm64_pmu_brbe_reset(struct pmu_hw_events *cpuc)
diff --git a/drivers/perf/arm_pmu_brbe.h b/drivers/perf/arm_pmu_brbe.h
index 22c4b25b1777..33da6fc9aefa 100644
--- a/drivers/perf/arm_pmu_brbe.h
+++ b/drivers/perf/arm_pmu_brbe.h
@@ -257,3 +257,11 @@ static inline void set_brbe_paused(void)
        write_sysreg_s(brbfcr | BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1);
        isb();
 }
+
+static inline void clr_brbe_paused(void)
+{
+       u64 brbfcr = read_sysreg_s(SYS_BRBFCR_EL1);
+
+       write_sysreg_s(brbfcr & ~BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1);
+       isb();
+}

For example:

./perf record -j any,u,k,save_type ls
./perf report -D | grep branch

Before this change -

# cat out | grep "branch stack: nr:64" | wc -l
4
# cat out | grep "branch stack: nr:0" | wc -l
57
# perf report -D | grep branch

... branch stack: nr:64
... branch stack: nr:0
... branch stack: nr:64
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:64
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:64
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0
... branch stack: nr:0

But after this change -

$ cat out | grep "branch stack: nr:64" | wc -l
107
# cat out | grep "branch stack: nr:0" | wc -l
0
$ perf report -D | grep branch

.......................

... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64
... branch stack: nr:64

.......................

- Anshuman