mm/mglru: clean up isolate_folios and scan_folios for readability and clarity
Subsystems (3)
Barry Song (Xiaomi)
reviewercover mm/mglru: clean up isolate_folios and scan_folios for readability and clarity
This is a cleanup series split out from the MGLRU swappiness series [1], with the cleanup changes separated to make them easier to review.
Right now, isolate_folios() is quite difficult to follow:
1. It uses for_each_evictable_type(i, swappiness) to iterate over the types, but i is not actually used as the type within the loop body.
2. It uses scanned == 0 to detect whether the current reclaim type is exhausted, but this is not an accurate indication.
3. It has an internal retry when no folios can be isolated after scanning some folios, but the retry is implemented in a way nobody can understand.
This patchset makes these behaviors explicit and much easier to follow.
Run kernel builds for several rounds in a 1 GB memcg and take the average build time. The patchset shows almost no performance impact, with a very small improvement that could simply be noise:
w/o patch w/patch Delta
real 109.149 108.990 -0.15% sys 223.213 222.592 -0.28%
pgscan_file 498,208 486,994 -2.25% refault_file 262,696 254,688 -3.05%
Thanks very much to Baolin and Lian for their previous reviews of the original RFC patchset for this part.
[1] https://lore.kernel.org/linux-mm/[email protected]/
Barry Song (Xiaomi) (2):
mm/mglru: improve scan_folios() exhaustion detection mm/mglru: retry the same type once if isolation fails due to races
Ridong Chen (1):
mm/mglru: improve readability of isolate_folios()
mm/vmscan.c | 78 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 54 insertions(+), 24 deletions(-)
Signature · 1 line
2.34.1
Barry Song (Xiaomi)
reviewerPATCH 1/3 mm/mglru: improve readability of isolate_folios()
From: Ridong Chen <[email protected]>
The for_each_evictable_type() loop in isolate_folios() is misleading: it does not actually iterate over each evictable type. Instead, get_type_to_scan() selects the type to scan, while the iterator i merely bounds the number of attempts.
Make the fallback behavior explicit in the code and remove the opaque for_each_evictable_type(i, swappiness).
--- mm/vmscan.c | 46 ++++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 20 deletions(-)
mm/vmscan.c +26 -20
@@ -4833,35 +4833,41 @@ static int get_type_to_scan(struct lruvec *lruvec, int swappiness)return positive_ctrl_err (&sp, &pv); } +static inline bool is_single_type_reclaim (int swappiness) +{ +return swappiness ==MIN_SWAPPINESS || + swappiness ==SWAPPINESS_ANON_ONLY ; +} +static int isolate_folios (unsigned long nr_to_scan,struct lruvec *lruvec,struct scan_control *sc,int swappiness,struct list_head *list,int *isolated,int *isolate_type,int *isolate_scanned) { -int i; -int total_scanned =0 ; +bool type_fallback_allowed = !is_single_type_reclaim (swappiness);int type =get_type_to_scan (lruvec, swappiness); +int total_scanned =0 , scanned, tier; -for_each_evictable_type (i, swappiness) { -int scanned; -int tier =get_tier_idx (lruvec, type); +retry: + tier =get_tier_idx (lruvec, type); + scanned =scan_folios (nr_to_scan, lruvec, sc, + type, tier, list, isolated); - scanned =scan_folios (nr_to_scan, lruvec, sc, - type, tier, list, isolated); + total_scanned += scanned; +if (*isolated) { + *isolate_type = type; + *isolate_scanned = scanned; +return total_scanned; + } - total_scanned += scanned; -if (*isolated) { - *isolate_type = type; - *isolate_scanned = scanned; -break ; - } -/* -* If scanned > 0 and isolated == 0, avoid falling back to the -* other type, as this type remains sufficient. Falling back -* too readily can disrupt the positive_ctrl_err() bias. -*/ -if (!scanned) - type = !type; +/* +* We are running out of the current reclaim type. Fall back to +* the other type if allowed. +*/ +if (!scanned && type_fallback_allowed) { + type = !type; + type_fallback_allowed = false; + goto retry; }return total_scanned;
Signature · 1 line
2.34.1
Re: [PATCH 1/3] mm/mglru: improve readability of isolate_folios()
Quoted 8 lines · Barry Song (Xiaomi)
From: Ridong Chen <[email protected]>
The
for_each_evictable_type()loop inisolate_folios()is misleading: it does not actually iterate over each evictable type. Instead,get_type_to_scan()selects the type to scan, while the iteratorimerely bounds the number of attempts.
Quoted 7 lines
Make the fallback behavior explicit in the code and remove the opaque
for_each_evictable_type(i, swappiness).Signed-off-by Ridong Chen <[email protected]>Co-developed-by Barry Song (Xiaomi) <[email protected]>Signed-off-by Barry Song (Xiaomi) <[email protected]>---
LGTM.
Quoted 8 lines
mm/vmscan.c | 46 ++++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 20 deletions(-)mm/vmscan.c +0
-0
mm/vmscan.c static int get_type_to_scan(struct lruvec *lruvec, int swappiness) · line 4840
Quoted 8 lines
mm/vmscan.c +5
-0@@ -4833,35 +4833,41 @@ static int get_type_to_scan(struct lruvec *lruvec, int swappiness)return positive_ctrl_err (&sp, &pv); } +static inline bool is_single_type_reclaim (int swappiness) +{ +return swappiness ==MIN_SWAPPINESS || + swappiness ==SWAPPINESS_ANON_ONLY ; +}
I can't think of a better function name either, so I'm fine with it :)
Re: [PATCH 1/3] mm/mglru: improve readability of isolate_folios()
In reply to Barry Song (Xiaomi)Quoted 31 lines · Barry Song (Xiaomi)
From: Ridong Chen <[email protected]>
The
for_each_evictable_type()loop inisolate_folios()is misleading: it does not actually iterate over each evictable type. Instead,get_type_to_scan()selects the type to scan, while the iteratorimerely bounds the number of attempts.Make the fallback behavior explicit in the code and remove the opaque
for_each_evictable_type(i, swappiness).Signed-off-by Ridong Chen <[email protected]>Co-developed-by Barry Song (Xiaomi) <[email protected]>Signed-off-by Barry Song (Xiaomi) <[email protected]>--- mm/vmscan.c | 46 ++++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 20 deletions(-)mm/vmscan.c +4
-0@@ -4833,35 +4833,41 @@ static int get_type_to_scan(struct lruvec *lruvec, int swappiness)return positive_ctrl_err (&sp, &pv); } +static inline bool is_single_type_reclaim (int swappiness) +{ +return swappiness ==MIN_SWAPPINESS || + swappiness ==SWAPPINESS_ANON_ONLY ;
mm/vmscan.c static int get_type_to_scan(struct lruvec *lruvec, int swappiness) · line 4847
Quoted 10 lines
mm/vmscan.c +3
-2+} +static int isolate_folios (unsigned long nr_to_scan,struct lruvec *lruvec,struct scan_control *sc,int swappiness,struct list_head *list,int *isolated,int *isolate_type,int *isolate_scanned) { -int i; -int total_scanned =0 ; +bool type_fallback_allowed = !is_single_type_reclaim (swappiness);
Hmm, this is the only user of the function and it takes the negative of the function, will it be better to just revert the conditions and rename the function?
e.g. is_mixed_reclaim / is_proportional_reclaim, return swappiness != MIN_SWAPPINESS && swappiness != SWAPPINESS_ANON_ONLY?
Quoted 32 lines
mm/vmscan.c +11
-17int type =get_type_to_scan (lruvec, swappiness); +int total_scanned =0 , scanned, tier; -for_each_evictable_type (i, swappiness) { -int scanned; -int tier =get_tier_idx (lruvec, type); +retry: + tier =get_tier_idx (lruvec, type); + scanned =scan_folios (nr_to_scan, lruvec, sc, + type, tier, list, isolated); - scanned =scan_folios (nr_to_scan, lruvec, sc, - type, tier, list, isolated); + total_scanned += scanned; +if (*isolated) { + *isolate_type = type; + *isolate_scanned = scanned; +return total_scanned; + } - total_scanned += scanned; -if (*isolated) { - *isolate_type = type; - *isolate_scanned = scanned; -break ; - } -/* -* If scanned > 0 and isolated == 0, avoid falling back to the -* other type, as this type remains sufficient. Falling back -* too readily can disrupt the positive_ctrl_err() bias. -*/ -if (!scanned)
mm/vmscan.c static int get_type_to_scan(struct lruvec *lruvec, int swappiness) · line 4871
Quoted 10 lines
mm/vmscan.c +8
-1- type = !type; +/* +* We are running out of the current reclaim type. Fall back to +* the other type if allowed. +*/ +if (!scanned && type_fallback_allowed) { + type = !type; + type_fallback_allowed = false; + goto retry; }
Looks good to me, the nitpick can be ignored.
Baoquan He
reviewerRe: [PATCH 1/3] mm/mglru: improve readability of isolate_folios()
In reply to Barry Song (Xiaomi)Quoted 11 lines · Barry Song (Xiaomi)
From: Ridong Chen <[email protected]>
The
for_each_evictable_type()loop inisolate_folios()is misleading: it does not actually iterate over each evictable type. Instead,get_type_to_scan()selects the type to scan, while the iteratorimerely bounds the number of attempts.Make the fallback behavior explicit in the code and remove the opaque
for_each_evictable_type(i, swappiness).
Quoted 6 lines
Signed-off-by Ridong Chen <[email protected]>Co-developed-by Barry Song (Xiaomi) <[email protected]>Signed-off-by Barry Song (Xiaomi) <[email protected]>--- mm/vmscan.c | 46 ++++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 20 deletions(-)
The subject doens't reflect the truth. This patch changes the behaviour, but not improve readability of isolate_folios() only.
I also noticed the confusion of isolate_folios() implementation, and made a patch to only change the local variable and added code comment to explain it in my local branch. Surely refactorying is also good.
+ * Scan at most one type per evictable type (anon/file), starting with + * the type get_type_to_scan() picked as statistically colder. + * + * After a scan: + * - isolated > 0: got folios, record the type and return. + * - scanned == 0: the type is empty; fall back to the other type. + * - otherwise: the type has folios but all were hot (or lost an + * isolate race); retry the same type rather than + * switch, so positive_ctrl_err()'s refault + * statistics stay unbiased. + */
While in Ridong's patch, the 3rd case disappeared. It doesn't rescan with the original type as the old code is doing, but return directly.
Quoted 70 lines
mm/vmscan.c +26
-20@@ -4833,35 +4833,41 @@ static int get_type_to_scan(struct lruvec *lruvec, int swappiness)return positive_ctrl_err (&sp, &pv); } +static inline bool is_single_type_reclaim (int swappiness) +{ +return swappiness ==MIN_SWAPPINESS || + swappiness ==SWAPPINESS_ANON_ONLY ; +} +static int isolate_folios (unsigned long nr_to_scan,struct lruvec *lruvec,struct scan_control *sc,int swappiness,struct list_head *list,int *isolated,int *isolate_type,int *isolate_scanned) { -int i; -int total_scanned =0 ; +bool type_fallback_allowed = !is_single_type_reclaim (swappiness);int type =get_type_to_scan (lruvec, swappiness); +int total_scanned =0 , scanned, tier; -for_each_evictable_type (i, swappiness) { -int scanned; -int tier =get_tier_idx (lruvec, type); +retry: + tier =get_tier_idx (lruvec, type); + scanned =scan_folios (nr_to_scan, lruvec, sc, + type, tier, list, isolated); - scanned =scan_folios (nr_to_scan, lruvec, sc, - type, tier, list, isolated); + total_scanned += scanned; +if (*isolated) { + *isolate_type = type; + *isolate_scanned = scanned; +return total_scanned; + } - total_scanned += scanned; -if (*isolated) { - *isolate_type = type; - *isolate_scanned = scanned; -break ; - } -/* -* If scanned > 0 and isolated == 0, avoid falling back to the -* other type, as this type remains sufficient. Falling back -* too readily can disrupt the positive_ctrl_err() bias. -*/ -if (!scanned) - type = !type; +/* +* We are running out of the current reclaim type. Fall back to +* the other type if allowed. +*/ +if (!scanned && type_fallback_allowed) { + type = !type; + type_fallback_allowed = false; + goto retry; }return total_scanned;Signature · 1 line
2.34.1
Barry Song
reviewerRe: [PATCH 1/3] mm/mglru: improve readability of isolate_folios()
Quoted 39 lines · Baoquan He
Quoted 11 lines · Barry Song (Xiaomi)
From: Ridong Chen <[email protected]>
The
for_each_evictable_type()loop inisolate_folios()is misleading: it does not actually iterate over each evictable type. Instead,get_type_to_scan()selects the type to scan, while the iteratorimerely bounds the number of attempts.Make the fallback behavior explicit in the code and remove the opaque
for_each_evictable_type(i, swappiness).Quoted 6 lines
Signed-off-by Ridong Chen <[email protected]>Co-developed-by Barry Song (Xiaomi) <[email protected]>Signed-off-by Barry Song (Xiaomi) <[email protected]>--- mm/vmscan.c | 46 ++++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 20 deletions(-)The subject doens't reflect the truth. This patch changes the behaviour, but not improve readability of
isolate_folios()only.I also noticed the confusion of
isolate_folios()implementation, and made a patch to only change the local variable and added code comment to explain it in my local branch. Surely refactorying is also good.(quoted diff) +11
-0+ * Scan at most one type per evictable type (anon/file), starting with + * the type get_type_to_scan() picked as statistically colder. + * + * After a scan: + * - isolated > 0: got folios, record the type and return. + * - scanned == 0: the type is empty; fall back to the other type. + * - otherwise: the type has folios but all were hot (or lost an + * isolate race); retry the same type rather than + * switch, so positive_ctrl_err()'s refault + * statistics stay unbiased. + */
Quoted 3 lines
While in Ridong's patch, the 3rd case disappeared. It doesn't rescan with the original type as the old code is doing, but return directly.
right, the 3rd case comes back in patch 3/3:
https://lore.kernel.org/linux-mm/[email protected]/
so i guess we can rename patch 1/3?
Best Regards
Barry
Baoquan He
reviewerRe: [PATCH 1/3] mm/mglru: improve readability of isolate_folios()
Quoted 40 lines · Barry Song
Quoted 39 lines · Baoquan He
Quoted 11 lines · Barry Song (Xiaomi)
From: Ridong Chen <[email protected]>
The
for_each_evictable_type()loop inisolate_folios()is misleading: it does not actually iterate over each evictable type. Instead,get_type_to_scan()selects the type to scan, while the iteratorimerely bounds the number of attempts.Make the fallback behavior explicit in the code and remove the opaque
for_each_evictable_type(i, swappiness).Quoted 6 lines
Signed-off-by Ridong Chen <[email protected]>Co-developed-by Barry Song (Xiaomi) <[email protected]>Signed-off-by Barry Song (Xiaomi) <[email protected]>--- mm/vmscan.c | 46 ++++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 20 deletions(-)The subject doens't reflect the truth. This patch changes the behaviour, but not improve readability of
isolate_folios()only.I also noticed the confusion of
isolate_folios()implementation, and made a patch to only change the local variable and added code comment to explain it in my local branch. Surely refactorying is also good.(quoted diff) +11
-0+ * Scan at most one type per evictable type (anon/file), starting with + * the type get_type_to_scan() picked as statistically colder. + * + * After a scan: + * - isolated > 0: got folios, record the type and return. + * - scanned == 0: the type is empty; fall back to the other type. + * - otherwise: the type has folios but all were hot (or lost an + * isolate race); retry the same type rather than + * switch, so positive_ctrl_err()'s refault + * statistics stay unbiased. + */
Quoted 8 lines
Quoted 3 lines
While in Ridong's patch, the 3rd case disappeared. It doesn't rescan with the original type as the old code is doing, but return directly.
right, the 3rd case comes back in patch 3/3:
https://lore.kernel.org/linux-mm/[email protected]/so i guess we can rename patch 1/3?
Both is fine as long as thing is correct and clear, thanks.
Barry Song (Xiaomi)
reviewerPATCH 2/3 mm/mglru: improve scan_folios() exhaustion detection
In reply to Barry Song (Xiaomi)Commit 16b475d2ac3c ("mm/mglru: avoid reclaim type fall back when isolation makes no progress") uses scanned == 0 to determine whether scan_folios() has exhausted a reclaim type. However, this is not always sufficient. It is possible for scanned > 0, while the oldest reclaimable generation is exhausted after the first scan_folios() call.
We detect early_stop in scan_folios(). If we stop early for any reason, it means the current reclaim type is not exhausted yet. If early_stop is never reached, it means we have exhausted the current oldest generation without hitting any scanning limit.
Another issue is that if the lruvec has 4 generations, we might have exhausted the oldest generation while the second oldest generation is still reclaimable. In that case, this type is not exhausted yet.
Add an exhausted output argument to scan_folios() so it can explicitly report whether the reclaimable lists for the current type have been exhausted.
--- mm/vmscan.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)
mm/vmscan.c +22 -6
@@ -4721,7 +4721,8 @@ static bool isolate_folio(struct lruvec *lruvec, struct folio *folio, struct scastatic int scan_folios (unsigned long nr_to_scan,struct lruvec *lruvec,struct scan_control *sc,int type,int tier, -struct list_head *list,int *isolatedp) +struct list_head *list,int *isolatedp, +bool *exhausted) {int i;int gen;
@@ -4732,12 +4733,15 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec,int skipped =0 ;unsigned long remaining = nr_to_scan;struct lru_gen_folio *lrugen = &lruvec->lrugen; +bool early_stop = false;VM_WARN_ON_ONCE (nr_to_scan >MAX_LRU_BATCH );VM_WARN_ON_ONCE (!list_empty (list)); -if (get_nr_gens (lruvec, type) ==MIN_NR_GENS ) +if (get_nr_gens (lruvec, type) ==MIN_NR_GENS ) { + *exhausted = true;return 0 ; + } gen =lru_gen_from_seq (lrugen->min_seq[type]);
@@ -4768,8 +4772,10 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec, skipped_zone += delta; } -if (!--remaining ||max (isolated, skipped_zone) >=MIN_LRU_BATCH ) +if (!--remaining ||max (isolated, skipped_zone) >=MIN_LRU_BATCH ) { + early_stop = true;break ; + } }if (skipped_zone) {
@@ -4778,8 +4784,10 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec, skipped += skipped_zone; } -if (!remaining || isolated >=MIN_LRU_BATCH ) +if (!remaining || isolated >=MIN_LRU_BATCH ) { + early_stop = true;break ; + } } item =PGSCAN_KSWAPD +reclaimer_offset (sc);
@@ -4790,6 +4798,13 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec, scanned, skipped, isolated, type ? LRU_INACTIVE_FILE : LRU_INACTIVE_ANON); +/* +* If we didn't stop early, all reclaimable folios in the current +* generation have been scanned. We are exhausted if this is the last +* reclaimable generation. +*/ + *exhausted = !early_stop && + lrugen->min_seq[type] +MIN_NR_GENS == lrugen->max_seq; *isolatedp = isolated;return scanned; }
@@ -4847,11 +4862,12 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,bool type_fallback_allowed = !is_single_type_reclaim (swappiness);int type =get_type_to_scan (lruvec, swappiness);int total_scanned =0 , scanned, tier; +bool exhausted; retry: tier =get_tier_idx (lruvec, type); scanned =scan_folios (nr_to_scan, lruvec, sc, - type, tier, list, isolated); + type, tier, list, isolated, &exhausted); total_scanned += scanned;if (*isolated) {
@@ -4864,7 +4880,7 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,* We are running out of the current reclaim type. Fall back to * the other type if allowed. */ -if (!scanned && type_fallback_allowed) { +if (exhausted && type_fallback_allowed) { type = !type; type_fallback_allowed = false; goto retry;
Signature · 1 line
2.34.1
Re: [PATCH 2/3] mm/mglru: improve scan_folios() exhaustion detection
Quoted 109 lines · Barry Song (Xiaomi)
Commit
16b475d2ac3c("mm/mglru: avoid reclaim type fall back when isolation makes no progress") uses scanned == 0 to determine whetherscan_folios()has exhausted a reclaim type. However, this is not always sufficient. It is possible for scanned > 0, while the oldest reclaimable generation is exhausted after the firstscan_folios()call.We detect
early_stopinscan_folios(). If we stop early for any reason, it means the current reclaim type is not exhausted yet. Ifearly_stopis never reached, it means we have exhausted the current oldest generation without hitting any scanning limit.Another issue is that if the lruvec has 4 generations, we might have exhausted the oldest generation while the second oldest generation is still reclaimable. In that case, this type is not exhausted yet.
Add an exhausted output argument to
scan_folios()so it can explicitly report whether the reclaimable lists for the current type have been exhausted.Signed-off-by Barry Song (Xiaomi) <[email protected]>--- mm/vmscan.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)mm/vmscan.c +21
-5@@ -4721,7 +4721,8 @@ static bool isolate_folio(struct lruvec *lruvec, struct folio *folio, struct scastatic int scan_folios (unsigned long nr_to_scan,struct lruvec *lruvec,struct scan_control *sc,int type,int tier, -struct list_head *list,int *isolatedp) +struct list_head *list,int *isolatedp, +bool *exhausted) {int i;int gen;@@ -4732,12 +4733,15 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec,int skipped =0 ;unsigned long remaining = nr_to_scan;struct lru_gen_folio *lrugen = &lruvec->lrugen; +bool early_stop = false;VM_WARN_ON_ONCE (nr_to_scan >MAX_LRU_BATCH );VM_WARN_ON_ONCE (!list_empty (list)); -if (get_nr_gens (lruvec, type) ==MIN_NR_GENS ) +if (get_nr_gens (lruvec, type) ==MIN_NR_GENS ) { + *exhausted = true;return 0 ; + } gen =lru_gen_from_seq (lrugen->min_seq[type]);@@ -4768,8 +4772,10 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec, skipped_zone += delta; } -if (!--remaining ||max (isolated, skipped_zone) >=MIN_LRU_BATCH ) +if (!--remaining ||max (isolated, skipped_zone) >=MIN_LRU_BATCH ) { + early_stop = true;break ; + } }if (skipped_zone) {@@ -4778,8 +4784,10 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec, skipped += skipped_zone; } -if (!remaining || isolated >=MIN_LRU_BATCH ) +if (!remaining || isolated >=MIN_LRU_BATCH ) { + early_stop = true;break ; + } } item =PGSCAN_KSWAPD +reclaimer_offset (sc);@@ -4790,6 +4798,13 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec, scanned, skipped, isolated, type ? LRU_INACTIVE_FILE : LRU_INACTIVE_ANON); +/* +* If we didn't stop early, all reclaimable folios in the current +* generation have been scanned. We are exhausted if this is the last +* reclaimable generation. +*/ + *exhausted = !early_stop && + lrugen->min_seq[type] +MIN_NR_GENS == lrugen->max_seq; *isolatedp = isolated;return scanned; }@@ -4847,11 +4862,12 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,bool type_fallback_allowed = !is_single_type_reclaim (swappiness);int type =get_type_to_scan (lruvec, swappiness);int total_scanned =0 , scanned, tier; +bool exhausted; retry: tier =get_tier_idx (lruvec, type); scanned =scan_folios (nr_to_scan, lruvec, sc, - type, tier, list, isolated); + type, tier, list, isolated, &exhausted); total_scanned += scanned;if (*isolated) {
mm/vmscan.c static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec, · line 4886
Quoted 8 lines
mm/vmscan.c +1
-1@@ -4864,7 +4880,7 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,* We are running out of the current reclaim type. Fall back to * the other type if allowed. */ -if (!scanned && type_fallback_allowed) { +if (exhausted && type_fallback_allowed) { type = !type; type_fallback_allowed = false; goto retry;
LGTM.
Signature · 2 lines
Best regards Ridong
Re: [PATCH 2/3] mm/mglru: improve scan_folios() exhaustion detection
In reply to Barry Song (Xiaomi)Quoted 16 lines · Barry Song (Xiaomi)
Commit
16b475d2ac3c("mm/mglru: avoid reclaim type fall back when isolation makes no progress") uses scanned == 0 to determine whetherscan_folios()has exhausted a reclaim type. However, this is not always sufficient. It is possible for scanned > 0, while the oldest reclaimable generation is exhausted after the firstscan_folios()call.We detect
early_stopinscan_folios(). If we stop early for any reason, it means the current reclaim type is not exhausted yet. Ifearly_stopis never reached, it means we have exhausted the current oldest generation without hitting any scanning limit.Another issue is that if the lruvec has 4 generations, we might have exhausted the oldest generation while the second oldest generation is still reclaimable. In that case, this type is not exhausted yet.
Quoted 6 lines
Add an exhausted output argument to
scan_folios()so it can explicitly report whether the reclaimable lists for the current type have been exhausted.Signed-off-by Barry Song (Xiaomi) <[email protected]>---
LGTM. One nit below.
Quoted 59 lines
mm/vmscan.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)mm/vmscan.c +12
-4@@ -4721,7 +4721,8 @@ static bool isolate_folio(struct lruvec *lruvec, struct folio *folio, struct scastatic int scan_folios (unsigned long nr_to_scan,struct lruvec *lruvec,struct scan_control *sc,int type,int tier, -struct list_head *list,int *isolatedp) +struct list_head *list,int *isolatedp, +bool *exhausted) {int i;int gen;@@ -4732,12 +4733,15 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec,int skipped =0 ;unsigned long remaining = nr_to_scan;struct lru_gen_folio *lrugen = &lruvec->lrugen; +bool early_stop = false;VM_WARN_ON_ONCE (nr_to_scan >MAX_LRU_BATCH );VM_WARN_ON_ONCE (!list_empty (list)); -if (get_nr_gens (lruvec, type) ==MIN_NR_GENS ) +if (get_nr_gens (lruvec, type) ==MIN_NR_GENS ) { + *exhausted = true;return 0 ; + } gen =lru_gen_from_seq (lrugen->min_seq[type]);@@ -4768,8 +4772,10 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec, skipped_zone += delta; } -if (!--remaining ||max (isolated, skipped_zone) >=MIN_LRU_BATCH ) +if (!--remaining ||max (isolated, skipped_zone) >=MIN_LRU_BATCH ) { + early_stop = true;break ; + } }if (skipped_zone) {@@ -4778,8 +4784,10 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec, skipped += skipped_zone; } -if (!remaining || isolated >=MIN_LRU_BATCH ) +if (!remaining || isolated >=MIN_LRU_BATCH ) { + early_stop = true;break ; + } } item =PGSCAN_KSWAPD +reclaimer_offset (sc);
mm/vmscan.c static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec, · line 4807
Quoted 10 lines
mm/vmscan.c +7
-0@@ -4790,6 +4798,13 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec, scanned, skipped, isolated, type ? LRU_INACTIVE_FILE : LRU_INACTIVE_ANON); +/* +* If we didn't stop early, all reclaimable folios in the current +* generation have been scanned. We are exhausted if this is the last +* reclaimable generation. +*/ + *exhausted = !early_stop && + lrugen->min_seq[type] +MIN_NR_GENS == lrugen->max_seq;
Nit: using get_nr_gens() looks more readable to me:
*exhausted = !early_stop && get_nr_gens(lruvec, type) == MIN_NR_GENS + 1;
Barry Song
reviewerRe: [PATCH 2/3] mm/mglru: improve scan_folios() exhaustion detection
Quoted 24 lines · Baolin Wang
Quoted 20 lines · Barry Song (Xiaomi)
Commit
16b475d2ac3c("mm/mglru: avoid reclaim type fall back when isolation makes no progress") uses scanned == 0 to determine whetherscan_folios()has exhausted a reclaim type. However, this is not always sufficient. It is possible for scanned > 0, while the oldest reclaimable generation is exhausted after the firstscan_folios()call.We detect
early_stopinscan_folios(). If we stop early for any reason, it means the current reclaim type is not exhausted yet. Ifearly_stopis never reached, it means we have exhausted the current oldest generation without hitting any scanning limit.Another issue is that if the lruvec has 4 generations, we might have exhausted the oldest generation while the second oldest generation is still reclaimable. In that case, this type is not exhausted yet.
Add an exhausted output argument to
scan_folios()so it can explicitly report whether the reclaimable lists for the current type have been exhausted.
Quoted 6 lines
Quoted 2 lines
Signed-off-by Barry Song (Xiaomi) <[email protected]>---
LGTM. One nit below.
Reviewed-by Baolin Wang <[email protected]>
Thanks for the review, Baolin.
[...]
Quoted 6 lines
mm/vmscan.c static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec, · line 4807
Quoted 2 lines
mm/vmscan.c +2
-0+ *exhausted = !early_stop && + lrugen->min_seq[type] +MIN_NR_GENS == lrugen->max_seq;Nit: using
get_nr_gens()looks more readable to me:*exhausted = !early_stop &&
get_nr_gens(lruvec, type)==MIN_NR_GENS+ 1;
Yes, it seems more readable.
Best Regards
Barry
Barry Song (Xiaomi)
reviewerPATCH 3/3 mm/mglru: retry the same type once if isolation fails due to races
In reply to Barry Song (Xiaomi)If we are not exhausted (i.e., there are still folios in the reclaimable generations) but fail to isolate any folios due to promotions, protections, or races, retry once more to avoid going through the outer loop again.
--- mm/vmscan.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
mm/vmscan.c +9 -1
@@ -4862,7 +4862,7 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,bool type_fallback_allowed = !is_single_type_reclaim (swappiness);int type =get_type_to_scan (lruvec, swappiness);int total_scanned =0 , scanned, tier; -bool exhausted; +bool exhausted, tried = false; retry: tier =get_tier_idx (lruvec, type);
@@ -4885,6 +4885,14 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec, type_fallback_allowed = false; goto retry; } +/* +* We are not exhausted, but failed to isolate any folios due to +* promotions, protections, or races. Retry once to avoid a larger loop. +*/ +if (!exhausted && !tried) { + tried = true; + goto retry; + }return total_scanned; }
Signature · 1 line
2.34.1
Re: [PATCH 3/3] mm/mglru: retry the same type once if isolation fails due to races
Quoted 28 lines · Barry Song (Xiaomi)
If we are not exhausted (i.e., there are still folios in the reclaimable generations) but fail to isolate any folios due to promotions, protections, or races, retry once more to avoid going through the outer loop again.
Signed-off-by Barry Song (Xiaomi) <[email protected]>--- mm/vmscan.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)mm/vmscan.c +2
-1@@ -4862,7 +4862,7 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,bool type_fallback_allowed = !is_single_type_reclaim (swappiness);int type =get_type_to_scan (lruvec, swappiness);int total_scanned =0 , scanned, tier; -bool exhausted; +bool exhausted, tried = false; retry: tier =get_tier_idx (lruvec, type);@@ -4885,6 +4885,14 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec, type_fallback_allowed = false; goto retry; } +/*
mm/vmscan.c static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec, · line 4898
Quoted 10 lines
mm/vmscan.c +7
-0+* We are not exhausted, but failed to isolate any folios due to +* promotions, protections, or races. Retry once to avoid a larger loop. +*/ +if (!exhausted && !tried) { + tried = true; + goto retry; + }return total_scanned; }
LGTM.
Signature · 2 lines
Best regards Ridong
Re: [PATCH 3/3] mm/mglru: retry the same type once if isolation fails due to races
In reply to Barry Song (Xiaomi)Quoted 7 lines · Barry Song (Xiaomi)
If we are not exhausted (i.e., there are still folios in the reclaimable generations) but fail to isolate any folios due to promotions, protections, or races, retry once more to avoid going through the outer loop again.
Signed-off-by Barry Song (Xiaomi) <[email protected]>---
Make sense to me.
From: Lian Wang <[email protected]>
Hi Barry,
I went through all three patches. The explicit fallback, exhaustion reporting, and bounded retry make the control flow much clearer.
In particular, the exhaustion check preserves the early-stop cases and avoids falling back after only the oldest generation has been drained when another generation remains reclaimable. The retry is still bounded to one extra attempt.
No issues from my side.
For the series:
Thanks,
Lian
Quoted 7 lines · On Thu, 20 Aug 2026 12:56:00 +0800 "Barry Song (Xiaomi)" <[email protected]>…
This is a cleanup series split out from the MGLRU swappiness series [1], with the cleanup changes separated to make them easier to review.
This patchset makes these behaviors explicit and much easier to follow.
Thanks very much to Baolin and Lian for their previous reviews of the original RFC patchset for this part.
Baolin Wang
reviewerQuoted 11 lines · Barry Song (Xiaomi)
This is a cleanup series split out from the MGLRU swappiness series [1], with the cleanup changes separated to make them easier to review.
Right now,
isolate_folios()is quite difficult to follow:1. It uses
for_each_evictable_type(i, swappiness)to iterate over the types, but i is not actually used as the type within the loop body.2. It uses scanned == 0 to detect whether the current reclaim type is exhausted, but this is not an accurate indication.
Quoted 8 lines
3. It has an internal retry when no folios can be isolated after scanning some folios, but the retry is implemented in a way nobody can understand.
This patchset makes these behaviors explicit and much easier to follow.
Run kernel builds for several rounds in a 1 GB memcg and take the average build time. The patchset shows almost no performance impact, with a very small improvement that could simply be noise:
Just FYI:
I tested this patchset with a 3G memcg limit and a 10G zram device, running 'make -j32' to build kernel on my 32-core Arm machines, and got some performance improvement for ths sys time:
w/o patch w/patch
sys 1845s 1570s
Quoted 23 lines
w/o patch w/patch Delta
real 109.149 108.990 -0.15% sys 223.213 222.592 -0.28%pgscan_file 498,208 486,994 -2.25% refault_file 262,696 254,688 -3.05%Thanks very much to Baolin and Lian for their previous reviews of the original RFC patchset for this part.
[1] https://lore.kernel.org/linux-mm/[email protected]/
Barry Song (Xiaomi) (2):
mm/mglru: improvescan_folios()exhaustion detection mm/mglru: retry the same type once if isolation fails due to racesRidong Chen (1):
mm/mglru: improve readability ofisolate_folios()mm/vmscan.c | 78 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 54 insertions(+), 24 deletions(-)
Quoted 21 lines · Baolin Wang
Quoted 11 lines · Barry Song (Xiaomi)
This is a cleanup series split out from the MGLRU swappiness series [1], with the cleanup changes separated to make them easier to review.
Right now,
isolate_folios()is quite difficult to follow:1. It uses
for_each_evictable_type(i, swappiness)to iterate over the types, but i is not actually used as the type within the loop body.2. It uses scanned == 0 to detect whether the current reclaim type is exhausted, but this is not an accurate indication.
Quoted 8 lines
3. It has an internal retry when no folios can be isolated after scanning some folios, but the retry is implemented in a way nobody can understand.
This patchset makes these behaviors explicit and much easier to follow.
Run kernel builds for several rounds in a 1 GB memcg and take the average build time. The patchset shows almost no performance impact, with a very small improvement that could simply be noise:
Quoted 8 lines
Just FYI:
I tested this patchset with a 3G memcg limit and a 10G zram device, running 'make -j32' to build kernel on my 32-core Arm machines, and got some performance improvement for ths sys time:
w/o patch w/patch
sys 1845s 1570s
Hi Baoliln
That's a very interesting result, can you share a bit more info about it? e.g. vmstat? I'm curious how this happens.
I suspect patch 2 or 3 changes the swappiness / reclaim / aging type selection behavior, or maybe it reduced the reclaim amount?
Baolin Wang
reviewerQuoted 32 lines · Kairui Song
Quoted 21 lines · Baolin Wang
Quoted 11 lines · Barry Song (Xiaomi)
This is a cleanup series split out from the MGLRU swappiness series [1], with the cleanup changes separated to make them easier to review.
Right now,
isolate_folios()is quite difficult to follow:1. It uses
for_each_evictable_type(i, swappiness)to iterate over the types, but i is not actually used as the type within the loop body.2. It uses scanned == 0 to detect whether the current reclaim type is exhausted, but this is not an accurate indication.
Quoted 8 lines
3. It has an internal retry when no folios can be isolated after scanning some folios, but the retry is implemented in a way nobody can understand.
This patchset makes these behaviors explicit and much easier to follow.
Run kernel builds for several rounds in a 1 GB memcg and take the average build time. The patchset shows almost no performance impact, with a very small improvement that could simply be noise:
Quoted 8 lines
Just FYI:
I tested this patchset with a 3G memcg limit and a 10G zram device, running 'make -j32' to build kernel on my 32-core Arm machines, and got some performance improvement for ths sys time:
w/o patch w/patch
sys 1845s 1570s
Quoted 4 lines
Hi Baoliln
That's a very interesting result, can you share a bit more info about it? e.g. vmstat? I'm curious how this happens.
Sure.
Quoted 2 lines
I suspect patch 2 or 3 changes the swappiness / reclaim / aging type selection behavior, or maybe it reduced the reclaim amount?
I gathered the memcg stats as shown below.
It looks like the direct reason for the performance improvement is an obvious reduction in anon refaults, which is what this patch aims to achieve I think. That is to say, we should respect the type chosen by the PID for reclaim, and try to exhaust that type before falling back to another.
Before this series:workingset_refault_anon 59706846workingset_refault_file 9229283workingset_activate_anon 15108561workingset_activate_file 365660workingset_restore_anon 15108561workingset_restore_file 1018493workingset_nodereclaim 0pgsteal_kswapd 0pgsteal_direct 92589340pgsteal_khugepaged 549pgsteal_proactive 0pgscan_kswapd 0pgscan_direct 456629355pgscan_khugepaged 549
After this series:workingset_refault_anon 41277086workingset_refault_file 9131522workingset_activate_anon 12924656workingset_activate_file 345901workingset_restore_anon 12924656workingset_restore_file 1005959workingset_nodereclaim 0pgsteal_kswapd 0pgsteal_direct 71723800pgsteal_khugepaged 4013pgsteal_proactive 0pgscan_kswapd 0pgscan_direct 342179445pgscan_khugepaged 9311
Quoted 67 lines · Baolin Wang
Quoted 32 lines · Kairui Song
Quoted 21 lines · Baolin Wang
Quoted 11 lines · Barry Song (Xiaomi)
This is a cleanup series split out from the MGLRU swappiness series [1], with the cleanup changes separated to make them easier to review.
Right now,
isolate_folios()is quite difficult to follow:1. It uses
for_each_evictable_type(i, swappiness)to iterate over the types, but i is not actually used as the type within the loop body.2. It uses scanned == 0 to detect whether the current reclaim type is exhausted, but this is not an accurate indication.
Quoted 8 lines
3. It has an internal retry when no folios can be isolated after scanning some folios, but the retry is implemented in a way nobody can understand.
This patchset makes these behaviors explicit and much easier to follow.
Run kernel builds for several rounds in a 1 GB memcg and take the average build time. The patchset shows almost no performance impact, with a very small improvement that could simply be noise:
Quoted 8 lines
Just FYI:
I tested this patchset with a 3G memcg limit and a 10G zram device, running 'make -j32' to build kernel on my 32-core Arm machines, and got some performance improvement for ths sys time:
w/o patch w/patch
sys 1845s 1570sQuoted 4 lines
Hi Baoliln
That's a very interesting result, can you share a bit more info about it? e.g. vmstat? I'm curious how this happens.
Sure.
Quoted 2 lines
I suspect patch 2 or 3 changes the swappiness / reclaim / aging type selection behavior, or maybe it reduced the reclaim amount?
I gathered the memcg stats as shown below.
It looks like the direct reason for the performance improvement is an obvious reduction in anon refaults, which is what this patch aims to achieve I think. That is to say, we should respect the type chosen by the PID for reclaim, and try to exhaust that type before falling back to another.
Before this series:
workingset_refault_anon59706846workingset_refault_file9229283workingset_activate_anon15108561workingset_activate_file365660workingset_restore_anon15108561workingset_restore_file1018493workingset_nodereclaim0pgsteal_kswapd0pgsteal_direct92589340pgsteal_khugepaged549pgsteal_proactive0pgscan_kswapd0pgscan_direct456629355pgscan_khugepaged549
Quoted 15 lines
After this series:
workingset_refault_anon41277086workingset_refault_file9131522workingset_activate_anon12924656workingset_activate_file345901workingset_restore_anon12924656workingset_restore_file1005959workingset_nodereclaim0pgsteal_kswapd0pgsteal_direct71723800pgsteal_khugepaged4013pgsteal_proactive0pgscan_kswapd0pgscan_direct342179445pgscan_khugepaged9311
Thanks for the info!
I think it might matches what I had in mind: we currently have a very subtle behavior for MGLRU: in try_to_inc_min_seq, it will refuse to increase the gen min_seq beyound (max_seq - MIN_NR_GENS) even if the last gen is empty, if another type still have MAX_NR_GENS left. As a result, if one type is reclaimed more, and another type have many folios stuck in the olest gen, the over reclaimed type will have a very tiny oldest generation (all new folios land on the oldest generation and are reclaimed immediately), that olest gen will stay at a near zero size and gets exhausted very frequently, with min_seq not increased.
This will happen to file type very frequently if swappiness is low, or happen very frequently to anon if files are frequently reclaimed, really depends on the workload.
The old "if (!scanned && type_fallback_allowed)" will return false and raise the priority very frequently. Raising the priority means the next reclaim cycle with run with larger reclaim ratio, resulting in over-reclaim on both folios and slab. What's more, it might trigger aging.
With this series, it will see that as exhausted, and fall back to other type, which in fact respects the swappiness / PID less, but also avoid the raised priority and over reclaim in some cases. I did a test on my machine and it seems matches that model:
Before:
*** Executing swappiness 1 ***
sys: 665.941refault_file: 945242refault_anon: 916129pgscan_anon: 13195435pgscan_file: 2419874
*** Executing swappiness 60 ***
sys: 650.801refault_file: 238839refault_anon: 1026646pgscan_anon: 13082867pgscan_file: 809642
*** Executing swappiness 100 ***
sys: 621.135refault_file: 211113refault_anon: 931943pgscan_anon: 12008517pgscan_file: 778586
*** Executing swappiness 150 ***
sys: 633.591refault_file: 198239refault_anon: 987093pgscan_anon: 12645094pgscan_file: 709927
*** Executing swappiness 200 ***
sys: 627.747refault_file: 163649refault_anon: 1024657pgscan_anon: 12088189pgscan_file: 653841
After:
*** Executing swappiness 1 ***
sys: 729.234refault_file: 962067refault_anon: 1175614pgscan_anon: 14588298pgscan_file: 2396037
*** Executing swappiness 60 ***
sys: 677.685refault_file: 238877refault_anon: 1008627pgscan_anon: 13744350pgscan_file: 888445
*** Executing swappiness 100 ***
sys: 668.947refault_file: 250359refault_anon: 994929pgscan_anon: 13307019pgscan_file: 794981
*** Executing swappiness 150 ***
sys: 649.764refault_file: 180899refault_anon: 971073pgscan_anon: 12238885pgscan_file: 709684
*** Executing swappiness 200 ***
sys: 648.377refault_file: 228112refault_anon: 1237758pgscan_anon: 12653346pgscan_file: 734499
So I think in theory we might see more ineffective swappiness after this series, I'm also a bit concerned about the "lrugen->min_seq[type]
+ MIN_NR_GENS == lrugen->max_seq;" check which is strictly related to the limitation of try_to_inc_min_seq I mentioned here... In the long term I think we better get rid of that limitation, not sure how the behavior will change or fit after this.
BTW the above tests were done with ordinary block SWAP, with ZRAM, I see an improvement instead (it's related to how SYNC SWAP discards the folio immediately and blocks the thread while ordinary SWAP does IO asynchronously so there would be less concurrent reclaim, and changes the dirty/writeback mix that evict_folios()'s
loop sees, especially nr_reclaimed, and here we will avoid raise of priority):
Before:
*** Executing swappiness 1 ***
sys: 599.135refault_file: 925843refault_anon: 1838025pgscan_anon: 20390687pgscan_file: 2423955
*** Executing swappiness 60 ***
sys: 591.367refault_file: 336184refault_anon: 2350551pgscan_anon: 21099503pgscan_file: 1109003
*** Executing swappiness 100 ***
sys: 593.672refault_file: 328276refault_anon: 2332940pgscan_anon: 20894727pgscan_file: 954910
*** Executing swappiness 150 ***
sys: 585.939refault_file: 296374refault_anon: 2063673pgscan_anon: 20537898pgscan_file: 935107
*** Executing swappiness 200 ***
sys: 574.283refault_file: 313356refault_anon: 2065897pgscan_anon: 20145043pgscan_file: 881381
After:
*** Executing swappiness 1 ***
sys: 578.699refault_file: 844457refault_anon: 1793171pgscan_anon: 19307170pgscan_file: 2364248
*** Executing swappiness 60 ***
sys: 582.053refault_file: 390580refault_anon: 2066277pgscan_anon: 20413030pgscan_file: 1201373
*** Executing swappiness 100 ***
sys: 567.520refault_file: 295709refault_anon: 1923291pgscan_anon: 19299837pgscan_file: 978007
*** Executing swappiness 150 ***
sys: 582.983refault_file: 311833refault_anon: 2041126pgscan_anon: 20547029pgscan_file: 940061
*** Executing swappiness 200 ***
sys: 562.710refault_file: 301914refault_anon: 2005846pgscan_anon: 19257095pgscan_file: 839522
So in summary I think the gain is avoiding priority increase (by falling back to another type and satisfying the reclaim target in one iteration, when one type have a tailing draning gen); the loss is less effective swappiness. In fact, I think we might fall back to another type MORE, especially when under pressure, not less, contrary to what the patch message suggests. Not really against this change, just a headup that there is a impact on swappiness / aging of this change in an unexpected way, and things may gets more interesting if we want to lift that limitation in try_to_inc_min_seq as this is directly related to that.
Right now I'm checkng if we can archive perfect swappiness as the IO cost factor for MGLRU, and avoid OOM when under high pressure, so at least the blocker "get_nr_gens(lruvec, type) == MIN_NR_GENS" for reclaim will have to gone I think (we are already seeing tons of OOM causes by that blocker).
Barry Song
reviewerQuoted 103 lines · Kairui Song
Quoted 67 lines · Baolin Wang
Quoted 32 lines · Kairui Song
Quoted 21 lines · Baolin Wang
Quoted 11 lines · Barry Song (Xiaomi)
This is a cleanup series split out from the MGLRU swappiness series [1], with the cleanup changes separated to make them easier to review.
Right now,
isolate_folios()is quite difficult to follow:1. It uses
for_each_evictable_type(i, swappiness)to iterate over the types, but i is not actually used as the type within the loop body.2. It uses scanned == 0 to detect whether the current reclaim type is exhausted, but this is not an accurate indication.
Quoted 8 lines
3. It has an internal retry when no folios can be isolated after scanning some folios, but the retry is implemented in a way nobody can understand.
This patchset makes these behaviors explicit and much easier to follow.
Run kernel builds for several rounds in a 1 GB memcg and take the average build time. The patchset shows almost no performance impact, with a very small improvement that could simply be noise:
Quoted 8 lines
Just FYI:
I tested this patchset with a 3G memcg limit and a 10G zram device, running 'make -j32' to build kernel on my 32-core Arm machines, and got some performance improvement for ths sys time:
w/o patch w/patch
sys 1845s 1570sQuoted 4 lines
Hi Baoliln
That's a very interesting result, can you share a bit more info about it? e.g. vmstat? I'm curious how this happens.
Sure.
Quoted 2 lines
I suspect patch 2 or 3 changes the swappiness / reclaim / aging type selection behavior, or maybe it reduced the reclaim amount?
I gathered the memcg stats as shown below.
It looks like the direct reason for the performance improvement is an obvious reduction in anon refaults, which is what this patch aims to achieve I think. That is to say, we should respect the type chosen by the PID for reclaim, and try to exhaust that type before falling back to another.
Before this series:
workingset_refault_anon59706846workingset_refault_file9229283workingset_activate_anon15108561workingset_activate_file365660workingset_restore_anon15108561workingset_restore_file1018493workingset_nodereclaim0pgsteal_kswapd0pgsteal_direct92589340pgsteal_khugepaged549pgsteal_proactive0pgscan_kswapd0pgscan_direct456629355pgscan_khugepaged549Quoted 15 lines
After this series:
workingset_refault_anon41277086workingset_refault_file9131522workingset_activate_anon12924656workingset_activate_file345901workingset_restore_anon12924656workingset_restore_file1005959workingset_nodereclaim0pgsteal_kswapd0pgsteal_direct71723800pgsteal_khugepaged4013pgsteal_proactive0pgscan_kswapd0pgscan_direct342179445pgscan_khugepaged9311Thanks for the info!
I think it might matches what I had in mind: we currently have a very subtle behavior for MGLRU: in
try_to_inc_min_seq, it will refuse to increase the genmin_seqbeyound (max_seq-MIN_NR_GENS) even if the last gen is empty, if another type still haveMAX_NR_GENSleft. As a result, if one type is reclaimed more, and another type have many folios stuck in the olest gen, the over reclaimed type will have a very tiny oldest generation (all new folios land on the oldest generation and are reclaimed immediately), that olest gen will stay at a near zero size and gets exhausted very frequently, withmin_seqnot increased.This will happen to file type very frequently if swappiness is low, or happen very frequently to anon if files are frequently reclaimed, really depends on the workload.
Quoted 8 lines
The old "if (!scanned &&
type_fallback_allowed)" will return false and raise the priority very frequently. Raising the priority means the next reclaim cycle with run with larger reclaim ratio, resulting in over-reclaim on both folios and slab. What's more, it might trigger aging.With this series, it will see that as exhausted, and fall back to other type, which in fact respects the swappiness / PID less, but also
We could have the following cases:
1. If the reclaimed type has 3 generations left (with its oldest generation exhausted), while !type still has 4 generations left, falling back might be the best behavior. There is no way to avoid the fallback by increasing sc->priority, because should_run_aging() will not trigger aging while !type still has 4 generations.
2. If the reclaimed type has 3 generations left (and its oldest generation is exhausted), while !type also has 3 generations left, falling back might violate swappiness. If we return and increase sc->priority, should_run_aging() may return true and trigger aging instead:
return evictable_min_seq(min_seq, swappiness) + MIN_NR_GENS == max_seq;
So maybe we could slightly adjust the code as shown below, although I’m not sure if the added complexity is worthwhile:
mm/vmscan.c +0 -0
@@ -4878,9 +4878,10 @@ static int isolate_folios(unsigned long
nr_to_scan, struct lruvec *lruvec,
/*
* We are running out of the current reclaim type. Fall back to
- * the other type if allowed.
+ * the other type if it has at least two generations to reclaim.
*/
- if (exhausted && type_fallback_allowed) {
+ if (exhausted && type_fallback_allowed &&
+ get_nr_gens(lruvec, !type) > MIN_NR_GENS + 1) {
type = !type;
type_fallback_allowed = false;
goto retry;Quoted 67 lines
avoid the raised priority and over reclaim in some cases. I did a test on my machine and it seems matches that model:
Before:
*** Executing swappiness 1 ***
sys: 665.941refault_file: 945242refault_anon: 916129pgscan_anon: 13195435pgscan_file: 2419874
*** Executing swappiness 60 ***
sys: 650.801refault_file: 238839refault_anon: 1026646pgscan_anon: 13082867pgscan_file: 809642
*** Executing swappiness 100 ***
sys: 621.135refault_file: 211113refault_anon: 931943pgscan_anon: 12008517pgscan_file: 778586
*** Executing swappiness 150 ***
sys: 633.591refault_file: 198239refault_anon: 987093pgscan_anon: 12645094pgscan_file: 709927
*** Executing swappiness 200 ***
sys: 627.747refault_file: 163649refault_anon: 1024657pgscan_anon: 12088189pgscan_file: 653841After:
*** Executing swappiness 1 ***
sys: 729.234refault_file: 962067refault_anon: 1175614pgscan_anon: 14588298pgscan_file: 2396037
*** Executing swappiness 60 ***
sys: 677.685refault_file: 238877refault_anon: 1008627pgscan_anon: 13744350pgscan_file: 888445
*** Executing swappiness 100 ***
sys: 668.947refault_file: 250359refault_anon: 994929pgscan_anon: 13307019pgscan_file: 794981
*** Executing swappiness 150 ***
sys: 649.764refault_file: 180899refault_anon: 971073pgscan_anon: 12238885pgscan_file: 709684
*** Executing swappiness 200 ***
sys: 648.377refault_file: 228112refault_anon: 1237758pgscan_anon: 12653346pgscan_file: 734499
Quoted 6 lines
So I think in theory we might see more ineffective swappiness after this series, I'm also a bit concerned about the "lrugen->min_seq[type]
+MIN_NR_GENS==lrugen->max_seq;" check which is strictly related to the limitation oftry_to_inc_min_seqI mentioned here... In the long term I think we better get rid of that limitation, not sure how the behavior will change or fit after this.
In the earlier RFC, I got rid of the generation sync in try_to_inc_min_seq():
https://lore.kernel.org/linux-mm/[email protected]/
It does help with swappiness, but also slightly increases sys time at swappiness values around 60, so I dropped it from RFC v4.
Basically, letting file and anon catch up with each other seems to offer the best performance at normal swappiness values such as 60–100, but it essentially makes MGLRU's swappiness a toy.
Quoted 72 lines
BTW the above tests were done with ordinary block SWAP, with ZRAM, I see an improvement instead (it's related to how SYNC SWAP discards the folio immediately and blocks the thread while ordinary SWAP does IO asynchronously so there would be less concurrent reclaim, and changes the dirty/writeback mix that evict_folios()'s
loop sees, especiallynr_reclaimed, and here we will avoid raise of priority):Before:
*** Executing swappiness 1 ***
sys: 599.135refault_file: 925843refault_anon: 1838025pgscan_anon: 20390687pgscan_file: 2423955
*** Executing swappiness 60 ***
sys: 591.367refault_file: 336184refault_anon: 2350551pgscan_anon: 21099503pgscan_file: 1109003
*** Executing swappiness 100 ***
sys: 593.672refault_file: 328276refault_anon: 2332940pgscan_anon: 20894727pgscan_file: 954910
*** Executing swappiness 150 ***
sys: 585.939refault_file: 296374refault_anon: 2063673pgscan_anon: 20537898pgscan_file: 935107
*** Executing swappiness 200 ***
sys: 574.283refault_file: 313356refault_anon: 2065897pgscan_anon: 20145043pgscan_file: 881381After:
*** Executing swappiness 1 ***
sys: 578.699refault_file: 844457refault_anon: 1793171pgscan_anon: 19307170pgscan_file: 2364248
*** Executing swappiness 60 ***
sys: 582.053refault_file: 390580refault_anon: 2066277pgscan_anon: 20413030pgscan_file: 1201373
*** Executing swappiness 100 ***
sys: 567.520refault_file: 295709refault_anon: 1923291pgscan_anon: 19299837pgscan_file: 978007
*** Executing swappiness 150 ***
sys: 582.983refault_file: 311833refault_anon: 2041126pgscan_anon: 20547029pgscan_file: 940061
*** Executing swappiness 200 ***
sys: 562.710refault_file: 301914refault_anon: 2005846pgscan_anon: 19257095pgscan_file: 839522
Quoted 10 lines
So in summary I think the gain is avoiding priority increase (by falling back to another type and satisfying the reclaim target in one iteration, when one type have a tailing draning gen); the loss is less effective swappiness. In fact, I think we might fall back to another type MORE, especially when under pressure, not less, contrary to what the patch message suggests. Not really against this change, just a headup that there is a impact on swappiness / aging of this change in an unexpected way, and things may gets more interesting if we want to lift that limitation in
try_to_inc_min_seqas this is directly related to that.
As mentioned above, I’m not sure if we want to increase the code complexity by distinguishing between the following two cases:
1. The other type has 2+ generations left to reclaim. In this case, even after sc->priority increases, we still have to fall back because should_run_aging() won’t trigger aging.
2. The other type has only 1 generation left to reclaim. In this case, should_run_aging() will trigger aging, so the previously reclaimed type might get a new reclaimable generation in the next iteration.
Quoted 6 lines
Right now I'm checkng if we can archive perfect swappiness as the IO cost factor for MGLRU, and avoid OOM when under high pressure, so at least the blocker "
get_nr_gens(lruvec, type)==MIN_NR_GENS" for reclaim will have to gone I think (we are already seeing tons of OOM causes by that blocker).
I tried a couple of ways to remove get_nr_gens(lruvec, type) == MIN_NR_GENS from scan_folios(), but all of them increased sys time at normal swappiness values. If we really want to remove it, we need to figure out how to do so without impacting performance :-)
Best Regards
Barry
Baolin Wang
reviewerQuoted 148 lines · Barry Song
Quoted 103 lines · Kairui Song
Quoted 67 lines · Baolin Wang
Quoted 32 lines · Kairui Song
Quoted 21 lines · Baolin Wang
Quoted 11 lines · Barry Song (Xiaomi)
This is a cleanup series split out from the MGLRU swappiness series [1], with the cleanup changes separated to make them easier to review.
Right now,
isolate_folios()is quite difficult to follow:1. It uses
for_each_evictable_type(i, swappiness)to iterate over the types, but i is not actually used as the type within the loop body.2. It uses scanned == 0 to detect whether the current reclaim type is exhausted, but this is not an accurate indication.
Quoted 8 lines
3. It has an internal retry when no folios can be isolated after scanning some folios, but the retry is implemented in a way nobody can understand.
This patchset makes these behaviors explicit and much easier to follow.
Run kernel builds for several rounds in a 1 GB memcg and take the average build time. The patchset shows almost no performance impact, with a very small improvement that could simply be noise:
Quoted 8 lines
Just FYI:
I tested this patchset with a 3G memcg limit and a 10G zram device, running 'make -j32' to build kernel on my 32-core Arm machines, and got some performance improvement for ths sys time:
w/o patch w/patch
sys 1845s 1570sQuoted 4 lines
Hi Baoliln
That's a very interesting result, can you share a bit more info about it? e.g. vmstat? I'm curious how this happens.
Sure.
Quoted 2 lines
I suspect patch 2 or 3 changes the swappiness / reclaim / aging type selection behavior, or maybe it reduced the reclaim amount?
I gathered the memcg stats as shown below.
It looks like the direct reason for the performance improvement is an obvious reduction in anon refaults, which is what this patch aims to achieve I think. That is to say, we should respect the type chosen by the PID for reclaim, and try to exhaust that type before falling back to another.
Before this series:
workingset_refault_anon59706846workingset_refault_file9229283workingset_activate_anon15108561workingset_activate_file365660workingset_restore_anon15108561workingset_restore_file1018493workingset_nodereclaim0pgsteal_kswapd0pgsteal_direct92589340pgsteal_khugepaged549pgsteal_proactive0pgscan_kswapd0pgscan_direct456629355pgscan_khugepaged549Quoted 15 lines
After this series:
workingset_refault_anon41277086workingset_refault_file9131522workingset_activate_anon12924656workingset_activate_file345901workingset_restore_anon12924656workingset_restore_file1005959workingset_nodereclaim0pgsteal_kswapd0pgsteal_direct71723800pgsteal_khugepaged4013pgsteal_proactive0pgscan_kswapd0pgscan_direct342179445pgscan_khugepaged9311Thanks for the info!
I think it might matches what I had in mind: we currently have a very subtle behavior for MGLRU: in
try_to_inc_min_seq, it will refuse to increase the genmin_seqbeyound (max_seq-MIN_NR_GENS) even if the last gen is empty, if another type still haveMAX_NR_GENSleft. As a result, if one type is reclaimed more, and another type have many folios stuck in the olest gen, the over reclaimed type will have a very tiny oldest generation (all new folios land on the oldest generation and are reclaimed immediately), that olest gen will stay at a near zero size and gets exhausted very frequently, withmin_seqnot increased.This will happen to file type very frequently if swappiness is low, or happen very frequently to anon if files are frequently reclaimed, really depends on the workload.
Quoted 8 lines
The old "if (!scanned &&
type_fallback_allowed)" will return false and raise the priority very frequently. Raising the priority means the next reclaim cycle with run with larger reclaim ratio, resulting in over-reclaim on both folios and slab. What's more, it might trigger aging.With this series, it will see that as exhausted, and fall back to other type, which in fact respects the swappiness / PID less, but also
We could have the following cases:
1. If the reclaimed type has 3 generations left (with its oldest generation exhausted), while !type still has 4 generations left, falling back might be the best behavior. There is no way to avoid the fallback by increasing
sc->priority, becauseshould_run_aging()will not trigger aging while !type still has 4 generations.2. If the reclaimed type has 3 generations left (and its oldest generation is exhausted), while !type also has 3 generations left, falling back might violate swappiness. If we return and increase
sc->priority,should_run_aging()may return true and trigger aging instead:return
evictable_min_seq(min_seq, swappiness)+MIN_NR_GENS==max_seq;So maybe we could slightly adjust the code as shown below, although I’m not sure if the added complexity is worthwhile:
mm/vmscan.c +3
-2@@ -4878,9 +4878,10 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,/* * We are running out of the current reclaim type. Fall back to -* the other type if allowed. +* the other type if it has at least two generations to reclaim. */ - if (exhausted && type_fallback_allowed) { + if (exhausted && type_fallback_allowed && + get_nr_gens(lruvec, !type) > MIN_NR_GENS + 1) { type = !type; type_fallback_allowed = false;
Quoted 4 lines
goto retry;Quoted 2 lines
avoid the raised priority and over reclaim in some cases. I did a test on my machine and it seems matches that model:
With the above changes, I can also reproduce the slight regression with NVMe swap (set swappiness = 100) mentioned by Kairui (but the zram case still looks good):
Before:
sys: 757refault_anon 5554715refault_file 3867998
After:
sys: 765refault_anon 5791349refault_file 3939552
Quoted 74 lines
Quoted 64 lines
Before:
*** Executing swappiness 1 ***
sys: 665.941refault_file: 945242refault_anon: 916129pgscan_anon: 13195435pgscan_file: 2419874
*** Executing swappiness 60 ***
sys: 650.801refault_file: 238839refault_anon: 1026646pgscan_anon: 13082867pgscan_file: 809642
*** Executing swappiness 100 ***
sys: 621.135refault_file: 211113refault_anon: 931943pgscan_anon: 12008517pgscan_file: 778586
*** Executing swappiness 150 ***
sys: 633.591refault_file: 198239refault_anon: 987093pgscan_anon: 12645094pgscan_file: 709927
*** Executing swappiness 200 ***
sys: 627.747refault_file: 163649refault_anon: 1024657pgscan_anon: 12088189pgscan_file: 653841After:
*** Executing swappiness 1 ***
sys: 729.234refault_file: 962067refault_anon: 1175614pgscan_anon: 14588298pgscan_file: 2396037
*** Executing swappiness 60 ***
sys: 677.685refault_file: 238877refault_anon: 1008627pgscan_anon: 13744350pgscan_file: 888445
*** Executing swappiness 100 ***
sys: 668.947refault_file: 250359refault_anon: 994929pgscan_anon: 13307019pgscan_file: 794981
*** Executing swappiness 150 ***
sys: 649.764refault_file: 180899refault_anon: 971073pgscan_anon: 12238885pgscan_file: 709684
*** Executing swappiness 200 ***
sys: 648.377refault_file: 228112refault_anon: 1237758pgscan_anon: 12653346pgscan_file: 734499Quoted 6 lines
So I think in theory we might see more ineffective swappiness after this series, I'm also a bit concerned about the "lrugen->min_seq[type]
+MIN_NR_GENS==lrugen->max_seq;" check which is strictly related to the limitation oftry_to_inc_min_seqI mentioned here... In the long term I think we better get rid of that limitation, not sure how the behavior will change or fit after this.In the earlier RFC, I got rid of the generation sync in
try_to_inc_min_seq():
Quoted 91 lines
https://lore.kernel.org/linux-mm/[email protected]/
It does help with swappiness, but also slightly increases sys time at swappiness values around 60, so I dropped it from RFC v4.
Basically, letting file and anon catch up with each other seems to offer the best performance at normal swappiness values such as 60–100, but it essentially makes MGLRU's swappiness a toy.
Quoted 82 lines
BTW the above tests were done with ordinary block SWAP, with ZRAM, I see an improvement instead (it's related to how SYNC SWAP discards the folio immediately and blocks the thread while ordinary SWAP does IO asynchronously so there would be less concurrent reclaim, and changes the dirty/writeback mix that evict_folios()'s
loop sees, especiallynr_reclaimed, and here we will avoid raise of priority):Before:
*** Executing swappiness 1 ***
sys: 599.135refault_file: 925843refault_anon: 1838025pgscan_anon: 20390687pgscan_file: 2423955
*** Executing swappiness 60 ***
sys: 591.367refault_file: 336184refault_anon: 2350551pgscan_anon: 21099503pgscan_file: 1109003
*** Executing swappiness 100 ***
sys: 593.672refault_file: 328276refault_anon: 2332940pgscan_anon: 20894727pgscan_file: 954910
*** Executing swappiness 150 ***
sys: 585.939refault_file: 296374refault_anon: 2063673pgscan_anon: 20537898pgscan_file: 935107
*** Executing swappiness 200 ***
sys: 574.283refault_file: 313356refault_anon: 2065897pgscan_anon: 20145043pgscan_file: 881381After:
*** Executing swappiness 1 ***
sys: 578.699refault_file: 844457refault_anon: 1793171pgscan_anon: 19307170pgscan_file: 2364248
*** Executing swappiness 60 ***
sys: 582.053refault_file: 390580refault_anon: 2066277pgscan_anon: 20413030pgscan_file: 1201373
*** Executing swappiness 100 ***
sys: 567.520refault_file: 295709refault_anon: 1923291pgscan_anon: 19299837pgscan_file: 978007
*** Executing swappiness 150 ***
sys: 582.983refault_file: 311833refault_anon: 2041126pgscan_anon: 20547029pgscan_file: 940061
*** Executing swappiness 200 ***
sys: 562.710refault_file: 301914refault_anon: 2005846pgscan_anon: 19257095pgscan_file: 839522So in summary I think the gain is avoiding priority increase (by falling back to another type and satisfying the reclaim target in one iteration, when one type have a tailing draning gen); the loss is less effective swappiness. In fact, I think we might fall back to another type MORE, especially when under pressure, not less, contrary to what the patch message suggests. Not really against this change, just a headup that there is a impact on swappiness / aging of this change in an unexpected way, and things may gets more interesting if we want to lift that limitation in
try_to_inc_min_seqas this is directly related to that.
Thanks Kairui for your data and anylysis.
I mostly agree. But on whether we should fallback more, I still prefer Barry's idea. The current patch might be a bit too aggressive, cause it only fallback to another type when the oldest generation is fully exhausted. I've always felt that falling back too easily doesn't really respect the PID's choice.
Of course, there's also the concern you raised about the limitation in try_to_inc_min_seq() when updating the min_seq. I think we can address that together later, so that we respect the PID's choice (i.e., try the reclaimed type as much as possible) without ending up with a tailing draining generation that blocks aging. That may not be simple, though.
Anyway, personally I'd prefer to keep the original code until more impact investigation is done.
Just my 2 cents.
Quoted 15 lines · Baolin Wang
With the above changes, I can also reproduce the slight regression with NVMe swap (set swappiness = 100) mentioned by Kairui (but the zram case still looks good):
Before:
sys: 757refault_anon5554715refault_file3867998After:
sys: 765refault_anon5791349refault_file3939552
...
Quoted 9 lines
Thanks Kairui for your data and anylysis.
I mostly agree. But on whether we should fallback more, I still prefer Barry's idea. The current patch might be a bit too aggressive, cause it only fallback to another type when the oldest generation is fully exhausted. I've always felt that falling back too easily doesn't really respect the PID's choice.
Quoted 5 lines
Of course, there's also the concern you raised about the limitation in
try_to_inc_min_seq()when updating themin_seq. I think we can address that together later, so that we respect the PID's choice (i.e., try the reclaimed type as much as possible) without ending up with a tailing draining generation that blocks aging. That may not be simple, though.
Actually in the long term I think we should get away from the PID rather than respect it more for eviction choice :), and use a actual calculation that respects swappiness and IO cost (we can still use the data collected by it though).
There is another series form Barry (and the clean up here is extracted from it):
https://lore.kernel.org/linux-mm/[email protected]/
To achieve reasonable swappiness following the PID and aging protection idea, we will inevitably increase overhead, and PID is really bad at protecting the cache anyway.
PID controllers generally correct long-term linear statistic well, but they aren't well-suited to reacting to abrupt workload changes. While MM workloads are full of such bursts and phase changes. What's worse the design aggregates folios into tiers based on access, and the limited high tier means folios staying in the same high tier can have dramatically different access behaviors.
I think PID still makes sense as a long-term reference: it can correct cross-generation behavior over time, but relying on it as the primary mechanism for for single type eviction or protection is problematic. In my view, this is also one of the major limitations that has made MGLRU difficult to generalize across a wide range of workloads.
See: https://lore.kernel.org/linux-mm/[email protected]/,
by softening PID protection (still used, just with a much lower
factor) and using FG protection, zipf and real workloads gets a much higher hit rate (much higher than 10%, and dramatically better in many cases, the basic 10% gain is end-to-end gain diluted by the overall cost).
So in the long term I think: For promotion, we will use FG; for eviction (the issue being patched here and in that Barry's other patch), we will respect swappiness & IO cost based scan budget calculation. Currently, the IO cost is just the refault count, which seems fine, this is similar to classical LRU, but ignores scan cost because MGLRU's scan is special, full of lazy-promoted folios that already offset the scan budget well. PID can stay to catch long term characteristics, at a weaker factor.
(BTW I know that PID have a way to be more adaptive to bursts; however, that's really hard to tune for MM, and doesn't fix the long time cold folio stay in same high tier issue).
Maybe I'll just post a series soon for the eviction part.
Quoted 5 lines
Anyway, personally I'd prefer to keep the original code until more impact investigation is done.
Just my 2 cents.
No problem, I'm OK with this change for now.
Barry Song
reviewerOn Thu, Aug 27, 2026 at 3:25 PM Kairui Song <[email protected]> wrote:
[...]
Quoted 6 lines
Quoted 4 lines
Anyway, personally I'd prefer to keep the original code until more impact investigation is done.
Just my 2 cents.
No problem, I'm OK with this change for now.
Thanks, Baolin and Kairui.
If the effect of the exhaustion check is unclear — it improves the zram case but might slightly regress the SSD/NVMe case — we could simply drop patch 2 and keep patches 1 and 3 in v2. That would make the patchset focused entirely on readability.
Best Regards
Barry