memcg: drain obj stock on cpu hotplug teardown
Subsystems (4)
Shakeel Butt
maintainerPATCH 1/1 memcg: drain obj stock on cpu hotplug teardown
Currently on cpu hotplug teardown, only memcg stock is drained but we need to drain the obj stock as well otherwise we will miss the stats accumulated on the target cpu as well as the nr_bytes cached. The stats include MEMCG_KMEM, NR_SLAB_RECLAIMABLE_B & NR_SLAB_UNRECLAIMABLE_B. In addition we are leaking reference to struct obj_cgroup object.
--- mm/memcontrol.c | 9 +++++++++ 1 file changed, 9 insertions(+)
mm/memcontrol.c +9 -0
@@ -1921,9 +1921,18 @@ void drain_all_stock(struct mem_cgroup *root_memcg)static int memcg_hotplug_cpu_dead (unsigned int cpu) {struct memcg_stock_pcp *stock; +struct obj_cgroup *old; +unsigned long flags; stock = &per_cpu (memcg_stock, cpu); + +/* drain_obj_stock requires stock_lock */ +local_lock_irqsave (&memcg_stock.stock_lock, flags); + old =drain_obj_stock (stock); +local_unlock_irqrestore (&memcg_stock.stock_lock, flags); +drain_stock (stock); +obj_cgroup_put (old);return 0 ; }
Signature · 1 line
2.47.1
Johannes Weiner
maintainerQuoted 9 lines · Shakeel Butt
Currently on cpu hotplug teardown, only memcg stock is drained but we need to drain the obj stock as well otherwise we will miss the stats accumulated on the target cpu as well as the
nr_bytescached. The stats includeMEMCG_KMEM,NR_SLAB_RECLAIMABLE_B&NR_SLAB_UNRECLAIMABLE_B. In addition we are leaking reference to structobj_cgroupobject.Fixes bf4f059954dc ("mm: memcg/slab: obj_cgroup API")Signed-off-by Shakeel Butt <[email protected]>Cc <[email protected]>
Wow, that's old. Good catch.
Quoted 14 lines
--- mm/memcontrol.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/memcontrol.c +2
-0@@ -1921,9 +1921,18 @@ void drain_all_stock(struct mem_cgroup *root_memcg)static int memcg_hotplug_cpu_dead (unsigned int cpu) {struct memcg_stock_pcp *stock; +struct obj_cgroup *old; +unsigned long flags;
mm/memcontrol.c void drain_all_stock(struct mem_cgroup *root_memcg) · line 1935
Quoted 10 lines
mm/memcontrol.c +7
-0stock = &per_cpu (memcg_stock, cpu); + +/* drain_obj_stock requires stock_lock */ +local_lock_irqsave (&memcg_stock.stock_lock, flags); + old =drain_obj_stock (stock); +local_unlock_irqrestore (&memcg_stock.stock_lock, flags); +drain_stock (stock); +obj_cgroup_put (old);
It might be better to call drain_local_stock() directly instead. That would prevent a bug of this type to reoccur in the future.
Shakeel Butt
maintainerQuoted 28 lines · Johannes Weiner
Quoted 9 lines · Shakeel Butt
Currently on cpu hotplug teardown, only memcg stock is drained but we need to drain the obj stock as well otherwise we will miss the stats accumulated on the target cpu as well as the
nr_bytescached. The stats includeMEMCG_KMEM,NR_SLAB_RECLAIMABLE_B&NR_SLAB_UNRECLAIMABLE_B. In addition we are leaking reference to structobj_cgroupobject.Fixes bf4f059954dc ("mm: memcg/slab: obj_cgroup API")Signed-off-by Shakeel Butt <[email protected]>Cc <[email protected]>Wow, that's old. Good catch.
Quoted 15 lines
--- mm/memcontrol.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/memcontrol.c +2
-0@@ -1921,9 +1921,18 @@ void drain_all_stock(struct mem_cgroup *root_memcg)static int memcg_hotplug_cpu_dead (unsigned int cpu) {struct memcg_stock_pcp *stock; +struct obj_cgroup *old; +unsigned long flags;
Quoted 12 lines
mm/memcontrol.c void drain_all_stock(struct mem_cgroup *root_memcg) · line 1935
Quoted 9 lines
mm/memcontrol.c +7
-0stock = &per_cpu (memcg_stock, cpu); + +/* drain_obj_stock requires stock_lock */ +local_lock_irqsave (&memcg_stock.stock_lock, flags); + old =drain_obj_stock (stock); +local_unlock_irqrestore (&memcg_stock.stock_lock, flags); +drain_stock (stock); +obj_cgroup_put (old);It might be better to call
drain_local_stock()directly instead. That would prevent a bug of this type to reoccur in the future.
The issue is drain_local_stock() works on the local cpu stock while here we are working on a remote cpu cpu which is dead (memcg_hotplug_cpu_dead is in PREPARE section of hotplug teardown which runs after the cpu is dead).
We can safely call drain_stock() on remote cpu stock here but drain_obj_stock() is a bit tricky as it can __refill_stock() to local cpu stock and can call __mod_objcg_mlstate to flush stats. Both of these requires irq disable for NON-RT kernels and thus I added the local_lock here.
Anyways I wanted a simple fix for the backports and in parallel I am working on cleaning up all the stock functions as I plan to add multi memcg support.
Thanks for taking a look.
Roman Gushchin
maintainerQuoted 53 lines · Shakeel Butt
Quoted 28 lines · Johannes Weiner
Quoted 9 lines · Shakeel Butt
Currently on cpu hotplug teardown, only memcg stock is drained but we need to drain the obj stock as well otherwise we will miss the stats accumulated on the target cpu as well as the
nr_bytescached. The stats includeMEMCG_KMEM,NR_SLAB_RECLAIMABLE_B&NR_SLAB_UNRECLAIMABLE_B. In addition we are leaking reference to structobj_cgroupobject.Fixes bf4f059954dc ("mm: memcg/slab: obj_cgroup API")Signed-off-by Shakeel Butt <[email protected]>Cc <[email protected]>Wow, that's old. Good catch.
Quoted 15 lines
--- mm/memcontrol.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/memcontrol.c +2
-0@@ -1921,9 +1921,18 @@ void drain_all_stock(struct mem_cgroup *root_memcg)static int memcg_hotplug_cpu_dead (unsigned int cpu) {struct memcg_stock_pcp *stock; +struct obj_cgroup *old; +unsigned long flags;Quoted 12 lines
mm/memcontrol.c void drain_all_stock(struct mem_cgroup *root_memcg) · line 1935
Quoted 9 lines
mm/memcontrol.c +7
-0stock = &per_cpu (memcg_stock, cpu); + +/* drain_obj_stock requires stock_lock */ +local_lock_irqsave (&memcg_stock.stock_lock, flags); + old =drain_obj_stock (stock); +local_unlock_irqrestore (&memcg_stock.stock_lock, flags); +drain_stock (stock); +obj_cgroup_put (old);It might be better to call
drain_local_stock()directly instead. That would prevent a bug of this type to reoccur in the future.The issue is
drain_local_stock()works on the local cpu stock while here we are working on a remote cpu cpu which is dead (memcg_hotplug_cpu_deadis in PREPARE section of hotplug teardown which runs after the cpu is dead).We can safely call
drain_stock()on remote cpu stock here butdrain_obj_stock()is a bit tricky as it can__refill_stock()to local cpu stock and can call__mod_objcg_mlstateto flush stats. Both of these requires irq disable for NON-RT kernels and thus I added thelocal_lockhere.
Quoted 3 lines
Anyways I wanted a simple fix for the backports and in parallel I am working on cleaning up all the stock functions as I plan to add multi memcg support.
Really curious to see patches/more details here, I have some ideas here as well (nothing materialized yet though).
Thanks!
Shakeel Butt
maintainerOn Tue, Mar 11, 2025 at 04:28:15PM +0000, Roman Gushchin wrote:
[...]
Quoted 8 lines
Quoted 4 lines
Anyways I wanted a simple fix for the backports and in parallel I am working on cleaning up all the stock functions as I plan to add multi memcg support.
Really curious to see patches/more details here, I have some ideas here as well (nothing materialized yet though).
My eventual goal is to add support for multi memcg stock to solve the charging cost of incoming networking traffic in multi-tenant environment. In the cleanups my plan is to reduce the number of irq disable operations in the most common paths and explore if for task context we can do without any irq disabling operation (possibly by separate stocks for in_task and !in_task contexts).
Quoted 53 lines · Shakeel Butt
Quoted 28 lines · Johannes Weiner
Quoted 9 lines · Shakeel Butt
Currently on cpu hotplug teardown, only memcg stock is drained but we need to drain the obj stock as well otherwise we will miss the stats accumulated on the target cpu as well as the
nr_bytescached. The stats includeMEMCG_KMEM,NR_SLAB_RECLAIMABLE_B&NR_SLAB_UNRECLAIMABLE_B. In addition we are leaking reference to structobj_cgroupobject.Fixes bf4f059954dc ("mm: memcg/slab: obj_cgroup API")Signed-off-by Shakeel Butt <[email protected]>Cc <[email protected]>Wow, that's old. Good catch.
Quoted 15 lines
--- mm/memcontrol.c | 9 +++++++++ 1 file changed, 9 insertions(+)mm/memcontrol.c +2
-0@@ -1921,9 +1921,18 @@ void drain_all_stock(struct mem_cgroup *root_memcg)static int memcg_hotplug_cpu_dead (unsigned int cpu) {struct memcg_stock_pcp *stock; +struct obj_cgroup *old; +unsigned long flags;Quoted 12 lines
mm/memcontrol.c void drain_all_stock(struct mem_cgroup *root_memcg) · line 1935
Quoted 9 lines
mm/memcontrol.c +7
-0stock = &per_cpu (memcg_stock, cpu); + +/* drain_obj_stock requires stock_lock */ +local_lock_irqsave (&memcg_stock.stock_lock, flags); + old =drain_obj_stock (stock); +local_unlock_irqrestore (&memcg_stock.stock_lock, flags); +drain_stock (stock); +obj_cgroup_put (old);It might be better to call
drain_local_stock()directly instead. That would prevent a bug of this type to reoccur in the future.The issue is
drain_local_stock()works on the local cpu stock while here we are working on a remote cpu cpu which is dead (memcg_hotplug_cpu_deadis in PREPARE section of hotplug teardown which runs after the cpu is dead).We can safely call
drain_stock()on remote cpu stock here butdrain_obj_stock()is a bit tricky as it can__refill_stock()to local cpu stock and can call__mod_objcg_mlstateto flush stats. Both of these requires irq disable for NON-RT kernels and thus I added thelocal_lockhere.
Quoted 3 lines
Anyways I wanted a simple fix for the backports and in parallel I am working on cleaning up all the stock functions as I plan to add multi memcg support.
True, it can be refactored separately.
Quoted 9 lines · Shakeel Butt
Currently on cpu hotplug teardown, only memcg stock is drained but we need to drain the obj stock as well otherwise we will miss the stats accumulated on the target cpu as well as the
nr_bytescached. The stats includeMEMCG_KMEM,NR_SLAB_RECLAIMABLE_B&NR_SLAB_UNRECLAIMABLE_B. In addition we are leaking reference to structobj_cgroupobject.Fixes bf4f059954dc ("mm: memcg/slab: obj_cgroup API")Signed-off-by Shakeel Butt <[email protected]>Cc <[email protected]>
Thanks!