Skip to contentSkip to conversation

mm/mglru: cacheline follow-ups on the reclaim hot paths

RFC PATCH In review Ridong Chen 3 participants 12 messages Nov 5 – Nov 8

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

cover mm/mglru: cacheline follow-ups on the reclaim hot paths

From: Ridong Chen <[email protected]>

Joanne's series to reduce cacheline contention around struct mem_cgroup_per_node [1], now in linux-next, made me look at the MGLRU side of the same structures, and at the stores MGLRU does on hot paths, to see whether any of them write a line for no reason. This RFC has the three candidates that showed a measurable difference. They are independent of each other.

1. lru_gen_use_mm() writes mm->lru_gen.bitmap on every context switch to a user task, although the bitmap is almost always already set. For threads of one process spread over several CPUs, the line bounces between them. Read first and skip the store: +2-3% context switches per CPU for a shared mm, no change for private mms.

2. folio_update_gen() and folio_set_lru_refs() always end with a lock cmpxchg, even when the flags do not change. During aging, a folio mapped by N processes is visited N times and only the first visit changes anything. Skip the no-op cmpxchg: an aging pass over a 2GiB file shared by 8-64 processes gets 18-22% faster.

3. lrugen->evicted[] and ->refaulted[] are updated locklessly on every eviction and refault, and the file type counters share a line with lrugen->enabled/gen/seg/list, which are read on every LRU add and by shrink_many(). Move the read-mostly fields up and give the counters their own lines: +4.3% fio IOPS with 24 jobs refaulting file pages in one memcg, no change at 8 jobs or fewer. This one costs a cacheline per memcg per node, and its gain needs many CPUs refaulting from a fast backing store, so I am not sure it is worth it on its own; feedback welcome.

All numbers are from an i7-13700HX (8P+8E, 24 threads, one socket) with turbo disabled and the performance governor, on next-20261023 with the same config for base and patched kernels. Details are in each patch. The series is rebased onto next-20261104, which includes [1], without code changes; the struct sizes in patch 3 are updated for it.

Limitations of the testing, where input would be welcome:

- Patch 1 does not show up in will-it-scale context_switch1 or sched_yield (explained in the patch), and perf bench sched messaging and schbench are too noisy here to resolve 2-3%. The numbers come from a small test that pins two sched_yield()ing threads per CPU, with a one-process-per-CPU control.

- Patch 2 has no standard benchmark either. In fio with many mmap() readers under memcg pressure, reclaim time goes to rmap walks on the eviction side and aging barely runs. The numbers come from triggering aging through debugfs with force_scan, which is the upper bound. On real systems the gain scales with how many processes in the same memcg share the hot file pages, e.g. preforked servers or Android without per-app memcgs.

- Patch 3 depends on the layout of the containing structs, which depends on the config (LRU_GEN_STATS changes the array sizes). I checked LRU_GEN_STATS=n only.

The test programs are small C files; I can post them if that helps.

[1] https://lore.kernel.org/[email protected]

Ridong Chen (3):
mm/mglru: skip redundant bitmap store in lru_gen_use_mm() mm/mglru: skip the flags cmpxchg when nothing changes mm/mglru: move lrugen atomic stats off the read-mostly lines

 include/linux/mm_inline.h |  2 ++
 include/linux/mm_types.h  |  3 ++-
 include/linux/mmzone.h    | 18 ++++++++++--------
 mm/vmscan.c               |  2 ++
 4 files changed, 16 insertions(+), 9 deletions(-)

base-commit: 8b7179210b22f8e48a82c5ceacb9d56a9c4cfec4

Signature · 1 line
2.34.1
#1 lore raw

RFC PATCH 1/3 mm/mglru: skip redundant bitmap store in lru_gen_use_mm()

From: Ridong Chen <[email protected]>

lru_gen_use_mm() is called from context_switch() on every switch to a user task and unconditionally sets mm->lru_gen.bitmap to -1. The bitmap is only cleared by the page table walkers in get_next_mm() during aging, so between aging passes it is already all ones and the store is redundant.

The store is not free when the mm_struct is shared: every CPU running a thread of the same process must take the cacheline in exclusive state, so the line bounces between CPUs on every context switch. On x86, switch_mm_irqs_off() returns early when switching between threads of the same mm, which can make this the only shared write on that path.

Read the bitmap first and only write it when it is not already set. The semantics are unchanged: the plain WRITE_ONCE() had no ordering guarantees, and a walker clearing a bit concurrently can be missed in either version.

Tested on an i7-13700HX (8P+8E cores, 24 threads), with turbo disabled and the performance governor. On each CPU two tasks sched_yield() to each other, either as threads of one process (shared mm) or as one process per CPU (private mm, control). Context switches per second per CPU, median of 5 runs, averaged over 2 boots of each kernel:

CPUs shared mm private mm

         base     patched       base     patched
     1   1114766  1106870 -0.7% 1104512  1109284 +0.4%
     2   1086513  1110204 +2.2% 1110334  1112209 +0.2%
     4   1085872  1112258 +2.4% 1113146  1112467 -0.1%
     8   1085945  1111371 +2.3% 1111662  1111876 +0.0%
    16    660870   666748 +0.9%  665470   667058 +0.2%
    24    579382   596472 +2.9%  596177   598500 +0.4%

The 16 CPU case uses both SMT siblings of each P-core, which share the L1/L2, so less of the traffic crosses cores. With the patch, the shared mm case is within noise of the private mm case.

will-it-scale context_switch1 and sched_yield do not show a difference (within 1% for both threads and processes): sched_yield runs one task per CPU and never switches, and context_switch1 leaves the pipe peer unpinned, so most switches stay on one CPU and the line does not move.

---
 include/linux/mm_types.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
include/linux/mm_types.h +2 -1
index 1ed35d258d2e..3680cc0fd714 100644
@@ -1553,7 +1553,8 @@ static inline void lru_gen_use_mm(struct mm_struct *mm)
 	 * used since the last time it cleared the bitmap. So it might be worth
 	 * walking the page tables of this mm_struct to clear the accessed bit.
 	 */
-	WRITE_ONCE(mm->lru_gen.bitmap, -1);
+	if (READ_ONCE(mm->lru_gen.bitmap) != -1UL)
+		WRITE_ONCE(mm->lru_gen.bitmap, -1UL);
 }
 
 #else /* !CONFIG_LRU_GEN_WALKS_MMU */
Signature · 1 line
2.34.1
#2 lore raw

RFC PATCH 2/3 mm/mglru: skip the flags cmpxchg when nothing changes

In reply to Ridong Chen

From: Ridong Chen <[email protected]>

folio_update_gen() and folio_set_lru_refs() always end with a try_cmpxchg() on folio->flags, even when the new value equals the old one. The locked instruction still takes the struct page cacheline in exclusive state.

That is common in practice: a folio mapped by many processes is visited once per mm during a page table walk, and after the first visit it is already in the youngest generation with refs cleared and PG_workingset set. isolate_folio() and the eviction paths also call folio_set_lru_refs(folio, 0) on folios whose refs are already zero.

Break out of the loop when the flags would not change. The result is the same as a successful no-op cmpxchg. The only difference is that the full barrier implied by the locked instruction is gone on that path. No caller relies on it: the eviction and isolation callers run under the LRU lock, and the others (page table walks, folio_mark_accessed(), lru_gen_clear_refs()) only care about the resulting flags value, which is already in place. Note that lru_gen_inc_refs() already returns without any atomic when refs is at LRU_REFS_MAX and the folio is in the workingset.

Tested on an i7-13700HX with turbo disabled and the performance governor. N processes map the same 2GiB tmpfs file (huge=never) and read every page, then one aging pass is triggered on their memcg through /sys/kernel/debug/lru_gen ("+ <memcg_id> 0 <max_seq> 200 1"). Time per aging pass in ms, median of 5 runs of 10 passes each, averaged over 2 boots of each kernel:

processes base patched

          1    32.9      31.7   -3.5%
          8   195.1     160.2  -17.9%
         32   755.8     592.2  -21.7%
         64  1597.2    1260.6  -21.1%

With one process, every visit changes the generation, so there is nothing to skip. Before the patch, perf annotate attributes 53-58% of the walk_update_folio() samples to the lock cmpxchg in the inlined folio_update_gen() in this test.

---
 include/linux/mm_inline.h | 2 ++
 mm/vmscan.c               | 2 ++
 2 files changed, 4 insertions(+)
include/linux/mm_inline.h +2 -0
index b3d85fe91385..4ba9079f4dc8 100644
@@ -244,6 +244,8 @@ static inline void folio_set_lru_refs(struct folio *folio, unsigned int refs)
 	do {
 		new_flags = old_flags;
 		lru_set_refs_flags(&new_flags, refs);
+		if (new_flags == old_flags)
+			break;
 	} while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
 }
 
mm/vmscan.c +2 -0
index 50c352e9ffef..3fa76715382d 100644
@@ -3398,6 +3398,8 @@ static int folio_update_gen(struct folio *folio, int new_gen, int *type,
 		lru_set_gen_flags(&new_flags, new_gen);
 		lru_set_refs_flags(&new_flags, 0);
 		new_flags |= BIT(PG_workingset);
+		if (new_flags == old_flags)
+			break;
 	} while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
 
 	*type = folio_flags_is_file_lru(&old_flags);
Signature · 1 line
2.34.1
Reviewed-by on v1 2/3
#3 lore raw

Re: [RFC PATCH 2/3] mm/mglru: skip the flags cmpxchg when nothing changes

Quoted 66 lines · Ridong Chen

From: Ridong Chen <[email protected]>

folio_update_gen() and folio_set_lru_refs() always end with a try_cmpxchg() on folio->flags, even when the new value equals the old one. The locked instruction still takes the struct page cacheline in exclusive state.

That is common in practice: a folio mapped by many processes is visited once per mm during a page table walk, and after the first visit it is already in the youngest generation with refs cleared and PG_workingset set. isolate_folio() and the eviction paths also call folio_set_lru_refs(folio, 0) on folios whose refs are already zero.

Break out of the loop when the flags would not change. The result is the same as a successful no-op cmpxchg. The only difference is that the full barrier implied by the locked instruction is gone on that path. No caller relies on it: the eviction and isolation callers run under the LRU lock, and the others (page table walks, folio_mark_accessed(), lru_gen_clear_refs()) only care about the resulting flags value, which is already in place. Note that lru_gen_inc_refs() already returns without any atomic when refs is at LRU_REFS_MAX and the folio is in the workingset.

Tested on an i7-13700HX with turbo disabled and the performance governor. N processes map the same 2GiB tmpfs file (huge=never) and read every page, then one aging pass is triggered on their memcg through /sys/kernel/debug/lru_gen ("+ <memcg_id> 0 <max_seq> 200 1"). Time per aging pass in ms, median of 5 runs of 10 passes each, averaged over 2 boots of each kernel:

processes base patched

          1    32.9      31.7   -3.5%
          8   195.1     160.2  -17.9%
         32   755.8     592.2  -21.7%
         64  1597.2    1260.6  -21.1%

With one process, every visit changes the generation, so there is nothing to skip. Before the patch, perf annotate attributes 53-58% of the walk_update_folio() samples to the lock cmpxchg in the inlined folio_update_gen() in this test.

---
 include/linux/mm_inline.h | 2 ++
 mm/vmscan.c               | 2 ++
 2 files changed, 4 insertions(+)
include/linux/mm_inline.h +2 -0
index b3d85fe91385..4ba9079f4dc8 100644
@@ -244,6 +244,8 @@ static inline void folio_set_lru_refs(struct folio *folio, unsigned int refs)
        do {
                new_flags = old_flags;
                lru_set_refs_flags(&new_flags, refs);
+               if (new_flags == old_flags)
+                       break;
        } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
 }
 
mm/vmscan.c +0 -0
index 50c352e9ffef..3fa76715382d 100644

mm/vmscan.c static int folio_update_gen(struct folio *folio, int new_gen, int *type, · line 3403

Quoted 6 lines
mm/vmscan.c +2 -0
@@ -3398,6 +3398,8 @@ static int folio_update_gen(struct folio *folio, int new_gen, int *type,
                lru_set_gen_flags(&new_flags, new_gen);
                lru_set_refs_flags(&new_flags, 0);
                new_flags |= BIT(PG_workingset);
+               if (new_flags == old_flags)
+                       break;
        } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));

Hi Ridong,

I had the same idea in V4 of MGLRU-FG which is posted a few weeks ago, with the same code:
https://lore.kernel.org/linux-mm/[email protected]/

+ if (new_flags == old_flags)
+ break;

I didn't have time to benchmark that one function but the e2e test showed very trivial improvement, it's a good improvement though. Feel free to add:

BTW there are a lot of concurrent activity is happening for MGLRU at the moment so conflicts occur very frequently...

#4 lore raw

Re: [RFC PATCH 2/3] mm/mglru: skip the flags cmpxchg when nothing changes

Quoted 76 lines · Kairui Song
Quoted 66 lines · Ridong Chen

From: Ridong Chen <[email protected]>

folio_update_gen() and folio_set_lru_refs() always end with a try_cmpxchg() on folio->flags, even when the new value equals the old one. The locked instruction still takes the struct page cacheline in exclusive state.

That is common in practice: a folio mapped by many processes is visited once per mm during a page table walk, and after the first visit it is already in the youngest generation with refs cleared and PG_workingset set. isolate_folio() and the eviction paths also call folio_set_lru_refs(folio, 0) on folios whose refs are already zero.

Break out of the loop when the flags would not change. The result is the same as a successful no-op cmpxchg. The only difference is that the full barrier implied by the locked instruction is gone on that path. No caller relies on it: the eviction and isolation callers run under the LRU lock, and the others (page table walks, folio_mark_accessed(), lru_gen_clear_refs()) only care about the resulting flags value, which is already in place. Note that lru_gen_inc_refs() already returns without any atomic when refs is at LRU_REFS_MAX and the folio is in the workingset.

Tested on an i7-13700HX with turbo disabled and the performance governor. N processes map the same 2GiB tmpfs file (huge=never) and read every page, then one aging pass is triggered on their memcg through /sys/kernel/debug/lru_gen ("+ <memcg_id> 0 <max_seq> 200 1"). Time per aging pass in ms, median of 5 runs of 10 passes each, averaged over 2 boots of each kernel:

   processes    base   patched
           1    32.9      31.7   -3.5%
           8   195.1     160.2  -17.9%
          32   755.8     592.2  -21.7%
          64  1597.2    1260.6  -21.1%

With one process, every visit changes the generation, so there is nothing to skip. Before the patch, perf annotate attributes 53-58% of the walk_update_folio() samples to the lock cmpxchg in the inlined folio_update_gen() in this test.

---
  include/linux/mm_inline.h | 2 ++
  mm/vmscan.c               | 2 ++
  2 files changed, 4 insertions(+)
include/linux/mm_inline.h +2 -0
index b3d85fe91385..4ba9079f4dc8 100644
@@ -244,6 +244,8 @@ static inline void folio_set_lru_refs(struct folio *folio, unsigned int refs)
         do {
                 new_flags = old_flags;
                 lru_set_refs_flags(&new_flags, refs);
+               if (new_flags == old_flags)
+                       break;
         } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
  }
 
mm/vmscan.c +0 -0
index 50c352e9ffef..3fa76715382d 100644

mm/vmscan.c static int folio_update_gen(struct folio *folio, int new_gen, int *type, · line 3403

Quoted 6 lines
mm/vmscan.c +2 -0
@@ -3398,6 +3398,8 @@ static int folio_update_gen(struct folio *folio, int new_gen, int *type,
                 lru_set_gen_flags(&new_flags, new_gen);
                 lru_set_refs_flags(&new_flags, 0);
                 new_flags |= BIT(PG_workingset);
+               if (new_flags == old_flags)
+                       break;
         } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));

Hi Ridong,

Quoted 7 lines

I had the same idea in V4 of MGLRU-FG which is posted a few weeks ago, with the same code:
https://lore.kernel.org/linux-mm/[email protected]/

+ if (new_flags == old_flags)
+ break;

Hi Kairui,

Oh. Sorry about that. I read the series a few weeks ago, but as it is large I missed this change. I should have checked it again before sending.

Quoted 6 lines

I didn't have time to benchmark that one function but the e2e test showed very trivial improvement, it's a good improvement though. Feel free to add:

Thanks for the Reviewed-by. If you don't mind, I'd suggest merging it as a separate patch, as the test shows the benefit of this small change, which makes the code easier to trace. I will also mention in the commit message that it is a small part of your series. Does that sound OK to you?

Quoted 2 lines

BTW there are a lot of concurrent activity is happening for MGLRU at the moment so conflicts occur very frequently...

Signature · 2 lines
Best regards
Ridong
#5 lore raw

Re: [RFC PATCH 2/3] mm/mglru: skip the flags cmpxchg when nothing changes

Quoted 95 lines · Ridong Chen
Quoted 76 lines · Kairui Song
Quoted 66 lines · Ridong Chen

From: Ridong Chen <[email protected]>

folio_update_gen() and folio_set_lru_refs() always end with a try_cmpxchg() on folio->flags, even when the new value equals the old one. The locked instruction still takes the struct page cacheline in exclusive state.

That is common in practice: a folio mapped by many processes is visited once per mm during a page table walk, and after the first visit it is already in the youngest generation with refs cleared and PG_workingset set. isolate_folio() and the eviction paths also call folio_set_lru_refs(folio, 0) on folios whose refs are already zero.

Break out of the loop when the flags would not change. The result is the same as a successful no-op cmpxchg. The only difference is that the full barrier implied by the locked instruction is gone on that path. No caller relies on it: the eviction and isolation callers run under the LRU lock, and the others (page table walks, folio_mark_accessed(), lru_gen_clear_refs()) only care about the resulting flags value, which is already in place. Note that lru_gen_inc_refs() already returns without any atomic when refs is at LRU_REFS_MAX and the folio is in the workingset.

Tested on an i7-13700HX with turbo disabled and the performance governor. N processes map the same 2GiB tmpfs file (huge=never) and read every page, then one aging pass is triggered on their memcg through /sys/kernel/debug/lru_gen ("+ <memcg_id> 0 <max_seq> 200 1"). Time per aging pass in ms, median of 5 runs of 10 passes each, averaged over 2 boots of each kernel:

   processes    base   patched
           1    32.9      31.7   -3.5%
           8   195.1     160.2  -17.9%
          32   755.8     592.2  -21.7%
          64  1597.2    1260.6  -21.1%

With one process, every visit changes the generation, so there is nothing to skip. Before the patch, perf annotate attributes 53-58% of the walk_update_folio() samples to the lock cmpxchg in the inlined folio_update_gen() in this test.

---
  include/linux/mm_inline.h | 2 ++
  mm/vmscan.c               | 2 ++
  2 files changed, 4 insertions(+)
include/linux/mm_inline.h +2 -0
index b3d85fe91385..4ba9079f4dc8 100644
@@ -244,6 +244,8 @@ static inline void folio_set_lru_refs(struct folio *folio, unsigned int refs)
         do {
                 new_flags = old_flags;
                 lru_set_refs_flags(&new_flags, refs);
+               if (new_flags == old_flags)
+                       break;
         } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
  }
 
mm/vmscan.c +0 -0
index 50c352e9ffef..3fa76715382d 100644

mm/vmscan.c static int folio_update_gen(struct folio *folio, int new_gen, int *type, · line 3403

Quoted 6 lines
mm/vmscan.c +2 -0
@@ -3398,6 +3398,8 @@ static int folio_update_gen(struct folio *folio, int new_gen, int *type,
                 lru_set_gen_flags(&new_flags, new_gen);
                 lru_set_refs_flags(&new_flags, 0);
                 new_flags |= BIT(PG_workingset);
+               if (new_flags == old_flags)
+                       break;
         } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));

Hi Ridong,

Quoted 7 lines

I had the same idea in V4 of MGLRU-FG which is posted a few weeks ago, with the same code:
https://lore.kernel.org/linux-mm/[email protected]/

+ if (new_flags == old_flags)
+ break;

Hi Kairui,

Oh. Sorry about that. I read the series a few weeks ago, but as it is large I missed this change. I should have checked it again before sending.

Quoted 4 lines

I didn't have time to benchmark that one function but the e2e test showed very trivial improvement, it's a good improvement though. Feel free to add:

Quoted 8 lines
Quoted 2 lines

Thanks for the Reviewed-by. If you don't mind, I'd suggest merging it as a separate patch, as the test shows the benefit of this small change, which makes the code easier to trace. I will also mention in the commit message that it is a small part of your series. Does that sound OK to you?

Sounds good to me! No need to mention that, though; I believe you came up with this idea by yourself. And that will make the change more traceable indeed.

#6 lore raw

Re: [RFC PATCH 2/3] mm/mglru: skip the flags cmpxchg when nothing changes

Quoted 105 lines · Kairui Song
Quoted 95 lines · Ridong Chen
Quoted 76 lines · Kairui Song
Quoted 66 lines · Ridong Chen

From: Ridong Chen <[email protected]>

folio_update_gen() and folio_set_lru_refs() always end with a try_cmpxchg() on folio->flags, even when the new value equals the old one. The locked instruction still takes the struct page cacheline in exclusive state.

That is common in practice: a folio mapped by many processes is visited once per mm during a page table walk, and after the first visit it is already in the youngest generation with refs cleared and PG_workingset set. isolate_folio() and the eviction paths also call folio_set_lru_refs(folio, 0) on folios whose refs are already zero.

Break out of the loop when the flags would not change. The result is the same as a successful no-op cmpxchg. The only difference is that the full barrier implied by the locked instruction is gone on that path. No caller relies on it: the eviction and isolation callers run under the LRU lock, and the others (page table walks, folio_mark_accessed(), lru_gen_clear_refs()) only care about the resulting flags value, which is already in place. Note that lru_gen_inc_refs() already returns without any atomic when refs is at LRU_REFS_MAX and the folio is in the workingset.

Tested on an i7-13700HX with turbo disabled and the performance governor. N processes map the same 2GiB tmpfs file (huge=never) and read every page, then one aging pass is triggered on their memcg through /sys/kernel/debug/lru_gen ("+ <memcg_id> 0 <max_seq> 200 1"). Time per aging pass in ms, median of 5 runs of 10 passes each, averaged over 2 boots of each kernel:

    processes    base   patched
            1    32.9      31.7   -3.5%
            8   195.1     160.2  -17.9%
           32   755.8     592.2  -21.7%
           64  1597.2    1260.6  -21.1%

With one process, every visit changes the generation, so there is nothing to skip. Before the patch, perf annotate attributes 53-58% of the walk_update_folio() samples to the lock cmpxchg in the inlined folio_update_gen() in this test.

---
   include/linux/mm_inline.h | 2 ++
   mm/vmscan.c               | 2 ++
   2 files changed, 4 insertions(+)
include/linux/mm_inline.h +2 -0
index b3d85fe91385..4ba9079f4dc8 100644
@@ -244,6 +244,8 @@ static inline void folio_set_lru_refs(struct folio *folio, unsigned int refs)
          do {
                  new_flags = old_flags;
                  lru_set_refs_flags(&new_flags, refs);
+               if (new_flags == old_flags)
+                       break;
          } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
   }
 
mm/vmscan.c +0 -0
index 50c352e9ffef..3fa76715382d 100644

mm/vmscan.c static int folio_update_gen(struct folio *folio, int new_gen, int *type, · line 3403

Quoted 6 lines
mm/vmscan.c +2 -0
@@ -3398,6 +3398,8 @@ static int folio_update_gen(struct folio *folio, int new_gen, int *type,
                  lru_set_gen_flags(&new_flags, new_gen);
                  lru_set_refs_flags(&new_flags, 0);
                  new_flags |= BIT(PG_workingset);
+               if (new_flags == old_flags)
+                       break;
          } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));

Hi Ridong,

Quoted 7 lines

I had the same idea in V4 of MGLRU-FG which is posted a few weeks ago, with the same code:
https://lore.kernel.org/linux-mm/[email protected]/

+ if (new_flags == old_flags)
+ break;

Hi Kairui,

Oh. Sorry about that. I read the series a few weeks ago, but as it is large I missed this change. I should have checked it again before sending.

Quoted 4 lines

I didn't have time to benchmark that one function but the e2e test showed very trivial improvement, it's a good improvement though. Feel free to add:

Quoted 8 lines
Quoted 2 lines

Thanks for the Reviewed-by. If you don't mind, I'd suggest merging it as a separate patch, as the test shows the benefit of this small change, which makes the code easier to trace. I will also mention in the commit message that it is a small part of your series. Does that sound OK to you?

Quoted 3 lines

Sounds good to me! No need to mention that, though; I believe you came up with this idea by yourself. And that will make the change more traceable indeed.

That's very kind of you. Thanks.

Signature · 2 lines
Best regards
Ridong
reviewer
#7 lore raw

Re: [RFC PATCH 2/3] mm/mglru: skip the flags cmpxchg when nothing changes

In reply to Ridong Chen
Quoted 14 lines · Ridong Chen

From: Ridong Chen <[email protected]>

folio_update_gen() and folio_set_lru_refs() always end with a try_cmpxchg() on folio->flags, even when the new value equals the old one. The locked instruction still takes the struct page cacheline in exclusive state.

That is common in practice: a folio mapped by many processes is visited once per mm during a page table walk, and after the first visit it is already in the youngest generation with refs cleared and PG_workingset set. isolate_folio() and the eviction paths also call folio_set_lru_refs(folio, 0) on folios whose refs are already zero.

Quoted 3 lines

Break out of the loop when the flags would not change. The result is the same as a successful no-op cmpxchg. The only difference is that the full barrier implied by the locked instruction is gone on that path.

Hi Ridong,

I'm not quite sure this is an accurate description. The atomic operation checks whether folio_flags(folio, 0) has changed. If it has, we need to recompute new_flags based on the updated old_flags.

For example:

CPU 0 CPU 1
----- -----
read old_flags = A

                               change flags A -> B

new_flags = A
new_flags == old_flags
break

This is how the CAS loop works. Whether we strictly need the guarantees provided by CAS is a separate question :-)

But skipping the CAS logic might slightly underestimate a folio's hotness if it gets promoted twice? Perhaps the race window is so small that the reduced CAS overhead outweighs the impact on hotness tracking.

Quoted 14 lines

No caller relies on it: the eviction and isolation callers run under the LRU lock, and the others (page table walks, folio_mark_accessed(), lru_gen_clear_refs()) only care about the resulting flags value, which is already in place. Note that lru_gen_inc_refs() already returns without any atomic when refs is at LRU_REFS_MAX and the folio is in the workingset.

Tested on an i7-13700HX with turbo disabled and the performance governor. N processes map the same 2GiB tmpfs file (huge=never) and read every page, then one aging pass is triggered on their memcg through /sys/kernel/debug/lru_gen ("+ <memcg_id> 0 <max_seq> 200 1"). Time per aging pass in ms, median of 5 runs of 10 passes each, averaged over 2 boots of each kernel:

Quoted 5 lines

processes base patched

          1    32.9      31.7   -3.5%
          8   195.1     160.2  -17.9%
         32   755.8     592.2  -21.7%
         64  1597.2    1260.6  -21.1%

Could we post the microbenchmark code here if it's short enough?

Quoted 39 lines

With one process, every visit changes the generation, so there is nothing to skip. Before the patch, perf annotate attributes 53-58% of the walk_update_folio() samples to the lock cmpxchg in the inlined folio_update_gen() in this test.

---
 include/linux/mm_inline.h | 2 ++
 mm/vmscan.c               | 2 ++
 2 files changed, 4 insertions(+)
include/linux/mm_inline.h +2 -0
index b3d85fe91385..4ba9079f4dc8 100644
@@ -244,6 +244,8 @@ static inline void folio_set_lru_refs(struct folio *folio, unsigned int refs)
        do {
                new_flags = old_flags;
                lru_set_refs_flags(&new_flags, refs);
+               if (new_flags == old_flags)
+                       break;
        } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
 }
 
mm/vmscan.c +2 -1
index 50c352e9ffef..3fa76715382d 100644
@@ -3398,6 +3398,8 @@ static int folio_update_gen(struct folio *folio, int new_gen, int *type,
                lru_set_gen_flags(&new_flags, new_gen);
                lru_set_refs_flags(&new_flags, 0);
                new_flags |= BIT(PG_workingset);
+               if (new_flags == old_flags)
+                       break;
        } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
 
        *type = folio_flags_is_file_lru(&old_flags);
--
Quoted 2 lines

2.34.1

Thanks
Barry

#8 lore raw

Re: [RFC PATCH 2/3] mm/mglru: skip the flags cmpxchg when nothing changes

Quoted 15 lines · Barry Song
Quoted 14 lines · Ridong Chen

From: Ridong Chen <[email protected]>

folio_update_gen() and folio_set_lru_refs() always end with a try_cmpxchg() on folio->flags, even when the new value equals the old one. The locked instruction still takes the struct page cacheline in exclusive state.

That is common in practice: a folio mapped by many processes is visited once per mm during a page table walk, and after the first visit it is already in the youngest generation with refs cleared and PG_workingset set. isolate_folio() and the eviction paths also call folio_set_lru_refs(folio, 0) on folios whose refs are already zero.

Quoted 7 lines
Quoted 3 lines

Break out of the loop when the flags would not change. The result is the same as a successful no-op cmpxchg. The only difference is that the full barrier implied by the locked instruction is gone on that path.

Hi Ridong,

I'm not quite sure this is an accurate description. The

You are right, "the same as a successful no-op cmpxchg" is not accurate. A better way to put it: when we break, the flags we read are already the target value, so the update can be considered done at the time of that read, and any concurrent change is ordered after it. The current code gives the same result whenever its cmpxchg wins the race, so the patch makes no new state reachable. What it drops is only the retry, which recomputes from the new value and overwrites the concurrent change with what this CPU wanted.

Quoted 14 lines

atomic operation checks whether folio_flags(folio, 0) has changed. If it has, we need to recompute new_flags based on the updated old_flags.

For example:

CPU 0 CPU 1
----- -----
read old_flags = A

                                change flags A -> B

new_flags = A
new_flags == old_flags
break

Quoted 9 lines

This is how the CAS loop works. Whether we strictly need the guarantees provided by CAS is a separate question :-)

But skipping the CAS logic might slightly underestimate a folio's hotness if it gets promoted twice? Perhaps the race window is so small that the reduced CAS overhead outweighs the impact on hotness tracking.

I think it goes the other way. For folio_update_gen(), take two walkers where CPU 0 read an older max_seq:

CPU 0 (new_gen = N) CPU 1 (new_gen = N+1)
read flags: gen N
set gen N+1
current: cmpxchg fails, retry writes gen N back
patched: break, folio stays in gen N+1

So the retry moves the folio back to the older generation, and the patch keeps the second promotion. Similarly for folio_set_lru_refs():
if CPU 1 increments refs from 0 to 1 while CPU 0 is setting it to 0, the retry would clear that access, and the patch keeps it.

I will reword the changelog in v2 along these lines.

Quoted 14 lines
Quoted 14 lines

No caller relies on it: the eviction and isolation callers run under the LRU lock, and the others (page table walks, folio_mark_accessed(), lru_gen_clear_refs()) only care about the resulting flags value, which is already in place. Note that lru_gen_inc_refs() already returns without any atomic when refs is at LRU_REFS_MAX and the folio is in the workingset.

Tested on an i7-13700HX with turbo disabled and the performance governor. N processes map the same 2GiB tmpfs file (huge=never) and read every page, then one aging pass is triggered on their memcg through /sys/kernel/debug/lru_gen ("+ <memcg_id> 0 <max_seq> 200 1"). Time per aging pass in ms, median of 5 runs of 10 passes each, averaged over 2 boots of each kernel:

Quoted 9 lines
Quoted 5 lines
   processes    base   patched
           1    32.9      31.7   -3.5%
           8   195.1     160.2  -17.9%
          32   755.8     592.2  -21.7%
          64  1597.2    1260.6  -21.1%

Could we post the microbenchmark code here if it's short enough?

Sure, it is below. Run it as root in a dedicated cgroup v2 memcg, on a tmpfs mounted with huge=never:

./p4age /mnt/tmpfs/f 2048 <nproc> 10

```
/*
 * P4 load: MGLRU aging over a file mapped by many processes.
 *
 * Creates <MiB> tmpfs file at <path> (mount it with huge=never so every
 * folio is a single page), forks <nproc> children that each map it
 * MAP_SHARED, then for <rounds> rounds:
 *   1. every child reads one byte per page (sets the accessed bit in its
 *      own page tables)
 *   2. the parent triggers one aging pass on its memcg through
 *      /sys/kernel/debug/lru_gen ("+ memcg_id nid max_seq 200 1") and times
 *      the write()
 *
 * The page table walk visits every page once per mm. The first visit moves
 * the folio to the youngest generation; the other nproc-1 visits find the
 * flags already at the target value, which is the no-op cmpxchg P4 skips.
 *
 * Run inside a dedicated cgroup v2 memcg, as root, with debugfs mounted.
 * Usage: p4age <path> <MiB> <nproc> <rounds>
 * Output: one "round=R ms=T" line per round, then a summary.
 */
#define _GNU_SOURCE
#include <fcntl.h>
#include <stdatomic.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/mman.h>
#include <sys/resource.h>
#include <sys/wait.h>
#include <time.h>
#include <unistd.h>

#define PAGE	4096UL
#define LRU_GEN	"/sys/kernel/debug/lru_gen"

struct shared {
	atomic_int round;	/* parent bumps to start a touch round */
	atomic_int done;	/* children finished the current round */
	atomic_int quit;
};

static struct shared *sh;

static double now(void)
{
	struct timespec t;

	clock_gettime(CLOCK_MONOTONIC, &t);
	return t.tv_sec + t.tv_nsec / 1e9;
}

static void child(int fd, size_t len)
{
	volatile char sink;
	int seen = 0;
	size_t i;
	char *p;

	p = mmap(NULL, len, PROT_READ, MAP_SHARED, fd, 0);
	if (p == MAP_FAILED) {
		perror("mmap");
		_exit(1);
	}
	for (;;) {
		while (atomic_load(&sh->round) == seen && !atomic_load(&sh->quit))
			usleep(100);
		if (atomic_load(&sh->quit))
			break;
		seen = atomic_load(&sh->round);
		for (i = 0; i < len; i += PAGE)
			sink = p[i];
		atomic_fetch_add(&sh->done, 1);
	}
	(void)sink;
	_exit(0);
}

/* find "memcg <id> <path>" for our cgroup, then max_seq of node 0 */
static int lookup(const char *cg, unsigned long long *id, unsigned long *max_seq)
{
	char line[4096], path[4096];
	int found = 0, in_node = 0;
	unsigned long long mid;
	unsigned long seq;
	FILE *f = fopen(LRU_GEN, "r");

	if (!f) {
		perror(LRU_GEN);
		return -1;
	}
	while (fgets(line, sizeof(line), f)) {
		if (sscanf(line, "memcg %llu %4095s", &mid, path) == 2) {
			if (found)
				break;
			found = !strcmp(path, cg);
			if (found)
				*id = mid;
			in_node = 0;
			continue;
		}
		if (!found)
			continue;
		if (sscanf(line, " node %d", &in_node) == 1) {
			in_node = in_node == 0 ? 1 : -1;
			continue;
		}
		if (in_node == 1 && sscanf(line, " %lu", &seq) == 1)
			*max_seq = seq;
	}
	fclose(f);
	return found ? 0 : -1;
}

static void own_cgroup(char *buf, size_t sz)
{
	FILE *f = fopen("/proc/self/cgroup", "r");

	/* cgroup v2: "0::/path" */
	if (!f || !fgets(buf, sz, f) || strncmp(buf, "0::", 3)) {
		fprintf(stderr, "need cgroup v2\n");
		exit(1);
	}
	fclose(f);
	memmove(buf, buf + 3, strlen(buf + 3) + 1);
	buf[strcspn(buf, "\n")] = 0;
}

int main(int argc, char **argv)
{
	double t, sum = 0, mn = 1e30, mx = 0, tw;
	unsigned long long id;
	unsigned long max_seq;
	int nproc, rounds, fd, r, i, dfd;
	char cg[4096], cmd[128];
	size_t len, off;
	char *buf;

	if (argc < 5) {
		fprintf(stderr, "usage: %s <path> <MiB> <nproc> <rounds>\n", argv[0]);
		return 1;
	}
	len = strtoul(argv[2], NULL, 0) << 20;
	nproc = atoi(argv[3]);
	rounds = atoi(argv[4]);
	own_cgroup(cg, sizeof(cg));

	fd = open(argv[1], O_RDWR | O_CREAT | O_TRUNC, 0600);
	if (fd < 0 || ftruncate(fd, len)) {
		perror(argv[1]);
		return 1;
	}
	/* populate the page cache (charged to our memcg) */
	buf = calloc(1, 1 << 20);
	memset(buf, 0x5a, 1 << 20);
	for (off = 0; off < len; off += 1 << 20)
		if (pwrite(fd, buf, 1 << 20, off) != 1 << 20) {
			perror("pwrite");
			return 1;
		}

	sh = mmap(NULL, sizeof(*sh), PROT_READ | PROT_WRITE,
		  MAP_SHARED | MAP_ANONYMOUS, -1, 0);
	for (i = 0; i < nproc; i++)
		if (fork() == 0)
			child(fd, len);

	dfd = open(LRU_GEN, O_WRONLY);
	if (dfd < 0) {
		perror(LRU_GEN);
		return 1;
	}

	/* round 0 is warm-up: first touch, folios gain PG_workingset */
	for (r = 0; r <= rounds; r++) {
		atomic_store(&sh->done, 0);
		atomic_fetch_add(&sh->round, 1);
		while (atomic_load(&sh->done) < nproc)
			usleep(100);

		if (lookup(cg, &id, &max_seq)) {
			fprintf(stderr, "memcg %s not in %s\n", cg, LRU_GEN);
			return 1;
		}
		/* swappiness 200: walk anon (tmpfs) too; force_scan=1 */
		snprintf(cmd, sizeof(cmd), "+ %llu 0 %lu 200 1\n", id, max_seq);
		tw = now();
		if (write(dfd, cmd, strlen(cmd)) < 0) {
			perror("lru_gen write");
			return 1;
		}
		t = (now() - tw) * 1000;
		if (!r)
			continue;
		printf("round=%d max_seq=%lu ms=%.2f\n", r, max_seq, t);
		sum += t;
		mn = t < mn ? t : mn;
		mx = t > mx ? t : mx;
	}

	atomic_store(&sh->quit, 1);
	while (wait(NULL) > 0)
		;
	close(fd);
	unlink(argv[1]);
	printf("p4age nproc=%d MiB=%zu rounds=%d avg_ms=%.2f min_ms=%.2f max_ms=%.2f\n",
	       nproc, len >> 20, rounds, sum / rounds, mn, mx);
	return 0;
}

```
Quoted 44 lines
Quoted 39 lines

With one process, every visit changes the generation, so there is nothing to skip. Before the patch, perf annotate attributes 53-58% of the walk_update_folio() samples to the lock cmpxchg in the inlined folio_update_gen() in this test.

---
  include/linux/mm_inline.h | 2 ++
  mm/vmscan.c               | 2 ++
  2 files changed, 4 insertions(+)
include/linux/mm_inline.h +2 -0
index b3d85fe91385..4ba9079f4dc8 100644
@@ -244,6 +244,8 @@ static inline void folio_set_lru_refs(struct folio *folio, unsigned int refs)
         do {
                 new_flags = old_flags;
                 lru_set_refs_flags(&new_flags, refs);
+               if (new_flags == old_flags)
+                       break;
         } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
  }
 
mm/vmscan.c +2 -1
index 50c352e9ffef..3fa76715382d 100644
@@ -3398,6 +3398,8 @@ static int folio_update_gen(struct folio *folio, int new_gen, int *type,
                 lru_set_gen_flags(&new_flags, new_gen);
                 lru_set_refs_flags(&new_flags, 0);
                 new_flags |= BIT(PG_workingset);
+               if (new_flags == old_flags)
+                       break;
         } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
 
         *type = folio_flags_is_file_lru(&old_flags);
--
Quoted 2 lines

2.34.1

Thanks
Barry

Signature · 2 lines
Best regards
Ridong
#9 lore raw

RFC PATCH 3/3 mm/mglru: move lrugen atomic stats off the read-mostly lines

In reply to Ridong Chen

From: Ridong Chen <[email protected]>

lru_gen_eviction() and lru_gen_refault() update lrugen->evicted[] and lrugen->refaulted[] locklessly with atomic_long_add(). The arrays sit at the tail of struct lru_gen_folio, right before enabled/gen/seg/list:
with LRU_GEN_STATS=n the file type counters share a cacheline with them.

Those fields are read far more often than they are written:
lru_gen_add_folio() reads ->enabled on every LRU add, and shrink_many() walks ->list and reads ->gen/->seg under RCU. With frequent file refaults from several CPUs, every refault invalidates that line on the CPUs doing LRU adds.

Move enabled/gen/seg/list next to max_seq/min_seq/timestamps, which are also mostly read, and put evicted[]/refaulted[] in their own cacheline group at the end of the struct. This grows struct lru_gen_folio from 1360 to 1408 bytes and struct mem_cgroup_per_node from 2176 to 2240 bytes on x86_64 (one more cacheline per memcg per node).

Tested on an i7-13700HX (24 threads) with turbo disabled and the performance governor. 24 files of 128MiB on ext4 on brd, read with fio (psync, randread, bs=4k, fadvise_hint=random, buffered), all jobs in one memcg with memory.max=1G, so nearly every read refaults. IOPS, median of 5 runs:

jobs base patched

     1    270537    274303   +1.4%  (sd 3.7%)
     8   1461025   1466293   +0.4%
    16   2455629   2508001   +2.1%
    24   2707720   2823776   +4.3%

A custom random pread() test of the same setup, with this patch alone against base over two boots of each kernel, shows +2.4% at 16 threads and +5.9% at 24 threads, and no change at 8 threads or fewer. With anonymous memory on zram, the refaults mostly hit the tier 0 counters on a different line, and there is no measurable change.

The gain grows with the number of CPUs refaulting into the same lruvec and needs a fast backing store; with real IO it is likely to be lost in the noise. The patch is mostly meant to keep the lockless counters away from the read-mostly fields regardless of where they end up.

---
 include/linux/mmzone.h | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)
include/linux/mmzone.h +10 -8
index 6442e662aa4f..eb84af674e23 100644
@@ -551,6 +551,14 @@ struct lru_gen_folio {
 	unsigned long min_seq[ANON_AND_FILE];
 	/* the birth time of each generation in jiffies */
 	unsigned long timestamps[MAX_NR_GENS];
+	/* whether the multi-gen LRU is enabled */
+	bool enabled;
+	/* the memcg generation this lru_gen_folio belongs to */
+	u8 gen;
+	/* the list segment this lru_gen_folio belongs to */
+	u8 seg;
+	/* per-node lru_gen_folio list for global reclaim */
+	struct hlist_nulls_node list;
 	/* the multi-gen LRU lists, lazily sorted on eviction */
 	struct list_head folios[MAX_NR_GENS][ANON_AND_FILE][MAX_NR_ZONES];
 	/* the multi-gen LRU sizes, eventually consistent */
@@ -562,16 +570,10 @@ struct lru_gen_folio {
 	/* can only be modified under the LRU lock */
 	unsigned long protected[NR_HIST_GENS][ANON_AND_FILE][MAX_NR_TIERS];
 	/* can be modified without holding the LRU lock */
+	__cacheline_group_begin_aligned(lrugen_stats);
 	atomic_long_t evicted[NR_HIST_GENS][ANON_AND_FILE][MAX_NR_TIERS];
 	atomic_long_t refaulted[NR_HIST_GENS][ANON_AND_FILE][MAX_NR_TIERS];
-	/* whether the multi-gen LRU is enabled */
-	bool enabled;
-	/* the memcg generation this lru_gen_folio belongs to */
-	u8 gen;
-	/* the list segment this lru_gen_folio belongs to */
-	u8 seg;
-	/* per-node lru_gen_folio list for global reclaim */
-	struct hlist_nulls_node list;
+	__cacheline_group_end_aligned(lrugen_stats);
 };
 
 enum {
Signature · 1 line
2.34.1
reviewer
#10 lore raw

Re: [RFC PATCH 3/3] mm/mglru: move lrugen atomic stats off the read-mostly lines

Quoted 7 lines · Ridong Chen

From: Ridong Chen <[email protected]>

lru_gen_eviction() and lru_gen_refault() update lrugen->evicted[] and lrugen->refaulted[] locklessly with atomic_long_add(). The arrays sit at the tail of struct lru_gen_folio, right before enabled/gen/seg/list:
with LRU_GEN_STATS=n the file type counters share a cacheline with them.

So basically, this is causing false sharing, right?

I'd prefer a text diagram showing how the false sharing occurs, specifically how freq read and freq write end up in the same cacheline.

This will help clarify why rearranging the struct is the right approach and how to rearrange it properly.

Quoted 11 lines

Those fields are read far more often than they are written:
lru_gen_add_folio() reads ->enabled on every LRU add, and shrink_many() walks ->list and reads ->gen/->seg under RCU. With frequent file refaults from several CPUs, every refault invalidates that line on the CPUs doing LRU adds.

Move enabled/gen/seg/list next to max_seq/min_seq/timestamps, which are also mostly read, and put evicted[]/refaulted[] in their own cacheline group at the end of the struct. This grows struct lru_gen_folio from 1360 to 1408 bytes and struct mem_cgroup_per_node from 2176 to 2240

Why is the difference exactly 2240 - 2176 = 64 bytes, which is one cacheline? I was expecting a value smaller than one cacheline :-)

What I'm not convinced about is that nr_pages should be updated even more frequently than evicted and refaulted. Why aren't they causing a false sharing?

long nr_pages[MAX_NR_GENS][ANON_AND_FILE][MAX_NR_ZONES];

It would be better to have a text diagram showing the potential false sharing, so we can consider the issue more comprehensively.

Quoted 25 lines

bytes on x86_64 (one more cacheline per memcg per node).

Tested on an i7-13700HX (24 threads) with turbo disabled and the performance governor. 24 files of 128MiB on ext4 on brd, read with fio (psync, randread, bs=4k, fadvise_hint=random, buffered), all jobs in one memcg with memory.max=1G, so nearly every read refaults. IOPS, median of 5 runs:

jobs base patched

     1    270537    274303   +1.4%  (sd 3.7%)
     8   1461025   1466293   +0.4%
    16   2455629   2508001   +2.1%
    24   2707720   2823776   +4.3%

A custom random pread() test of the same setup, with this patch alone against base over two boots of each kernel, shows +2.4% at 16 threads and +5.9% at 24 threads, and no change at 8 threads or fewer. With anonymous memory on zram, the refaults mostly hit the tier 0 counters on a different line, and there is no measurable change.

The gain grows with the number of CPUs refaulting into the same lruvec and needs a fast backing store; with real IO it is likely to be lost in the noise. The patch is mostly meant to keep the lockless counters away from the read-mostly fields regardless of where they end up.

Quoted 5 lines
---
 include/linux/mmzone.h | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Thanks,
Barry

#11 lore raw

Re: [RFC PATCH 3/3] mm/mglru: move lrugen atomic stats off the read-mostly lines

Quoted 9 lines · Barry Song
Quoted 7 lines · Ridong Chen

From: Ridong Chen <[email protected]>

lru_gen_eviction() and lru_gen_refault() update lrugen->evicted[] and lrugen->refaulted[] locklessly with atomic_long_add(). The arrays sit at the tail of struct lru_gen_folio, right before enabled/gen/seg/list:
with LRU_GEN_STATS=n the file type counters share a cacheline with them.

Hi Barry,

Thanks for the review. Note that the sizes in the changelog were measured with lockdep enabled.

Offsets below are from pahole on x86_64, LRU_GEN_STATS=n, counted from the start of struct mem_cgroup_per_node (cacheline aligned), so line N = bytes N*64 .. N*64+63.

The nesting is:
mem_cgroup_per_node
lruvec at 128
lrugen at 128 + 176 = 304 (before the patch)
at 128 + 192 = 320 (after the patch)

Quoted 2 lines

So basically, this is causing false sharing, right?

Yes. Before the patch:

field                    offset       line
refaulted[0][file][*]    1608-1639    25    atomic, no lru_lock
enabled/gen/seg          1640-1642    25    read on every LRU add
list                     1648-1663    25    read by shrink_many()

line 25 (1600-1663)

+------------------------+-----------------+------+------------+
| refaulted[0][file][*]  | enabled,gen,seg | pad  | list       |
| 1608-1639              | 1640-1642       |      | 1648-1663  |
+------------------------+-----------------+------+------------+

CPU A (file refault) CPU B (LRU add)
atomic_long_add(&refaulted[..])

				read lrugen->enabled -> miss
atomic_long_add(&refaulted[..])
				read lrugen->enabled -> miss

CPU B doesn't need refaulted[], but it keeps missing on line 25.

After the patch:

field                    offset       line
max_seq/min_seq/ts       320-375      5
enabled/gen/seg          376-378      5     read-mostly
list                     384-399      6
nr_pages[]               1040-1359    16-21 under lru_lock
evicted[]                1600-1663    25    own line
refaulted[]              1664-1727    26    own line
Quoted 14 lines

I'd prefer a text diagram showing how the false sharing occurs, specifically how freq read and freq write end up in the same cacheline.

This will help clarify why rearranging the struct is the right approach and how to rearrange it properly.

Quoted 7 lines

Those fields are read far more often than they are written:
lru_gen_add_folio() reads ->enabled on every LRU add, and shrink_many() walks ->list and reads ->gen/->seg under RCU. With frequent file refaults from several CPUs, every refault invalidates that line on the CPUs doing LRU adds.

Quoted 9 lines
Quoted 4 lines

Move enabled/gen/seg/list next to max_seq/min_seq/timestamps, which are also mostly read, and put evicted[]/refaulted[] in their own cacheline group at the end of the struct. This grows struct lru_gen_folio from 1360 to 1408 bytes and struct mem_cgroup_per_node from 2176 to 2240

Why is the difference exactly 2240 - 2176 = 64 bytes, which is one cacheline? I was expecting a value smaller than one cacheline :-)

The aligned group makes lru_gen_folio cacheline aligned, and the padding adds up on the way out:

lru_gen_folio         1360 -> 1408  (+48, hole before evicted[])
lruvec                1624 -> 1728  (+104, lrugen moves from offset
				     176 to 192, plus tail padding to 64)

mem_cgroup_per_node 2048 -> 2112 (+64)

mem_cgroup_per_node is cacheline aligned, so its size only changes in whole lines. It had 40 bytes of padding after lruvec, which absorbs part of the 104 bytes, and the rest takes one more line.

Quoted 4 lines

What I'm not convinced about is that nr_pages should be updated even more frequently than evicted and refaulted. Why aren't they causing a false sharing?

nr_pages[] (lines 16-21) is read and written only under lru_lock. Its lines move between CPUs only when the lock does, and the new lock holder needs the updated values anyway, so that is true sharing. refaulted[] is written by CPUs that don't hold the lock, so it can steal the line from the lock holder at any time, and the lock holder doesn't even need it.

Quoted 39 lines

long nr_pages[MAX_NR_GENS][ANON_AND_FILE][MAX_NR_ZONES];

It would be better to have a text diagram showing the potential false sharing, so we can consider the issue more comprehensively.

Quoted 25 lines

bytes on x86_64 (one more cacheline per memcg per node).

Tested on an i7-13700HX (24 threads) with turbo disabled and the performance governor. 24 files of 128MiB on ext4 on brd, read with fio (psync, randread, bs=4k, fadvise_hint=random, buffered), all jobs in one memcg with memory.max=1G, so nearly every read refaults. IOPS, median of 5 runs:

   jobs      base   patched
      1    270537    274303   +1.4%  (sd 3.7%)
      8   1461025   1466293   +0.4%
     16   2455629   2508001   +2.1%
     24   2707720   2823776   +4.3%

A custom random pread() test of the same setup, with this patch alone against base over two boots of each kernel, shows +2.4% at 16 threads and +5.9% at 24 threads, and no change at 8 threads or fewer. With anonymous memory on zram, the refaults mostly hit the tier 0 counters on a different line, and there is no measurable change.

The gain grows with the number of CPUs refaulting into the same lruvec and needs a fast backing store; with real IO it is likely to be lost in the noise. The patch is mostly meant to keep the lockless counters away from the read-mostly fields regardless of where they end up.

Quoted 5 lines
---
  include/linux/mmzone.h | 18 ++++++++++--------
  1 file changed, 10 insertions(+), 8 deletions(-)

Thanks,
Barry

Signature · 2 lines
Best regards
Ridong