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

On 8/3/26 12:46 PM, Barry Song wrote:
> On Mon, Aug 3, 2026 at 11:22 AM Baolin Wang
> <[email protected]> wrote:
>>
>>
>>
>> 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);
> 
> 
> Yep. It seems we can simply use type_fallback_allowed and drop
> tried entirely:

Thanks. That's what I meant:)

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 1ffd96c87bc2..358388b49a70 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -4840,10 +4840,9 @@ static int isolate_folios(unsigned long
> nr_to_scan, struct lruvec *lruvec,
>                            struct list_head *list, int *isolated,
>                            int *isolate_type, int *isolate_scanned)
>   {
> -       bool single_type = is_single_type_reclaim(swappiness);
> +       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);
> @@ -4861,9 +4860,9 @@ 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 (!tried && !scanned && !single_type) {
> +       if (type_fallback_allowed && !scanned) {
>                  type = !type;
> -               tried = true;
> +               type_fallback_allowed = false;
>                  goto retry;
>          }