[PATCH v2 0/2] Fix perf cgroup problem

[PATCH v2 0/2] Fix perf cgroup problem

[Luo Gengkun]

---
Changes in v2:
1. First patch adapted based on the lastest mainline code
2. update commit message for the second patch.
Link to v1: https://lore.kernel.org/all/20250514064758.4156497-1-luogengkun@huaweicloud.com/

Luo Gengkun (2):
  perf/core: Fix nr_cgroups/cpuctx->cgrp is not updated correctly
  perf/core: Fix WARN_ON_ONCE(cpuctx->ctx.nr_cgroups == 0) in
    perf_cgroup_switch

 kernel/events/core.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] perf/core: Fix nr_cgroups/cpuctx->cgrp is not updated correctly

[Luo Gengkun]

Since __perf_remove_from_context updates event->state before
list_del_event, this prevents list_del_event from calling
perf_cgroup_event_disable, resulting in will not update nr_cgroups and
cpuctx->cgrp.

To fix this problem, move perf_cgroup_event_disable into
__perf_remove_from_context as:

commit a3c3c66670ce ("perf/core: Fix child_total_time_enabled accounting bug at task exit")

did.

Fixes: a3c3c66670ce ("perf/core: Fix child_total_time_enabled accounting bug at task exit")
Signed-off-by: Luo Gengkun <luogengkun@huaweicloud.com>
---
 kernel/events/core.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index f34c99f8ce8f..280d42b40b34 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -2120,18 +2120,6 @@ list_del_event(struct perf_event *event, struct perf_event_context *ctx)
 	if (event->group_leader == event)
 		del_event_from_groups(event, ctx);
 
-	/*
-	 * If event was in error state, then keep it
-	 * that way, otherwise bogus counts will be
-	 * returned on read(). The only way to get out
-	 * of error state is by explicit re-enabling
-	 * of the event
-	 */
-	if (event->state > PERF_EVENT_STATE_OFF) {
-		perf_cgroup_event_disable(event, ctx);
-		perf_event_set_state(event, PERF_EVENT_STATE_OFF);
-	}
-
 	ctx->generation++;
 	event->pmu_ctx->nr_events--;
 }
@@ -2498,6 +2486,9 @@ __perf_remove_from_context(struct perf_event *event,
 		state = PERF_EVENT_STATE_DEAD;
 	}
 	event_sched_out(event, ctx);
+
+	if (event->state > PERF_EVENT_STATE_OFF)
+		perf_cgroup_event_disable(event, ctx);
 	perf_event_set_state(event, min(event->state, state));
 
 	if (flags & DETACH_GROUP)
-- 
2.34.1

Re: [PATCH v2 1/2] perf/core: Fix nr_cgroups/cpuctx->cgrp is not updated correctly

[Peter Zijlstra]

On Wed, Jun 04, 2025 at 03:39:23AM +0000, Luo Gengkun wrote:
> Since __perf_remove_from_context updates event->state before
> list_del_event, this prevents list_del_event from calling
> perf_cgroup_event_disable, resulting in will not update nr_cgroups and
> cpuctx->cgrp.
> 
> To fix this problem, move perf_cgroup_event_disable into
> __perf_remove_from_context as:
> 
> commit a3c3c66670ce ("perf/core: Fix child_total_time_enabled accounting bug at task exit")
> 
> did.
> 
> Fixes: a3c3c66670ce ("perf/core: Fix child_total_time_enabled accounting bug at task exit")
> Signed-off-by: Luo Gengkun <luogengkun@huaweicloud.com>

So I already have Yeoreum's patch for this:

  https://lkml.kernel.org/r/20250603144414.GC38114@noisy.programming.kicks-ass.net

Re: [PATCH v2 1/2] perf/core: Fix nr_cgroups/cpuctx->cgrp is not updated correctly

[Luo Gengkun]

On 2025/6/4 17:19, Peter Zijlstra wrote:
> On Wed, Jun 04, 2025 at 03:39:23AM +0000, Luo Gengkun wrote:
>> Since __perf_remove_from_context updates event->state before
>> list_del_event, this prevents list_del_event from calling
>> perf_cgroup_event_disable, resulting in will not update nr_cgroups and
>> cpuctx->cgrp.
>>
>> To fix this problem, move perf_cgroup_event_disable into
>> __perf_remove_from_context as:
>>
>> commit a3c3c66670ce ("perf/core: Fix child_total_time_enabled accounting bug at task exit")
>>
>> did.
>>
>> Fixes: a3c3c66670ce ("perf/core: Fix child_total_time_enabled accounting bug at task exit")
>> Signed-off-by: Luo Gengkun <luogengkun@huaweicloud.com>
> So I already have Yeoreum's patch for this:
>
>    https://lkml.kernel.org/r/20250603144414.GC38114@noisy.programming.kicks-ass.net
> Oh! okay. But the second patch can still be reviewd. Thanks.

[PATCH v2 2/2] perf/core: Fix WARN_ON_ONCE(cpuctx->ctx.nr_cgroups == 0) in perf_cgroup_switch

[Luo Gengkun]

There may be concurrency between perf_cgroup_switch and
perf_cgroup_event_disable. Consider the following scenario: after a new
perf cgroup event is created on CPU0, the new event may not trigger
a reprogramming, causing ctx->is_active to be 0. In this case, when CPU1
disables this perf event, it executes __perf_remove_from_context->
list _del_event->perf_cgroup_event_disable on CPU1, which causes a race
with perf_cgroup_switch running on CPU0.

The following describes the details of this concurrency scenario:

CPU0						CPU1

perf_cgroup_switch:
   ...
   # cpuctx->cgrp is not NULL here
   if (READ_ONCE(cpuctx->cgrp) == NULL)
   	return;

						perf_remove_from_context:
						   ...
						   raw_spin_lock_irq(&ctx->lock);
						   ...
						   # ctx->is_active == 0 because reprogramm is not
						   # tigger, so CPU1 can do __perf_remove_from_context
						   # for CPU0
						   __perf_remove_from_context:
						         perf_cgroup_event_disable:
							    ...
							    if (--ctx->nr_cgroups)
							    ...

   # this warning will happened because CPU1 changed
   # ctx.nr_cgroups to 0.
   WARN_ON_ONCE(cpuctx->ctx.nr_cgroups == 0);

To fix this problem, expand the lock-holding critical section in
perf_cgroup_switch.

Fixes: db4a835601b7 ("perf/core: Set cgroup in CPU contexts for new cgroup events")
Signed-off-by: Luo Gengkun <luogengkun@huaweicloud.com>
---
 kernel/events/core.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 280d42b40b34..1e442897ebde 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -931,20 +931,20 @@ static void perf_cgroup_switch(struct task_struct *task)
 	struct perf_cpu_context *cpuctx = this_cpu_ptr(&perf_cpu_context);
 	struct perf_cgroup *cgrp;
 
+	cgrp = perf_cgroup_from_task(task, NULL);
+	perf_ctx_lock(cpuctx, cpuctx->task_ctx);
 	/*
 	 * cpuctx->cgrp is set when the first cgroup event enabled,
 	 * and is cleared when the last cgroup event disabled.
 	 */
 	if (READ_ONCE(cpuctx->cgrp) == NULL)
-		return;
+		goto unlock;
 
 	WARN_ON_ONCE(cpuctx->ctx.nr_cgroups == 0);
 
-	cgrp = perf_cgroup_from_task(task, NULL);
 	if (READ_ONCE(cpuctx->cgrp) == cgrp)
-		return;
+		goto unlock;
 
-	perf_ctx_lock(cpuctx, cpuctx->task_ctx);
 	perf_ctx_disable(&cpuctx->ctx, true);
 
 	ctx_sched_out(&cpuctx->ctx, NULL, EVENT_ALL|EVENT_CGROUP);
@@ -962,6 +962,7 @@ static void perf_cgroup_switch(struct task_struct *task)
 	ctx_sched_in(&cpuctx->ctx, NULL, EVENT_ALL|EVENT_CGROUP);
 
 	perf_ctx_enable(&cpuctx->ctx, true);
+unlock:
 	perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
 }
 
-- 
2.34.1

Re: [PATCH v2 2/2] perf/core: Fix WARN_ON_ONCE(cpuctx->ctx.nr_cgroups == 0) in perf_cgroup_switch

[Peter Zijlstra]

On Wed, Jun 04, 2025 at 03:39:24AM +0000, Luo Gengkun wrote:
> There may be concurrency between perf_cgroup_switch and
> perf_cgroup_event_disable. Consider the following scenario: after a new
> perf cgroup event is created on CPU0, the new event may not trigger
> a reprogramming, causing ctx->is_active to be 0. In this case, when CPU1
> disables this perf event, it executes __perf_remove_from_context->
> list _del_event->perf_cgroup_event_disable on CPU1, which causes a race
> with perf_cgroup_switch running on CPU0.
> 
> The following describes the details of this concurrency scenario:
> 
> CPU0						CPU1
> 
> perf_cgroup_switch:
>    ...
>    # cpuctx->cgrp is not NULL here
>    if (READ_ONCE(cpuctx->cgrp) == NULL)
>    	return;
> 
> 						perf_remove_from_context:
> 						   ...
> 						   raw_spin_lock_irq(&ctx->lock);
> 						   ...
> 						   # ctx->is_active == 0 because reprogramm is not
> 						   # tigger, so CPU1 can do __perf_remove_from_context
> 						   # for CPU0
> 						   __perf_remove_from_context:
> 						         perf_cgroup_event_disable:
> 							    ...
> 							    if (--ctx->nr_cgroups)
> 							    ...
> 
>    # this warning will happened because CPU1 changed
>    # ctx.nr_cgroups to 0.
>    WARN_ON_ONCE(cpuctx->ctx.nr_cgroups == 0);
> 
> To fix this problem, expand the lock-holding critical section in
> perf_cgroup_switch.
> 
> Fixes: db4a835601b7 ("perf/core: Set cgroup in CPU contexts for new cgroup events")
> Signed-off-by: Luo Gengkun <luogengkun@huaweicloud.com>
> ---

Right, so how about we simply re-check the condition once we take the
lock?

Also, take the opportunity to convert to guard instead of adding goto
unlock.

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -207,6 +207,19 @@ static void perf_ctx_unlock(struct perf_
 	__perf_ctx_unlock(&cpuctx->ctx);
 }
 
+typedef struct {
+	struct perf_cpu_context *cpuctx;
+	struct perf_event_context *ctx;
+} class_perf_ctx_lock_t;
+
+static inline void class_perf_ctx_lock_destructor(class_perf_ctx_lock_t *_T)
+{ perf_ctx_unlock(_T->cpuctx, _T->ctx); }
+
+static inline class_perf_ctx_lock_t
+class_perf_ctx_lock_constructor(struct perf_cpu_context *cpuctx,
+				struct perf_event_context *ctx)
+{ perf_ctx_lock(cpuctx, ctx); return (class_perf_ctx_lock_t){ cpuctx, ctx }; }
+
 #define TASK_TOMBSTONE ((void *)-1L)
 
 static bool is_kernel_event(struct perf_event *event)
@@ -944,7 +957,13 @@ static void perf_cgroup_switch(struct ta
 	if (READ_ONCE(cpuctx->cgrp) == cgrp)
 		return;
 
-	perf_ctx_lock(cpuctx, cpuctx->task_ctx);
+	guard(perf_ctx_lock)(cpuctx, cpuctx->task_ctx);
+	/*
+	 * Re-check, could've raced vs perf_remove_from_context().
+	 */
+	if (READ_ONCE(cpuctx->cgrp) == NULL)
+		return;
+
 	perf_ctx_disable(&cpuctx->ctx, true);
 
 	ctx_sched_out(&cpuctx->ctx, NULL, EVENT_ALL|EVENT_CGROUP);
@@ -962,7 +981,6 @@ static void perf_cgroup_switch(struct ta
 	ctx_sched_in(&cpuctx->ctx, NULL, EVENT_ALL|EVENT_CGROUP);
 
 	perf_ctx_enable(&cpuctx->ctx, true);
-	perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
 }
 
 static int perf_cgroup_ensure_storage(struct perf_event *event,

Re: [PATCH v2 2/2] perf/core: Fix WARN_ON_ONCE(cpuctx->ctx.nr_cgroups == 0) in perf_cgroup_switch

[Luo Gengkun]

On 2025/6/4 18:00, Peter Zijlstra wrote:
> On Wed, Jun 04, 2025 at 03:39:24AM +0000, Luo Gengkun wrote:
>> There may be concurrency between perf_cgroup_switch and
>> perf_cgroup_event_disable. Consider the following scenario: after a new
>> perf cgroup event is created on CPU0, the new event may not trigger
>> a reprogramming, causing ctx->is_active to be 0. In this case, when CPU1
>> disables this perf event, it executes __perf_remove_from_context->
>> list _del_event->perf_cgroup_event_disable on CPU1, which causes a race
>> with perf_cgroup_switch running on CPU0.
>>
>> The following describes the details of this concurrency scenario:
>>
>> CPU0						CPU1
>>
>> perf_cgroup_switch:
>>     ...
>>     # cpuctx->cgrp is not NULL here
>>     if (READ_ONCE(cpuctx->cgrp) == NULL)
>>     	return;
>>
>> 						perf_remove_from_context:
>> 						   ...
>> 						   raw_spin_lock_irq(&ctx->lock);
>> 						   ...
>> 						   # ctx->is_active == 0 because reprogramm is not
>> 						   # tigger, so CPU1 can do __perf_remove_from_context
>> 						   # for CPU0
>> 						   __perf_remove_from_context:
>> 						         perf_cgroup_event_disable:
>> 							    ...
>> 							    if (--ctx->nr_cgroups)
>> 							    ...
>>
>>     # this warning will happened because CPU1 changed
>>     # ctx.nr_cgroups to 0.
>>     WARN_ON_ONCE(cpuctx->ctx.nr_cgroups == 0);
>>
>> To fix this problem, expand the lock-holding critical section in
>> perf_cgroup_switch.
>>
>> Fixes: db4a835601b7 ("perf/core: Set cgroup in CPU contexts for new cgroup events")
>> Signed-off-by: Luo Gengkun <luogengkun@huaweicloud.com>
>> ---
> Right, so how about we simply re-check the condition once we take the
> lock?
>
> Also, take the opportunity to convert to guard instead of adding goto
> unlock.
>
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -207,6 +207,19 @@ static void perf_ctx_unlock(struct perf_
>   	__perf_ctx_unlock(&cpuctx->ctx);
>   }
>   
> +typedef struct {
> +	struct perf_cpu_context *cpuctx;
> +	struct perf_event_context *ctx;
> +} class_perf_ctx_lock_t;
> +
> +static inline void class_perf_ctx_lock_destructor(class_perf_ctx_lock_t *_T)
> +{ perf_ctx_unlock(_T->cpuctx, _T->ctx); }
> +
> +static inline class_perf_ctx_lock_t
> +class_perf_ctx_lock_constructor(struct perf_cpu_context *cpuctx,
> +				struct perf_event_context *ctx)
> +{ perf_ctx_lock(cpuctx, ctx); return (class_perf_ctx_lock_t){ cpuctx, ctx }; }
> +
>   #define TASK_TOMBSTONE ((void *)-1L)
>   
>   static bool is_kernel_event(struct perf_event *event)
> @@ -944,7 +957,13 @@ static void perf_cgroup_switch(struct ta
>   	if (READ_ONCE(cpuctx->cgrp) == cgrp)
>   		return;
>   
> -	perf_ctx_lock(cpuctx, cpuctx->task_ctx);
> +	guard(perf_ctx_lock)(cpuctx, cpuctx->task_ctx);
> +	/*
> +	 * Re-check, could've raced vs perf_remove_from_context().
> +	 */
> +	if (READ_ONCE(cpuctx->cgrp) == NULL)
> +		return;
> +
>   	perf_ctx_disable(&cpuctx->ctx, true);
>   
>   	ctx_sched_out(&cpuctx->ctx, NULL, EVENT_ALL|EVENT_CGROUP);
> @@ -962,7 +981,6 @@ static void perf_cgroup_switch(struct ta
>   	ctx_sched_in(&cpuctx->ctx, NULL, EVENT_ALL|EVENT_CGROUP);
>   
>   	perf_ctx_enable(&cpuctx->ctx, true);
> -	perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
>   }
>   
>   static int perf_cgroup_ensure_storage(struct perf_event *event,

Thank for your review, I will make changes based on your suggestions.

Re: [PATCH v2 2/2] perf/core: Fix WARN_ON_ONCE(cpuctx->ctx.nr_cgroups == 0) in perf_cgroup_switch

[Peter Zijlstra]

On Thu, Jun 05, 2025 at 11:55:03AM +0800, Luo Gengkun wrote:
> 
> On 2025/6/4 18:00, Peter Zijlstra wrote:
> > On Wed, Jun 04, 2025 at 03:39:24AM +0000, Luo Gengkun wrote:
> > > There may be concurrency between perf_cgroup_switch and
> > > perf_cgroup_event_disable. Consider the following scenario: after a new
> > > perf cgroup event is created on CPU0, the new event may not trigger
> > > a reprogramming, causing ctx->is_active to be 0. In this case, when CPU1
> > > disables this perf event, it executes __perf_remove_from_context->
> > > list _del_event->perf_cgroup_event_disable on CPU1, which causes a race
> > > with perf_cgroup_switch running on CPU0.
> > > 
> > > The following describes the details of this concurrency scenario:
> > > 
> > > CPU0						CPU1
> > > 
> > > perf_cgroup_switch:
> > >     ...
> > >     # cpuctx->cgrp is not NULL here
> > >     if (READ_ONCE(cpuctx->cgrp) == NULL)
> > >     	return;
> > > 
> > > 						perf_remove_from_context:
> > > 						   ...
> > > 						   raw_spin_lock_irq(&ctx->lock);
> > > 						   ...
> > > 						   # ctx->is_active == 0 because reprogramm is not
> > > 						   # tigger, so CPU1 can do __perf_remove_from_context
> > > 						   # for CPU0
> > > 						   __perf_remove_from_context:
> > > 						         perf_cgroup_event_disable:
> > > 							    ...
> > > 							    if (--ctx->nr_cgroups)
> > > 							    ...
> > > 
> > >     # this warning will happened because CPU1 changed
> > >     # ctx.nr_cgroups to 0.
> > >     WARN_ON_ONCE(cpuctx->ctx.nr_cgroups == 0);
> > > 
> > > To fix this problem, expand the lock-holding critical section in
> > > perf_cgroup_switch.
> > > 
> > > Fixes: db4a835601b7 ("perf/core: Set cgroup in CPU contexts for new cgroup events")
> > > Signed-off-by: Luo Gengkun <luogengkun@huaweicloud.com>
> > > ---
> > Right, so how about we simply re-check the condition once we take the
> > lock?
> > 
> > Also, take the opportunity to convert to guard instead of adding goto
> > unlock.
> > 
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -207,6 +207,19 @@ static void perf_ctx_unlock(struct perf_
> >   	__perf_ctx_unlock(&cpuctx->ctx);
> >   }
> > +typedef struct {
> > +	struct perf_cpu_context *cpuctx;
> > +	struct perf_event_context *ctx;
> > +} class_perf_ctx_lock_t;
> > +
> > +static inline void class_perf_ctx_lock_destructor(class_perf_ctx_lock_t *_T)
> > +{ perf_ctx_unlock(_T->cpuctx, _T->ctx); }
> > +
> > +static inline class_perf_ctx_lock_t
> > +class_perf_ctx_lock_constructor(struct perf_cpu_context *cpuctx,
> > +				struct perf_event_context *ctx)
> > +{ perf_ctx_lock(cpuctx, ctx); return (class_perf_ctx_lock_t){ cpuctx, ctx }; }
> > +
> >   #define TASK_TOMBSTONE ((void *)-1L)
> >   static bool is_kernel_event(struct perf_event *event)
> > @@ -944,7 +957,13 @@ static void perf_cgroup_switch(struct ta
> >   	if (READ_ONCE(cpuctx->cgrp) == cgrp)
> >   		return;
> > -	perf_ctx_lock(cpuctx, cpuctx->task_ctx);
> > +	guard(perf_ctx_lock)(cpuctx, cpuctx->task_ctx);
> > +	/*
> > +	 * Re-check, could've raced vs perf_remove_from_context().
> > +	 */
> > +	if (READ_ONCE(cpuctx->cgrp) == NULL)
> > +		return;
> > +
> >   	perf_ctx_disable(&cpuctx->ctx, true);
> >   	ctx_sched_out(&cpuctx->ctx, NULL, EVENT_ALL|EVENT_CGROUP);
> > @@ -962,7 +981,6 @@ static void perf_cgroup_switch(struct ta
> >   	ctx_sched_in(&cpuctx->ctx, NULL, EVENT_ALL|EVENT_CGROUP);
> >   	perf_ctx_enable(&cpuctx->ctx, true);
> > -	perf_ctx_unlock(cpuctx, cpuctx->task_ctx);
> >   }
> >   static int perf_cgroup_ensure_storage(struct perf_event *event,
> 
> Thank for your review, I will make changes based on your suggestions.
> 

No need to resend. I've got your patch with modifications. But please
confirm it does work :-)