memcg: clear FLUSHING_CACHED_CHARGE on cpu offline
Subsystems (2)
Shakeel Butt
maintainerPATCH 1/1 memcg: clear FLUSHING_CACHED_CHARGE on cpu offline
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.
--- mm/memcontrol.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
mm/memcontrol.c +14 -2
@@ -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 ; }
Signature · 1 line
2.53.0-Meta
Quoted 15 lines · Shakeel Butt
Sashiko [1] reported that
memcg_hotplug_cpu_dead()drains the stocks of the CPU which went away but leavesFLUSHING_CACHED_CHARGEalone.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, wheredrain_local_memcg_stock()operates onthis_cpu_ptr()and thus drains and clears the flag of that other CPU instead. Nothing clears the flag
of the dead CPU, sodrain_all_stock()would skip its stock forever once
the CPU comes back online.
Quoted 8 lines
Clear the flag of both stocks after draining them.
Signed-off-by Shakeel Butt <[email protected]>Reported-by Sashiko <[email protected]>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.
Signature · 1 line
All Rights Reversed.
Quoted 12 lines · Shakeel Butt
Sashiko [1] reported that
memcg_hotplug_cpu_dead()drains the stocks of the CPU which went away but leavesFLUSHING_CACHED_CHARGEalone.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, wheredrain_local_memcg_stock()operates onthis_cpu_ptr()and thus drains and clears the flag of that other CPU instead. Nothing clears the flag of the dead CPU, sodrain_all_stock()would skip its stock forever once the CPU comes back online.
Quoted 5 lines
Clear the flag of both stocks after draining them.
Signed-off-by Shakeel Butt <[email protected]>Reported-by Sashiko <[email protected]>
I suspect this goes all the way down to
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.
Quoted 34 lines
--- mm/memcontrol.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)mm/memcontrol.c +14
-2@@ -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 ; }Signature · 1 line
2.53.0-Meta
Signature · 2 lines
Michal Hocko SUSE Labs