Re: [RFC PATCH v3 3/6] mm/mglru: improve readability of isolate_folios()

On 7/31/26 4:38 PM, Barry Song (Xiaomi) wrote:
> 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.
> 
> Signed-off-by: Ridong Chen <[email protected]>
> Co-developed-by: Barry Song (Xiaomi) <[email protected]>
> Signed-off-by: Barry Song (Xiaomi) <[email protected]>
> ---

Good cleanup. Some comments below.

>   mm/vmscan.c | 47 +++++++++++++++++++++++++++--------------------
>   1 file changed, 27 insertions(+), 20 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 31947fa60f18..0038f33aa318 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -4828,35 +4828,42 @@ 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 single_type = is_single_type_reclaim(swappiness);
>   	int type = get_type_to_scan(lruvec, swappiness);
> +	int total_scanned = 0, scanned, tier;
> +	bool tried = false;

The tried' is a bit confusing. How about 'type_fallback_allowed'? Or 
other more readable variable name?

In addition, the 'single_type' variable can be removed, and just 
initialize 'type_fallback_allowed' directly:

bool type_fallback_allowed = !is_single_type_reclaim(swappiness);


> -	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 (!tried && !scanned && !single_type) {
> +		type = !type;
> +		tried = true;
> +		goto retry;
>   	}
>   
>   	return total_scanned;