"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
