Re: [PATCH 1/1] perf/core: fix dangling cgroup pointer in cpuctx

[Peter Zijlstra <peterz@infradead.org>]

On Wed, Jun 04, 2025 at 04:46:39PM +0100, Leo Yan wrote:
> On Wed, Jun 04, 2025 at 04:16:40PM +0200, Peter Zijlstra wrote:
> 
> [...]
> 
> > It might be prudent to do something like so:
> 
> Thanks for the patch.
> 
> > +static void __event_disable(struct perf_event *event,
> > +			    struct perf_event_context *ctx,
> > +			    enum perf_event_state state)
> > +{
> > +	if (event == event->group_leader)
> > +		group_sched_out(event, ctx);
> 
> I am a bit struggle for this code line. It disables all events in a
> group, but only clear cgroup pointer for group leader but miss to clear
> for sibling events.

What happens is that perf_event_disable() will only mark the group
leader as OFF. By having the group leader marked OFF, the whole group
becomes ineligible to run. But the siblings are still INACTIVE and thus
don't need to get perf_cgroup_event_disable() called.

perf_event_enable() does the symmetric thing, it only marks the group
leader INACTIVE (and then tries to schedule it, possibly resulting in
ACTIVE).

Now, it is entirely possible to call {dis,en}able() on siblings, in
which case only the sibling gets marked OFF and gets scheduled out, but
the rest of the group can continue functioning.

So I think this was indeed correct.

> If the cgroup pointer is only used for group leader, maybe we only
> maintain (set and clear) the cgroup pointer for the leader?

And the patch re-used this function on siblings, do it would never hit
this group_sched_out() case.

But yes, slightly confusing. Let me see if I can make a less confusing
patch, and if not, sprinkle comments.