Skip to contentSkip to conversation

mm/memcontrol: fix stuck FLUSHING_CACHED_CHARGE bit on isolated cpus

PATCH Applied Rik van Riel 4 participants 7 messages Aug 28 – Aug 31

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 mm/memcontrol: fix stuck FLUSHING_CACHED_CHARGE bit on isolated cpus

When drain_all_stock() sets FLUSHING_CACHED_CHARGE before checking isolation, schedule_drain_work() can drop the work in a separate RCU critical section, and housekeeping_update()'s synchronize_rcu() can race that second check, leaving the flag set.

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

Have schedule_drain_work() return whether the work was queued, and clear FLUSHING_CACHED_CHARGE in drain_all_stock() when the remote CPU is isolated, so future drains can retry.

Fixes 6a792697a53a ("memcg: do not drain charge pcp caches on remote isolated cpus")

---
v2: use the approach suggested by Michal and Shakeel

 mm/memcontrol.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
mm/memcontrol.c +12 -7
index 1271d390b617..309398e943ca 100644
@@ -2306,7 +2306,7 @@ 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)
 {
 	/*
 	 * Protect housekeeping cpumask read and work enqueue together
@@ -2315,8 +2315,11 @@ 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))
+		return false;
+
+	queue_work_on(cpu, memcg_wq, work);
+	return true;
 }
 
 /*
@@ -2348,8 +2351,9 @@ 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) &&
@@ -2358,8 +2362,9 @@ 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();
Signature · 1 line
2.55.0
maintainer Acked-by on v2 1/1
#1 lore raw
Quoted 12 lines · Rik van Riel

When drain_all_stock() sets FLUSHING_CACHED_CHARGE before checking isolation, schedule_drain_work() can drop the work in a separate RCU critical section, and housekeeping_update()'s synchronize_rcu() can race that second check, leaving the flag set.

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

Have schedule_drain_work() return whether the work was queued, and clear FLUSHING_CACHED_CHARGE in drain_all_stock() when the remote CPU is isolated, so future drains can retry.

Quoted 5 lines
Fixes 6a792697a53a ("memcg: do not drain charge pcp caches on remote isolated cpus")
maintainer
#2 lore raw
In reply to Rik van Riel
Quoted 11 lines · On Fri, 28 Aug 2026 13:50:36 -0400 Rik van Riel <[email protected]> wrote:

When drain_all_stock() sets FLUSHING_CACHED_CHARGE before checking isolation, schedule_drain_work() can drop the work in a separate RCU critical section, and housekeeping_update()'s synchronize_rcu() can race that second check, leaving the flag set.

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

Have schedule_drain_work() return whether the work was queued, and clear FLUSHING_CACHED_CHARGE in drain_all_stock() when the remote CPU is isolated, so future drains can retry.

Is there some Reported-by: or reproducer for this?

Given the complexity of the reproducers which Gemini developed for me, I'm suspecting "nope".

Quoted 2 lines
Fixes 6a792697a53a ("memcg: do not drain charge pcp caches on remote isolated cpus")

Why is a backport being proposed? How does that benefit those we serve?

Sashiko suggests that we ain't done yet:

	https://sashiko.dev/#/patchset/20260828135036.7d44361f@fangorn
maintainer
#3 lore raw
Quoted 19 lines · Andrew Morton
Quoted 11 lines · On Fri, 28 Aug 2026 13:50:36 -0400 Rik van Riel <[email protected]> wrote:

When drain_all_stock() sets FLUSHING_CACHED_CHARGE before checking isolation, schedule_drain_work() can drop the work in a separate RCU critical section, and housekeeping_update()'s synchronize_rcu() can race that second check, leaving the flag set.

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

Have schedule_drain_work() return whether the work was queued, and clear FLUSHING_CACHED_CHARGE in drain_all_stock() when the remote CPU is isolated, so future drains can retry.

Is there some Reported-by: or reproducer for this?

Given the complexity of the reproducers which Gemini developed for me, I'm suspecting "nope".

Quoted 8 lines
Quoted 2 lines
Fixes 6a792697a53a ("memcg: do not drain charge pcp caches on remote isolated cpus")

Why is a backport being proposed? How does that benefit those we serve?

Sashiko suggests that we ain't done yet:

	https://sashiko.dev/#/patchset/20260828135036.7d44361f@fangorn

Haha we are never done. Just sent the fix for this new report:

http://lore.kernel.org/[email protected]

maintainer
#4 lore raw
Quoted 26 lines · On Fri, 28 Aug 2026 12:26:15 -0700 Shakeel Butt <[email protected]> wrote:
Quoted 25 lines · Andrew Morton
Quoted 11 lines · On Fri, 28 Aug 2026 13:50:36 -0400 Rik van Riel <[email protected]> wrote:

When drain_all_stock() sets FLUSHING_CACHED_CHARGE before checking isolation, schedule_drain_work() can drop the work in a separate RCU critical section, and housekeeping_update()'s synchronize_rcu() can race that second check, leaving the flag set.

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

Have schedule_drain_work() return whether the work was queued, and clear FLUSHING_CACHED_CHARGE in drain_all_stock() when the remote CPU is isolated, so future drains can retry.

Is there some Reported-by: or reproducer for this?

Given the complexity of the reproducers which Gemini developed for me, I'm suspecting "nope".

Quoted 2 lines
Fixes 6a792697a53a ("memcg: do not drain charge pcp caches on remote isolated cpus")

Why is a backport being proposed? How does that benefit those we serve?

Quoted 6 lines
Quoted 2 lines

Sashiko suggests that we ain't done yet:

	https://sashiko.dev/#/patchset/20260828135036.7d44361f@fangorn

Haha we are never done. Just sent the fix for this new report:

http://lore.kernel.org/[email protected]

Cool.

And... guess what?

	https://sashiko.dev/#/patchset/[email protected]
Acked-by on v2 1/1
#6 lore raw
In reply to Rik van Riel
Quoted 12 lines · Rik van Riel

When drain_all_stock() sets FLUSHING_CACHED_CHARGE before checking isolation, schedule_drain_work() can drop the work in a separate RCU critical section, and housekeeping_update()'s synchronize_rcu() can race that second check, leaving the flag set.

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

Have schedule_drain_work() return whether the work was queued, and clear FLUSHING_CACHED_CHARGE in drain_all_stock() when the remote CPU is isolated, so future drains can retry.

Quoted 5 lines
Fixes 6a792697a53a ("memcg: do not drain charge pcp caches on remote isolated cpus")

Thanks

Quoted 63 lines

---
v2: use the approach suggested by Michal and Shakeel

 mm/memcontrol.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)
mm/memcontrol.c +12 -7
index 1271d390b617..309398e943ca 100644
@@ -2306,7 +2306,7 @@ 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)
 {
 	/*
 	 * Protect housekeeping cpumask read and work enqueue together
@@ -2315,8 +2315,11 @@ 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))
+		return false;
+
+	queue_work_on(cpu, memcg_wq, work);
+	return true;
 }
 
 /*
@@ -2348,8 +2351,9 @@ 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) &&
@@ -2358,8 +2362,9 @@ 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();
Signature · 1 line
2.55.0
Signature · 2 lines
Michal Hocko
SUSE Labs