Messages in this thread | | | Date | Mon, 12 Jun 2023 15:42:38 +0530 | Subject | Re: [PATCH V11 06/10] arm64/perf: Enable branch stack events via FEAT_BRBE | From | 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.
| |