[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3f76748b-8f99-4e63-ba39-adadc2f58838@redhat.com>
Date: Mon, 8 Sep 2025 16:35:16 +0200
From: David Hildenbrand <david@...hat.com>
To: Hugh Dickins <hughd@...gle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Will Deacon <will@...nel.org>,
Shivank Garg <shivankg@....com>, Matthew Wilcox <willy@...radead.org>,
Christoph Hellwig <hch@...radead.org>, Keir Fraser <keirf@...gle.com>,
Jason Gunthorpe <jgg@...pe.ca>, John Hubbard <jhubbard@...dia.com>,
Frederick Mayle <fmayle@...gle.com>, Peter Xu <peterx@...hat.com>,
"Aneesh Kumar K.V" <aneesh.kumar@...nel.org>,
Johannes Weiner <hannes@...xchg.org>, Vlastimil Babka <vbabka@...e.cz>,
Alexander Krabler <Alexander.Krabler@...a.com>, Ge Yang
<yangge1116@....com>, Li Zhe <lizhe.67@...edance.com>,
Chris Li <chrisl@...nel.org>, Yu Zhao <yuzhao@...gle.com>,
Axel Rasmussen <axelrasmussen@...gle.com>, Yuanchu Xie <yuanchu@...gle.com>,
Wei Xu <weixugc@...gle.com>, Konstantin Khlebnikov <koct9i@...il.com>,
linux-kernel@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH 6/7] mm: folio_may_be_cached() unless folio_test_large()
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@...hat.com>
>>> Signed-off-by: Hugh Dickins <hughd@...gle.com>
>>> Cc: <stable@...r.kernel.org>
>>> ---
>>> 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
Powered by blists - more mailing lists