On 2025-08-26 2:03 pm, Peter Zijlstra wrote:
On Wed, Aug 13, 2025 at 06:01:04PM +0100, Robin Murphy wrote:
It may have been different long ago, but today it seems wrong for these
drivers to skip counting disabled sibling events in group validation,
given that perf_event_enable() could make them schedulable again, and
thus increase the effective size of the group later. Conversely, if a
sibling event is truly dead then it stands to reason that the whole
group is dead, so it's not worth going to any special effort to try to
squeeze in a new event that's never going to run anyway. Thus, we can
simply remove all these checks.
So currently you can do sort of a manual event rotation inside an
over-sized group and have it work.
I'm not sure if anybody actually does this, but its possible.
Eg. on a PMU that supports only 4 counters, create a group of 5 and
periodically cycle which of the 5 events is off.
So I'm not against changing this, but changing stuff like this always
makes me a little fearful -- it wouldn't be the first time that when it
finally trickles down to some 'enterprise' user in 5 years someone comes
and finally says, oh hey, you broke my shit :-(
Eww, I see what you mean... and I guess that's probably lower-overhead
than actually deleting and recreating the sibling event(s) each time,
and potentially less bother then wrangling multiple groups for different
combinations of subsets when one simply must still approximate a complex
metric that requires more counters than the hardware offers.
I'm also not keen to break anything that wasn't already somewhat broken,
especially since this patch is only intended as cleanup, so either we
could just drop it altogether, or perhaps I can wrap the existing
behaviour in a helper that can at least document this assumption and
discourage new drivers from copying it. Am I right that only
PERF_EVENT_STATE_{OFF,ERROR} would matter for this, though, and my
reasoning for state <= PERF_EVENT_STATE_EXIT should still stand? As for
the fiddly discrepancy with enable_on_exec between arm_pmu and others
I'm not really sure what to think...
Thanks,
Robin.
