drain_all_stock() can leave FLUSHING_CACHED_CHARGE set after the
work is dropped.  It sets the bit before checking isolation and
schedule_drain_work() checks isolation and queues in a separate RCU
critical section, so housekeeping_update()'s synchronize_rcu() can
race the second check.

drain_local_stock() only clears the bit for work that ran, so the bit
remains set and the stock is never drained again.

Reorganize the drain_all_stock() loop, reducing nesting, splitting
out local vs remote cpu handling, and skipping everything on isolated
cpus, which solves the stuck FLUSHING_CACHED_CHARGE flag.

Fixes: 6a792697a53a ("memcg: do not drain charge pcp caches on remote isolated cpus")
Cc: stable@vger.kernel.org
Assisted-by: Hermes:muse-spark-1.2
Signed-off-by: Rik van Riel <riel@surriel.com>

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index a660ea0f820b..b0d8ec042d48 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2330,19 +2330,6 @@ static bool obj_stock_should_drain(struct obj_stock_pcp *stock,
 	return true;
 }
 
-static void schedule_drain_work(int cpu, struct work_struct *work)
-{
-	/*
-	 * Protect housekeeping cpumask read and work enqueue together
-	 * in the same RCU critical section so that later cpuset isolated
-	 * partition update only need to wait for an RCU GP and flush the
-	 * pending work on newly isolated CPUs.
-	 */
-	guard(rcu)();
-	if (!cpu_is_isolated(cpu))
-		queue_work_on(cpu, memcg_wq, work);
-}
-
 /*
  * Drains all per-CPU charge caches for given root_memcg resp. subtree
  * of the hierarchy under it.
@@ -2366,19 +2353,23 @@ void drain_all_stock(struct mem_cgroup *root_memcg)
 		struct memcg_stock_pcp *memcg_st = &per_cpu(memcg_stock, cpu);
 		struct obj_stock_pcp *obj_st = &per_cpu(obj_stock, cpu);
 
-		if (memcg_stock_should_drain(memcg_st, root_memcg)) {
-			if (cpu == curcpu)
+		if (cpu == curcpu) {
+			if (memcg_stock_should_drain(memcg_st, root_memcg))
 				drain_local_memcg_stock(&memcg_st->work);
-			else
-				schedule_drain_work(cpu, &memcg_st->work);
-		}
-
-		if (obj_stock_should_drain(obj_st, root_memcg)) {
-			if (cpu == curcpu)
+			if (obj_stock_should_drain(obj_st, root_memcg))
 				drain_local_obj_stock(&obj_st->work);
-			else
-				schedule_drain_work(cpu, &obj_st->work);
+			continue;
 		}
+
+		/* Pairs with RCU barrier in housekeeping_update(). */
+		guard(rcu)();
+		if (cpu_is_isolated(cpu))
+			continue;
+
+		if (memcg_stock_should_drain(memcg_st, root_memcg))
+			queue_work_on(cpu, memcg_wq, &memcg_st->work);
+		if (obj_stock_should_drain(obj_st, root_memcg))
+			queue_work_on(cpu, memcg_wq, &obj_st->work);
 	}
 	migrate_enable();
 	mutex_unlock(&percpu_charge_mutex);

On Fri 28-08-26 09:46:21, Rik van Riel wrote:
> drain_all_stock() can leave FLUSHING_CACHED_CHARGE set after the
> work is dropped.  It sets the bit before checking isolation and
> schedule_drain_work() checks isolation and queues in a separate RCU
> critical section, so housekeeping_update()'s synchronize_rcu() can
> race the second check.
> 
> drain_local_stock() only clears the bit for work that ran, so the bit
> remains set and the stock is never drained again.
> 
> Reorganize the drain_all_stock() loop, reducing nesting, splitting
> out local vs remote cpu handling, and skipping everything on isolated
> cpus, which solves the stuck FLUSHING_CACHED_CHARGE flag.

Is there any reason why we cannot simply clear the flag if the work is
not scheduled?
---

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 69b37f63a307..907ac63e5067 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2261,8 +2261,10 @@ static bool is_memcg_drain_needed(struct memcg_stock_pcp *stock,
 	return flush;
 }
 
-static void schedule_drain_work(int cpu, struct work_struct *work)
+static bool schedule_drain_work(int cpu, struct work_struct *work)
 {
+	int ret = false;
+
 	/*
 	 * Protect housekeeping cpumask read and work enqueue together
 	 * in the same RCU critical section so that later cpuset isolated
@@ -2270,8 +2272,12 @@ static void schedule_drain_work(int cpu, struct work_struct *work)
 	 * pending work on newly isolated CPUs.
 	 */
 	guard(rcu)();
-	if (!cpu_is_isolated(cpu))
-		queue_work_on(cpu, memcg_wq, work);
+	if (!cpu_is_isolated(cpu)) {
+		queue_work_on(cpu, memcg_wq, &memcg_st->work);
+		ret = true;
+	}
+
+	return ret;
 }
 
 /*
@@ -2303,8 +2309,8 @@ void drain_all_stock(struct mem_cgroup *root_memcg)
 				      &memcg_st->flags)) {
 			if (cpu == curcpu)
 				drain_local_memcg_stock(&memcg_st->work);
-			else
-				schedule_drain_work(cpu, &memcg_st->work);
+			else if (!schedule_drain_work(cpu, &memcg_st->work))
+				clear_bit(FLUSHING_CACHED_CHARGE, &memcg_st->flags)
 		}
 
 		if (!test_bit(FLUSHING_CACHED_CHARGE, &obj_st->flags) &&
@@ -2313,8 +2319,8 @@ void drain_all_stock(struct mem_cgroup *root_memcg)
 				      &obj_st->flags)) {
 			if (cpu == curcpu)
 				drain_local_obj_stock(&obj_st->work);
-			else
-				schedule_drain_work(cpu, &obj_st->work);
+			else if (!schedule_drain_work(cpu, &obj_st->work))
+				clear_bit(FLUSHING_CACHED_CHARGE, &obj_st->flags);
 		}
 	}
 	migrate_enable();
-- 
Michal Hocko
SUSE Labs

On Fri, Aug 28, 2026 at 04:25:41PM +0200, Michal Hocko wrote:
> On Fri 28-08-26 09:46:21, Rik van Riel wrote:
> > drain_all_stock() can leave FLUSHING_CACHED_CHARGE set after the
> > work is dropped.  It sets the bit before checking isolation and
> > schedule_drain_work() checks isolation and queues in a separate RCU
> > critical section, so housekeeping_update()'s synchronize_rcu() can
> > race the second check.
> > 
> > drain_local_stock() only clears the bit for work that ran, so the bit
> > remains set and the stock is never drained again.
> > 
> > Reorganize the drain_all_stock() loop, reducing nesting, splitting
> > out local vs remote cpu handling, and skipping everything on isolated
> > cpus, which solves the stuck FLUSHING_CACHED_CHARGE flag.
> 
> Is there any reason why we cannot simply clear the flag if the work is
> not scheduled?
> ---
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 69b37f63a307..907ac63e5067 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2261,8 +2261,10 @@ static bool is_memcg_drain_needed(struct memcg_stock_pcp *stock,
>  	return flush;
>  }
>  
> -static void schedule_drain_work(int cpu, struct work_struct *work)
> +static bool schedule_drain_work(int cpu, struct work_struct *work)
>  {
> +	int ret = false;
> +
>  	/*
>  	 * Protect housekeeping cpumask read and work enqueue together
>  	 * in the same RCU critical section so that later cpuset isolated
> @@ -2270,8 +2272,12 @@ static void schedule_drain_work(int cpu, struct work_struct *work)
>  	 * pending work on newly isolated CPUs.
>  	 */
>  	guard(rcu)();
> -	if (!cpu_is_isolated(cpu))
> -		queue_work_on(cpu, memcg_wq, work);
> +	if (!cpu_is_isolated(cpu)) {
> +		queue_work_on(cpu, memcg_wq, &memcg_st->work);
> +		ret = true;
> +	}
> +
> +	return ret;

Let's go with this patch. We can simplify above by inversing the check:

	if (cpu_is_isolated(cpu))
		return false;
	
	queue_work_on(cpu, memcg_wq, &memcg_st->work);
	return true;

>  }
>  
>  /*
> @@ -2303,8 +2309,8 @@ void drain_all_stock(struct mem_cgroup *root_memcg)
>  				      &memcg_st->flags)) {
>  			if (cpu == curcpu)
>  				drain_local_memcg_stock(&memcg_st->work);
> -			else
> -				schedule_drain_work(cpu, &memcg_st->work);
> +			else if (!schedule_drain_work(cpu, &memcg_st->work))
> +				clear_bit(FLUSHING_CACHED_CHARGE, &memcg_st->flags)
>  		}
>  
>  		if (!test_bit(FLUSHING_CACHED_CHARGE, &obj_st->flags) &&
> @@ -2313,8 +2319,8 @@ void drain_all_stock(struct mem_cgroup *root_memcg)
>  				      &obj_st->flags)) {
>  			if (cpu == curcpu)
>  				drain_local_obj_stock(&obj_st->work);
> -			else
> -				schedule_drain_work(cpu, &obj_st->work);
> +			else if (!schedule_drain_work(cpu, &obj_st->work))
> +				clear_bit(FLUSHING_CACHED_CHARGE, &obj_st->flags);
>  		}
>  	}
>  	migrate_enable();
> -- 
> Michal Hocko
> SUSE Labs

On Fri, 2026-08-28 at 08:41 -0700, Shakeel Butt wrote:
> 
> Let's go with this patch. We can simplify above by inversing the
> check:
> 
> 	if (cpu_is_isolated(cpu))
> 		return false;
> 
At that point, we could also clear the bit here, and
have the function return void again, so there's no
duplication of that check and clear in drain_all_stock()
> 	
> 	queue_work_on(cpu, memcg_wq, &memcg_st->work);
> 	return true;

Let me send that.

-- 
All Rights Reversed.

On Fri, 2026-08-28 at 16:25 +0200, Michal Hocko wrote:
> On Fri 28-08-26 09:46:21, Rik van Riel wrote:
> > drain_all_stock() can leave FLUSHING_CACHED_CHARGE set after the
> > work is dropped.  It sets the bit before checking isolation and
> > schedule_drain_work() checks isolation and queues in a separate RCU
> > critical section, so housekeeping_update()'s synchronize_rcu() can
> > race the second check.
> > 
> > drain_local_stock() only clears the bit for work that ran, so the
> > bit
> > remains set and the stock is never drained again.
> > 
> > Reorganize the drain_all_stock() loop, reducing nesting, splitting
> > out local vs remote cpu handling, and skipping everything on
> > isolated
> > cpus, which solves the stuck FLUSHING_CACHED_CHARGE flag.
> 
> Is there any reason why we cannot simply clear the flag if the work
> is
> not scheduled?
> 
That should work, too.

I'm fine with whatever variant people prefer.

-- 
All Rights Reversed.