memcg: shrink obj_stock_pcp and cache multiple objcgs
Subsystems (3)
Shakeel Butt
maintainercover memcg: shrink obj_stock_pcp and cache multiple objcgs
Commit 01b9da291c49 ("mm: memcontrol: convert objcg to be per-memcg per-node type") split a memcg's single obj_cgroup into one per NUMA node so that reparenting LRU folios can take per-node lru locks. As a side effect, the per-CPU obj_stock_pcp -- which caches a single cached_objcg pointer -- thrashes on workloads where threads of the same memcg run on different NUMA nodes. The kernel test robot reported a 67.7% regression on stress-ng.switch.ops_per_sec from this pattern.
Commit d0211878ce06 ("memcg: cache obj_stock by memcg, not by objcg pointer") landed as a temporary fix by treating sibling per-node objcgs as equivalent for the cache lookup, intended to be reverted once per-node kmem accounting is introduced. This series takes a more general approach: cache multiple objcgs per CPU using the multi-slot pattern memcg_stock_pcp already uses, so the per-node objcg variants of one memcg can all coexist in the stock without ever forcing a drain. The temporary fix can then be reverted.
To avoid increasing the per-CPU cache footprint, the first three patches shrink the existing single-slot obj_stock_pcp fields. The final patch converts cached_objcg and nr_bytes into NR_OBJ_STOCK=5 slot arrays and reorders the struct so the entire consume/refill/account hot path fits within a single 64-byte cache line on non-debug 64-bit builds (verified with pahole).
Shakeel Butt (4):
memcg: store node_id instead of pglist_data pointer
memcg: uint16_t for nr_bytes in obj_stock_pcp
memcg: int16_t for cached slab stats
memcg: multi objcg charge support
mm/memcontrol.c | 214 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 157 insertions(+), 57 deletions(-)
--
2.53.0-Meta
Shakeel Butt
maintainerPATCH 1/4 memcg: store node_id instead of pglist_data pointer
The struct obj_stock_pcp stores a pointer to pglist_data for the slab stats cached on the cpu. On 64-bit machines, this costs 8 bytes. The pointer is not strictly required: NODE_DATA() can recover it from the node id. Replace cached_pgdat with int16_t node_id and use NUMA_NO_NODE as the "no stats cached" sentinel.
At the moment all the archs limit MAX_NUMNODES to 1024 so int16_t is plenty; a BUILD_BUG_ON() makes sure we notice if that ever changes.
--- mm/memcontrol.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
mm/memcontrol.c +19 -7
@@ -2021,7 +2021,7 @@ struct obj_stock_pcp {local_trylock_t lock;unsigned int nr_bytes;struct obj_cgroup *cached_objcg; -struct pglist_data *cached_pgdat; +int16_t node_id;int nr_slab_reclaimable_b;int nr_slab_unreclaimable_b;
@@ -2031,6 +2031,7 @@ struct obj_stock_pcp { static DEFINE_PER_CPU_ALIGNED(struct obj_stock_pcp, obj_stock) = { .lock =INIT_LOCAL_TRYLOCK (lock), + .node_id =NUMA_NO_NODE , };static DEFINE_MUTEX (percpu_charge_mutex);
@@ -3159,6 +3160,13 @@ static void __account_obj_stock(struct obj_cgroup *objcg, {int *bytes; +/* +* Though at the moment MAX_NUMNODES <= 1024 in all archs but let's make +* sure it does not exceed S16_MAX otherwise we need to fix node_id type +* in struct obj_stock_pcp. +*/ +BUILD_BUG_ON (MAX_NUMNODES >=S16_MAX ); +if (!stock ||READ_ONCE (stock->cached_objcg) != objcg) goto direct;
@@ -3166,9 +3174,11 @@ static void __account_obj_stock(struct obj_cgroup *objcg,* Save vmstat data in stock and skip vmstat array update unless * accumulating over a page of vmstat data or when pgdat changes. */ -if (stock->cached_pgdat != pgdat) { +if (stock->node_id ==NUMA_NO_NODE ) { + stock->node_id = pgdat->node_id; + }else if (stock->node_id != pgdat->node_id) {/* Flush the existing cached vmstat data */ -struct pglist_data *oldpg = stock->cached_pgdat; +struct pglist_data *oldpg =NODE_DATA (stock->node_id);if (stock->nr_slab_reclaimable_b) { mod_objcg_mlstate(objcg, oldpg, NR_SLAB_RECLAIMABLE_B,
@@ -3180,7 +3190,7 @@ static void __account_obj_stock(struct obj_cgroup *objcg, stock->nr_slab_unreclaimable_b); stock->nr_slab_unreclaimable_b =0 ; } - stock->cached_pgdat = pgdat; + stock->node_id = pgdat->node_id; } bytes = (idx == NR_SLAB_RECLAIMABLE_B) ? &stock->nr_slab_reclaimable_b
@@ -3276,19 +3286,21 @@ static void drain_obj_stock(struct obj_stock_pcp *stock)* Flush the vmstat data in current stock */ if (stock->nr_slab_reclaimable_b || stock->nr_slab_unreclaimable_b) { +struct pglist_data *oldpg =NODE_DATA (stock->node_id); +if (stock->nr_slab_reclaimable_b) { -mod_objcg_mlstate (old, stock->cached_pgdat, +mod_objcg_mlstate (old, oldpg,NR_SLAB_RECLAIMABLE_B , stock->nr_slab_reclaimable_b); stock->nr_slab_reclaimable_b =0 ; }if (stock->nr_slab_unreclaimable_b) { -mod_objcg_mlstate (old, stock->cached_pgdat, +mod_objcg_mlstate (old, oldpg,NR_SLAB_UNRECLAIMABLE_B , stock->nr_slab_unreclaimable_b); stock->nr_slab_unreclaimable_b =0 ; } - stock->cached_pgdat =NULL ; + stock->node_id =NUMA_NO_NODE ; }WRITE_ONCE (stock->cached_objcg,NULL );
Signature · 1 line
2.53.0-Meta
Re: [PATCH 1/4] memcg: store node_id instead of pglist_data pointer
Quoted 6 lines · Shakeel Butt
The struct
obj_stock_pcpstores a pointer topglist_datafor the slab stats cached on the cpu. On 64-bit machines, this costs 8 bytes. The pointer is not strictly required:NODE_DATA()can recover it from the node id. Replacecached_pgdatwithint16_tnode_idand useNUMA_NO_NODEas the "no stats cached" sentinel.
Quoted 6 lines
At the moment all the archs limit
MAX_NUMNODESto 1024 soint16_tis plenty; aBUILD_BUG_ON()makes sure we notice if that ever changes.Signed-off-by Shakeel Butt <[email protected]>Tested-by kernel test robot <[email protected]>---
Signature · 2 lines
Cheers, Harry / Hyeonggon
Re: [PATCH 1/4] memcg: store node_id instead of pglist_data pointer
In reply to Shakeel ButtQuoted 10 lines · Shakeel Butt
The struct
obj_stock_pcpstores a pointer topglist_datafor the slab stats cached on the cpu. On 64-bit machines, this costs 8 bytes. The pointer is not strictly required:NODE_DATA()can recover it from the node id. Replacecached_pgdatwithint16_tnode_idand useNUMA_NO_NODEas the "no stats cached" sentinel.
Quoted 5 lines
At the moment all the archs limit
MAX_NUMNODESto 1024 soint16_tis plenty; aBUILD_BUG_ON()makes sure we notice if that ever changes.Signed-off-by Shakeel Butt <[email protected]>Tested-by kernel test robot <[email protected]>
Thanks.
Shakeel Butt
maintainerPATCH 2/4 memcg: uint16_t for nr_bytes in obj_stock_pcp
In reply to Shakeel ButtCurrently struct obj_stock_pcp stores nr_bytes in an 'unsigned int' which is 4 bytes on 64-bit machines. Switch the field to uint16_t to shrink the per-CPU cache.
The kernel supports PAGE_SIZE_4KB, _8KB, _16KB, _32KB, _64KB and _256KB (see HAVE_PAGE_SIZE_* in arch/Kconfig). After the PAGE_SIZE-aligned flush in __refill_obj_stock(), the sub-page remainder fits in uint16_t up through 64KiB pages where PAGE_SIZE - 1 == U16_MAX, but on 256KiB pages PAGE_SIZE - 1 == 0x3FFFF exceeds U16_MAX. The accumulator also needs to stay within uint16_t between page-aligned flushes on 64KiB pages where PAGE_SIZE itself is U16_MAX + 1.
Accumulate the new total in an 'unsigned int' local, then:
1. Flush whenever the accumulator would hit U16_MAX. Together with the existing allow_uncharge flush at PAGE_SIZE, this keeps the uint16_t safe on PAGE_SIZE <= 64KiB.
2. On configs with PAGE_SHIFT > 16 (PAGE_SIZE_256KB on hexagon and powerpc 44x), push any sub-page remainder above U16_MAX into objcg->nr_charged_bytes via atomic_add before storing back, so the store cannot silently truncate. The PAGE_SHIFT > 16 guard folds the branch out at compile time on smaller page sizes.
--- mm/memcontrol.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-)
mm/memcontrol.c +27 -6
@@ -2019,7 +2019,7 @@ static DEFINE_PER_CPU_ALIGNED(struct memcg_stock_pcp, memcg_stock) = {struct obj_stock_pcp {local_trylock_t lock; -unsigned int nr_bytes; +uint16_t nr_bytes;struct obj_cgroup *cached_objcg;int16_t node_id;int nr_slab_reclaimable_b;
@@ -3331,6 +3331,7 @@ static void __refill_obj_stock(struct obj_cgroup *objcg, bool allow_uncharge) {unsigned int nr_pages =0 ; +unsigned int stock_nr_bytes;if (!stock) { nr_pages = nr_bytes >>PAGE_SHIFT ;
@@ -3339,21 +3340,41 @@ static void __refill_obj_stock(struct obj_cgroup *objcg, goto out; } + stock_nr_bytes = stock->nr_bytes;if (READ_ONCE (stock->cached_objcg) != objcg) {/* reset if necessary */ drain_obj_stock (stock);obj_cgroup_get (objcg); - stock->nr_bytes =atomic_read (&objcg->nr_charged_bytes) + stock_nr_bytes =atomic_read (&objcg->nr_charged_bytes) ?atomic_xchg (&objcg->nr_charged_bytes,0 ) :0 ;WRITE_ONCE (stock->cached_objcg, objcg); allow_uncharge = true;/* Allow uncharge when objcg changes */ } - stock->nr_bytes += nr_bytes; + stock_nr_bytes += nr_bytes; + +/* Since stock->nr_bytes is uint16_t, don't refill >= U16_MAX */ +if ((allow_uncharge && (stock_nr_bytes >PAGE_SIZE )) || + stock_nr_bytes >=U16_MAX ) { + nr_pages = stock_nr_bytes >>PAGE_SHIFT ; + stock_nr_bytes &= (PAGE_SIZE -1 ); + +/* +* On configs with PAGE_SHIFT > 16 (PAGE_SIZE_256KB on +* hexagon and powerpc 44x), the sub-page remainder can +* still exceed U16_MAX. Push the excess back to +* objcg->nr_charged_bytes so the store into uint16_t +* cannot silently truncate; folded out at compile time +* on smaller page sizes. +*/ +if (PAGE_SHIFT >16 && stock_nr_bytes >U16_MAX ) { +unsigned int kept = stock_nr_bytes &U16_MAX ; -if (allow_uncharge && (stock->nr_bytes >PAGE_SIZE )) { - nr_pages = stock->nr_bytes >>PAGE_SHIFT ; - stock->nr_bytes &= (PAGE_SIZE -1 ); +atomic_add (stock_nr_bytes - kept, + &objcg->nr_charged_bytes); + stock_nr_bytes = kept; + } } + stock->nr_bytes = stock_nr_bytes; out:if (nr_pages)
Signature · 1 line
2.53.0-Meta
Re: [PATCH 2/4] memcg: uint16_t for nr_bytes in obj_stock_pcp
Quoted 25 lines · Shakeel Butt
Currently struct
obj_stock_pcpstoresnr_bytesin an 'unsigned int' which is 4 bytes on 64-bit machines. Switch the field touint16_tto shrink the per-CPU cache.The kernel supports
PAGE_SIZE_4KB,_8KB,_16KB,_32KB,_64KBand_256KB(see HAVE_PAGE_SIZE_* inarch/Kconfig). After the PAGE_SIZE-aligned flush in__refill_obj_stock(), the sub-page remainder fits inuint16_tup through 64KiB pages wherePAGE_SIZE- 1 ==U16_MAX, but on 256KiB pagesPAGE_SIZE- 1 ==0x3FFFFexceedsU16_MAX. The accumulator also needs to stay withinuint16_tbetween page-aligned flushes on 64KiB pages wherePAGE_SIZEitself isU16_MAX+ 1.Accumulate the new total in an 'unsigned int' local, then:
1. Flush whenever the accumulator would hit
U16_MAX. Together with the existingallow_unchargeflush atPAGE_SIZE, this keeps theuint16_tsafe onPAGE_SIZE<= 64KiB.2. On configs with
PAGE_SHIFT> 16 (PAGE_SIZE_256KBon hexagon and powerpc 44x), push any sub-page remainder aboveU16_MAXintoobjcg->nr_charged_bytesviaatomic_addbefore storing back, so the store cannot silently truncate. ThePAGE_SHIFT> 16 guard folds the branch out at compile time on smaller page sizes.
Quoted 3 lines
Signed-off-by Shakeel Butt <[email protected]>Tested-by kernel test robot <[email protected]>---
Signature · 2 lines
Cheers, Harry / Hyeonggon
Re: [PATCH 2/4] memcg: uint16_t for nr_bytes in obj_stock_pcp
In reply to Shakeel ButtQuoted 46 lines · Shakeel Butt
Currently struct
obj_stock_pcpstoresnr_bytesin an 'unsigned int' which is 4 bytes on 64-bit machines. Switch the field touint16_tto shrink the per-CPU cache.The kernel supports
PAGE_SIZE_4KB,_8KB,_16KB,_32KB,_64KBand_256KB(see HAVE_PAGE_SIZE_* inarch/Kconfig). After the PAGE_SIZE-aligned flush in__refill_obj_stock(), the sub-page remainder fits inuint16_tup through 64KiB pages wherePAGE_SIZE- 1 ==U16_MAX, but on 256KiB pagesPAGE_SIZE- 1 ==0x3FFFFexceedsU16_MAX. The accumulator also needs to stay withinuint16_tbetween page-aligned flushes on 64KiB pages wherePAGE_SIZEitself isU16_MAX+ 1.Accumulate the new total in an 'unsigned int' local, then:
1. Flush whenever the accumulator would hit
U16_MAX. Together with the existingallow_unchargeflush atPAGE_SIZE, this keeps theuint16_tsafe onPAGE_SIZE<= 64KiB.2. On configs with
PAGE_SHIFT> 16 (PAGE_SIZE_256KBon hexagon and powerpc 44x), push any sub-page remainder aboveU16_MAXintoobjcg->nr_charged_bytesviaatomic_addbefore storing back, so the store cannot silently truncate. ThePAGE_SHIFT> 16 guard folds the branch out at compile time on smaller page sizes.Signed-off-by Shakeel Butt <[email protected]>Tested-by kernel test robot <[email protected]>--- mm/memcontrol.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-)mm/memcontrol.c +2
-1@@ -3339,21 +3340,41 @@ static void __refill_obj_stock(struct obj_cgroup *objcg, goto out; } + stock_nr_bytes = stock->nr_bytes;if (READ_ONCE (stock->cached_objcg) != objcg) {/* reset if necessary */ drain_obj_stock (stock);obj_cgroup_get (objcg); - stock->nr_bytes =atomic_read (&objcg->nr_charged_bytes) + stock_nr_bytes =atomic_read (&objcg->nr_charged_bytes) ?atomic_xchg (&objcg->nr_charged_bytes,0 ) :0 ;
mm/memcontrol.c static void __refill_obj_stock(struct obj_cgroup *objcg, · line 3357
Quoted 10 lines
mm/memcontrol.c +5
-1WRITE_ONCE (stock->cached_objcg, objcg); allow_uncharge = true;/* Allow uncharge when objcg changes */ } - stock->nr_bytes += nr_bytes; + stock_nr_bytes += nr_bytes; + +/* Since stock->nr_bytes is uint16_t, don't refill >= U16_MAX */ +if ((allow_uncharge && (stock_nr_bytes >PAGE_SIZE )) || + stock_nr_bytes >=U16_MAX ) {
nit: This should be > U16_MAX?
Quoted 26 lines
mm/memcontrol.c +18
-3+ nr_pages = stock_nr_bytes >>PAGE_SHIFT ; + stock_nr_bytes &= (PAGE_SIZE -1 ); + +/* +* On configs with PAGE_SHIFT > 16 (PAGE_SIZE_256KB on +* hexagon and powerpc 44x), the sub-page remainder can +* still exceed U16_MAX. Push the excess back to +* objcg->nr_charged_bytes so the store into uint16_t +* cannot silently truncate; folded out at compile time +* on smaller page sizes. +*/ +if (PAGE_SHIFT >16 && stock_nr_bytes >U16_MAX ) { +unsigned int kept = stock_nr_bytes &U16_MAX ; -if (allow_uncharge && (stock->nr_bytes >PAGE_SIZE )) { - nr_pages = stock->nr_bytes >>PAGE_SHIFT ; - stock->nr_bytes &= (PAGE_SIZE -1 ); +atomic_add (stock_nr_bytes - kept, + &objcg->nr_charged_bytes); + stock_nr_bytes = kept; + } } + stock->nr_bytes = stock_nr_bytes; out:if (nr_pages)
Signature · 2 lines
Cheers, Harry / Hyeonggon
Shakeel Butt
maintainerRe: [PATCH 2/4] memcg: uint16_t for nr_bytes in obj_stock_pcp
Quoted 51 lines · Harry Yoo
Quoted 48 lines · Shakeel Butt
Currently struct
obj_stock_pcpstoresnr_bytesin an 'unsigned int' which is 4 bytes on 64-bit machines. Switch the field touint16_tto shrink the per-CPU cache.The kernel supports
PAGE_SIZE_4KB,_8KB,_16KB,_32KB,_64KBand_256KB(see HAVE_PAGE_SIZE_* inarch/Kconfig). After the PAGE_SIZE-aligned flush in__refill_obj_stock(), the sub-page remainder fits inuint16_tup through 64KiB pages wherePAGE_SIZE- 1 ==U16_MAX, but on 256KiB pagesPAGE_SIZE- 1 ==0x3FFFFexceedsU16_MAX. The accumulator also needs to stay withinuint16_tbetween page-aligned flushes on 64KiB pages wherePAGE_SIZEitself isU16_MAX+ 1.Accumulate the new total in an 'unsigned int' local, then:
1. Flush whenever the accumulator would hit
U16_MAX. Together with the existingallow_unchargeflush atPAGE_SIZE, this keeps theuint16_tsafe onPAGE_SIZE<= 64KiB.2. On configs with
PAGE_SHIFT> 16 (PAGE_SIZE_256KBon hexagon and powerpc 44x), push any sub-page remainder aboveU16_MAXintoobjcg->nr_charged_bytesviaatomic_addbefore storing back, so the store cannot silently truncate. ThePAGE_SHIFT> 16 guard folds the branch out at compile time on smaller page sizes.Signed-off-by Shakeel Butt <[email protected]>Tested-by kernel test robot <[email protected]>--- mm/memcontrol.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-)mm/memcontrol.c +2
-1@@ -3339,21 +3340,41 @@ static void __refill_obj_stock(struct obj_cgroup *objcg, goto out; } + stock_nr_bytes = stock->nr_bytes;if (READ_ONCE (stock->cached_objcg) != objcg) {/* reset if necessary */ drain_obj_stock (stock);obj_cgroup_get (objcg); - stock->nr_bytes =atomic_read (&objcg->nr_charged_bytes) + stock_nr_bytes =atomic_read (&objcg->nr_charged_bytes) ?atomic_xchg (&objcg->nr_charged_bytes,0 ) :0 ;WRITE_ONCE (stock->cached_objcg, objcg); allow_uncharge = true;/* Allow uncharge when objcg changes */ }
Quoted 8 lines
mm/memcontrol.c static void __refill_obj_stock(struct obj_cgroup *objcg, · line 3357
Quoted 6 lines
mm/memcontrol.c +5
-1- stock->nr_bytes += nr_bytes; + stock_nr_bytes += nr_bytes; + +/* Since stock->nr_bytes is uint16_t, don't refill >= U16_MAX */ +if ((allow_uncharge && (stock_nr_bytes >PAGE_SIZE )) || + stock_nr_bytes >=U16_MAX ) {nit: This should be >
U16_MAX?
Ack.
Re: [PATCH 2/4] memcg: uint16_t for nr_bytes in obj_stock_pcp
In reply to Shakeel ButtQuoted 36 lines · On Tue, 19 May 2026 22:31:20 -0700 Shakeel Butt <[email protected]> wrote:
Currently struct
obj_stock_pcpstoresnr_bytesin an 'unsigned int' which is 4 bytes on 64-bit machines. Switch the field touint16_tto shrink the per-CPU cache.The kernel supports
PAGE_SIZE_4KB,_8KB,_16KB,_32KB,_64KBand_256KB(see HAVE_PAGE_SIZE_* inarch/Kconfig). After the PAGE_SIZE-aligned flush in__refill_obj_stock(), the sub-page remainder fits inuint16_tup through 64KiB pages wherePAGE_SIZE- 1 ==U16_MAX, but on 256KiB pagesPAGE_SIZE- 1 ==0x3FFFFexceedsU16_MAX. The accumulator also needs to stay withinuint16_tbetween page-aligned flushes on 64KiB pages wherePAGE_SIZEitself isU16_MAX+ 1.Accumulate the new total in an 'unsigned int' local, then:
1. Flush whenever the accumulator would hit
U16_MAX. Together with the existingallow_unchargeflush atPAGE_SIZE, this keeps theuint16_tsafe onPAGE_SIZE<= 64KiB.2. On configs with
PAGE_SHIFT> 16 (PAGE_SIZE_256KBon hexagon and powerpc 44x), push any sub-page remainder aboveU16_MAXintoobjcg->nr_charged_bytesviaatomic_addbefore storing back, so the store cannot silently truncate. ThePAGE_SHIFT> 16 guard folds the branch out at compile time on smaller page sizes.Signed-off-by Shakeel Butt <[email protected]>Tested-by kernel test robot <[email protected]>--- mm/memcontrol.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-)mm/memcontrol.c +0
-0
mm/memcontrol.c static DEFINE_PER_CPU_ALIGNED(struct memcg_stock_pcp, memcg_stock) = { · line 2024
Quoted 7 lines
mm/memcontrol.c +1
-1@@ -2019,7 +2019,7 @@ static DEFINE_PER_CPU_ALIGNED(struct memcg_stock_pcp, memcg_stock) = {struct obj_stock_pcp {local_trylock_t lock; -unsigned int nr_bytes; +uint16_t nr_bytes;struct obj_cgroup *cached_objcg;int16_t node_id;
You might want to move it to this hole.
The size of 'lock' depends on kernel build options.
-- David
Quoted 56 lines
mm/memcontrol.c +26
-5int nr_slab_reclaimable_b;@@ -3331,6 +3331,7 @@ static void __refill_obj_stock(struct obj_cgroup *objcg, bool allow_uncharge) {unsigned int nr_pages =0 ; +unsigned int stock_nr_bytes;if (!stock) { nr_pages = nr_bytes >>PAGE_SHIFT ;@@ -3339,21 +3340,41 @@ static void __refill_obj_stock(struct obj_cgroup *objcg, goto out; } + stock_nr_bytes = stock->nr_bytes;if (READ_ONCE (stock->cached_objcg) != objcg) {/* reset if necessary */ drain_obj_stock (stock);obj_cgroup_get (objcg); - stock->nr_bytes =atomic_read (&objcg->nr_charged_bytes) + stock_nr_bytes =atomic_read (&objcg->nr_charged_bytes) ?atomic_xchg (&objcg->nr_charged_bytes,0 ) :0 ;WRITE_ONCE (stock->cached_objcg, objcg); allow_uncharge = true;/* Allow uncharge when objcg changes */ } - stock->nr_bytes += nr_bytes; + stock_nr_bytes += nr_bytes; + +/* Since stock->nr_bytes is uint16_t, don't refill >= U16_MAX */ +if ((allow_uncharge && (stock_nr_bytes >PAGE_SIZE )) || + stock_nr_bytes >=U16_MAX ) { + nr_pages = stock_nr_bytes >>PAGE_SHIFT ; + stock_nr_bytes &= (PAGE_SIZE -1 ); + +/* +* On configs with PAGE_SHIFT > 16 (PAGE_SIZE_256KB on +* hexagon and powerpc 44x), the sub-page remainder can +* still exceed U16_MAX. Push the excess back to +* objcg->nr_charged_bytes so the store into uint16_t +* cannot silently truncate; folded out at compile time +* on smaller page sizes. +*/ +if (PAGE_SHIFT >16 && stock_nr_bytes >U16_MAX ) { +unsigned int kept = stock_nr_bytes &U16_MAX ; -if (allow_uncharge && (stock->nr_bytes >PAGE_SIZE )) { - nr_pages = stock->nr_bytes >>PAGE_SHIFT ; - stock->nr_bytes &= (PAGE_SIZE -1 ); +atomic_add (stock_nr_bytes - kept, + &objcg->nr_charged_bytes); + stock_nr_bytes = kept; + } } + stock->nr_bytes = stock_nr_bytes; out:if (nr_pages)
Shakeel Butt
maintainerRe: [PATCH 2/4] memcg: uint16_t for nr_bytes in obj_stock_pcp
Quoted 40 lines · David Laight
Quoted 37 lines · On Tue, 19 May 2026 22:31:20 -0700 Shakeel Butt <[email protected]> wrote:
Currently struct
obj_stock_pcpstoresnr_bytesin an 'unsigned int' which is 4 bytes on 64-bit machines. Switch the field touint16_tto shrink the per-CPU cache.The kernel supports
PAGE_SIZE_4KB,_8KB,_16KB,_32KB,_64KBand_256KB(see HAVE_PAGE_SIZE_* inarch/Kconfig). After the PAGE_SIZE-aligned flush in__refill_obj_stock(), the sub-page remainder fits inuint16_tup through 64KiB pages wherePAGE_SIZE- 1 ==U16_MAX, but on 256KiB pagesPAGE_SIZE- 1 ==0x3FFFFexceedsU16_MAX. The accumulator also needs to stay withinuint16_tbetween page-aligned flushes on 64KiB pages wherePAGE_SIZEitself isU16_MAX+ 1.Accumulate the new total in an 'unsigned int' local, then:
1. Flush whenever the accumulator would hit
U16_MAX. Together with the existingallow_unchargeflush atPAGE_SIZE, this keeps theuint16_tsafe onPAGE_SIZE<= 64KiB.2. On configs with
PAGE_SHIFT> 16 (PAGE_SIZE_256KBon hexagon and powerpc 44x), push any sub-page remainder aboveU16_MAXintoobjcg->nr_charged_bytesviaatomic_addbefore storing back, so the store cannot silently truncate. ThePAGE_SHIFT> 16 guard folds the branch out at compile time on smaller page sizes.Signed-off-by Shakeel Butt <[email protected]>Tested-by kernel test robot <[email protected]>--- mm/memcontrol.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-)mm/memcontrol.c +0
-0@@ -2019,7 +2019,7 @@ static DEFINE_PER_CPU_ALIGNED(struct memcg_stock_pcp, memcg_stock) = {
Quoted 9 lines
mm/memcontrol.c static DEFINE_PER_CPU_ALIGNED(struct memcg_stock_pcp, memcg_stock) = { · line 2024
Quoted 6 lines
mm/memcontrol.c +1
-1struct obj_stock_pcp {local_trylock_t lock; -unsigned int nr_bytes; +uint16_t nr_bytes;struct obj_cgroup *cached_objcg;int16_t node_id;You might want to move it to this hole.
The size of 'lock' depends on kernel build options.
Thanks. In the final patch, I am rearranging the fields for better packing. Please take a look at 4th patch and see if it still need fixing.
Shakeel Butt
maintainerPATCH 3/4 memcg: int16_t for cached slab stats
In reply to Shakeel ButtCurrently struct obj_stock_pcp stores cached slab stats in 'int' which is 4 bytes per counter on 64-bit machines. Switch them to int16_t to shrink the cached metadata.
The existing PAGE_SIZE flush in __account_obj_stock() bounds *bytes at PAGE_SIZE on 4KiB and 16KiB page archs, well within int16_t. On 64KiB pages PAGE_SIZE is well above S16_MAX so that flush never fires, and a sufficiently long run of accumulations would overflow the cache. Add an explicit S16_MAX guard before each add: when the next add would push abs(*bytes) past S16_MAX, fold the cached value into @nr and flush directly via mod_objcg_mlstate() before the accumulation.
--- mm/memcontrol.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
mm/memcontrol.c +13 -3
@@ -2022,8 +2022,8 @@ struct obj_stock_pcp {uint16_t nr_bytes;struct obj_cgroup *cached_objcg;int16_t node_id; -int nr_slab_reclaimable_b; -int nr_slab_unreclaimable_b; +int16_t nr_slab_reclaimable_b; +int16_t nr_slab_unreclaimable_b;struct work_struct work;unsigned long flags;
@@ -3158,7 +3158,7 @@ static void __account_obj_stock(struct obj_cgroup *objcg, struct obj_stock_pcp *stock, int nr, struct pglist_data *pgdat, enum node_stat_item idx) { - int *bytes; + int16_t *bytes;/* * Though at the moment MAX_NUMNODES <= 1024 in all archs but let's make
@@ -3195,6 +3195,16 @@ static void __account_obj_stock(struct obj_cgroup *objcg, bytes = (idx ==NR_SLAB_RECLAIMABLE_B ) ? &stock->nr_slab_reclaimable_b : &stock->nr_slab_unreclaimable_b; +/* +* To avoid overflow or underflow, flush directly if accumulating @nr +* would push the cached value past S16_MAX. +*/ +if (abs (nr + *bytes) >=S16_MAX ) { + nr += *bytes; + *bytes =0 ; + goto direct; + } +/* * Even for large object >= PAGE_SIZE, the vmstat data will still be * cached locally at least once before pushing it out.
Signature · 1 line
2.53.0-Meta
Re: [PATCH 3/4] memcg: int16_t for cached slab stats
Quoted 12 lines · Shakeel Butt
Currently struct
obj_stock_pcpstores cached slab stats in 'int' which is 4 bytes per counter on 64-bit machines. Switch them toint16_tto shrink the cached metadata.The existing
PAGE_SIZEflush in__account_obj_stock()bounds *bytes atPAGE_SIZEon 4KiB and 16KiB page archs, well withinint16_t. On 64KiB pagesPAGE_SIZEis well aboveS16_MAXso that flush never fires, and a sufficiently long run of accumulations would overflow the cache. Add an explicitS16_MAXguard before each add: when the next add would pushabs(*bytes)pastS16_MAX, fold the cached value into @nr and flush directly viamod_objcg_mlstate()before the accumulation.
Quoted 3 lines
Signed-off-by Shakeel Butt <[email protected]>Tested-by kernel test robot <[email protected]>---
So arches with 64KiB sizes won't benefit from
"Even for large object >= PAGE_SIZE, the vmstat data will still be cached locally at least once before pushing it out" case.
But its benefit is questionable (to me), might be ok.
Quoted 17 lines
mm/memcontrol.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)mm/memcontrol.c +1
-1@@ -3158,7 +3158,7 @@ static void __account_obj_stock(struct obj_cgroup *objcg, struct obj_stock_pcp *stock, int nr, struct pglist_data *pgdat, enum node_stat_item idx) { - int *bytes; + int16_t *bytes;/* * Though at the moment MAX_NUMNODES <= 1024 in all archs but let's make
mm/memcontrol.c static void __account_obj_stock(struct obj_cgroup *objcg, · line 3202
Quoted 8 lines
mm/memcontrol.c +5
-0@@ -3195,6 +3195,16 @@ static void __account_obj_stock(struct obj_cgroup *objcg, bytes = (idx ==NR_SLAB_RECLAIMABLE_B ) ? &stock->nr_slab_reclaimable_b : &stock->nr_slab_unreclaimable_b; +/* +* To avoid overflow or underflow, flush directly if accumulating @nr +* would push the cached value past S16_MAX. +*/ +if (abs (nr + *bytes) >=S16_MAX ) {
nit: should be > S16_MAX?
mm/memcontrol.c static void __account_obj_stock(struct obj_cgroup *objcg, · line 3210
Quoted 8 lines
mm/memcontrol.c +5
-0+ nr += *bytes; + *bytes =0 ; + goto direct; + } +/* * Even for large object >= PAGE_SIZE, the vmstat data will still be * cached locally at least once before pushing it out.
FWIW:
Signature · 2 lines
Cheers, Harry / Hyeonggon
Shakeel Butt
maintainerPATCH 4/4 memcg: multi objcg charge support
In reply to Shakeel ButtCommit 01b9da291c49 ("mm: memcontrol: convert objcg to be per-memcg per-node type") split a memcg's single obj_cgroup into one per NUMA node so that reparenting LRU folios can take per-node lru locks. As a side effect, the per-CPU obj_stock_pcp -- which caches exactly one cached_objcg -- thrashes on workloads where threads of the same memcg run on different NUMA nodes. The kernel test robot reported a 67.7% regression on stress-ng.switch.ops_per_sec from this pattern.
Mirror the multi-slot pattern already used by memcg_stock_pcp: turn nr_bytes and cached_objcg into NR_OBJ_STOCK-element arrays, scan all slots on consume/refill/account, prefer empty slots when inserting, and evict a random slot only when full. With multiple slots a CPU can hold the per-node objcg variants of one memcg plus a few siblings without ever forcing a drain.
A single int8_t index records which slot the cached slab stats belong to; the stats are flushed on slot or pgdat change. With NR_OBJ_STOCK = 5 the layout (verified with pahole) is:
offset 0 : lock(1) + index(1) + node_id(2) + slab stats(4) = 8B
offset 8 : nr_bytes[5] = 10B offset 18 : padding = 6B offset 24 : cached[5] = 40B
offset 64 : (line 2) work_struct + flags (cold)
so consume_obj_stock, refill_obj_stock and the slab account path each touch exactly one 64-byte cache line on non-debug 64-bit builds.
--- mm/memcontrol.c | 185 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 133 insertions(+), 52 deletions(-)
mm/memcontrol.c +133 -52
@@ -150,14 +150,14 @@ static void obj_cgroup_release(struct percpu_ref *ref)* However, it can be PAGE_SIZE or (x * PAGE_SIZE). * * The following sequence can lead to it: -* 1) CPU0: objcg == stock->cached_objcg +* 1) CPU0: objcg cached in one of stock->cached[i] * 2) CPU1: we do a small allocation (e.g. 92 bytes), * PAGE_SIZE bytes are charged * 3) CPU1: a process from another memcg is allocating something, * the stock if flushed, * objcg->nr_charged_bytes = PAGE_SIZE - 92 * 5) CPU0: we do release this object, -* 92 bytes are added to stock->nr_bytes +* 92 bytes are added to stock->nr_bytes[i] * 6) CPU0: stock is flushed, * 92 bytes are added to objcg->nr_charged_bytes *
@@ -2017,13 +2017,25 @@ static DEFINE_PER_CPU_ALIGNED(struct memcg_stock_pcp, memcg_stock) = { .lock = INIT_LOCAL_TRYLOCK(lock), }; +/* +* NR_OBJ_STOCK is sized so the entire hot path of obj_stock_pcp +* (lock, accounting metadata, nr_bytes[] and cached[]) fits within a +* single 64-byte cache line on non-debug 64-bit builds. With 5 slots: +* lock(1) + index(1) + node_id(2) + slab stats(4) + nr_bytes(10) +* + pad(6) + cached(40) == 64 bytes. +* A CPU can thus consume/refill/account against five different objcgs +* (typically per-node variants of the same memcg) while incurring at +* most one cache miss on the stock. +*/ +#define NR_OBJ_STOCK 5struct obj_stock_pcp {local_trylock_t lock; -uint16_t nr_bytes; -struct obj_cgroup *cached_objcg; +int8_t index;int16_t node_id;int16_t nr_slab_reclaimable_b;int16_t nr_slab_unreclaimable_b; +uint16_t nr_bytes[NR_OBJ_STOCK ]; +struct obj_cgroup *cached[NR_OBJ_STOCK ];struct work_struct work;unsigned long flags;
@@ -2031,11 +2043,13 @@ struct obj_stock_pcp { static DEFINE_PER_CPU_ALIGNED(struct obj_stock_pcp, obj_stock) = { .lock =INIT_LOCAL_TRYLOCK (lock), + .index =-1 , .node_id =NUMA_NO_NODE , };static DEFINE_MUTEX (percpu_charge_mutex); +static void drain_obj_stock_slot (struct obj_stock_pcp *stock,int i);static void drain_obj_stock (struct obj_stock_pcp *stock);static bool obj_stock_flush_required (struct obj_stock_pcp *stock,struct mem_cgroup *root_memcg);
@@ -3153,12 +3167,13 @@ static void unlock_stock(struct obj_stock_pcp *stock)local_unlock (&obj_stock.lock); } -/* Call after __refill_obj_stock() to ensure stock->cached_objg == objcg */ +/* Call after __refill_obj_stock() so a slot for objcg exists in the stock */ static void __account_obj_stock (struct obj_cgroup *objcg,struct obj_stock_pcp *stock,int nr,struct pglist_data *pgdat,enum node_stat_item idx) {int16_t *bytes; +int i;/* * Though at the moment MAX_NUMNODES <= 1024 in all archs but let's make
@@ -3167,29 +3182,39 @@ static void __account_obj_stock(struct obj_cgroup *objcg,*/ BUILD_BUG_ON (MAX_NUMNODES >=S16_MAX ); -if (!stock ||READ_ONCE (stock->cached_objcg) != objcg) +if (!stock) + goto direct; + +for (i =0 ; i <NR_OBJ_STOCK ; ++i) { +if (READ_ONCE (stock->cached[i]) == objcg) +break ; + } +if (i ==NR_OBJ_STOCK ) goto direct;/* * Save vmstat data in stock and skip vmstat array update unless -* accumulating over a page of vmstat data or when pgdat changes. +* accumulating over a page of vmstat data or when the objcg slot or +* pgdat the stats belong to changes. */ -if (stock->node_id ==NUMA_NO_NODE ) { +if (stock->index <0 ) { + stock->index = i; stock->node_id = pgdat->node_id; - }else if (stock->node_id != pgdat->node_id) { -/* Flush the existing cached vmstat data */ + }else if (stock->index != i || stock->node_id != pgdat->node_id) { +struct obj_cgroup *old =READ_ONCE (stock->cached[stock->index]);struct pglist_data *oldpg =NODE_DATA (stock->node_id);if (stock->nr_slab_reclaimable_b) { -mod_objcg_mlstate (objcg, oldpg,NR_SLAB_RECLAIMABLE_B , +mod_objcg_mlstate (old, oldpg,NR_SLAB_RECLAIMABLE_B , stock->nr_slab_reclaimable_b); stock->nr_slab_reclaimable_b =0 ; }if (stock->nr_slab_unreclaimable_b) { -mod_objcg_mlstate (objcg, oldpg,NR_SLAB_UNRECLAIMABLE_B , +mod_objcg_mlstate (old, oldpg,NR_SLAB_UNRECLAIMABLE_B , stock->nr_slab_unreclaimable_b); stock->nr_slab_unreclaimable_b =0 ; } + stock->index = i; stock->node_id = pgdat->node_id; }
@@ -3230,10 +3255,16 @@ static bool __consume_obj_stock(struct obj_cgroup *objcg,struct obj_stock_pcp *stock, unsigned int nr_bytes) { - if (objcg == READ_ONCE(stock->cached_objcg) && - stock->nr_bytes >= nr_bytes) { - stock->nr_bytes -= nr_bytes; -return true; + int i; + +for (i =0 ; i <NR_OBJ_STOCK ; ++i) { +if (READ_ONCE (stock->cached[i]) != objcg) +continue ; +if (stock->nr_bytes[i] >= nr_bytes) { + stock->nr_bytes[i] -= nr_bytes; +return true; + } +return false; }return false;
@@ -3254,16 +3285,42 @@ static bool consume_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes)return ret; } -static void drain_obj_stock (struct obj_stock_pcp *stock) +/* Flush the cached slab stats (if any) back to their owning objcg/pgdat. */ +static void drain_obj_stock_stats (struct obj_stock_pcp *stock) { -struct obj_cgroup *old =READ_ONCE (stock->cached_objcg); +struct obj_cgroup *old; +struct pglist_data *oldpg; + +if (stock->index <0 ) +return ; + + old =READ_ONCE (stock->cached[stock->index]); + oldpg =NODE_DATA (stock->node_id); + +if (stock->nr_slab_reclaimable_b) { +mod_objcg_mlstate (old, oldpg,NR_SLAB_RECLAIMABLE_B , + stock->nr_slab_reclaimable_b); + stock->nr_slab_reclaimable_b =0 ; + } +if (stock->nr_slab_unreclaimable_b) { +mod_objcg_mlstate (old, oldpg,NR_SLAB_UNRECLAIMABLE_B , + stock->nr_slab_unreclaimable_b); + stock->nr_slab_unreclaimable_b =0 ; + } + stock->index =-1 ; + stock->node_id =NUMA_NO_NODE ; +} + +static void drain_obj_stock_slot (struct obj_stock_pcp *stock,int i) +{ +struct obj_cgroup *old =READ_ONCE (stock->cached[i]);if (!old)return ; -if (stock->nr_bytes) { -unsigned int nr_pages = stock->nr_bytes >>PAGE_SHIFT ; -unsigned int nr_bytes = stock->nr_bytes & (PAGE_SIZE -1 ); +if (stock->nr_bytes[i]) { +unsigned int nr_pages = stock->nr_bytes[i] >>PAGE_SHIFT ; +unsigned int nr_bytes = stock->nr_bytes[i] & (PAGE_SIZE -1 );if (nr_pages) {struct mem_cgroup *memcg;
@@ -3289,46 +3346,43 @@ static void drain_obj_stock(struct obj_stock_pcp *stock)* so it might be changed in the future. */ atomic_add (nr_bytes, &old->nr_charged_bytes); - stock->nr_bytes =0 ; + stock->nr_bytes[i] =0 ; } -/* -* Flush the vmstat data in current stock -*/ -if (stock->nr_slab_reclaimable_b || stock->nr_slab_unreclaimable_b) { -struct pglist_data *oldpg =NODE_DATA (stock->node_id); - -if (stock->nr_slab_reclaimable_b) { -mod_objcg_mlstate (old, oldpg, -NR_SLAB_RECLAIMABLE_B , - stock->nr_slab_reclaimable_b); - stock->nr_slab_reclaimable_b =0 ; - } -if (stock->nr_slab_unreclaimable_b) { -mod_objcg_mlstate (old, oldpg, -NR_SLAB_UNRECLAIMABLE_B , - stock->nr_slab_unreclaimable_b); - stock->nr_slab_unreclaimable_b =0 ; - } - stock->node_id =NUMA_NO_NODE ; - } +/* Flush vmstat data when its owning slot is being drained. */ +if (stock->index == i) +drain_obj_stock_stats (stock); -WRITE_ONCE (stock->cached_objcg,NULL ); +WRITE_ONCE (stock->cached[i],NULL );obj_cgroup_put (old); } +static void drain_obj_stock (struct obj_stock_pcp *stock) +{ +int i; + +for (i =0 ; i <NR_OBJ_STOCK ; ++i) +drain_obj_stock_slot (stock, i); +} +static bool obj_stock_flush_required (struct obj_stock_pcp *stock,struct mem_cgroup *root_memcg) { -struct obj_cgroup *objcg =READ_ONCE (stock->cached_objcg); +struct obj_cgroup *objcg;struct mem_cgroup *memcg;bool flush = false; +int i;rcu_read_lock (); -if (objcg) { +for (i =0 ; i <NR_OBJ_STOCK ; ++i) { + objcg =READ_ONCE (stock->cached[i]); +if (!objcg) +continue ; memcg =obj_cgroup_memcg (objcg); -if (memcg &&mem_cgroup_is_descendant (memcg, root_memcg)) +if (memcg &&mem_cgroup_is_descendant (memcg, root_memcg)) { flush = true; +break ; + } }rcu_read_unlock ();
@@ -3342,6 +3396,7 @@ static void __refill_obj_stock(struct obj_cgroup *objcg, {unsigned int nr_pages =0 ;unsigned int stock_nr_bytes; +int i, slot =-1 , empty_slot =-1 ;if (!stock) { nr_pages = nr_bytes >>PAGE_SHIFT ;
@@ -3350,19 +3405,45 @@ static void __refill_obj_stock(struct obj_cgroup *objcg, goto out; } - stock_nr_bytes = stock->nr_bytes; -if (READ_ONCE (stock->cached_objcg) != objcg) {/* reset if necessary */ -drain_obj_stock (stock); +for (i =0 ; i <NR_OBJ_STOCK ; ++i) { +struct obj_cgroup *cached =READ_ONCE (stock->cached[i]); + +if (!cached) { +if (empty_slot ==-1 ) + empty_slot = i; +continue ; + } +if (cached == objcg) { + slot = i; +break ; + } + } + +if (slot ==-1 ) { + slot = empty_slot; +if (slot ==-1 ) { + slot =get_random_u32_below (NR_OBJ_STOCK ); +drain_obj_stock_slot (stock, slot); + }obj_cgroup_get (objcg); +/* +* Keep the xchg result in the unsigned int local; storing +* it directly into stock->nr_bytes[slot] (uint16_t) would +* silently truncate values >= U16_MAX and bypass the flush +* guard below, leaking page-counter charges. +*/ stock_nr_bytes =atomic_read (&objcg->nr_charged_bytes) ?atomic_xchg (&objcg->nr_charged_bytes,0 ) :0 ; -WRITE_ONCE (stock->cached_objcg, objcg); +WRITE_ONCE (stock->cached[slot], objcg); allow_uncharge = true;/* Allow uncharge when objcg changes */ + }else { + stock_nr_bytes = stock->nr_bytes[slot]; } + stock_nr_bytes += nr_bytes; -/* Since stock->nr_bytes is uint16_t, don't refill >= U16_MAX */ +/* nr_bytes[] is uint16_t; flush if we would refill >= U16_MAX. */ if ((allow_uncharge && (stock_nr_bytes >PAGE_SIZE )) || stock_nr_bytes >=U16_MAX ) { nr_pages = stock_nr_bytes >>PAGE_SHIFT ;
@@ -3384,7 +3465,7 @@ static void __refill_obj_stock(struct obj_cgroup *objcg, stock_nr_bytes = kept; } } - stock->nr_bytes = stock_nr_bytes; + stock->nr_bytes[slot] = stock_nr_bytes; out:if (nr_pages)
Signature · 1 line
2.53.0-Meta
Re: [PATCH 4/4] memcg: multi objcg charge support
Quoted 49 lines · Shakeel Butt
Commit
01b9da291c49("mm: memcontrol: convert objcg to be per-memcg per-node type") split a memcg's singleobj_cgroupinto one per NUMA node so that reparenting LRU folios can take per-node lru locks. As a side effect, the per-CPUobj_stock_pcp-- which caches exactly onecached_objcg-- thrashes on workloads where threads of the same memcg run on different NUMA nodes. The kernel test robot reported a 67.7% regression on stress-ng.switch.ops_per_sec from this pattern.Mirror the multi-slot pattern already used by
memcg_stock_pcp: turnnr_bytesandcached_objcginto NR_OBJ_STOCK-element arrays, scan all slots on consume/refill/account, prefer empty slots when inserting, and evict a random slot only when full. With multiple slots a CPU can hold the per-node objcg variants of one memcg plus a few siblings without ever forcing a drain.A single
int8_tindex records which slot the cached slab stats belong to; the stats are flushed on slot or pgdat change. WithNR_OBJ_STOCK= 5 the layout (verified with pahole) is:offset 0 : lock(1) + index(1) +
node_id(2)+ slab stats(4) = 8Boffset 8 : nr_bytes[5] = 10B offset 18 : padding = 6B offset 24 : cached[5] = 40Boffset 64 : (line 2)
work_struct+ flags (cold)so
consume_obj_stock,refill_obj_stockand the slab account path each touch exactly one 64-byte cache line on non-debug 64-bit builds.Reported-by kernel test robot <[email protected]>Fixes 01b9da291c49 ("mm: memcontrol: convert objcg to be per-memcg per-node type")Signed-off-by Shakeel Butt <[email protected]>Tested-by kernel test robot <[email protected]>---
mm/memcontrol.c +8
-3@@ -3350,19 +3405,45 @@ static void __refill_obj_stock(struct obj_cgroup *objcg, goto out; } - stock_nr_bytes = stock->nr_bytes; -if (READ_ONCE (stock->cached_objcg) != objcg) {/* reset if necessary */ -drain_obj_stock (stock); +for (i =0 ; i <NR_OBJ_STOCK ; ++i) { +struct obj_cgroup *cached =READ_ONCE (stock->cached[i]); + +if (!cached) { +if (empty_slot ==-1 ) + empty_slot = i; +continue ; + }
mm/memcontrol.c static void __refill_obj_stock(struct obj_cgroup *objcg, · line 3425
Quoted 10 lines
mm/memcontrol.c +10
-0+if (cached == objcg) { + slot = i; +break ; + } + } + +if (slot ==-1 ) { + slot = empty_slot; +if (slot ==-1 ) { + slot =get_random_u32_below (NR_OBJ_STOCK );
It would break kmalloc_nolock() because _get_random_bytes() uses a spinlock. perhaps prandom_u32_state() should be sufficient in this case.
Is there a reason why it uses random eviction, unlike multi-memcg percpu charge cache?
Otherwise LGTM!
Signature · 2 lines
Cheers, Harry / Hyeonggon
Shakeel Butt
maintainerRe: [PATCH 4/4] memcg: multi objcg charge support
Quoted 65 lines · Harry Yoo
Quoted 48 lines · Shakeel Butt
Commit
01b9da291c49("mm: memcontrol: convert objcg to be per-memcg per-node type") split a memcg's singleobj_cgroupinto one per NUMA node so that reparenting LRU folios can take per-node lru locks. As a side effect, the per-CPUobj_stock_pcp-- which caches exactly onecached_objcg-- thrashes on workloads where threads of the same memcg run on different NUMA nodes. The kernel test robot reported a 67.7% regression on stress-ng.switch.ops_per_sec from this pattern.Mirror the multi-slot pattern already used by
memcg_stock_pcp: turnnr_bytesandcached_objcginto NR_OBJ_STOCK-element arrays, scan all slots on consume/refill/account, prefer empty slots when inserting, and evict a random slot only when full. With multiple slots a CPU can hold the per-node objcg variants of one memcg plus a few siblings without ever forcing a drain.A single
int8_tindex records which slot the cached slab stats belong to; the stats are flushed on slot or pgdat change. WithNR_OBJ_STOCK= 5 the layout (verified with pahole) is:offset 0 : lock(1) + index(1) +
node_id(2)+ slab stats(4) = 8Boffset 8 : nr_bytes[5] = 10B offset 18 : padding = 6B offset 24 : cached[5] = 40Boffset 64 : (line 2)
work_struct+ flags (cold)so
consume_obj_stock,refill_obj_stockand the slab account path each touch exactly one 64-byte cache line on non-debug 64-bit builds.Reported-by kernel test robot <[email protected]>Fixes 01b9da291c49 ("mm: memcontrol: convert objcg to be per-memcg per-node type")Signed-off-by Shakeel Butt <[email protected]>Tested-by kernel test robot <[email protected]>---
mm/memcontrol.c +8
-3@@ -3350,19 +3405,45 @@ static void __refill_obj_stock(struct obj_cgroup *objcg, goto out; } - stock_nr_bytes = stock->nr_bytes; -if (READ_ONCE (stock->cached_objcg) != objcg) {/* reset if necessary */ -drain_obj_stock (stock); +for (i =0 ; i <NR_OBJ_STOCK ; ++i) { +struct obj_cgroup *cached =READ_ONCE (stock->cached[i]); + +if (!cached) { +if (empty_slot ==-1 ) + empty_slot = i; +continue ; + }mm/memcontrol.c static void __refill_obj_stock(struct obj_cgroup *objcg, · line 3424
Quoted 10 lines
mm/memcontrol.c +10
-0+if (cached == objcg) { + slot = i; +break ; + } + } + +if (slot ==-1 ) { + slot = empty_slot; +if (slot ==-1 ) { + slot =get_random_u32_below (NR_OBJ_STOCK );It would break
kmalloc_nolock()because_get_random_bytes()uses a spinlock. perhapsprandom_u32_state()should be sufficient in this case.
Quoted 2 lines
Is there a reason why it uses random eviction, unlike multi-memcg percpu charge cache?
Oh I didn't know and actually we are already using get_random_u32_below() in refill_stock(). So, it need fixing as well. That would be a separate patch.
I will explore prandom_u32_state().
Re: [PATCH 4/4] memcg: multi objcg charge support
Quoted 20 lines · Shakeel Butt
Quoted 19 lines · Harry Yoo
Quoted 19 lines
mm/memcontrol.c +13
-3@@ -3350,19 +3405,45 @@ static void __refill_obj_stock(struct obj_cgroup *objcg, goto out; } - stock_nr_bytes = stock->nr_bytes; -if (READ_ONCE (stock->cached_objcg) != objcg) {/* reset if necessary */ -drain_obj_stock (stock); +for (i =0 ; i <NR_OBJ_STOCK ; ++i) { +struct obj_cgroup *cached =READ_ONCE (stock->cached[i]); + +if (!cached) { +if (empty_slot ==-1 ) + empty_slot = i; +continue ; + } +if (cached == objcg) { + slot = i; +break ; + } + }
Quoted 8 lines
Quoted 8 lines
mm/memcontrol.c static void __refill_obj_stock(struct obj_cgroup *objcg, · line 3425
Quoted 5 lines
mm/memcontrol.c +5
-0+ +if (slot ==-1 ) { + slot = empty_slot; +if (slot ==-1 ) { + slot =get_random_u32_below (NR_OBJ_STOCK );It would break
kmalloc_nolock()because_get_random_bytes()uses a spinlock. perhapsprandom_u32_state()should be sufficient in this case.
s/spinlock/local_lock/
Quoted 5 lines
Quoted 2 lines
Is there a reason why it uses random eviction, unlike multi-memcg percpu charge cache?
Oh I didn't know and actually we are already using
get_random_u32_below()inrefill_stock(). So, it need fixing as well. That would be a separate patch.
Ouch, I see.
Quoted 1 line
I will explore
prandom_u32_state().
Thanks!
FYI, SLUB had a similar issue that was recently fixed:
commit a1e244a9f1778 ("mm/slab: use prandom if !allow_spin").
It uses prandom if spinning is not allowed when shuffling slab freelist.
Signature · 2 lines
Cheers, Harry / Hyeonggon
Shakeel Butt
maintainerRe: [PATCH 4/4] memcg: multi objcg charge support
Quoted 32 lines · Harry Yoo
Quoted 20 lines · Shakeel Butt
Quoted 19 lines · Harry Yoo
Quoted 19 lines
mm/memcontrol.c +13
-3@@ -3350,19 +3405,45 @@ static void __refill_obj_stock(struct obj_cgroup *objcg, goto out; } - stock_nr_bytes = stock->nr_bytes; -if (READ_ONCE (stock->cached_objcg) != objcg) {/* reset if necessary */ -drain_obj_stock (stock); +for (i =0 ; i <NR_OBJ_STOCK ; ++i) { +struct obj_cgroup *cached =READ_ONCE (stock->cached[i]); + +if (!cached) { +if (empty_slot ==-1 ) + empty_slot = i; +continue ; + } +if (cached == objcg) { + slot = i; +break ; + } + }Quoted 8 lines
Quoted 8 lines
mm/memcontrol.c static void __refill_obj_stock(struct obj_cgroup *objcg, · line 3425
Quoted 5 lines
mm/memcontrol.c +5
-0+ +if (slot ==-1 ) { + slot = empty_slot; +if (slot ==-1 ) { + slot =get_random_u32_below (NR_OBJ_STOCK );It would break
kmalloc_nolock()because_get_random_bytes()uses a spinlock. perhapsprandom_u32_state()should be sufficient in this case.
Quoted 1 line
s/spinlock/local_lock/
I do see spinlock in crng_make_state() for some code paths.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Is there a reason why it uses random eviction, unlike multi-memcg percpu charge cache?
Oh I didn't know and actually we are already using
get_random_u32_below()inrefill_stock(). So, it need fixing as well. That would be a separate patch.Ouch, I see.
Quoted 8 lines
Quoted 1 line
I will explore
prandom_u32_state().Thanks!
FYI, SLUB had a similar issue that was recently fixed:
commita1e244a9f1778("mm/slab: use prandom if !allow_spin").It uses prandom if spinning is not allowed when shuffling slab freelist.
The drain does not really need a random number. Fixing an index like 0 makes it much simpler but it might expose some corner cases. Round robin might be enough for this though. I will see what would be the easiest way forward.
Re: [PATCH 4/4] memcg: multi objcg charge support
In reply to Shakeel ButtQuoted 66 lines · On Wed, 20 May 2026 18:05:23 -0700 Shakeel Butt <[email protected]> wrote:
Quoted 64 lines · Harry Yoo
Quoted 48 lines · Shakeel Butt
Commit
01b9da291c49("mm: memcontrol: convert objcg to be per-memcg per-node type") split a memcg's singleobj_cgroupinto one per NUMA node so that reparenting LRU folios can take per-node lru locks. As a side effect, the per-CPUobj_stock_pcp-- which caches exactly onecached_objcg-- thrashes on workloads where threads of the same memcg run on different NUMA nodes. The kernel test robot reported a 67.7% regression on stress-ng.switch.ops_per_sec from this pattern.Mirror the multi-slot pattern already used by
memcg_stock_pcp: turnnr_bytesandcached_objcginto NR_OBJ_STOCK-element arrays, scan all slots on consume/refill/account, prefer empty slots when inserting, and evict a random slot only when full. With multiple slots a CPU can hold the per-node objcg variants of one memcg plus a few siblings without ever forcing a drain.A single
int8_tindex records which slot the cached slab stats belong to; the stats are flushed on slot or pgdat change. WithNR_OBJ_STOCK= 5 the layout (verified with pahole) is:offset 0 : lock(1) + index(1) +
node_id(2)+ slab stats(4) = 8Boffset 8 : nr_bytes[5] = 10B offset 18 : padding = 6B offset 24 : cached[5] = 40Boffset 64 : (line 2)
work_struct+ flags (cold)so
consume_obj_stock,refill_obj_stockand the slab account path each touch exactly one 64-byte cache line on non-debug 64-bit builds.Reported-by kernel test robot <[email protected]>Fixes 01b9da291c49 ("mm: memcontrol: convert objcg to be per-memcg per-node type")Signed-off-by Shakeel Butt <[email protected]>Tested-by kernel test robot <[email protected]>---
mm/memcontrol.c +8
-3@@ -3350,19 +3405,45 @@ static void __refill_obj_stock(struct obj_cgroup *objcg, goto out; } - stock_nr_bytes = stock->nr_bytes; -if (READ_ONCE (stock->cached_objcg) != objcg) {/* reset if necessary */ -drain_obj_stock (stock); +for (i =0 ; i <NR_OBJ_STOCK ; ++i) { +struct obj_cgroup *cached =READ_ONCE (stock->cached[i]); + +if (!cached) { +if (empty_slot ==-1 ) + empty_slot = i; +continue ; + }mm/memcontrol.c static void __refill_obj_stock(struct obj_cgroup *objcg, · line 3424
Quoted 10 lines
mm/memcontrol.c +10
-0+if (cached == objcg) { + slot = i; +break ; + } + } + +if (slot ==-1 ) { + slot = empty_slot; +if (slot ==-1 ) { + slot =get_random_u32_below (NR_OBJ_STOCK );It would break
kmalloc_nolock()because_get_random_bytes()uses a spinlock. perhapsprandom_u32_state()should be sufficient in this case.
Quoted 5 lines
Quoted 2 lines
Is there a reason why it uses random eviction, unlike multi-memcg percpu charge cache?
Oh I didn't know and actually we are already using
get_random_u32_below()inrefill_stock(). So, it need fixing as well. That would be a separate patch.
Hello Harry and Shakeel!
I just wanted to note that my work to push the memcg stock to page_counter [1] actually gets rid of the random eviction and also the get_random_u32_below. So the problem will no longer exist, at least for memcg/page_counter stock.
I was also thinking whether there was any way we can abstract or push down the objcg stock to a different per_X slot (with finer granularity than per-node) so we don't do random evictions, but it doesn't seem like there's a natural point to do this from a quick glance.
The fix for get_random_32_below to use prandom_u32_state seems quite simple though, so if either of you would like to send that in as a small hotfix it shouldn't be a big deal to rebase on top of that instead too.
(Sorry for joining the conversation late too. I'll take a closer look at this entire series tomorrow as well).
Thank you both. Have a great day!
Joshua