Re: [PATCH v3 14/18] mm/huge_memory: clean up after-split folio freeing in __folio_split

[Kairui Song]

On Fri, Aug 28, 2026 at 12:48 AM David Hildenbrand (Arm)
<david@xxxxxxxxxx> wrote:
>
> On 8/20/26 20:55, Kairui Song via B4 Relay wrote:
> > From: Kairui Song <kasong@xxxxxxxxxxx>
> >
> > Replace free_folio_and_swap_cache() with an explicit folio_free_swap()
> > and folio_put() in the after-split loop.  free_folio_and_swap_cache()
> > unlocks the folio, then free_swap_cache() must trylock it again and
> > re-check folio_mapped() before freeing the swap cache entries; if the
> > trylock loses a race, the entries are left behind even though the folio
> > reference is dropped.  The sub folios are still locked and unmapped
> > here, so just directly call folio_free_swap() directly under the lock,
> > unlock and drop the reference.  This makes the swap cache freeing
> > deterministic and the reference drop explicit.
> >
> > Reviewed-by: Zi Yan <ziy@xxxxxxxxxx>
> > Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> > ---
> >  mm/huge_memory.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index 84c6e4bbaa88..113a33cddace 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -4337,14 +4337,16 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
> >               if (new_folio == page_folio(lock_at))
> >                       continue;
> >
> > -             folio_unlock(new_folio);
> >               /*
> >                * Subpages whose mapping has been zapped may be freed
> >                * earlier, but freeing them requires taking the
> > -              * lru_lock, so we defer put_page() on tail pages until
> > +              * lru_lock, so we defer folio_put() on tail pages until
> >                * after the split completes.
> >                */
> > -             free_folio_and_swap_cache(new_folio);
> > +             if (is_swapcache && !folio_mapped(new_folio))
> > +                     folio_free_swap(new_folio);
>
> But this is effectively duplicating code that belongs into swap code.
>
> Should we instead have a helper next to free_folio_and_swap_cache that consumes
> an already locked folio?

I think the only duplication here is the is_swapcache and folio_mapped
check? folio_free_swap is a common swap helper. And putting the
folio_unlock and folio_put in the swap side looks odd to me, since the
folio is locked here and refs get here through the function's
convention. Pairing these unlock and put closer to the convention
doesn't seem too bad IMO.