[PATCH v23 4/4] perf: arm_pmuv3: Add support for the Branch Record Buffer Extension (BRBE)

[Rob Herring]

On Fri, Jul 4, 2025 at 12:18 PM Mark Rutland <mark.rutland at arm.com> wrote:
>
> Hi Rob,
>
> Thanks for this; I think this is in good shape now. There's a couple of
> thing I think we should fixup, described below and diff provided at the
> end of this mail. That aside I reckon we should apply this shortly.
>
> Will, are you happy to apply that diff when picking this up?
>
> On Wed, Jun 11, 2025 at 01:01:14PM -0500, Rob Herring (Arm) wrote:
> > +/*
> > + * BRBE is assumed to be disabled/paused on entry
> > + */
> > +void brbe_enable(const struct arm_pmu *arm_pmu)
> > +{
> > +     struct pmu_hw_events *cpuc = this_cpu_ptr(arm_pmu->hw_events);
> > +     u64 brbfcr = 0, brbcr = 0;
> > +
> > +     /*
> > +      * Merge the permitted branch filters of all events.
> > +      */
> > +     for (int i = 0; i < ARMPMU_MAX_HWEVENTS; i++) {
> > +             struct perf_event *event = cpuc->events[i];
> > +
> > +             if (event && has_branch_stack(event)) {
> > +                     brbfcr |= event->hw.branch_reg.config;
> > +                     brbcr |= event->hw.extra_reg.config;
> > +             }
> > +     }
>
> I see that in v20 you moved the brbe_invaliate() form here into
> brbe_read_filtered_entries(), when entries are read upon an event
> overflowing. The changelog says:
>
> | Rework BRBE invalidation to avoid invalidating in interrupt handler
> | when no handled events capture the branch stack (i.e. when there are
> | multiple users).
>
> I don't think that's quite right. Since BRBCR_ELx.FZP causes a freeze
> BRBFCR_EL1.PAUSE to be set when *any* event overflows, not discarding
> across an overflow or other transient disable/enable can introduce a
> discontinuity in the branch records.

My thought was the discontinuity is samples from the PMU interrupt
which we don't want to sample anyways, so better to keep samples from
prior to the unrelated interrupt.

> The rationale for doing the invalidation here was to avoid the
> possiblity of any such discontinuity, and to do so simply, at the cost
> of discarding records in some cases where we could theoretically keep
> them around.

That's fine by me as well. It probably doesn't really matter much in
practice as you would have to get 2 PMU interrupts fairly close
together to lose samples (from before the first interrupt).

>
> I would prefer the invalidate to be performed within brbe_enable()
> (before the actual enable/unpause), and for armv8pmu_restart() to be
> folded back into armv8pmu_start().
>
> I've provided a diff/fixup at the end of this reply.

Thanks.

Rob