Message-ID: <CAL_JsqJ4cSGVjkBqU9uZdB2gjF6FyfQaD=jmcqEamEoqWLYGaQ@mail.gmail.com>
Date: Tue, 25 Feb 2025 09:35:09 -0600
From: Rob Herring <robh@...nel.org>
To: Leo Yan <leo.yan@....com>
Cc: Mark Rutland <mark.rutland@....com>, Will Deacon <will@...nel.org>, 
	Catalin Marinas <catalin.marinas@....com>, Jonathan Corbet <corbet@....net>, Marc Zyngier <maz@...nel.org>, 
	Oliver Upton <oliver.upton@...ux.dev>, Joey Gouly <joey.gouly@....com>, 
	Suzuki K Poulose <suzuki.poulose@....com>, Zenghui Yu <yuzenghui@...wei.com>, 
	James Clark <james.clark@...aro.org>, Anshuman Khandual <anshuman.khandual@....com>, 
	linux-arm-kernel@...ts.infradead.org, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org, 
	kvmarm@...ts.linux.dev
Subject: Re: [PATCH v20 11/11] perf: arm_pmuv3: Add support for the Branch
 Record Buffer Extension (BRBE)

On Tue, Feb 25, 2025 at 6:38 AM Leo Yan <leo.yan@....com> wrote:
>
> On Mon, Feb 24, 2025 at 07:31:52PM -0600, Rob Herring wrote:
>
> [...]
>
> > > > > When event rotation happens, if without context switch, in theory we
> > > > > should can directly use the branch record (no invalidation, no injection)
> > > > > for all events.
> > > >
> > > > No; that only works in *some* cases, and will produce incorrect results
> > > > in others.
> > > >
> > > > For example, consider filtering. Imagine a PMU with a single counter,
> > > > and two events, where event-A filters for calls-and-returns and event-B
> > > > filters for calls-only. When switching from event-A to event-B, it's
> > > > theoretically possible to keep the existing records around, knowing that
> > > > the returns can be filtered out later. When switching from event-B to
> > > > event-A we cannot keep the existing records, since there are gaps
> > > > whenever a return should have been recorded.
> > >
> > > Seems to me, the problem is not caused by event rotation.  We need to
> > > calculate a correct filter in the first place - the BRBE driver should
> > > calculate a superset for all filters of events for a session.  Then,
> > > generate branch record based event's specific filter.
> >
> > The driver doesn't have enough information. If it is told to schedule
> > event A, it doesn't know anything about event B. It could in theory
> > try to remember event B if event B had already been scheduled, but it
> > never knows when event B is gone.
>
> E.g., I tried below command for enabling 10 events in a perf session:
>
>   perf record -e armv9_nevis/r04/ -e armv9_nevis/r05/ \
>               -e armv9_nevis/r06/ -e armv9_nevis/r07/ \
>               -e armv9_nevis/r08/ -e armv9_nevis/r09/ \
>               -e armv9_nevis/r10/ -e armv9_nevis/r11/ \
>               -e armv9_nevis/r12/ -e armv9_nevis/r13/ \
>               -- sleep 1
>
> For Arm PMU, the flow below is invoked for every event on every
> affinied CPU in initialization phase:
>
>   armpmu_event_init() {
>     armv8pmu_set_event_filter();
>   }

That function is passed *1* event. It is not intended to go looking at
all events or muck with any global state. Could we go poking around
all the data structures? Probably, it's C and data structures are
often not opaque when they should be, so that wouldn't be a good idea.
If you think it is, I'd recommend you stay away from Rust.

Furthermore, an event here may not actually be enabled. A user could
open 2 events and handle them as mutually exclusive continuously
disabling one and enabling the other. If the branch filters that they
want are not overlapping, we'd be reducing our effective branch record
size. Maximizing the size seems much more important to me than keeping
some branches in a few corner cases.

> Shouldn't we calculate a superset branch filter for all events, store
> it into a per-CPU data structure and then apply the filter on BRBE?

Suppose we do that, what happens when a 2nd session (as Mark pointed
out, sessions only exist in the perf tool, not the kernel) adds more
events. We configured the filters and now we have to change them
again. So we have to invalidate the branch record. It's the same thing
with event rotation (though less frequent (probably)).

> > > > There are a number of cases of that shape given the set of configurable
> > > > filters. In theory it's possible to retain those in some cases, but I
> > > > don't think that the complexity is justified.
> > > >
> > > > Similarly, whenever kernel branches are recorded it's necessary to drop
> > > > the stale branches whenever branch recording is paused, as there's
> > > > necessarily a blackout period and hence a gap in the records.
> > >
> > > If we save BRBE record when a process is switched out and then restore
> > > the record when a process is switched in, should we can keep a decent
> > > branch record for performance profiling?
> >
> > Keep in mind that there's only 64 branches recorded at most. How many
> > branches in a context switch plus reconfiguring the PMU? Not a small
> > percentage of 64 I think. In traces where freeze on overflow was not
> > working (there's an example in v18), just the interrupt entry until
> > BRBE was stopped was a significant part of the trace. A context switch
> > is going to be similar.
>
> That is true for kernel mode enabled tracing.  But we will have no
> such kind noises for userspace only mode tracing.
>
> [...]
>
> > > > Do you have a reason why you think we *must* keep events around?
> > >
> > > Here I am really concerned are cases when a process is preempted or
> > > migrated.  The driver doesn't save and restore branch records for these
> > > cases, it just invalidates all records when a task is scheduled in.
> > >
> > > As a result, if an event overflow is close to context switch, it is
> > > likely to capture incomplete branch records.  For a userspace-only
> > > tracing, it is risk to capture empty branch record after preemption
> > > and migrations.
> >
> > There's the same risk if something else is recording kernel branches
> > when you are recording userspace only. I think the user has to be
> > aware if other things like context switches are perturbing their data.
>
> I am confused for the decription above.  Does it refer to branch
> recording cross different sessions?  It is fine for me that the branch
> data is interleaved by different sessions (e.g. one is global tracing
> and another is only per-thread tracing).
>
> We might need to consider an intact branch record for the single perf
> session case.  E.g. if userspace program calls:
>
>     func_a -> func_b -> func_c
>
> In a case for only userspace tracing, we will have no chance to preserve
> the call sequence of these functions after the program is switched out.

So you miss the few times that happens in a context switch. But we are
sampling and all/most of the other samples are going to be fine. How
much is that really going to affect your profile?

Rob

Powered by blists - more mailing lists