[PATCH] memcg: clear FLUSHING_CACHED_CHARGE on cpu offline

[PATCH] memcg: clear FLUSHING_CACHED_CHARGE on cpu offline

[Shakeel Butt]

Sashiko [1] reported that memcg_hotplug_cpu_dead() drains the stocks
of the CPU which went away but leaves FLUSHING_CACHED_CHARGE alone.

The flag can be set at that point: drain_all_stock() may have claimed
the stock and queued the drain work shortly before the CPU went down.
workqueue_offline_cpu() unbinds the per-cpu workers, so such a pending
work item is executed by an unbound worker on some other CPU, where
drain_local_memcg_stock() operates on this_cpu_ptr() and thus drains
and clears the flag of that other CPU instead.  Nothing clears the flag
of the dead CPU, so drain_all_stock() would skip its stock forever once
the CPU comes back online.

Clear the flag of both stocks after draining them.

Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260828135036.7d44361f%40fangorn [1]
---
 mm/memcontrol.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index e082aa68fa5c..872115c6b0f2 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2378,9 +2378,21 @@ void drain_all_stock(struct mem_cgroup *root_memcg)
 
 static int memcg_hotplug_cpu_dead(unsigned int cpu)
 {
+	struct memcg_stock_pcp *memcg_st = &per_cpu(memcg_stock, cpu);
+	struct obj_stock_pcp *obj_st = &per_cpu(obj_stock, cpu);
+
 	/* no need for the local lock */
-	drain_obj_stock(&per_cpu(obj_stock, cpu));
-	drain_stock_fully(&per_cpu(memcg_stock, cpu));
+	drain_obj_stock(obj_st);
+	drain_stock_fully(memcg_st);
+
+	/*
+	 * A drain work queued before the CPU went away is executed by an
+	 * unbound worker on some other CPU and clears that CPU's flag, so
+	 * clear the flags here to make these stocks drainable again once
+	 * the CPU comes back online.
+	 */
+	clear_bit(FLUSHING_CACHED_CHARGE, &memcg_st->flags);
+	clear_bit(FLUSHING_CACHED_CHARGE, &obj_st->flags);
 
 	return 0;
 }
-- 
2.53.0-Meta

Re: [PATCH] memcg: clear FLUSHING_CACHED_CHARGE on cpu offline

[Rik van Riel]

On Fri, 2026-08-28 at 12:24 -0700, Shakeel Butt wrote:
> Sashiko [1] reported that memcg_hotplug_cpu_dead() drains the stocks
> of the CPU which went away but leaves FLUSHING_CACHED_CHARGE alone.
> 
> The flag can be set at that point: drain_all_stock() may have claimed
> the stock and queued the drain work shortly before the CPU went down.
> workqueue_offline_cpu() unbinds the per-cpu workers, so such a
> pending
> work item is executed by an unbound worker on some other CPU, where
> drain_local_memcg_stock() operates on this_cpu_ptr() and thus drains
> and clears the flag of that other CPU instead.  Nothing clears the
> flag
> of the dead CPU, so drain_all_stock() would skip its stock forever
> once
> the CPU comes back online.
> 
> Clear the flag of both stocks after draining them.
> 
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link:
> https://sashiko.dev/#/patchset/20260828135036.7d44361f%40fangorn [1]
> ---
> 
Funny, my AI came up with the same solution, and
I just asked it to do a safety analysis on it.

It was quite thorough.

Reviewed-by: Rik van Riel <riel@surriel.com>


-- 
All Rights Reversed.

Re: [PATCH] memcg: clear FLUSHING_CACHED_CHARGE on cpu offline

[Michal Hocko]

On Fri 28-08-26 12:24:19, Shakeel Butt wrote:
> Sashiko [1] reported that memcg_hotplug_cpu_dead() drains the stocks
> of the CPU which went away but leaves FLUSHING_CACHED_CHARGE alone.
> 
> The flag can be set at that point: drain_all_stock() may have claimed
> the stock and queued the drain work shortly before the CPU went down.
> workqueue_offline_cpu() unbinds the per-cpu workers, so such a pending
> work item is executed by an unbound worker on some other CPU, where
> drain_local_memcg_stock() operates on this_cpu_ptr() and thus drains
> and clears the flag of that other CPU instead.  Nothing clears the flag
> of the dead CPU, so drain_all_stock() would skip its stock forever once
> the CPU comes back online.
> 
> Clear the flag of both stocks after draining them.
> 
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Link: https://sashiko.dev/#/patchset/20260828135036.7d44361f%40fangorn [1]

I suspect this goes all the way down to
Fixes: 26fe61684449 ("memcg: fix percpu cached charge draining frequency")

Acked-by: Michal Hocko <mhocko@suse.com>

Btw. is there any good reason why we are not clearing the bit directly
in drain_obj_stock resp. drain_stock_fully. Doing so would simplify the
code flow and prevent from bugs like this one.

> ---
>  mm/memcontrol.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index e082aa68fa5c..872115c6b0f2 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2378,9 +2378,21 @@ void drain_all_stock(struct mem_cgroup *root_memcg)
>  
>  static int memcg_hotplug_cpu_dead(unsigned int cpu)
>  {
> +	struct memcg_stock_pcp *memcg_st = &per_cpu(memcg_stock, cpu);
> +	struct obj_stock_pcp *obj_st = &per_cpu(obj_stock, cpu);
> +
>  	/* no need for the local lock */
> -	drain_obj_stock(&per_cpu(obj_stock, cpu));
> -	drain_stock_fully(&per_cpu(memcg_stock, cpu));
> +	drain_obj_stock(obj_st);
> +	drain_stock_fully(memcg_st);
> +
> +	/*
> +	 * A drain work queued before the CPU went away is executed by an
> +	 * unbound worker on some other CPU and clears that CPU's flag, so
> +	 * clear the flags here to make these stocks drainable again once
> +	 * the CPU comes back online.
> +	 */
> +	clear_bit(FLUSHING_CACHED_CHARGE, &memcg_st->flags);
> +	clear_bit(FLUSHING_CACHED_CHARGE, &obj_st->flags);
>  
>  	return 0;
>  }
> -- 
> 2.53.0-Meta

-- 
Michal Hocko
SUSE Labs