Re: [PATCH 18/19] perf: Introduce positive capability for raw events

On Tue, Aug 26, 2025 at 11:46:02PM +0100, Robin Murphy wrote:
> On 2025-08-26 2:43 pm, Mark Rutland wrote:
> > On Wed, Aug 13, 2025 at 06:01:10PM +0100, Robin Murphy wrote:
> > To bikeshed a little here, I'm not keen on the PERF_PMU_CAP_RAW_EVENTS
> > name, because it's not clear what "RAW" really means, and people will
> > definitely read that to mean something else.
> > 
> > Could we go with something like PERF_PMU_CAP_COMMON_CPU_EVENTS, to make
> > it clear that this is about opting into CPU-PMU specific event types (of
> > which PERF_TYPE_RAW is one of)?
> 
> Indeed I started with that very intention after our previous discussion, but
> soon realised that in fact nowhere in the code is there any definition or
> even established notion of what "common" means in this context, so it's
> hardly immune to misinterpretation either.

We can document that; it's everything less than PERF_TYPE_MAX:

	enum perf_type_id {
		PERF_TYPE_HARDWARE                      = 0, 
		PERF_TYPE_SOFTWARE                      = 1, 
		PERF_TYPE_TRACEPOINT                    = 2, 
		PERF_TYPE_HW_CACHE                      = 3, 
		PERF_TYPE_RAW                           = 4, 
		PERF_TYPE_BREAKPOINT                    = 5, 

		PERF_TYPE_MAX,                          /* non-ABI */
	};

... and maybe you could use "PERF_PMU_CAP_ABI_TYPES" to align with that
comment?

> Furthermore the semantics of the cap as it ended up are specifically
> that the PMU wants the same behaviour as if it had registered as
> PERF_TYPE_RAW, so having "raw" in the name started to look like the
> more intuitive option after all (plus being nice and short helps.)

I appreciate the shortness, but I think it's misleading to tie this to
"RAW" specifically, when really this is a capabiltiy to say "please let
me try to init any events for non-dynamic types, in addition to whatever
specific type I am registered with".

> If anything, it's "events" that carries the implication that's proving hard
> to capture precisely and concisely here, so maybe the answer to avoid
> ambiguity is to lean further away from a "what it represents" to a "what it
> actually does" naming - PERF_PMU_CAP_TYPE_RAW, anyone?

I'm happy with TYPE in the name; it's just RAW specifically that I'm
objecting to.

> > Likewise, s/is_raw_pmu()/pmu_supports_common_cpu_events()/.
> 
> Case in point: is it any more logical and expected that supporting common
> CPU events implies a PMU should be offered software or breakpoint events as
> well? Because that's what such a mere rename would currently mean :/

Yes, I think it is.

> > > ---
> > > 
> > > A further possibility is to automatically add the cap to PERF_TYPE_RAW
> > > PMUs in perf_pmu_register() to have a single point-of-use condition; I'm
> > > undecided...
> > 
> > I reckon we don't need to automagically do that, but I reckon that
> > is_raw_pmu()/pmu_supports_common_cpu_events() should only check the cap,
> > and we don't read anything special into any of
> > PERF_TYPE_{RAW,HARDWARE,HW_CACHE}.
> 
> OK, but that would then necessitate having to explicitly add the cap to all
> 15-odd other drivers which register as PERF_TYPE_RAW as well, at which point
> it starts to look like a more general "I am a CPU PMU in terms of most
> typical assumptions you might want to make about that" flag...
> 
> To clarify (and perhaps something for a v2 commit message), we currently
> have 3 categories of PMU driver:
> 
> 1: (Older/simpler CPUs) Registers as PERF_TYPE_RAW, wants
> PERF_TYPE_RAW/HARDWARE/HW_CACHE events
> 2: (Heterogeneous CPUs) Registers as dynamic type, wants
> PERF_TYPE_RAW/HARDWARE/HW_CACHE events plus events of its own type
> 3: (Mostly uncore) Registers as dynamic type, only wants events of its own
> type

Sure, but I think that separating 1 and 2 is an artificial distinction,
and what we really have is:

(a) Wants to handle (some of) the non-dynamic/common/ABI types (in
    addition to whatever specific type it was registered with). Needs to
    have a switch statement somewhere in pmu::event_init().

(b) Only wants to handle the specific type the PMU was registered with.

> My vested interest is in making category 3 the default behaviour, given that
> the growing majority of new drivers are uncore (and I keep having to write
> them...) 

Yes, we're aligned on that.

> However unclear the type overlaps in category 1 might be, it's been
> like that for 15 years, so I didn't feel compelled to churn fossils like
> Alpha more than reasonably necessary. Category 2 is only these 5 drivers, so
> a relatively small tweak to distinguish them from category 3 and let them
> retain the effective category 1 behaviour (which remains the current one of
> potentially still being offered software etc. events too) seemed like the
> neatest way to make progress.

I just think we should combine 1 and 2 (into categroy a as above), since
that removes the need to treat RAW specially going forwards.

> I'm not saying I'm necessarily against a general overhaul of CPU PMUs being
> attempted too, just that it seems more like a whole other side-quest, and
> I'd really like to slay the uncore-boilerplate dragon first.

I think that adding the cap to those 15 PMUs would take less time than
it has taken me to write this email, so I do not understand the
objection.

Mark.