Re: [PATCH 4/8] mm/mglru: scan and count the exact number of folios

Chen Ridong
2026-03-24T07:22:59Z

On 2026/3/23 0:20, Kairui Song wrote:
> On Sat, Mar 21, 2026 at 4:59 AM Axel Rasmussen <axelrasmussen@google.com> wrote:
>>
>> On Tue, Mar 17, 2026 at 12:11 PM Kairui Song via B4 Relay
>> <devnull+kasong.tencent.com@kernel.org> wrote:
>>>
>>> From: Kairui Song <kasong@tencent.com>
>>>
>>> Make the scan helpers return the exact number of folios being scanned
>>> or isolated. This should make the scan more accurate and easier to
>>> follow.
>>>
>>> Now there is no more need for special handling when there is no
>>> progress made. The old livelock prevention `(return isolated ||
>>> !remaining ? scanned : 0)` is replaced by the natural scan budget
>>> exhaustion in try_to_shrink_lruvec, and sort_folio moves ineligible
>>> folios to newer generations.
>>>
>>> Signed-off-by: Kairui Song <kasong@tencent.com>
>>> ---
>>> mm/vmscan.c | 27 +++++++++++----------------
>>> 1 file changed, 11 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>> index ed5b5f8dd3c7..4f4548ff3a17 100644
>>> --- a/mm/vmscan.c
>>> +++ b/mm/vmscan.c
>>> @@ -4680,7 +4680,7 @@ static bool isolate_folio(struct lruvec *lruvec, struct folio *folio, struct sca
>>>
>>> static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
>>> struct scan_control *sc, int type, int tier,
>>> - struct list_head *list)
>>> + struct list_head *list, int *isolatedp)
>>> {
>>> int i;
>>> int gen;
>>> @@ -4750,11 +4750,9 @@ static int scan_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
>>> type ? LRU_INACTIVE_FILE : LRU_INACTIVE_ANON);
>>> if (type == LRU_GEN_FILE)
>>> sc->nr.file_taken += isolated;
>>> - /*
>>> - * There might not be eligible folios due to reclaim_idx. Check the
>>> - * remaining to prevent livelock if it's not making progress.
>>> - */
>>> - return isolated || !remaining ? scanned : 0;
>>> +
>>> + *isolatedp = isolated;
>>> + return scanned;
>>> }
>>>
>>> static int get_tier_idx(struct lruvec *lruvec, int type)
>>> @@ -4819,23 +4817,24 @@ static int isolate_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
>>> int *type_scanned, struct list_head *list)
>>> {
>>> int i;
>>> + int scanned = 0;
>>> + int isolated = 0;
>>> int type = get_type_to_scan(lruvec, swappiness);
>>>
>>> for_each_evictable_type(i, swappiness) {
>>> - int scanned;
>>> int tier = get_tier_idx(lruvec, type);
>>>
>>> *type_scanned = type;
>>
>> I think this is problematic, now `isolate_folios` can scan a nonzero
>> amount of > 1 type of memory. Then the caller (`evict_folios`) calls
>> `trace_mm_vmscan_lru_shrink_inactive` with the total scanned amount,
>> with only the last type we scanned (misattributing part of the scan,
>> potentially). Not a "functional" issue, but it could mean confusing
>> data for anyone watching the tracepoint.
>
> Thanks! Nice catch, I'll introduce another variable for the tracepoint
> then it should be fine.
>
>>
>>
>>>
>>> - scanned = scan_folios(nr_to_scan, lruvec, sc,
>>> - type, tier, list);
>>> - if (scanned)
>>> + scanned += scan_folios(nr_to_scan, lruvec, sc,
>>> + type, tier, list, &isolated);
>>> + if (isolated)
>>> return scanned;
>>>
>>> type = !type;
>>> }
>>>
>>> - return 0;
>>> + return scanned;
>>> }
>>>
>>> static int evict_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
>>> @@ -4852,7 +4851,6 @@ static int evict_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
>>> struct reclaim_stat stat;
>>> struct lru_gen_mm_walk *walk;
>>> bool skip_retry = false;
>>> - struct lru_gen_folio *lrugen = &lruvec->lrugen;
>>> struct mem_cgroup *memcg = lruvec_memcg(lruvec);
>>> struct pglist_data *pgdat = lruvec_pgdat(lruvec);
>>>
>>> @@ -4860,10 +4858,7 @@ static int evict_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
>>>
>>> scanned = isolate_folios(nr_to_scan, lruvec, sc, swappiness, &type, &list);
>>>
>>> - scanned += try_to_inc_min_seq(lruvec, swappiness);
>>> -
>>> - if (evictable_min_seq(lrugen->min_seq, swappiness) + MIN_NR_GENS > lrugen->max_seq)
>>> - scanned = 0;
>>> + try_to_inc_min_seq(lruvec, swappiness);
>>
>> IIUC, this change is what introduces the issue patch 6 is trying to
>> resolve. Is it worth squashing patch 6 in to this one, so we don't
>> have this non-ideal intermediate state?
>
> Well it's not, patch 6 is fixing an existing problem, see the cover
> letter about the OOM issue.
>
> This part of changing is just cleanup the loop code. It looks really
> strange to me that increasing min_seq is considered as scanning one
> folio. Aborting the scan if there is only 2 gen kind of make sense but
> this doesn't seems the right place. These strange parts to avoid
> livelock can be dropped since we have an exact count of folios being
> scanned now. I'll add more words in the commit message.

This change confused me too.

IIUC, this change looks conceptually tied to patch 3. The following change means
that evict_folios should not be invoked if aging is needed. So the judge can be
dropped there, right?

```
static bool try_to_shrink_lruvec(struct lruvec *lruvec, struct scan_control *sc)
{
...
+ if (should_run_aging(lruvec, max_seq, sc, swappiness)) {
+ if (try_to_inc_max_seq(lruvec, max_seq, swappiness, false))
+ need_rotate = true;
+ break;
+ }
```

--
Best regards,
Ridong