mm/mglru: clean up isolate_folios for readability and clarity
Subsystems (3)
Barry Song (Xiaomi)
reviewerv2 cover mm/mglru: clean up isolate_folios for readability and clarity
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 retries the same type when folios were scanned but none could be isolated, but the retry is implemented in a rather subtle way that is difficult to understand.
This patchset makes both behaviors explicit and much easier to follow.
There are no functional changes for swappiness values from 1 to 200. There is a slight functional change for 0 and 201: with the existing code, there is no chance to retry for these values because for_each_evictable_type() only iterates once. After this patch, 0 and 201 have behavior that is more consistent with the 1-200 range.
-v2:
* Rename patch 1, to address Baoquan's comments;
* Drop patches 2/3. Patch 2 seems to improve the zRAM case but negatively affect the SSD/NVMe case, according to Baolin and Kairui. Drop it to keep the patchset focused on readability.
* We received many tags from Baolin, Kairui, Ridong, and Lian. Since patch 1/3 and 3/3 were renamed and patches 2/3 were dropped, I did not carry the tags forward. Many thanks for the reviews, and hopefully you can re-review this version.
-v1:
https://lore.kernel.org/linux-mm/[email protected]/
Barry Song (Xiaomi) (1):
mm/mglru: make retry logic explicit in isolate_folios()
Ridong Chen (1):
mm/mglru: make type fallback logic explicit in isolate_folios()
mm/vmscan.c | 56 ++++++++++++++++++++++++++++++++++------------------- 1 file changed, 36 insertions(+), 20 deletions(-)
Signature · 1 line
2.34.1
Barry Song (Xiaomi)
reviewerPATCH v2 1/2 mm/mglru: make type fallback logic explicit in 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
@@ -4838,35 +4838,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 v2 1/2] mm/mglru: make type fallback logic explicit in 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.
Re: [PATCH v2 1/2] mm/mglru: make type fallback logic explicit in isolate_folios()
In reply to Barry Song (Xiaomi)Quoted 2 lines
Make the fallback behavior explicit in the code and remove the opaque
for_each_evictable_type(i, swappiness).
Looks good to me.
Re: [PATCH v2 1/2] mm/mglru: make type fallback logic explicit in 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(-)
Quoted 70 lines
mm/vmscan.c +26
-20@@ -4838,35 +4838,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 (Xiaomi)
reviewerPATCH v2 2/2 mm/mglru: make retry logic explicit in isolate_folios()
In reply to Barry Song (Xiaomi)The existing mainline code retries the same type once in a rather subtle way. for_each_evictable_type() may provide one more iteration, allowing the same type to be retried if we scanned some folios but failed to isolate any due to protections, promotions, or races. This patch makes the retry behavior explicit.
--- mm/vmscan.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
mm/vmscan.c +10 -0
@@ -4852,6 +4852,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 tried = false; retry: tier =get_tier_idx (lruvec, type);
@@ -4871,9 +4872,18 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,*/ if (!scanned && type_fallback_allowed) { type = !type; + tried = true; type_fallback_allowed = false; goto retry; } +/* +* We scanned some folios but failed to isolate any due to promotions, +* protections, or races. Retry once to avoid a larger loop. +*/ +if (scanned && !tried) { + tried = true; + goto retry; + }return total_scanned; }
Signature · 1 line
2.34.1
Re: [PATCH v2 2/2] mm/mglru: make retry logic explicit in isolate_folios()
Quoted 8 lines · Barry Song (Xiaomi)
The existing mainline code retries the same type once in a rather subtle way.
for_each_evictable_type()may provide one more iteration, allowing the same type to be retried if we scanned some folios but failed to isolate any due to protections, promotions, or races. This patch makes the retry behavior explicit.Signed-off-by Barry Song (Xiaomi) <[email protected]>---
LGTM.
Baoquan He
reviewerRe: [PATCH v2 2/2] mm/mglru: make retry logic explicit in isolate_folios()
In reply to Barry Song (Xiaomi)Hi Barry,
Quoted 28 lines · Barry Song (Xiaomi)
The existing mainline code retries the same type once in a rather subtle way.
for_each_evictable_type()may provide one more iteration, allowing the same type to be retried if we scanned some folios but failed to isolate any due to protections, promotions, or races. This patch makes the retry behavior explicit.Signed-off-by Barry Song (Xiaomi) <[email protected]>--- mm/vmscan.c | 10 ++++++++++ 1 file changed, 10 insertions(+)mm/vmscan.c +2
-0@@ -4852,6 +4852,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 tried = false; retry: tier =get_tier_idx (lruvec, type);@@ -4871,9 +4872,18 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,*/ if (!scanned && type_fallback_allowed) { type = !type; + tried = true;
mm/vmscan.c static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec, · line 4885
Quoted 10 lines
mm/vmscan.c +7
-0type_fallback_allowed = false; goto retry; } +/* +* We scanned some folios but failed to isolate any due to promotions, +* protections, or races. Retry once to avoid a larger loop. +*/ +if (scanned && !tried) { + tried = true; + goto retry;
Seems patch 1 and 2 makes not minor difference than mainline kernel on behaviour.
1, if swappiness is 0 because no swap, it will run two times if (scanned != 0). This is not corner case, but usually seen on some systems w/o swap device. The 2nd no gain run could decrease efficiency.
static int get_swappiness(struct lruvec *lruvec, struct scan_control *sc)
{
...
if (!sc->may_swap)
return 0;
...
}2, for swappiness (0, 200), the behavious is minor changed.
Mark one scan_folios() result as one of:
iso *isolated > 0
empty scanned == 0 && !*isolated
busy scanned > 0 && !*isolatedmainline: T(busy) -> T(empty) -> return (2 scans, no fallback) v2: T(busy) -> T(empty) -> !T(...) (a 3rd scan_folios())
Maybe we can go like below:
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)
...
for (attempt = 0; attempt < 2; attempt++) {
int scanned = scan_folios(nr_to_scan, lruvec, sc, type,
get_tier_idx(lruvec, type), list, isolated);
total_scanned += scanned;
if (*isolated) {
*isolate_type = type;
*isolate_scanned = scanned;
return total_scanned;
}
if (attempt) /* already retried / fell back once */
break;
if (scanned)
continue; /* retry the same type once */
if (single_type)
break; /* no fallback for 0 / anon-only */
type = !type; /* empty: fall back to the other type */
}
return total_scanned;
}This preserves mainline for 1..200 exactly, keeps the intended 0/201 same-type retry, and cannot produce a third scan. Just personal opinion.
Quoted 7 lines
mm/vmscan.c +1
-0+ }return total_scanned; }Signature · 1 line
2.34.1
Barry Song
reviewerRe: [PATCH v2 2/2] mm/mglru: make retry logic explicit in isolate_folios()
Quoted 50 lines · Baoquan He
Hi Barry,
Quoted 28 lines · Barry Song (Xiaomi)
The existing mainline code retries the same type once in a rather subtle way.
for_each_evictable_type()may provide one more iteration, allowing the same type to be retried if we scanned some folios but failed to isolate any due to protections, promotions, or races. This patch makes the retry behavior explicit.Signed-off-by Barry Song (Xiaomi) <[email protected]>--- mm/vmscan.c | 10 ++++++++++ 1 file changed, 10 insertions(+)mm/vmscan.c +2
-0@@ -4852,6 +4852,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 tried = false; retry: tier =get_tier_idx (lruvec, type);@@ -4871,9 +4872,18 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,*/ if (!scanned && type_fallback_allowed) { type = !type; + tried = true;mm/vmscan.c static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec, · line 4885
Quoted 10 lines
mm/vmscan.c +7
-0type_fallback_allowed = false; goto retry; } +/* +* We scanned some folios but failed to isolate any due to promotions, +* protections, or races. Retry once to avoid a larger loop. +*/ +if (scanned && !tried) { + tried = true; + goto retry;Seems patch 1 and 2 makes not minor difference than mainline kernel on behaviour.
1, if swappiness is 0 because no swap, it will run two times if (scanned != 0). This is not corner case, but usually seen on some systems w/o swap device. The 2nd no gain run could decrease efficiency.
Quoted 8 lines
static int
get_swappiness(struct lruvec *lruvec, struct scan_control *sc){ ... if (!sc->may_swap) return 0; ... }
Yep. For swappiness 0 and 201, this patch slightly changes the behavior, as I mentioned in the cover letter:
"
There is a slight functional change for 0 and 201: with the existing code, there is no chance to retry for these values because for_each_evictable_type() only iterates once. After this patch, 0 and 201 have behavior that is more consistent with the 1-200 range.
"
I did this intentionally, as it makes the behavior more consistent with the 1-200 range, where we retry the same type once to avoid having a larger outer loop.
Quoted 2 lines
2, for swappiness (0, 200), the behavious is minor changed.
I guess you actually mean swappiness (1, 200)?
Quoted 10 lines
Mark one
scan_folios()result as one of:iso *isolated > 0 empty scanned == 0 && !*isolated busy scanned > 0 && !*isolatedmainline: T(busy) -> T(empty) -> return (2 scans, no fallback) v2: T(busy) -> T(empty) -> !T(...) (a 3rd scan_folios())Maybe we can go like below:
Yes, you're right. For swappiness (1, 200), I didn't realize there was this slight change.
Quoted 11 lines
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) ... for (attempt = 0; attempt < 2; attempt++) { int scanned = scan_folios(nr_to_scan, lruvec, sc, type, get_tier_idx(lruvec, type), list, isolated);
Quoted 8 lines
total_scanned += scanned;
if (*isolated) {
*isolate_type = type;
*isolate_scanned = scanned;
return total_scanned;
}
if (attempt) /* already retried / fell back once */
break;This if (attempt) break makes the loop look rather strange, especially for a loop with a maximum of 2 iterations, where we break when attempt reaches 1 :-)
What about just changing one line?
mm/vmscan.c +0 -0
@@ -4939,7 +4939,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 (!scanned && !tried && type_fallback_allowed) {
type = !type;
tried = true;
type_fallback_allowed = false;Quoted 11 lines
if (scanned)
continue; /* retry the same type once */
if (single_type)
break; /* no fallback for 0 / anon-only */
type = !type; /* empty: fall back to the other type */
}return
total_scanned;
}This preserves mainline for 1..200 exactly, keeps the intended 0/201
I guess you mean removing the same-type retry for swappiness 0/201, rather than keeping it?
Thanks
Barry
Baoquan He
reviewerRe: [PATCH v2 2/2] mm/mglru: make retry logic explicit in isolate_folios()
Quoted 111 lines · Barry Song
Quoted 50 lines · Baoquan He
Hi Barry,
Quoted 28 lines · Barry Song (Xiaomi)
The existing mainline code retries the same type once in a rather subtle way.
for_each_evictable_type()may provide one more iteration, allowing the same type to be retried if we scanned some folios but failed to isolate any due to protections, promotions, or races. This patch makes the retry behavior explicit.Signed-off-by Barry Song (Xiaomi) <[email protected]>--- mm/vmscan.c | 10 ++++++++++ 1 file changed, 10 insertions(+)mm/vmscan.c +2
-0@@ -4852,6 +4852,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 tried = false; retry: tier =get_tier_idx (lruvec, type);@@ -4871,9 +4872,18 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,*/ if (!scanned && type_fallback_allowed) { type = !type; + tried = true;mm/vmscan.c static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec, · line 4885
Quoted 10 lines
mm/vmscan.c +7
-0type_fallback_allowed = false; goto retry; } +/* +* We scanned some folios but failed to isolate any due to promotions, +* protections, or races. Retry once to avoid a larger loop. +*/ +if (scanned && !tried) { + tried = true; + goto retry;Seems patch 1 and 2 makes not minor difference than mainline kernel on behaviour.
1, if swappiness is 0 because no swap, it will run two times if (scanned != 0). This is not corner case, but usually seen on some systems w/o swap device. The 2nd no gain run could decrease efficiency.
Quoted 8 lines
static int
get_swappiness(struct lruvec *lruvec, struct scan_control *sc){ ... if (!sc->may_swap) return 0; ... }Yep. For swappiness 0 and 201, this patch slightly changes the behavior, as I mentioned in the cover letter:
"
There is a slight functional change for 0 and 201: with the existing code, there is no chance to retry for these values becausefor_each_evictable_type()only iterates once. After this patch, 0 and 201 have behavior that is more consistent with the 1-200 range.
"
I did this intentionally, as it makes the behavior more consistent with the 1-200 range, where we retry the same type once to avoid having a larger outer loop.Quoted 2 lines
2, for swappiness (0, 200), the behavious is minor changed.
I guess you actually mean swappiness (1, 200)?
Quoted 10 lines
Mark one
scan_folios()result as one of:iso *isolated > 0 empty scanned == 0 && !*isolated busy scanned > 0 && !*isolatedmainline: T(busy) -> T(empty) -> return (2 scans, no fallback) v2: T(busy) -> T(empty) -> !T(...) (a 3rd scan_folios())Maybe we can go like below:
Yes, you're right. For swappiness (1, 200), I didn't realize there was this slight change.
Quoted 11 lines
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) ... for (attempt = 0; attempt < 2; attempt++) { int scanned = scan_folios(nr_to_scan, lruvec, sc, type, get_tier_idx(lruvec, type), list, isolated);Quoted 8 lines
total_scanned += scanned; if (*isolated) { *isolate_type = type; *isolate_scanned = scanned; return total_scanned; } if (attempt) /* already retried / fell back once */ break;
Quoted 5 lines
This
if (attempt) breakmakes the loop look rather strange, especially for a loop with a maximum of 2 iterations, where we break whenattemptreaches 1 :-)What about just changing one line?
Hi Barry,
Agreed on the one-line change for the (1, 200) case - I traced it and it now matches mainline exactly (no extra third scan). I personally prefer the for (attempt = 0... ) style because I feel that makes logic clearer, while everybody truly has different code taste, LOL, just a weak opinion.
For 0/201: my concern is that on no-swap systems (swappiness 0 is file-only), the same-type retry when the first scan is busy may be a no-gain run if the file generation is dominated by protected/ineligible folios - the retry re-scans the same sort results. But if you see a case where the retry does isolate folios on the second pass for single-type reclaim, keeping it for consistency is defensible. Do you have such a case, or should we drop the retry for 0/201?
Quoted 23 lines
mm/vmscan.c +1
-1@@ -4939,7 +4939,7 @@ static int isolate_folios(unsigned longnr_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 (!scanned && !tried && type_fallback_allowed) { type = !type; tried = true;type_fallback_allowed = false;Quoted 7 lines
if (scanned)
continue; /* retry the same type once */
if (single_type)
break; /* no fallback for 0 / anon-only */
type = !type; /* empty: fall back to the other type */
}
Quoted 7 lines
Quoted 4 lines
return total_scanned; }This preserves mainline for 1..200 exactly, keeps the intended 0/201
I guess you mean removing the same-type retry for swappiness 0/201, rather than keeping it?
You are right, it was a slip of the tongue.
Barry Song
reviewerRe: [PATCH v2 2/2] mm/mglru: make retry logic explicit in isolate_folios()
Quoted 4 lines · Baoquan He
Quoted 2 lines · Barry Song
Quoted 1 line · Baoquan He
[...]
Quoted 8 lines
Hi Barry,
Agreed on the one-line change for the (1, 200) case - I traced it and it now matches mainline exactly (no extra third scan). I personally prefer the for (attempt = 0... ) style because I feel that makes logic clearer, while everybody truly has different code taste, LOL, just a weak opinion.
Quoted 8 lines
For 0/201: my concern is that on no-swap systems (swappiness 0 is file-only), the same-type retry when the first scan is busy may be a no-gain run if the file generation is dominated by protected/ineligible folios - the retry re-scans the same sort results. But if you see a case where the retry does isolate folios on the second pass for single-type reclaim, keeping it for consistency is defensible. Do you have such a case, or should we drop the retry for 0/201?
201 only applies to proactive reclamation. I believe the retry helps avoid having an outer loop. For 0, I ran a kernel build test on x86 with swap disabled:
# free
total used free shared buff/cache available
Mem: 23991248 1315020 20554564 331312 2121664 22099256
Swap: 0 0 0# time systemd-run --scope --unit=kernel-build -p MemoryMax=1500M
make ARCH=arm64 \
CROSS_COMPILE=aarch64-linux-gnu- vmlinux -j20 1>/dev/null 2>/dev/nullWith the following patch for counting:
mm/vmscan.c +24 -0
@@ -4913,6 +4913,28 @@ static inline bool is_single_type_reclaim(int swappiness) swappiness ==SWAPPINESS_ANON_ONLY ; } +#include <linux/proc_fs.h> + +static atomic64_t tried_isolated; +static atomic64_t tried_not_isolated; +static int reclaim_stats_show (struct seq_file *m,void *v) +{ +seq_printf (m,"tried_isolated: %lld\n" , +atomic64_read (&tried_isolated)); +seq_printf (m,"tried_not_isolated: %lld\n" , +atomic64_read (&tried_not_isolated)); + +return 0 ; +} +return 0 ; +} +static int __init reclaim_stats_init (void ) +{ +proc_create_single ("reclaim_stats" ,0444 ,NULL , + reclaim_stats_show); + +return 0 ; +} +fs_initcall (reclaim_stats_init); + static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
struct scan_control *sc, int swappiness, struct list_head *list, int *isolated,
@@ -4928,6 +4950,13 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
scanned = scan_folios(nr_to_scan, lruvec, sc,
type, tier, list, isolated);
+ if (tried) {
+ if (*isolated)
+ atomic64_inc(&tried_isolated);
+ else
+ atomic64_inc(&tried_not_isolated);
+ }
+
total_scanned += scanned;
if (*isolated) {
*isolate_type = type;I got:
# cat /proc/reclaim_stats tried_isolated: 12096 tried_not_isolated: 23061
So we see some cases where the retry gets isolated folios, while in others we still encounter promoted or protected folios. But my gut feeling is that even if we don't retry and instead go back to the outer loop for another iteration, we'll still encounter those folios, since they are still on the LRU. We would just reach those folios in a more costly way.
Thanks
Barry
Baoquan He
reviewerRe: [PATCH v2 2/2] mm/mglru: make retry logic explicit in isolate_folios()
Quoted 95 lines · Barry Song
Quoted 4 lines · Baoquan He
Quoted 2 lines · Barry Song
Quoted 1 line · Baoquan He
[...]
Quoted 8 lines
Hi Barry,
Agreed on the one-line change for the (1, 200) case - I traced it and it now matches mainline exactly (no extra third scan). I personally prefer the for (attempt = 0... ) style because I feel that makes logic clearer, while everybody truly has different code taste, LOL, just a weak opinion.
Quoted 8 lines
For 0/201: my concern is that on no-swap systems (swappiness 0 is file-only), the same-type retry when the first scan is busy may be a no-gain run if the file generation is dominated by protected/ineligible folios - the retry re-scans the same sort results. But if you see a case where the retry does isolate folios on the second pass for single-type reclaim, keeping it for consistency is defensible. Do you have such a case, or should we drop the retry for 0/201?
201 only applies to proactive reclamation. I believe the retry helps avoid having an outer loop. For 0, I ran a kernel build test on x86 with swap disabled:
# free total used free shared buff/cache available Mem: 23991248 1315020 20554564 331312 2121664 22099256 Swap: 0 0 0# time systemd-run --scope --unit=kernel-build -p MemoryMax=1500M
make ARCH=arm64 \ CROSS_COMPILE=aarch64-linux-gnu- vmlinux -j20 1>/dev/null 2>/dev/nullWith the following patch for counting:
mm/vmscan.c +31
-0@@ -4913,6 +4913,28 @@ static inline bool is_single_type_reclaim(int swappiness) swappiness ==SWAPPINESS_ANON_ONLY ; } +#include <linux/proc_fs.h> + +static atomic64_t tried_isolated; +static atomic64_t tried_not_isolated; +static int reclaim_stats_show (struct seq_file *m,void *v) +{ +seq_printf (m,"tried_isolated: %lld\n" , +atomic64_read (&tried_isolated)); +seq_printf (m,"tried_not_isolated: %lld\n" , +atomic64_read (&tried_not_isolated)); + +return 0 ; +} +return 0 ; +} +static int __init reclaim_stats_init (void ) +{ +proc_create_single ("reclaim_stats" ,0444 ,NULL , + reclaim_stats_show); + +return 0 ; +} +fs_initcall (reclaim_stats_init); + static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec, struct scan_control *sc, int swappiness, struct list_head *list, int *isolated,@@ -4928,6 +4950,13 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec, scanned =scan_folios (nr_to_scan, lruvec, sc, type, tier, list, isolated); +if (tried) { +if (*isolated) +atomic64_inc (&tried_isolated); +else +atomic64_inc (&tried_not_isolated); + } + total_scanned += scanned;if (*isolated) {*isolate_type = type;I got:
# cat /proc/reclaim_stats tried_isolated: 12096 tried_not_isolated: 23061
Quoted 6 lines
So we see some cases where the retry gets isolated folios, while in others we still encounter promoted or protected folios. But my gut feeling is that even if we don't retry and instead go back to the outer loop for another iteration, we'll still encounter those folios, since they are still on the LRU. We would just reach those folios in a more costly way.
Thanks, Barry. These number is very convincing. The retry for swappiness 0 is worthy. Then the patchset feels like doing two things: refactoring the for() loop; improving the eviction for swappiness 0/201 by adding a retry and this also makes them be consistent with (1, 200). While the cover letter subject, patch 1 and patch 2 feels like it's not easy to match them to the corresponding part. Maybe merging them to one patch, or rearranging them? Just personal opinion.
Barry Song (Xiaomi)
reviewerRe: [PATCH v2 2/2] mm/mglru: make retry logic explicit in isolate_folios()
Quoted 1 line · Baoquan He
[...]
Quoted 9 lines
Thanks, Barry. These number is very convincing. The retry for swappiness 0 is worthy. Then the patchset feels like doing two things: refactoring the
for()loop; improving the eviction for swappiness 0/201 by adding a retry and this also makes them be consistent with (1, 200). While the cover letter subject, patch 1 and patch 2 feels like it's not easy to match them to the corresponding part. Maybe merging them to one patch, or rearranging them? Just personal opinion.
Hi Baoquan,
Thanks very much for your suggestions and review.
We have two patches:
1. mm/mglru: make type fallback logic explicit in isolate_folios()
2. mm/mglru: make retry logic explicit in isolate_folios()
One handles fallback, while the other handles retry. I think this separation makes the logic clearer. Could we keep the current separation?
But we may really need Andrew's kind help to squash the change you found below:
From 1fb8046662a8b23be1b87a21cc9964e9ff9e1eb0 Mon Sep 17 00:00:00 2001
From: "Barry Song (Xiaomi)" <[email protected]>
Date: Thu, 3 Sep 2026 14:48:52 +0800
Subject: [PATCH] mm: revert slight behavior change for swappiness 1-200
Baoquan's review found that we unexpectedly introduced a slight behavior change for swappiness 1-200. We could now have a case like:
1. First scan -> scanned != 0
2. Second scan -> scanned = 0
3. Type fallback
Step 3 was impossible before. Let's remove this possibility.
--- mm/vmscan.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
mm/vmscan.c +1 -1
@@ -4939,7 +4939,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 (!scanned && !tried && type_fallback_allowed) { type = !type; tried = true; type_fallback_allowed = false;
Signature · 1 line
2.39.3 (Apple Git-146)
Baoquan He
reviewerRe: [PATCH v2 2/2] mm/mglru: make retry logic explicit in isolate_folios()
Quoted 18 lines · Barry Song (Xiaomi)
Quoted 1 line · Baoquan He
[...]
Quoted 9 lines
Thanks, Barry. These number is very convincing. The retry for swappiness 0 is worthy. Then the patchset feels like doing two things: refactoring the
for()loop; improving the eviction for swappiness 0/201 by adding a retry and this also makes them be consistent with (1, 200). While the cover letter subject, patch 1 and patch 2 feels like it's not easy to match them to the corresponding part. Maybe merging them to one patch, or rearranging them? Just personal opinion.Hi Baoquan,
Thanks very much for your suggestions and review.
We have two patches:
Quoted 6 lines
1.
mm/mglru: make type fallback logic explicit in isolate_folios()
2.mm/mglru: make retry logic explicit in isolate_folios()One handles fallback, while the other handles retry. I think this separation makes the logic clearer. Could we keep the current separation?
Yeah, it's also fine to me. Thanks for the effort.
Quoted 42 lines
But we may really need Andrew's kind help to squash the change you found below:
From
1fb8046662a8b23be1b87a21cc9964e9ff9e1eb0Mon Sep 17 00:00:00 2001
From: "Barry Song (Xiaomi)" <[email protected]>
Date: Thu, 3 Sep 2026 14:48:52 +0800
Subject: [PATCH] mm: revert slight behavior change for swappiness 1-200Baoquan's review found that we unexpectedly introduced a slight behavior change for swappiness 1-200. We could now have a case like:
1. First scan ->
scanned != 0
2. Second scan ->scanned = 0
3. Type fallbackStep 3 was impossible before. Let's remove this possibility.
Reported-by Baoquan He <[email protected]>Signed-off-by Barry Song (Xiaomi) <[email protected]>--- mm/vmscan.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)mm/vmscan.c +1
-1@@ -4939,7 +4939,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 (!scanned && !tried && type_fallback_allowed) { type = !type; tried = true; type_fallback_allowed = false;Signature · 1 line
2.39.3 (Apple Git-146)