Re: [PATCH v2 1/4] mm: avoid unnecessary lru drain for wp_can_reuse_anon_folio()

[Barry Song]

On Sat, Jun 27, 2026 at 10:44 AM Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote:
>
> On Fri, Jun 26, 2026 at 06:25:48PM +0200, David Hildenbrand (Arm) wrote:
> > On 6/24/26 23:04, Barry Song wrote:
> > > On Wed, Jun 24, 2026 at 11:02 PM David Hildenbrand (Arm)
> > > <david@xxxxxxxxxx> wrote:
> > >>
> > >> On 6/24/26 01:16, Barry Song (Xiaomi) wrote:
> > >>> We always unconditionally drain the LRU before retrying anon folio
> > >>> reuse in wp_can_reuse_anon_folio(). Instead, assume !LRU anon folios
> > >>> are in lru_cache, and use the refcount to avoid many unnecessary LRU
> > >>> drains.
> > >>>
> > >>> Acked-by: Shakeel Butt <shakeel.butt@xxxxxxxxx>
> > >>> Reviewed-by: Baoquan He <baoquan.he@xxxxxxxxx>
> > >>> Signed-off-by: Barry Song (Xiaomi) <baohua@xxxxxxxxxx>
> > >>> ---
> > >>>  mm/memory.c | 8 +++++++-
> > >>>  1 file changed, 7 insertions(+), 1 deletion(-)
> > >>>
> > >>> diff --git a/mm/memory.c b/mm/memory.c
> > >>> index ff338c2abe92..f6848f4234a6 100644
> > >>> --- a/mm/memory.c
> > >>> +++ b/mm/memory.c
> > >>> @@ -4193,12 +4193,18 @@ static bool wp_can_reuse_anon_folio(struct folio *folio,
> > >>>        */
> > >>>       if (folio_test_ksm(folio) || folio_ref_count(folio) > 3)
> > >>>               return false;
> > >>> -     if (!folio_test_lru(folio))
> > >>> +     if (!folio_test_lru(folio)) {
> > >>> +             /*
> > >>> +              * Assume folio is on lru_cache and holds a cache reference.
> > >>> +              */
> > >>> +             if (folio_ref_count(folio) > 2 + folio_test_swapcache(folio))
> > >>> +                     return false;
> > >>
> > >> I'm not keen on making this function even uglier, so no, not like that.
> > >>
> > >> We have the earlier "folio_ref_count(folio) > 3" check.
> > >>
> > >> In which scenarios can you trigger this such that we would care?
> > >>
> > >> If the answer is "I don't know" there is no reason for a change.
> > >
> > > As I replied to Shakeel in the v1 discussion [1], this can avoid a
> > > large number of drains, both during Ubuntu boot and under normal
> > > workloads:
> > >
> > > "I booted the system into Ubuntu, and after
> >
> > is this with this patch only?

Right. wp_reuse_skipped_drain accounts for the behavior
introduced by this patch, while do_swap_skipped_drain comes
from patch 3/3.

> >
> > >
> > > boot completed I observed:
> > >
> > > wp_reuse_skipped_drain: 5542
> > > do_swap_skipped_drain: 0
> > >
> > > Then I built the kernel in a 1GB memcg using zRAM swap, and observed:
> > >
> > > wp_reuse_skipped_drain: 25017
> > > do_swap_skipped_drain: 43595
> >
> > This is all data that belongs into this patch description, not hidden
> > somewhere on the internet :)

Agreed. Sorry, I was being a bit lazy.

>
> I asked the same i.e. to include this information in the commit message [1].
>
> [1] https://lore.kernel.org/linux-mm/ajK4N2zK3sPZFQuf@xxxxxxxxx/

Sorry, I missed this one. I'll add it in v3.

>
> >
> >
> > And why do we care about local draining? This is not a drain-all.
>
> Let me take a stab at it. Local draining can potentially make the lru cache's
> batching ineffective. The whole reason for lru caches is to batch the lru
> operations to keep the lru lock contention low but if we keep draining
> prematurely we will continue to make lru lock contention worse. This is
> particularly bad for workloads which chrun a lot of LRU pages through
> allocation, free and/or reclaim.

I actually asked the same question myself in the RFC here[1]:

"On the other hand, the folio may be sitting in the lru_cache of
another CPU, which the current drain cannot flush. As things stand
today, the drain only succeeds if the folio happens to be queued on
the current CPU's lru_cache, giving it roughly a 1/nr_cpus chance of
working. drain_all would clearly be too expensive to avoid.
So another possibility is to drop this drain as well. The folio can
be released later, at the cost of missing some opportunities for
reuse."

After thinking about it for a couple of days, my gut feeling is
that keeping it might increase the chances of reuse in at least
two cases:
1. It could be a newly allocated folio in do_swap_page(), and
do_swap_page() may subsequently call do_wp_page(). In
that case, task migration is less likely to have occurred.
2. On systems with fewer CPUs (and typically less memory), the
chance of having the folio in the same CPU's lru_cache,
which is roughly 1 / nr_cpus, may still be reasonably high.

I'm not entirely sure about this, so I chose a relatively
conservative approach: making an obviously correct improvement
without introducing dramatic changes. Another reason is that it's
also hard to evaluate the impact of removing the local drain
across all systems and workloads.

Shakeel, David, what are your thoughts on this? Should we go
with a code refinement in v3 as David suggested, or completely
remove this drain?

[1] https://lore.kernel.org/linux-mm/CAGsJ_4yFgcvpTAZ_YV73+U=BEnOiSxaFtGNFpbf0DWBkVcOQ3Q@xxxxxxxxxxxxxx/

Best Regards
Barry