Re: [PATCH v9 14/41] mm: swap: Introduce lru_add_drain_progressive()

"David Hildenbrand (Arm)" <[email protected]> writes:

> On 7/29/26 02:35, Ackerley Tng via B4 Relay wrote:
>> From: Ackerley Tng <[email protected]>
>>
>> Extract the progressive LRU drain retry logic from
>> collect_longterm_unpinnable_folios() into a reusable helper,
>> lru_add_drain_progressive().
>>
>> When attempting to isolate folios that may still reside in per-CPU folio
>> batches, draining is escalated progressively:
>>
>> 1. State 0: Call lru_add_drain() to flush local CPU batches.
>> 2. State 1: Call lru_add_drain_all() to flush all CPU batches.
>> 3. State >= 2: Return false to stop retrying.
>>
>> Refactor collect_longterm_unpinnable_folios() to use this new helper.
>>
>> The helper will be used by KVM's guest_memfd in a later patch.
>>
>> Signed-off-by: Ackerley Tng <[email protected]>
>> ---
>>  include/linux/swap.h |  2 ++
>>  mm/gup.c             | 19 ++++++-------------
>>  mm/swap.c            | 15 +++++++++++++++
>>  3 files changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/linux/swap.h b/include/linux/swap.h
>> index 8f0f68e245baa..cd54f73f34f39 100644
>> --- a/include/linux/swap.h
>> +++ b/include/linux/swap.h
>> @@ -344,6 +344,8 @@ extern void lru_add_drain(void);
>>  extern void lru_add_drain_cpu(int cpu);
>>  extern void lru_add_drain_cpu_zone(struct zone *zone);
>>  extern void lru_add_drain_all(void);
>> +bool lru_add_drain_progressive(int *drain_state);
>> +
>>  void folio_deactivate(struct folio *folio);
>>  void folio_mark_lazyfree(struct folio *folio);
>>  extern void swap_setup(void);
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 0692119b79043..5f00435e2c635 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -2268,7 +2268,7 @@ static unsigned long collect_longterm_unpinnable_folios(
>>  {
>>  	unsigned long collected = 0;
>>  	struct folio *folio;
>> -	int drained = 0;
>> +	int drain_state = 0;
>>  	long i = 0;
>>
>>  	for (folio = pofs_get_folio(pofs, i); folio;
>> @@ -2287,18 +2287,11 @@ static unsigned long collect_longterm_unpinnable_folios(
>>  			continue;
>>  		}
>>
>> -		if (drained == 0 && folio_may_be_lru_cached(folio) &&
>> -				folio_ref_count(folio) !=
>> -				folio_expected_ref_count(folio) + 1) {
>> -			lru_add_drain();
>> -			drained = 1;
>> -		}
>> -		if (drained == 1 && folio_may_be_lru_cached(folio) &&
>> -				folio_ref_count(folio) !=
>> -				folio_expected_ref_count(folio) + 1) {
>> -			lru_add_drain_all();
>> -			drained = 2;
>> -		}
>> +		while (folio_may_be_lru_cached(folio) &&
>> +		       folio_ref_count(folio) !=
>> +		       folio_expected_ref_count(folio) + 1 &&
>> +		       lru_add_drain_progressive(&drain_state))
>> +			;
>
> That's rather nasty.
>

I didn't really like this either, the drain_state thing is really
awkward, but in both usages (collect_longterm_unpinnable_folios() and
guest_memfd), there's an outer loop where if the draining happened on
the local CPU before it should skip straight to just draining on all the
other CPUs.

> I was hoping that we could embed more logic in a helper. The history [1] of the
> refcount check is rather sad:
>
> https://lore.kernel.org/all/[email protected]/
>
> ... primarily because of mlock() handling.
>

In the original code in collect_longterm_unpinnable_folios(), I couldn't
find anything that handles folio_test_mlock(), so I was lost for a while
until I realized mlock() doesn't add a refcount to the folio.

Are you kind of proposing an optimization to
collect_longterm_unpinnable_folios() by adding a check for
!folio_test_mlock()? I hope we can put that in a separate patch series,
I'm still hoping to get this series in for 7.3!!

> For guest_memfd(), would mlock() ever apply on a path where you need that check?
>

For guest_memfd, if a folio were mlocked, unmapping it would fail, and
so the refcount on the folio would be elevated. The
kvm_gmem_is_safe_for_conversion() check in the patch after this one
would fail, correctly.

I really wanted the caller of the lru_add_drain_progressive() function
to control whether to do the drain (see below), which would solve the
mlock problem by not assuming the use of folio_expected_ref_count() to
determine whether to do draining.

> Conceptually, I wonder whether we can do the following, and rely on the refcount
> check only on the mlock path.
>
>
> From ae30f7594692b0c47b44922c26b6480dd9584872 Mon Sep 17 00:00:00 2001
> From: "David Hildenbrand (Arm)" <[email protected]>
> Date: Thu, 30 Jul 2026 12:20:14 +0200
> Subject: [PATCH] tmp
>
> Signed-off-by: David Hildenbrand (Arm) <[email protected]>
> ---
>  mm/gup.c | 66 ++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 52 insertions(+), 14 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 99902c15703b0..ec43071bd035a 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2259,6 +2259,56 @@ static struct folio *pofs_next_folio(struct folio *folio,
>  	return pofs_get_folio(pofs, i);
>  }
>
> +enum lru_cache_drained {
> +	LRU_CACHE_NOT_DRAINED,
> +	LRU_CACHE_DRAINED,
> +	LRU_CACHE_DRAINED_ALL,
> +};
> +
> +static inline void folio_likely_lru_cached(const struct folio *folio)
> +{
> +	if (!folio_may_be_lru_cached(folio))
> +		return false;
> +	/*
> +	 * Having the LRU flag clear either indicates LRU cache references
> +	 * or LRU isolation.
> +	 */
> +	if (!folio_test_lru(folio))
> +		return true;
> +	/*
> +	 * For mlocked folios, we don't have a real indication: we can only
> +	 * take a guess based on the refcount.
> +	 */
> +	if (!folio_test_mlock(folio))
> +		return false;
> +	return folio_expected_ref_count(folio) == folio_ref_count(folio) = 1;
> +}
> +
> +/**
> + * lru_cache_drain_for_folio() - progressively try draining the lru cache
> + * @folio: The folio.
> + * @drained: Status initialized to LRU_CACHE_NOT_DRAINED by the caller.
> + *
> + * TODO
> + */
> +static void lru_cache_drain_for_folio(const struct folio *folio,
> +		enum lru_cache_drained *drained)

The main thing I wanted in the proposed version
(lru_add_drain_progressive()), was to let the caller determine whether
to try or to continue draining.

I wanted the caller to have full control over whether to drain or not,
so I didn't want to pass folio into the function. I thought the caller
should first determine whether to drain, then call the function.

This version assumes that the caller wants to continue draining based on
something to do with a folio, and the folio may or may not be on the
lru_add fbatch at all, which is a little strange to me.

Also, this version enforces a certain definition of "expected" number of
refcounts on the folio. I guess in this case this definition works with
guest_memfd, but guest_memfd doesn't support swap so the swapcache check
in folio_expected_ref_count() isn't necessary. Also, if the definition
folio_expected_ref_count() changes, then guest_memfd is implicitly
affected. Not sure if the "expected" definition is the same for all
callers?

I guess if there was a function named
folio_ref_counts_indicate_presence_only_in_the_filemap() instead of
folio_expected_ref_count(), then it'd be the perfect function for
guest_memfd to use. The current definition of folio_expected_ref_count()
also includes page table mappings, but in this check guest_memfd really
wants to make sure that there are no page table mappings.

This version folds folio_likely_lru_cached() into the check, and I
adopted the check for guest_memfd because it'd help with huge pages,
though technically guest_memfd is always 4K now so the check is also
pointless.

I tried a macro version of this where the macro caller can pass in a
full condition, which would look like this:

  lru_add_drain_while(folio_may_be_lru_cached(folio) &&
                      folio_ref_count(folio) != expected_refcount,
                      drain_state);

but I thought that just created something people have to jump to, to
first understand how the macro works, so I left it as an explicit while
loop instead.

> +{
> +	if (!folio_likely_lru_cached(folio))
> +		return false;
> +
> +	/* Try local draining first, if not already done previously. */
> +	if (*drained == LRU_CACHE_NOT_DRAINED) {
> +		lru_add_drain();
> +		*drained = LRU_CACHE_DRAINED;
> +	}
> +	/* Try draining all CPUs next if still not an LRU folio. */
> +	if (folio_likely_lru_cached(folio) && *drained == LRU_CACHE_DRAINED) {
> +		lru_add_drain_all();
> +		*drained = LRU_CACHE_DRAINED_ALL;
> +	}
> +}
> +
>  /*
>   * Returns the number of collected folios. Return value is always >= 0.
>   */
> @@ -2266,9 +2316,9 @@ static unsigned long collect_longterm_unpinnable_folios(
>  		struct list_head *movable_folio_list,
>  		struct pages_or_folios *pofs)
>  {
> +	enum lru_cache_drained drained = LRU_CACHE_NOT_DRAINED;

I also considered an enum, but it would be another thing to export. I
was thinking to have drain_state just be opaque to the caller and the
only thing the caller needs to know is to initialize it to 0.

Perhaps there's a better way to "make it opaque"?

>  	unsigned long collected = 0;
>  	struct folio *folio;
> -	int drained = 0;
>  	long i = 0;
>
>  	for (folio = pofs_get_folio(pofs, i); folio;
> @@ -2287,19 +2337,7 @@ static unsigned long collect_longterm_unpinnable_folios(
>  			continue;
>  		}
>
> -		if (drained == 0 && folio_may_be_lru_cached(folio) &&
> -				folio_ref_count(folio) !=
> -				folio_expected_ref_count(folio) + 1) {
> -			lru_add_drain();
> -			drained = 1;
> -		}
> -		if (drained == 1 && folio_may_be_lru_cached(folio) &&
> -				folio_ref_count(folio) !=
> -				folio_expected_ref_count(folio) + 1) {
> -			lru_add_drain_all();
> -			drained = 2;
> -		}
> -
> +		lru_cache_drain_for_folio(folio, &drained);
>  		if (!folio_isolate_lru(folio))
>  			continue;
>
> --
> 2.43.0
>
>
> --
> Cheers,
>
> David