memcg: group struct fields by access pattern
Subsystems (3)
Shakeel Butt
maintainercover memcg: group struct fields by access pattern
Every so often we get a memcg performance regression caused by nothing more than a field moving. Someone adds a field, removes one, or puts a few behind a config option. The layout shifts, fields with different access patterns land on the same cache line, and a bot reports a regression.
Two examples. commit 98c9daf5ae6b ("mm: memcg: guard memcg1-specific members of struct mem_cgroup_per_node") moved lruvec next to lru_zone_size[] and needed commit f59adcf59332 ("mm: memcg: add cacheline padding after lruvec in mem_cgroup_per_node") to fix it. commit c1afbd5de131 ("mm/memcontrol: avoid false sharing between vmstats and events") had to add ____cacheline_aligned_in_smp for the same reason.
Each fix was correct but nothing stops the next field addition from undoing it.
This series makes the layout a contract the compiler checks, the same way struct net_device does it. Fields are sorted into named cache line groups by access pattern, and memcg_struct_check() verifies at build time that every field sits in its group. A field added in the wrong place now breaks the build instead of quietly costing a few percent.
struct mem_cgroup gets three groups:
memcg_write_hot written on the charge, reclaim and socket paths memcg_cold only the cgroup control paths touch these memcg_read_mostly set when the memcg is created, then only read
Testing
=======
The cgroup selftests give identical results with and without the series.
For performance, two identical 30 core Xeon machines each ran both kernels, with the boot order swapped between them so that machine and order effects cancel. The useful tests run two workloads at once in one cgroup, because false sharing only shows up when one side reads a field that the other side writes.
slab allocs + page faults, slab side +1.2%
page faults + memory.stat readers, fault side +1.3% page faults + memory.stat readers, reader side +0.8%
everything else no change
No test regressed. The gains are small but the point of the series is the build time contract.
Shakeel Butt (6):
memcg: move per-node objcg to the read-mostly fields
memcg: split mem_cgroup_private_id into two fields
memcg: group the write-hot fields of struct mem_cgroup
memcg: group the cold fields of struct mem_cgroup
memcg: group the read-mostly fields of struct mem_cgroup
memcg: group the fields of struct mem_cgroup_per_node
include/linux/memcontrol.h | 169 ++++++++++++++++++++++--------------- mm/memcontrol.c | 122 ++++++++++++++++++++++++-- 2 files changed, 213 insertions(+), 78 deletions(-)
base-commit: 817d340204c513316223ba084615f5906ac49ddb
Signature · 1 line
2.53.0-Meta
Shakeel Butt
maintainerPATCH 1/6 memcg: move per-node objcg to the read-mostly fields
current_obj_cgroup() reads memcg->nodeinfo[nid]->objcg on every accounted allocation. The field sits at the end of struct mem_cgroup_per_node, on the same cache line as lru_zone_size[] and iter. lru_zone_size[] is written on every LRU add and remove, and iter is written on every reclaim iteration.
Move objcg next to the other read-mostly pointers at the start of the struct.
No functional change.
--- include/linux/memcontrol.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
include/linux/memcontrol.h +2 -3
@@ -94,6 +94,7 @@ struct mem_cgroup_per_node {struct lruvec_stats_percpu __percpu *lruvec_stats_percpu;struct lruvec_stats *lruvec_stats;struct shrinker_info __rcu *shrinker_info; +struct obj_cgroup __rcu *objcg;CACHELINE_PADDING (_pad1_);
@@ -104,11 +105,9 @@ struct mem_cgroup_per_node {struct mem_cgroup_reclaim_iter iter;/* -* objcg is wiped out as a part of the objcg repaprenting process. * orig_objcg preserves a pointer (and a reference) to the original -* objcg until the end of live of memcg. +* objcg until the end of life of memcg. */ -struct obj_cgroup __rcu *objcg;struct obj_cgroup *orig_objcg;/* list of inherited objcgs, protected by objcg_lock */ struct list_head objcg_list;
Signature · 1 line
2.53.0-Meta
Shakeel Butt
maintainerPATCH 2/6 memcg: split mem_cgroup_private_id into two fields
In reply to Shakeel ButtThe two members of struct mem_cgroup_private_id have different access patterns. The id is read on every eviction and refault through mem_cgroup_private_id(), and is only written when the memcg is created and destroyed. The ref is written on every swap charge and uncharge.
Split them into private_id and private_id_ref so a later patch can put them into different cache line groups. A struct member cannot be split across two groups.
No functional change.
--- include/linux/memcontrol.h | 10 +++------- mm/memcontrol.c | 18 +++++++++--------- 2 files changed, 12 insertions(+), 16 deletions(-)
include/linux/memcontrol.h +3 -7
@@ -66,11 +66,6 @@ struct mem_cgroup_reclaim_cookie {#define MEM_CGROUP_ID_SHIFT 16 -struct mem_cgroup_private_id { -int id; -refcount_t ref; -}; -struct memcg_vmstats_percpu ;struct memcg1_events_percpu ;struct memcg_vmstats ;
@@ -189,7 +184,8 @@ struct mem_cgroup {struct cgroup_subsys_state css;/* Private memcg ID. Used to ID objects that outlive the cgroup */ -struct mem_cgroup_private_id id; +int private_id; +refcount_t private_id_ref;/* Accounted resources */ struct page_counter memory;/* Both v1 & v2 */
@@ -811,7 +807,7 @@ static inline unsigned short mem_cgroup_private_id(struct mem_cgroup *memcg)if (mem_cgroup_disabled ())return 0 ; -return memcg->id.id; +return memcg->private_id; }struct mem_cgroup *mem_cgroup_from_private_id (unsigned short id);
mm/memcontrol.c +9 -9
@@ -3797,7 +3797,7 @@ static void memcg_online_kmem(struct mem_cgroup *memcg)static_branch_enable (&memcg_kmem_online_key); - memcg->kmemcg_id = memcg->id.id; + memcg->kmemcg_id = memcg->private_id; }static void memcg_offline_kmem (struct mem_cgroup *memcg)
@@ -4056,15 +4056,15 @@ static DEFINE_XARRAY_ALLOC1(mem_cgroup_private_ids);static void mem_cgroup_private_id_remove (struct mem_cgroup *memcg) { -if (memcg->id.id >0 ) { -xa_erase (&mem_cgroup_private_ids, memcg->id.id); - memcg->id.id =0 ; +if (memcg->private_id >0 ) { +xa_erase (&mem_cgroup_private_ids, memcg->private_id); + memcg->private_id =0 ; } }static inline void mem_cgroup_private_id_put (struct mem_cgroup *memcg,unsigned int n) { -if (refcount_sub_and_test (n, &memcg->id.ref)) { +if (refcount_sub_and_test (n, &memcg->private_id_ref)) {mem_cgroup_private_id_remove (memcg);/* Memcg ID pins CSS */
@@ -4074,7 +4074,7 @@ static inline void mem_cgroup_private_id_put(struct mem_cgroup *memcg, unsignedstruct mem_cgroup *mem_cgroup_private_id_get_online (struct mem_cgroup *memcg,unsigned int n) { -while (!refcount_add_not_zero (n, &memcg->id.ref)) { +while (!refcount_add_not_zero (n, &memcg->private_id_ref)) {/* * The root cgroup cannot be destroyed, so it's refcount must * always be >= 1.
@@ -4198,7 +4198,7 @@ static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent)if (!memcg)return ERR_PTR (-ENOMEM ); - error =xa_alloc (&mem_cgroup_private_ids, &memcg->id.id,NULL , + error =xa_alloc (&mem_cgroup_private_ids, &memcg->private_id,NULL ,XA_LIMIT (1 ,MEM_CGROUP_ID_MAX ),GFP_KERNEL );if (error) goto fail;
@@ -4345,7 +4345,7 @@ static int mem_cgroup_css_online(struct cgroup_subsys_state *css)lru_gen_online_memcg (memcg);/* Online state pins memcg ID, memcg ID pins CSS */ -refcount_set (&memcg->id.ref,1 ); +refcount_set (&memcg->private_id_ref,1 );css_get (css);/*
@@ -4358,7 +4358,7 @@ static int mem_cgroup_css_online(struct cgroup_subsys_state *css)* publish it here at the end of onlining. This matches the * regular ID destruction during offlining. */ -xa_store (&mem_cgroup_private_ids, memcg->id.id, memcg,GFP_KERNEL ); +xa_store (&mem_cgroup_private_ids, memcg->private_id, memcg,GFP_KERNEL );return 0 ; free_objcg:
Signature · 1 line
2.53.0-Meta
Shakeel Butt
maintainerPATCH 3/6 memcg: group the write-hot fields of struct mem_cgroup
In reply to Shakeel ButtThese fields are written on the charge, reclaim and socket paths:
socket_pressure written by reclaim, read on every socket charge
memory_events bumped for this memcg and every ancestor, so a
busy child dirties the whole chain
memory_events_local
vmpressure written on every reclaim iteration
private_id_ref written on every swap charge and uncharge
kmem_stat
high_irq_work, high_work
They are spread over the struct today and share cache lines with read-mostly fields. Put them in one cache line group.
socket_pressure is kept next to memory_events because mem_cgroup_sk_under_memory_pressure() reads one and bumps the other.
Add memcg_struct_check() so the build fails if a field lands outside its group.
No functional change.
--- include/linux/memcontrol.h | 59 ++++++++++++++++++++++---------------- mm/memcontrol.c | 32 +++++++++++++++++++++ 2 files changed, 66 insertions(+), 25 deletions(-)
include/linux/memcontrol.h +34 -25
@@ -185,7 +185,6 @@ struct mem_cgroup {/* Private memcg ID. Used to ID objects that outlive the cgroup */ int private_id; -refcount_t private_id_ref;/* Accounted resources */ struct page_counter memory;/* Both v1 & v2 */
@@ -195,15 +194,45 @@ struct mem_cgroup {struct page_counter memsw;/* v1 only */ }; -/* registered local peak watchers */ -struct list_head memory_peaks; -struct list_head swap_peaks; -spinlock_t peaks_lock; +/* Written on the charge, reclaim and socket paths. */ +__cacheline_group_begin_aligned (memcg_write_hot); +/* +* Hint of reclaim pressure for socket memory management. Note +* that this indicator should NOT be used in legacy cgroup mode +* where socket memory is accounted/charged separately. +*/ +u64 socket_pressure; +#if BITS_PER_LONG <64 +seqlock_t socket_pressure_seqlock; +#endif +/* +* memory.events is bumped for this memcg and all its ancestors, so a +* busy child dirties every ancestor. +*/ +atomic_long_t memory_events[MEMCG_NR_MEMORY_EVENTS ]; +atomic_long_t memory_events_local[MEMCG_NR_MEMORY_EVENTS ]; + +/* vmpressure notifications. Written on every reclaim iteration. */ +struct vmpressure vmpressure; + +/* Written on every swap charge and uncharge. */ +refcount_t private_id_ref; +#ifdef CONFIG_MEMCG_NMI_SAFETY_REQUIRES_ATOMIC +/* MEMCG_KMEM for nmi context */ +atomic_t kmem_stat; +#endif /* Range enforcement for interrupt charges */ struct irq_work high_irq_work;struct work_struct high_work; +__cacheline_group_end_aligned (memcg_write_hot); + +/* registered local peak watchers */ +struct list_head memory_peaks; +struct list_head swap_peaks; +spinlock_t peaks_lock; +#ifdef CONFIG_ZSWAP unsigned long zswap_max;
@@ -214,9 +243,6 @@ struct mem_cgroup {bool zswap_writeback;#endif -/* vmpressure notifications */ -struct vmpressure vmpressure; -/* * Should the OOM killer kill all belonging tasks, had it kill one? */
@@ -232,23 +258,6 @@ struct mem_cgroup {/* memory.stat */ struct memcg_vmstats *vmstats; -/* memory.events */ -atomic_long_t memory_events[MEMCG_NR_MEMORY_EVENTS ]; -atomic_long_t memory_events_local[MEMCG_NR_MEMORY_EVENTS ]; - -#ifdef CONFIG_MEMCG_NMI_SAFETY_REQUIRES_ATOMIC -/* MEMCG_KMEM for nmi context */ -atomic_t kmem_stat; -#endif -/* -* Hint of reclaim pressure for socket memroy management. Note -* that this indicator should NOT be used in legacy cgroup mode -* where socket memory is accounted/charged separately. -*/ -u64 socket_pressure; -#if BITS_PER_LONG <64 -seqlock_t socket_pressure_seqlock; -#endif int kmemcg_id;#ifdef CONFIG_CGROUP_WRITEBACK
mm/memcontrol.c +32 -0
@@ -5726,6 +5726,36 @@ __setup("cgroup.memory=", cgroup_memory);* basically everything that doesn't depend on a specific mem_cgroup structure * should be initialized from here. */ +/* +* Fields are grouped by access pattern. Putting a field in the wrong group +* breaks the build here. +*/ +static void __init memcg_struct_check (void ) +{ + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_write_hot, + socket_pressure); +#if BITS_PER_LONG <64 + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_write_hot, + socket_pressure_seqlock); +#endif + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_write_hot, + memory_events); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_write_hot, + memory_events_local); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_write_hot, + vmpressure); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_write_hot, + private_id_ref); +#ifdef CONFIG_MEMCG_NMI_SAFETY_REQUIRES_ATOMIC + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_write_hot, + kmem_stat); +#endif + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_write_hot, + high_irq_work); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_write_hot, + high_work); +} +int __init mem_cgroup_init (void ) {unsigned int memcg_size;
@@ -5739,6 +5769,8 @@ int __init mem_cgroup_init(void)*/ BUILD_BUG_ON (MEMCG_CHARGE_BATCH >S32_MAX /PAGE_SIZE ); +memcg_struct_check (); +cpuhp_setup_state_nocalls (CPUHP_MM_MEMCQ_DEAD ,"mm/memctrl:dead" ,NULL , memcg_hotplug_cpu_dead);
Signature · 1 line
2.53.0-Meta
Shakeel Butt
maintainerPATCH 4/6 memcg: group the cold fields of struct mem_cgroup
In reply to Shakeel ButtThese fields are only touched by the cgroup control paths:
memory_peaks, swap_peaks, peaks_lock memory.peak open/read/release
events_file, events_local_file,
swap_events_file cgroup_file_notify()
cgwb_list, cgwb_domain, cgwb_frn writeback setup and the
foreign dirty slow path
mm_list MGLRU mm list
They sit in the middle of the struct today. The three cgroup_file members alone are 192 bytes of notify state next to the vmstats pointer. Put them in one cache line group.
No functional change.
--- include/linux/memcontrol.h | 49 ++++++++++++++++++++++---------------- mm/memcontrol.c | 25 +++++++++++++++++++ 2 files changed, 53 insertions(+), 21 deletions(-)
include/linux/memcontrol.h +28 -21
@@ -228,11 +228,37 @@ struct mem_cgroup {__cacheline_group_end_aligned (memcg_write_hot); +/* +* Off the charge and fault paths. Not write free: cgwb_domain is +* written on every writeout completion and mm_list on fork, exit and +* MGLRU aging. They are grouped here so those writes cannot land on +* a line that the fast paths read. +*/ +__cacheline_group_begin_aligned (memcg_cold);/* registered local peak watchers */ struct list_head memory_peaks;struct list_head swap_peaks;spinlock_t peaks_lock; +/* memory.events and memory.events.local */ +struct cgroup_file events_file; +struct cgroup_file events_local_file; + +/* handle for "memory.swap.events" */ +struct cgroup_file swap_events_file; + +#ifdef CONFIG_CGROUP_WRITEBACK +struct list_head cgwb_list; +struct wb_domain cgwb_domain; +struct memcg_cgwb_frn cgwb_frn[MEMCG_CGWB_FRN_CNT ]; +#endif + +#ifdef CONFIG_LRU_GEN_WALKS_MMU +/* per-memcg mm_struct list */ +struct lru_gen_mm_list mm_list; +#endif +__cacheline_group_end_aligned (memcg_cold); +#ifdef CONFIG_ZSWAP unsigned long zswap_max;
@@ -248,37 +274,18 @@ struct mem_cgroup {*/ bool oom_group; -/* memory.events and memory.events.local */ -struct cgroup_file events_file; -struct cgroup_file events_local_file; - -/* handle for "memory.swap.events" */ -struct cgroup_file swap_events_file; -/* memory.stat */ struct memcg_vmstats *vmstats;int kmemcg_id; -#ifdef CONFIG_CGROUP_WRITEBACK -struct list_head cgwb_list; -#endif -/* Keep the hot per-CPU stats pointer away from memory event counters. */ struct memcg_vmstats_percpu __percpu *vmstats_percpu____cacheline_aligned_in_smp ; -#ifdef CONFIG_CGROUP_WRITEBACK -struct wb_domain cgwb_domain; -struct memcg_cgwb_frn cgwb_frn[MEMCG_CGWB_FRN_CNT ]; -#endif - -#ifdef CONFIG_LRU_GEN_WALKS_MMU -/* per-memcg mm_struct list */ -struct lru_gen_mm_list mm_list; -#endif -#ifdef CONFIG_MEMCG_V1 +/* v1 only. Not grouped: v1 is legacy, sorting it is not worth it. */ +/* Legacy consumer-oriented counters */ struct page_counter kmem;/* v1 only */ struct page_counter tcpmem;/* v1 only */
mm/memcontrol.c +25 -0
@@ -5754,6 +5754,31 @@ static void __init memcg_struct_check(void) high_irq_work); CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_write_hot, high_work); + + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_cold, + memory_peaks); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_cold, + swap_peaks); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_cold, + peaks_lock); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_cold, + events_file); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_cold, + events_local_file); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_cold, + swap_events_file); +#ifdef CONFIG_CGROUP_WRITEBACK + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_cold, + cgwb_list); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_cold, + cgwb_domain); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_cold, + cgwb_frn); +#endif +#ifdef CONFIG_LRU_GEN_WALKS_MMU + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_cold, + mm_list); +#endif }int __init mem_cgroup_init (void )
Signature · 1 line
2.53.0-Meta
Shakeel Butt
maintainerPATCH 5/6 memcg: group the read-mostly fields of struct mem_cgroup
In reply to Shakeel ButtThese fields are set when the memcg is created and only read after that:
vmstats_percpu read on every stat update
vmstats
zswap_max, zswap_writeback
private_id read on every eviction and refault
kmemcg_id read on every list_lru lookup
oom_group
Put them in one cache line group at the end of the struct, right before nodeinfo[]. nodeinfo[] is read-mostly too but it is a flexible array, so it cannot sit inside a group. The group ends without padding so the two share a line.
This also drops the ____cacheline_aligned_in_smp on vmstats_percpu added by commit c1afbd5de131 ("mm/memcontrol: avoid false sharing between vmstats and events"). That only aligned the start of the field. cgwb_domain followed it on the same line and is written on every writeout completion. A group boundary covers both sides.
No functional change.
--- include/linux/memcontrol.h | 62 +++++++++++++++++++++----------------- mm/memcontrol.c | 17 +++++++++++ 2 files changed, 52 insertions(+), 27 deletions(-)
include/linux/memcontrol.h +35 -27
@@ -183,9 +183,6 @@ struct obj_cgroup {struct mem_cgroup {struct cgroup_subsys_state css; -/* Private memcg ID. Used to ID objects that outlive the cgroup */ -int private_id; -/* Accounted resources */ struct page_counter memory;/* Both v1 & v2 */
@@ -259,30 +256,6 @@ struct mem_cgroup {#endif __cacheline_group_end_aligned (memcg_cold); -#ifdef CONFIG_ZSWAP -unsigned long zswap_max; - -/* -* Prevent pages from this memcg from being written back from zswap to -* swap, and from being swapped out on zswap store failures. -*/ -bool zswap_writeback; -#endif - -/* -* Should the OOM killer kill all belonging tasks, had it kill one? -*/ -bool oom_group; - -/* memory.stat */ -struct memcg_vmstats *vmstats; - -int kmemcg_id; - -/* Keep the hot per-CPU stats pointer away from memory event counters. */ -struct memcg_vmstats_percpu __percpu *vmstats_percpu -____cacheline_aligned_in_smp ; -#ifdef CONFIG_MEMCG_V1 /* v1 only. Not grouped: v1 is legacy, sorting it is not worth it. */
@@ -322,6 +295,41 @@ struct mem_cgroup {int swappiness;#endif /* CONFIG_MEMCG_V1 */ +/* +* Set when the memcg is created and cleared when it is offlined. +* Never written on a hot path. +*/ +__cacheline_group_begin_aligned (memcg_read_mostly); +/* Read on every stat update */ +struct memcg_vmstats_percpu __percpu *vmstats_percpu; + +/* memory.stat */ +struct memcg_vmstats *vmstats; + +#ifdef CONFIG_ZSWAP +unsigned long zswap_max; +#endif + +/* Private memcg ID. Used to ID objects that outlive the cgroup */ +int private_id; + +int kmemcg_id; + +/* +* Should the OOM killer kill all belonging tasks, had it kill one? +*/ +bool oom_group; + +#ifdef CONFIG_ZSWAP +/* +* Prevent pages from this memcg from being written back from zswap to +* swap, and from being swapped out on zswap store failures. +*/ +bool zswap_writeback; +#endif +/* Not padded: nodeinfo[] is read-mostly too, let it share the line. */ +__cacheline_group_end (memcg_read_mostly); +struct mem_cgroup_per_node *nodeinfo[]; };
mm/memcontrol.c +17 -0
@@ -5779,6 +5779,23 @@ static void __init memcg_struct_check(void) CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_cold, mm_list);#endif + + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_read_mostly, + vmstats_percpu); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_read_mostly, + vmstats); +#ifdef CONFIG_ZSWAP + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_read_mostly, + zswap_max); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_read_mostly, + zswap_writeback); +#endif + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_read_mostly, + private_id); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_read_mostly, + kmemcg_id); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_read_mostly, + oom_group); }int __init mem_cgroup_init (void )
Signature · 1 line
2.53.0-Meta
Shakeel Butt
maintainerPATCH 6/6 memcg: group the fields of struct mem_cgroup_per_node
In reply to Shakeel ButtReplace the two ad-hoc CACHELINE_PADDING members with named cache line groups:
memcg_pn_read_mostly memcg, lruvec_stats_percpu, lruvec_stats,
shrinker_info, objcg
memcg_pn_lruvec lruvec memcg_pn_write_hot lru_zone_size, iter, nmi slab stats memcg_pn_cold orig_objcg, objcg_list
The group markers give the same isolation the padding did, but they are named and the build now checks them.
lruvec still gets its own lines. Commit f59adcf59332 ("mm: memcg: add cacheline padding after lruvec in mem_cgroup_per_node") showed why that matters: lru_zone_size[] is written under lru_lock but read without it by lruvec_lru_size(), so it must not share a line with lruvec.
Splitting the cold fields out costs one extra cache line per node per memcg.
No functional change.
--- include/linux/memcontrol.h | 32 ++++++++++++++++++++++---------- mm/memcontrol.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 10 deletions(-)
include/linux/memcontrol.h +22 -10
@@ -82,7 +82,8 @@ struct mem_cgroup_reclaim_iter {* per-node information in memory controller. */ struct mem_cgroup_per_node { -/* Keep the read-only fields at the start */ +/* Set when the memcg is created, then only read. */ +__cacheline_group_begin_aligned (memcg_pn_read_mostly);struct mem_cgroup *memcg;/* Back pointer, we cannot */ /* use container_of */
@@ -91,14 +92,30 @@ struct mem_cgroup_per_node {struct shrinker_info __rcu *shrinker_info;struct obj_cgroup __rcu *objcg; -CACHELINE_PADDING (_pad1_); +__cacheline_group_end_aligned (memcg_pn_read_mostly); -/* Fields which get updated often at the end. */ +/* +* Keep lruvec on its own lines. Sharing them with lru_zone_size[] +* regressed, see commit f59adcf59332 ("mm: memcg: add cacheline +* padding after lruvec in mem_cgroup_per_node"). +*/ +__cacheline_group_begin_aligned (memcg_pn_lruvec);struct lruvec lruvec; -CACHELINE_PADDING (_pad2_); +__cacheline_group_end_aligned (memcg_pn_lruvec); + +/* Written on every LRU update and on every reclaim iteration. */ +__cacheline_group_begin_aligned (memcg_pn_write_hot);unsigned long lru_zone_size[MAX_NR_ZONES ][NR_LRU_LISTS ];struct mem_cgroup_reclaim_iter iter; +#ifdef CONFIG_MEMCG_NMI_SAFETY_REQUIRES_ATOMIC +/* slab stats for nmi context */ +atomic_t slab_reclaimable; +atomic_t slab_unreclaimable; +#endif +__cacheline_group_end_aligned (memcg_pn_write_hot); +/* Touched only when the memcg is reparented or freed. */ +__cacheline_group_begin_aligned (memcg_pn_cold);/* * orig_objcg preserves a pointer (and a reference) to the original * objcg until the end of life of memcg.
@@ -106,12 +123,7 @@ struct mem_cgroup_per_node {struct obj_cgroup *orig_objcg;/* list of inherited objcgs, protected by objcg_lock */ struct list_head objcg_list; - -#ifdef CONFIG_MEMCG_NMI_SAFETY_REQUIRES_ATOMIC -/* slab stats for nmi context */ -atomic_t slab_reclaimable; -atomic_t slab_unreclaimable; -#endif +__cacheline_group_end_aligned (memcg_pn_cold); };struct mem_cgroup_threshold {
mm/memcontrol.c +30 -0
@@ -5796,6 +5796,36 @@ static void __init memcg_struct_check(void) kmemcg_id); CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup, memcg_read_mostly, oom_group); + + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup_per_node, + memcg_pn_read_mostly, memcg); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup_per_node, + memcg_pn_read_mostly, lruvec_stats_percpu); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup_per_node, + memcg_pn_read_mostly, lruvec_stats); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup_per_node, + memcg_pn_read_mostly, shrinker_info); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup_per_node, + memcg_pn_read_mostly, objcg); + + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup_per_node, + memcg_pn_lruvec, lruvec); + + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup_per_node, + memcg_pn_write_hot, lru_zone_size); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup_per_node, + memcg_pn_write_hot, iter); +#ifdef CONFIG_MEMCG_NMI_SAFETY_REQUIRES_ATOMIC + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup_per_node, + memcg_pn_write_hot, slab_reclaimable); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup_per_node, + memcg_pn_write_hot, slab_unreclaimable); +#endif + + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup_per_node, + memcg_pn_cold, orig_objcg); + CACHELINE_ASSERT_GROUP_MEMBER(struct mem_cgroup_per_node, + memcg_pn_cold, objcg_list); }int __init mem_cgroup_init (void )
Signature · 1 line
2.53.0-Meta
Andrew Morton
maintainerQuoted 6 lines · On Fri, 4 Sep 2026 20:05:16 -0700 Shakeel Butt <[email protected]> wrote:
Every so often we get a memcg performance regression caused by nothing more than a field moving. Someone adds a field, removes one, or puts a few behind a config option. The layout shifts, fields with different access patterns land on the same cache line, and a bot reports a regression.
Quoted 7 lines
...
This series makes the layout a contract the compiler checks, the same way struct
net_devicedoes it. Fields are sorted into named cache line groups by access pattern, andmemcg_struct_check()verifies at build time that every field sits in its group. A field added in the wrong place now breaks the build instead of quietly costing a few percent.
Sounds smart.
Significant repair work was needed for the struct mem_cgroup_per_node and struct mem_cgroup alterations, due to the below pending changes. I think I got it all, please check.
include/linux/memcontrol.h +1 -16
@@ -95,25 +95,12 @@ struct mem_cgroup_per_node {struct lruvec_stats *lruvec_stats;struct shrinker_info __rcu *shrinker_info; -#ifdef CONFIG_MEMCG_V1 -/* -* Memcg-v1 only stuff in middle as buffer between read mostly fields -* and update often fields to avoid false sharing. If v1 stuff is -* not present, an explicit padding is needed. -*/ - -struct rb_node tree_node;/* RB tree node */ -unsigned long usage_in_excess;/* Set to the value by which */ -/* the soft limit is exceeded*/ -bool on_tree; -#else CACHELINE_PADDING (_pad1_); -#endif /* Fields which get updated often at the end. */ struct lruvec lruvec;CACHELINE_PADDING (_pad2_); -unsigned long lru_zone_size[MAX_NR_ZONES ][NR_LRU_LISTS ]; +long lru_zone_size[MAX_NR_ZONES ][NR_LRU_LISTS ];struct mem_cgroup_reclaim_iter iter;/*
@@ -293,8 +280,6 @@ struct mem_cgroup {struct memcg1_events_percpu __percpu *events_percpu; -unsigned long soft_limit; -/* protected by memcg_oom_lock */ bool oom_lock;int under_oom;