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

[Anshuman Khandual]

On 6/9/23 19:04, James Clark wrote:
> 
> 
> On 09/06/2023 13:47, 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:
>>>>> +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;
>>
>>>> | 
>>>> | 	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!
>>
>>>> | 
>>>> | 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.
> 
> It looks like it's a unique ID/last position updated in the LBR FIFO and
> it's used to stitch callchains together when the stack depth exceeds the
> buffer size. Perf takes the previous one that got filled to the limit
> and and the new one and stitches them together if the hw_idx matches.

Right.

> 
> There are some options in perf you need to provide to make it happen, so
> I think for BRBE it doesn't matter what value is assigned to it. -1
> seems to be a 'not used' value which we should probably set in case the
> event is opened with PERF_SAMPLE_BRANCH_HW_INDEX

-1 indeed did seem like a "not used" option rather than an "unkwown" option.
 
> 
> You could also fail to open the event if PERF_SAMPLE_BRANCH_HW_INDEX is
> set, and that would save writing out -1 every time for every branch
> stack. Although it's not enabled by default, so maybe that's not necessary.

Yeah blocking events with PERF_SAMPLE_BRANCH_HW_INDEX is not necessary IMHO.