[PATCH v3 0/2] memcg: reduce cacheline contention from lruvec stat

[PATCH v3 0/2] memcg: reduce cacheline contention from lruvec stat

[Joanne Koong]

Memcg lruvec stat updates read fields of struct mem_cgroup_per_node and struct
lruvec that can share a 128-byte cacheline pair with lines the lru_lock holder
writes constantly (the LRU list heads, lru_lock or lru_zone_size[]), depending
on where struct mem_cgroup_per_node lands in its slab. On CPUs that do spatial
prefetching (eg Intel CPUs), this makes the stat updates contend with the
lru_lock holder.

Patch 1 rearranges struct mem_cgroup_per_node so that its read-mostly line is
paired with a cold line, and patch 2 makes the folio and kmem stat updates use
the memcg and node they already know instead of going through the lruvec,
whose pgdat field shares a cacheline pair with lru_lock or lru_zone_size[].

Results on a 2-socket Skylake (80 CPUs), average pages per second for
threads faulting in and freeing memory in a single memcg:

	   base     patch 1   patch 1+2 
shmem, 20 threads  4.74M    5.59M     5.77M
shmem, 80 threads  7.94M    8.66M     8.82M
anon,  20 threads  3.04M    3.08M     3.10M

Changelog
---------
v2: https://lore.kernel.org/linux-mm/20261022223930.138779-1-joannelkoong@gmail.com/
Changes from v2 -> v3:
- Add in a comment about memcg_pn_cold placement after memcg_pn_read_mostly
  (Shakeel)
- Add Shakeel's Acked-bys

v1: https://lore.kernel.org/linux-mm/20261020200730.250189-1-joannelkoong@gmail.com/
Changes from v1 -> v2:
- Replace the lruvec_stats_percpu pointers in struct mem_cgroup with a
  rearranged struct mem_cgroup_per_node layout (Shakeel)
- Move lruvec_stat_mod_folio()/mod_lruvec_kmem_state() change into its own
  patch (Shakeel)

Joanne Koong (2):
  memcg: reduce cacheline contention between lruvec stat updates and LRU
    lists
  memcg: don't go through the lruvec for folio and kmem lruvec stat
    updates

 include/linux/memcontrol.h | 29 ++++++++++------
 mm/memcontrol.c            | 71 ++++++++++++++++----------------------
 2 files changed, 47 insertions(+), 53 deletions(-)


base-commit: fcec4f4bcc1101dbd7e3aff267856b3bbfe128c6
-- 
2.52.0

[PATCH v3 1/2] memcg: reduce cacheline contention between lruvec stat updates and LRU lists

[Joanne Koong]

Every memcg lruvec (per-node) stat update reads the memcg and
lruvec_stats_percpu pointers in the first 64 bytes of struct
mem_cgroup_per_node. These updates are extremely frequent (eg these
updates happen several times for every page that's faulted in, freed,
dirtied, or written back).

The memcg_pn_read_mostly cacheline group keeps those read-mostly fields
on a different cacheline from the lruvec, but Intel CPUs do spatial
prefetching [1] and fetch cachelines in 128-byte aligned pairs.
Depending on where struct mem_cgroup_per_node lands in its slab, that
cacheline can be paired with the cacheline holding the LRU list heads.

When this happens, every stat update also pulls in a copy of the LRU
list heads cacheline, which is written to constantly by other CPUs under
the lru_lock. The stat update has to fetch it from another CPU, which is
unnecessary overhead since the stat update doesn't use the LRU list
heads at all. But even worse, the lru_lock holder then has to invalidate
those copies and wait for the acknowledgements to come back before it
can do its next write, which makes the lru_lock critical section longer
for everyone waiting on the lock.

This was found when analyzing the output of perf c2c for a benchmark
where many threads fault in pages in one memcg. It showed the first 64
bytes of mem_cgroup_per_node as the most contended cacheline (over a
third of all HITMs and 5x more than the lru_lock cacheline) even though
there are no writes to it. A quick experiment adding extra padding so
that the cacheline no longer shared a pairing with the LRU list heads
showed that this removed most of the contention, which confirmed the
cause.

Fix this by moving the memcg_pn_cold group, which is only touched when
the memcg is reparented or freed, right after memcg_pn_read_mostly, so
that the read-mostly cacheline is paired with the cold cacheline
instead of the LRU list heads.

Results from a multi-threaded fault/free benchmark in a single memcg,
run on an Intel Skylake machine (80 threads, 2 NUMA nodes) with threads
pinned to CPUs showed average pages faulted in (and freed) per second
across all threads, over 7 freshly created memcgs per run:

                     before    after
  shmem, 20 threads  4.74M     5.59M   (+18%)
  shmem, 80 threads  7.94M     8.66M   (+9%)
  anon,  20 threads  3.04M     3.08M   (+1%)

[1] https://www.intel.com/content/dam/doc/manual/64-ia-32-architectures-optimization-manual.pdf
(bottom of page 62/800)

Link: https://lore.kernel.org/linux-fsdevel/CAJnrk1ZezmmzrYdf8KP6re_a76izMS=qGPeC8ZZXiteHyZZC-Q@mail.gmail.com/
Suggested-by: Shakeel Butt <shakeel.butt@linux.dev>
Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 include/linux/memcontrol.h | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 69e1bcc67c58..6139195b7fa4 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -95,6 +95,24 @@ struct mem_cgroup_per_node {
 
 	__cacheline_group_end_aligned(memcg_pn_read_mostly);
 
+	/*
+	 * Touched only when the memcg is reparented or freed. Placed right
+	 * after memcg_pn_read_mostly because CPUs with spatial prefetching
+	 * (eg most Intel CPUs) fetch cachelines in 128-byte aligned pairs.
+	 * This keeps the read-mostly line, which every lruvec stat update
+	 * reads, from being paired with the LRU list heads, which are written
+	 * constantly under lru_lock.
+	 */
+	__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.
+	 */
+	struct obj_cgroup	*orig_objcg;
+	/* list of inherited objcgs, protected by objcg_lock */
+	struct list_head objcg_list;
+	__cacheline_group_end_aligned(memcg_pn_cold);
+
 	/*
 	 * Keep lruvec on its own lines. Sharing them with lru_zone_size[]
 	 * regressed, see commit f59adcf59332 ("mm: memcg: add cacheline
@@ -114,17 +132,6 @@ struct mem_cgroup_per_node {
 	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.
-	 */
-	struct obj_cgroup	*orig_objcg;
-	/* list of inherited objcgs, protected by objcg_lock */
-	struct list_head objcg_list;
-	__cacheline_group_end_aligned(memcg_pn_cold);
 };
 
 struct mem_cgroup_threshold {
-- 
2.52.0

Re: [PATCH v3 1/2] memcg: reduce cacheline contention between lruvec stat updates and LRU lists

[Michal Hocko]

On Fri 23-10-26 12:54:16, Joanne Koong wrote:
> Every memcg lruvec (per-node) stat update reads the memcg and
> lruvec_stats_percpu pointers in the first 64 bytes of struct
> mem_cgroup_per_node. These updates are extremely frequent (eg these
> updates happen several times for every page that's faulted in, freed,
> dirtied, or written back).
> 
> The memcg_pn_read_mostly cacheline group keeps those read-mostly fields
> on a different cacheline from the lruvec, but Intel CPUs do spatial
> prefetching [1] and fetch cachelines in 128-byte aligned pairs.
> Depending on where struct mem_cgroup_per_node lands in its slab, that
> cacheline can be paired with the cacheline holding the LRU list heads.
> 
> When this happens, every stat update also pulls in a copy of the LRU
> list heads cacheline, which is written to constantly by other CPUs under
> the lru_lock. The stat update has to fetch it from another CPU, which is
> unnecessary overhead since the stat update doesn't use the LRU list
> heads at all. But even worse, the lru_lock holder then has to invalidate
> those copies and wait for the acknowledgements to come back before it
> can do its next write, which makes the lru_lock critical section longer
> for everyone waiting on the lock.
> 
> This was found when analyzing the output of perf c2c for a benchmark
> where many threads fault in pages in one memcg. It showed the first 64
> bytes of mem_cgroup_per_node as the most contended cacheline (over a
> third of all HITMs and 5x more than the lru_lock cacheline) even though
> there are no writes to it. A quick experiment adding extra padding so
> that the cacheline no longer shared a pairing with the LRU list heads
> showed that this removed most of the contention, which confirmed the
> cause.
> 
> Fix this by moving the memcg_pn_cold group, which is only touched when
> the memcg is reparented or freed, right after memcg_pn_read_mostly, so
> that the read-mostly cacheline is paired with the cold cacheline
> instead of the LRU list heads.
> 
> Results from a multi-threaded fault/free benchmark in a single memcg,
> run on an Intel Skylake machine (80 threads, 2 NUMA nodes) with threads
> pinned to CPUs showed average pages faulted in (and freed) per second
> across all threads, over 7 freshly created memcgs per run:
> 
>                      before    after
>   shmem, 20 threads  4.74M     5.59M   (+18%)
>   shmem, 80 threads  7.94M     8.66M   (+9%)
>   anon,  20 threads  3.04M     3.08M   (+1%)
> 
> [1] https://www.intel.com/content/dam/doc/manual/64-ia-32-architectures-optimization-manual.pdf
> (bottom of page 62/800)
> 
> Link: https://lore.kernel.org/linux-fsdevel/CAJnrk1ZezmmzrYdf8KP6re_a76izMS=qGPeC8ZZXiteHyZZC-Q@mail.gmail.com/
> Suggested-by: Shakeel Butt <shakeel.butt@linux.dev>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>

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

> ---
>  include/linux/memcontrol.h | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 69e1bcc67c58..6139195b7fa4 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -95,6 +95,24 @@ struct mem_cgroup_per_node {
>  
>  	__cacheline_group_end_aligned(memcg_pn_read_mostly);
>  
> +	/*
> +	 * Touched only when the memcg is reparented or freed. Placed right
> +	 * after memcg_pn_read_mostly because CPUs with spatial prefetching
> +	 * (eg most Intel CPUs) fetch cachelines in 128-byte aligned pairs.
> +	 * This keeps the read-mostly line, which every lruvec stat update
> +	 * reads, from being paired with the LRU list heads, which are written
> +	 * constantly under lru_lock.
> +	 */
> +	__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.
> +	 */
> +	struct obj_cgroup	*orig_objcg;
> +	/* list of inherited objcgs, protected by objcg_lock */
> +	struct list_head objcg_list;
> +	__cacheline_group_end_aligned(memcg_pn_cold);
> +
>  	/*
>  	 * Keep lruvec on its own lines. Sharing them with lru_zone_size[]
>  	 * regressed, see commit f59adcf59332 ("mm: memcg: add cacheline
> @@ -114,17 +132,6 @@ struct mem_cgroup_per_node {
>  	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.
> -	 */
> -	struct obj_cgroup	*orig_objcg;
> -	/* list of inherited objcgs, protected by objcg_lock */
> -	struct list_head objcg_list;
> -	__cacheline_group_end_aligned(memcg_pn_cold);
>  };
>  
>  struct mem_cgroup_threshold {
> -- 
> 2.52.0

-- 
Michal Hocko
SUSE Labs

[PATCH v3 2/2] memcg: don't go through the lruvec for folio and kmem lruvec stat updates

[Joanne Koong]

lruvec_stat_mod_folio() and mod_lruvec_kmem_state() already know the memcg
and the node, but look up the lruvec with mem_cgroup_lruvec() and update
the stats through mod_lruvec_state(), which reads lruvec->pgdat and
mem_cgroup_per_node->memcg to get them back. lruvec->pgdat shares a
128-byte cacheline pair with lines that the lru_lock holder writes
constantly (lru_lock or lru_zone_size[], depending on the alignment of
struct mem_cgroup_per_node), so on CPUs that do spatial prefetching
these per-folio stat updates contend with the lru_lock holder.

Pass the memcg and node they already know to mod_memcg_node_state()
instead.

On top of the mem_cgroup_per_node rearrangement, on a 2-socket Skylake
(80 CPUs) with threads faulting in and freeing shmem in a single memcg,
this improves throughput by about 3% at 20 threads (5.59M -> 5.77M
pages/sec) and 2% at 80 threads (8.66M -> 8.82M).

Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
---
 mm/memcontrol.c | 71 ++++++++++++++++++++-----------------------------
 1 file changed, 29 insertions(+), 42 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index a05d9a8baa1b..9f33cc5c8293 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -587,7 +587,7 @@ unsigned long lruvec_page_state_local(const struct lruvec *lruvec,
 	return x;
 }
 
-static void __mod_memcg_lruvec_state(struct mem_cgroup_per_node *pn,
+static void __mod_memcg_lruvec_state(struct mem_cgroup *memcg, int nid,
 				     enum node_stat_item idx, long val);
 
 void reparent_memcg_lruvec_state_local(struct mem_cgroup *memcg,
@@ -596,16 +596,11 @@ void reparent_memcg_lruvec_state_local(struct mem_cgroup *memcg,
 	int nid;
 
 	for_each_node(nid) {
-		struct lruvec *child_lruvec = mem_cgroup_lruvec(memcg, NODE_DATA(nid));
-		struct lruvec *parent_lruvec = mem_cgroup_lruvec(parent, NODE_DATA(nid));
-		unsigned long value = lruvec_page_state_local(child_lruvec, idx);
-		struct mem_cgroup_per_node *child_pn, *parent_pn;
+		struct lruvec *lruvec = mem_cgroup_lruvec(memcg, NODE_DATA(nid));
+		unsigned long value = lruvec_page_state_local(lruvec, idx);
 
-		child_pn = container_of(child_lruvec, struct mem_cgroup_per_node, lruvec);
-		parent_pn = container_of(parent_lruvec, struct mem_cgroup_per_node, lruvec);
-
-		__mod_memcg_lruvec_state(child_pn, idx, -value);
-		__mod_memcg_lruvec_state(parent_pn, idx, value);
+		__mod_memcg_lruvec_state(memcg, nid, idx, -value);
+		__mod_memcg_lruvec_state(parent, nid, idx, value);
 	}
 }
 
@@ -990,10 +985,10 @@ void reparent_memcg_state_local(struct mem_cgroup *memcg,
 }
 #endif
 
-static void __mod_memcg_lruvec_state(struct mem_cgroup_per_node *pn,
+static void __mod_memcg_lruvec_state(struct mem_cgroup *memcg, int nid,
 				     enum node_stat_item idx, long val)
 {
-	struct mem_cgroup *memcg = pn->memcg;
+	struct mem_cgroup_per_node *pn = memcg->nodeinfo[nid];
 	int i = memcg_stats_index(idx);
 	int cpu;
 
@@ -1015,21 +1010,23 @@ static void __mod_memcg_lruvec_state(struct mem_cgroup_per_node *pn,
 	put_cpu();
 }
 
+static void mod_memcg_node_state(struct mem_cgroup *memcg, int nid,
+				 enum node_stat_item idx, int val)
+{
+	bool rcu_locked = false;
+
+	memcg = get_non_dying_memcg_start(memcg, &rcu_locked);
+	__mod_memcg_lruvec_state(memcg, nid, idx, val);
+	get_non_dying_memcg_end(rcu_locked);
+}
+
 void mod_memcg_lruvec_state(struct lruvec *lruvec,
 			    enum node_stat_item idx, int val)
 {
-	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
 	struct mem_cgroup_per_node *pn;
-	struct mem_cgroup *memcg;
-	bool rcu_locked = false;
 
 	pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
-	memcg = get_non_dying_memcg_start(pn->memcg, &rcu_locked);
-	pn = memcg->nodeinfo[pgdat->node_id];
-
-	__mod_memcg_lruvec_state(pn, idx, val);
-
-	get_non_dying_memcg_end(rcu_locked);
+	mod_memcg_node_state(pn->memcg, lruvec_pgdat(lruvec)->node_id, idx, val);
 }
 
 /**
@@ -1058,19 +1055,13 @@ void lruvec_stat_mod_folio(struct folio *folio, enum node_stat_item idx,
 {
 	struct mem_cgroup *memcg;
 	pg_data_t *pgdat = folio_pgdat(folio);
-	struct lruvec *lruvec;
+
+	mod_node_page_state(pgdat, idx, val);
 
 	rcu_read_lock();
 	memcg = folio_memcg(folio);
-	/* Untracked pages have no memcg, no lruvec. Update only the node */
-	if (!memcg) {
-		rcu_read_unlock();
-		mod_node_page_state(pgdat, idx, val);
-		return;
-	}
-
-	lruvec = mem_cgroup_lruvec(memcg, pgdat);
-	mod_lruvec_state(lruvec, idx, val);
+	if (memcg)
+		mod_memcg_node_state(memcg, pgdat->node_id, idx, val);
 	rcu_read_unlock();
 }
 EXPORT_SYMBOL(lruvec_stat_mod_folio);
@@ -1079,23 +1070,19 @@ void mod_lruvec_kmem_state(void *p, enum node_stat_item idx, int val)
 {
 	pg_data_t *pgdat = page_pgdat(virt_to_page(p));
 	struct mem_cgroup *memcg;
-	struct lruvec *lruvec;
+
+	mod_node_page_state(pgdat, idx, val);
 
 	rcu_read_lock();
 	memcg = mem_cgroup_from_virt(p);
 
 	/*
-	 * Untracked pages have no memcg, no lruvec. Update only the
-	 * node. If we reparent the slab objects to the root memcg,
-	 * when we free the slab object, we need to update the per-memcg
-	 * vmstats to keep it correct for the root memcg.
+	 * If we reparent the slab objects to the root memcg, when we free the
+	 * slab object, we need to update the per-memcg vmstats to keep it
+	 * correct for the root memcg.
 	 */
-	if (!memcg) {
-		mod_node_page_state(pgdat, idx, val);
-	} else {
-		lruvec = mem_cgroup_lruvec(memcg, pgdat);
-		mod_lruvec_state(lruvec, idx, val);
-	}
+	if (memcg)
+		mod_memcg_node_state(memcg, pgdat->node_id, idx, val);
 	rcu_read_unlock();
 }
 
-- 
2.52.0

Re: [PATCH v3 2/2] memcg: don't go through the lruvec for folio and kmem lruvec stat updates

[Michal Hocko]

On Fri 23-10-26 12:54:17, Joanne Koong wrote:
> lruvec_stat_mod_folio() and mod_lruvec_kmem_state() already know the memcg
> and the node, but look up the lruvec with mem_cgroup_lruvec() and update
> the stats through mod_lruvec_state(), which reads lruvec->pgdat and
> mem_cgroup_per_node->memcg to get them back. lruvec->pgdat shares a
> 128-byte cacheline pair with lines that the lru_lock holder writes
> constantly (lru_lock or lru_zone_size[], depending on the alignment of
> struct mem_cgroup_per_node), so on CPUs that do spatial prefetching
> these per-folio stat updates contend with the lru_lock holder.
> 
> Pass the memcg and node they already know to mod_memcg_node_state()
> instead.
> 
> On top of the mem_cgroup_per_node rearrangement, on a 2-socket Skylake
> (80 CPUs) with threads faulting in and freeing shmem in a single memcg,
> this improves throughput by about 3% at 20 threads (5.59M -> 5.77M
> pages/sec) and 2% at 80 threads (8.66M -> 8.82M).
> 
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>

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

> ---
>  mm/memcontrol.c | 71 ++++++++++++++++++++-----------------------------
>  1 file changed, 29 insertions(+), 42 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index a05d9a8baa1b..9f33cc5c8293 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -587,7 +587,7 @@ unsigned long lruvec_page_state_local(const struct lruvec *lruvec,
>  	return x;
>  }
>  
> -static void __mod_memcg_lruvec_state(struct mem_cgroup_per_node *pn,
> +static void __mod_memcg_lruvec_state(struct mem_cgroup *memcg, int nid,
>  				     enum node_stat_item idx, long val);
>  
>  void reparent_memcg_lruvec_state_local(struct mem_cgroup *memcg,
> @@ -596,16 +596,11 @@ void reparent_memcg_lruvec_state_local(struct mem_cgroup *memcg,
>  	int nid;
>  
>  	for_each_node(nid) {
> -		struct lruvec *child_lruvec = mem_cgroup_lruvec(memcg, NODE_DATA(nid));
> -		struct lruvec *parent_lruvec = mem_cgroup_lruvec(parent, NODE_DATA(nid));
> -		unsigned long value = lruvec_page_state_local(child_lruvec, idx);
> -		struct mem_cgroup_per_node *child_pn, *parent_pn;
> +		struct lruvec *lruvec = mem_cgroup_lruvec(memcg, NODE_DATA(nid));
> +		unsigned long value = lruvec_page_state_local(lruvec, idx);
>  
> -		child_pn = container_of(child_lruvec, struct mem_cgroup_per_node, lruvec);
> -		parent_pn = container_of(parent_lruvec, struct mem_cgroup_per_node, lruvec);
> -
> -		__mod_memcg_lruvec_state(child_pn, idx, -value);
> -		__mod_memcg_lruvec_state(parent_pn, idx, value);
> +		__mod_memcg_lruvec_state(memcg, nid, idx, -value);
> +		__mod_memcg_lruvec_state(parent, nid, idx, value);
>  	}
>  }
>  
> @@ -990,10 +985,10 @@ void reparent_memcg_state_local(struct mem_cgroup *memcg,
>  }
>  #endif
>  
> -static void __mod_memcg_lruvec_state(struct mem_cgroup_per_node *pn,
> +static void __mod_memcg_lruvec_state(struct mem_cgroup *memcg, int nid,
>  				     enum node_stat_item idx, long val)
>  {
> -	struct mem_cgroup *memcg = pn->memcg;
> +	struct mem_cgroup_per_node *pn = memcg->nodeinfo[nid];
>  	int i = memcg_stats_index(idx);
>  	int cpu;
>  
> @@ -1015,21 +1010,23 @@ static void __mod_memcg_lruvec_state(struct mem_cgroup_per_node *pn,
>  	put_cpu();
>  }
>  
> +static void mod_memcg_node_state(struct mem_cgroup *memcg, int nid,
> +				 enum node_stat_item idx, int val)
> +{
> +	bool rcu_locked = false;
> +
> +	memcg = get_non_dying_memcg_start(memcg, &rcu_locked);
> +	__mod_memcg_lruvec_state(memcg, nid, idx, val);
> +	get_non_dying_memcg_end(rcu_locked);
> +}
> +
>  void mod_memcg_lruvec_state(struct lruvec *lruvec,
>  			    enum node_stat_item idx, int val)
>  {
> -	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>  	struct mem_cgroup_per_node *pn;
> -	struct mem_cgroup *memcg;
> -	bool rcu_locked = false;
>  
>  	pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
> -	memcg = get_non_dying_memcg_start(pn->memcg, &rcu_locked);
> -	pn = memcg->nodeinfo[pgdat->node_id];
> -
> -	__mod_memcg_lruvec_state(pn, idx, val);
> -
> -	get_non_dying_memcg_end(rcu_locked);
> +	mod_memcg_node_state(pn->memcg, lruvec_pgdat(lruvec)->node_id, idx, val);
>  }
>  
>  /**
> @@ -1058,19 +1055,13 @@ void lruvec_stat_mod_folio(struct folio *folio, enum node_stat_item idx,
>  {
>  	struct mem_cgroup *memcg;
>  	pg_data_t *pgdat = folio_pgdat(folio);
> -	struct lruvec *lruvec;
> +
> +	mod_node_page_state(pgdat, idx, val);
>  
>  	rcu_read_lock();
>  	memcg = folio_memcg(folio);
> -	/* Untracked pages have no memcg, no lruvec. Update only the node */
> -	if (!memcg) {
> -		rcu_read_unlock();
> -		mod_node_page_state(pgdat, idx, val);
> -		return;
> -	}
> -
> -	lruvec = mem_cgroup_lruvec(memcg, pgdat);
> -	mod_lruvec_state(lruvec, idx, val);
> +	if (memcg)
> +		mod_memcg_node_state(memcg, pgdat->node_id, idx, val);
>  	rcu_read_unlock();
>  }
>  EXPORT_SYMBOL(lruvec_stat_mod_folio);
> @@ -1079,23 +1070,19 @@ void mod_lruvec_kmem_state(void *p, enum node_stat_item idx, int val)
>  {
>  	pg_data_t *pgdat = page_pgdat(virt_to_page(p));
>  	struct mem_cgroup *memcg;
> -	struct lruvec *lruvec;
> +
> +	mod_node_page_state(pgdat, idx, val);
>  
>  	rcu_read_lock();
>  	memcg = mem_cgroup_from_virt(p);
>  
>  	/*
> -	 * Untracked pages have no memcg, no lruvec. Update only the
> -	 * node. If we reparent the slab objects to the root memcg,
> -	 * when we free the slab object, we need to update the per-memcg
> -	 * vmstats to keep it correct for the root memcg.
> +	 * If we reparent the slab objects to the root memcg, when we free the
> +	 * slab object, we need to update the per-memcg vmstats to keep it
> +	 * correct for the root memcg.
>  	 */
> -	if (!memcg) {
> -		mod_node_page_state(pgdat, idx, val);
> -	} else {
> -		lruvec = mem_cgroup_lruvec(memcg, pgdat);
> -		mod_lruvec_state(lruvec, idx, val);
> -	}
> +	if (memcg)
> +		mod_memcg_node_state(memcg, pgdat->node_id, idx, val);
>  	rcu_read_unlock();
>  }
>  
> -- 
> 2.52.0

-- 
Michal Hocko
SUSE Labs

Re: [PATCH v3 0/2] memcg: reduce cacheline contention from lruvec stat

[Andrew Morton]

On Fri, 23 Oct 2026 12:54:15 -0700 Joanne Koong <joannelkoong@gmail.com> wrote:

> Memcg lruvec stat updates read fields of struct mem_cgroup_per_node and struct
> lruvec that can share a 128-byte cacheline pair with lines the lru_lock holder
> writes constantly (the LRU list heads, lru_lock or lru_zone_size[]), depending
> on where struct mem_cgroup_per_node lands in its slab. On CPUs that do spatial
> prefetching (eg Intel CPUs), this makes the stat updates contend with the
> lru_lock holder.
> 
> Patch 1 rearranges struct mem_cgroup_per_node so that its read-mostly line is
> paired with a cold line, and patch 2 makes the folio and kmem stat updates use
> the memcg and node they already know instead of going through the lruvec,
> whose pgdat field shares a cacheline pair with lru_lock or lru_zone_size[].
> 
> Results on a 2-socket Skylake (80 CPUs), average pages per second for
> threads faulting in and freeing memory in a single memcg:
> 
> 	   base     patch 1   patch 1+2 
> shmem, 20 threads  4.74M    5.59M     5.77M
> shmem, 80 threads  7.94M    8.66M     8.82M
> anon,  20 threads  3.04M    3.08M     3.10M

Thanks.  A local AI review pointed out a thing:

  One limitation worth mentioning in the [1/2] changelog: the slab
  uses SLAB_HWCACHE_ALIGN, which need not align each object to a
  128-byte boundary.  If an object starts 64 bytes into such a pair,
  its first line can still be paired with the end of the preceding
  object rather than its own cold line.  This isn't a correctness
  issue, and the measured Skylake improvement may well be real, but the
  layout doesn't guarantee that every read-mostly line pairs with a
  cold line.

Re: [PATCH v3 0/2] memcg: reduce cacheline contention from lruvec stat

[Joanne Koong]

On Fri, Oct 23, 2026 at 3:54 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 23 Oct 2026 12:54:15 -0700 Joanne Koong <joannelkoong@gmail.com> wrote:
>
> > Memcg lruvec stat updates read fields of struct mem_cgroup_per_node and struct
> > lruvec that can share a 128-byte cacheline pair with lines the lru_lock holder
> > writes constantly (the LRU list heads, lru_lock or lru_zone_size[]), depending
> > on where struct mem_cgroup_per_node lands in its slab. On CPUs that do spatial
> > prefetching (eg Intel CPUs), this makes the stat updates contend with the
> > lru_lock holder.
> >
> > Patch 1 rearranges struct mem_cgroup_per_node so that its read-mostly line is
> > paired with a cold line, and patch 2 makes the folio and kmem stat updates use
> > the memcg and node they already know instead of going through the lruvec,
> > whose pgdat field shares a cacheline pair with lru_lock or lru_zone_size[].
> >
> > Results on a 2-socket Skylake (80 CPUs), average pages per second for
> > threads faulting in and freeing memory in a single memcg:
> >
> >          base     patch 1   patch 1+2
> > shmem, 20 threads  4.74M    5.59M     5.77M
> > shmem, 80 threads  7.94M    8.66M     8.82M
> > anon,  20 threads  3.04M    3.08M     3.10M
>
> Thanks.  A local AI review pointed out a thing:
>
>   One limitation worth mentioning in the [1/2] changelog: the slab
>   uses SLAB_HWCACHE_ALIGN, which need not align each object to a
>   128-byte boundary.  If an object starts 64 bytes into such a pair,
>   its first line can still be paired with the end of the preceding
>   object rather than its own cold line.  This isn't a correctness
>   issue, and the measured Skylake improvement may well be real, but the
>   layout doesn't guarantee that every read-mostly line pairs with a
>   cold line.

Hi Andrew,

Thanks for taking a look at this series!

The AI review's commentary is correct but it doesn't seem to matter in
practice. In the cases where the read-mostly line isn't paired with
the cold line, it's paired with the line that holds the end of
lru_zone_size[] and the reclaim iterator, which are also rarely
written outside of reclaim so it doesn't see much contention. In my
benchmarking runs, the memcgs are freshly created and contain a mix of
both alignments. Before the changes in this series, the per-memcg
throughput was bimodal depending on the alignment, but afterwards, the
throughput is uniform across memcgs.

Here's an updated version of the "Fix this by moving the memcg_pn_cold
group..." paragraph in patch 1 that mentions this:

Fix this by moving the memcg_pn_cold group, which is only touched when
the memcg is reparented or freed, right after memcg_pn_read_mostly.
SLAB_HWCACHE_ALIGN only aligns objects to a cacheline, and the struct
is 576 bytes, so depending on where an object lands, its read-mostly
cacheline is now paired either with its cold cacheline or with the last
cacheline of the preceding object, which holds the end of
lru_zone_size[] and the reclaim iterator. Both are rarely written
outside of reclaim, so either way the read-mostly cacheline is no
longer paired with the LRU list heads.

Would you be able to swap it in or would you prefer that I send a v4?

Thanks,
Joanne

Re: [PATCH v3 0/2] memcg: reduce cacheline contention from lruvec stat

[Andrew Morton]

On Tue, 27 Oct 2026 15:57:43 -0700 Joanne Koong <joannelkoong@gmail.com> wrote:

> Here's an updated version of the "Fix this by moving the memcg_pn_cold
> group..." paragraph in patch 1 that mentions this:
> 
> Fix this by moving the memcg_pn_cold group, which is only touched when
> the memcg is reparented or freed, right after memcg_pn_read_mostly.
> SLAB_HWCACHE_ALIGN only aligns objects to a cacheline, and the struct
> is 576 bytes, so depending on where an object lands, its read-mostly
> cacheline is now paired either with its cold cacheline or with the last
> cacheline of the preceding object, which holds the end of
> lru_zone_size[] and the reclaim iterator. Both are rarely written
> outside of reclaim, so either way the read-mostly cacheline is no
> longer paired with the LRU list heads.
> 
> Would you be able to swap it in or would you prefer that I send a v4?

Thanks.  I'll take care of this when I process the patchset.  Which
will be quite soon now we're all merged up.

Re: [PATCH v3 0/2] memcg: reduce cacheline contention from lruvec stat

[Joanne Koong]

On Tue, Oct 27, 2026 at 7:24 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Tue, 27 Oct 2026 15:57:43 -0700 Joanne Koong <joannelkoong@gmail.com> wrote:
>
> > Here's an updated version of the "Fix this by moving the memcg_pn_cold
> > group..." paragraph in patch 1 that mentions this:
> >
> > Fix this by moving the memcg_pn_cold group, which is only touched when
> > the memcg is reparented or freed, right after memcg_pn_read_mostly.
> > SLAB_HWCACHE_ALIGN only aligns objects to a cacheline, and the struct
> > is 576 bytes, so depending on where an object lands, its read-mostly
> > cacheline is now paired either with its cold cacheline or with the last
> > cacheline of the preceding object, which holds the end of
> > lru_zone_size[] and the reclaim iterator. Both are rarely written
> > outside of reclaim, so either way the read-mostly cacheline is no
> > longer paired with the LRU list heads.
> >
> > Would you be able to swap it in or would you prefer that I send a v4?
>
> Thanks.  I'll take care of this when I process the patchset.  Which
> will be quite soon now we're all merged up.

Sounds great. Thanks, Andrew!