by Anshuman Khandual

[permalink] [raw]

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

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.

2023-06-12 10:45:59

by Anshuman Khandual

[permalink] [raw]

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

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.