Re: [PATCH 6/7] mm: folio_may_be_cached() unless folio_test_large()

[David Hildenbrand]

On 08.09.25 13:19, Hugh Dickins wrote:
> On Mon, 1 Sep 2025, David Hildenbrand wrote:
> > On 31.08.25 11:16, Hugh Dickins wrote:
> > > mm/swap.c and mm/mlock.c agree to drain any per-CPU batch as soon as
> > > a large folio is added: so collect_longterm_unpinnable_folios() just
> > > wastes effort when calling lru_add_drain_all() on a large folio.
> > >
> > > But although there is good reason not to batch up PMD-sized folios,
> > > we might well benefit from batching a small number of low-order mTHPs
> > > (though unclear how that "small number" limitation will be implemented).
> > >
> > > So ask if folio_may_be_cached() rather than !folio_test_large(), to
> > > insulate those particular checks from future change.  Name preferred
> > > to "folio_is_batchable" because large folios can well be put on a batch:
> > > it's just the per-CPU LRU caches, drained much later, which need care.
> > >
> > > Marked for stable, to counter the increase in lru_add_drain_all()s
> > > from "mm/gup: check ref_count instead of lru before migration".
> > >
> > > Suggested-by: David Hildenbrand <david@xxxxxxxxxx>
> > > Signed-off-by: Hugh Dickins <hughd@xxxxxxxxxx>
> > > Cc: <stable@xxxxxxxxxxxxxxx>
> > > ---
> > >    include/linux/swap.h | 10 ++++++++++
> > >    mm/gup.c             |  5 +++--
> > >    mm/mlock.c           |  6 +++---
> > >    mm/swap.c            |  2 +-
> > >    4 files changed, 17 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/include/linux/swap.h b/include/linux/swap.h
> > > index 2fe6ed2cc3fd..b49a61c32238 100644
> > > --- a/include/linux/swap.h
> > > +++ b/include/linux/swap.h
> > > @@ -385,6 +385,16 @@ void folio_add_lru_vma(struct folio *, struct
> > > vm_area_struct *);
> > >    void mark_page_accessed(struct page *);
> > >    void folio_mark_accessed(struct folio *);
> > >    
> >
> > Two smaller things:
> >
> > (1) We have other "folio_maybe_*" functions, so this one should likely
> >      better start with that as well.
> >
> > (2) With things like fscache in mind, the function can be a bit
> >      misleading.
> >
> > So I wonder if (a) we should just add kerneldoc to document it clearly (lru
> > cache, mlock cache?) and (b) maybe call it folio_may_be_lru_cached(). Not sure
> > if we can find a better abstraction for these two caches.
> >
> > Thinking again, "maybe_cached" might be a bit misleading because it implements
> > a very very very bad heuristic for small folios.
> >
> > Maybe it's more like "supports being cached".
> >
> > folio_lru_caching_supported()
>
> folio_may_be_cached() -> folio_may_be_lru_cached(), yes, that's
> very much better, thanks.
>
> (Settimg aside that I've never perceived those pagevecs/batches as a
> "cache"; but lru_cache_disable() gave us that terminology, and we've
> gone with the flow ever since.  lru_add_drain() would be better named
> lru_cache_drain() now, I've always got hung up on "adding a drain".)

Yeah, the terminology is not that intuitive :)

Not sure if using "batched" instead of "cached" might be clearer long-term?

> "may be" rather than "maybe" was intentional: perhaps too subtle,
> but to a native speaker it neatly expresses both the "we can do this"
> and "might this have been done" cases.

I would wish we could find something that also non-native speakers can 
immediately understand ;)

"may_get_lru_cached" / "may_get_lru_batched"?

/me could not even phrase it in German properly

> kernel-doc?  I don't think so, this is very much an mm-internal
> matter, and I don't care for the way kernel-doc forces us towards
> boilerplate ("@folio: The folio.") rather than helpful comment.

So a comment that this is an internal helper might be nice. Or we just 
move it straight to mm/internal.h ?

--
Cheers

David / dhildenb