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

[David Hildenbrand (Arm)]

On 8/27/26 19:47, Kairui Song wrote:
> 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.

I was more concerned about the folio_mapped() check. Because the code now has to
guess under which circumstances it makes sense to call folio_free_swap(), which
is precisely what free_folio_and_swap_cache() avoided.

And we already do have other code doing that, like KSM's replace_page(), and it
feels like that should all be cleaned up.

But no strong objection to your current code. It just looks a bit suboptimal
that we have no helper that wraps the swapcache and folio_mapped() check.

-- 
Cheers,

David