Re: [Patch v2 2/2] mm/huge_memory: Optimize and simplify __split_unmapped_folio() logic

[David Hildenbrand <david@redhat.com>]

On 17.10.25 16:26, Zi Yan wrote:
> On 17 Oct 2025, at 5:44, Lorenzo Stoakes wrote:
> 
>> On Thu, Oct 16, 2025 at 12:46:13AM +0000, Wei Yang wrote:
>>> Existing __split_unmapped_folio() code splits the given folio and update
>>> stats, but it is complicated to understand.
>>>
>>> After simplification, __split_unmapped_folio() directly calculate and
>>> update the folio statistics upon a successful split:
>>>
>>> * All resulting folios are @split_order.
>>>
>>> * The number of new folios are calculated directly from @old_order
>>>    and @split_order.
>>>
>>> * The folio for the next split is identified as the one containing
>>>    @split_at.
>>>
>>> * An xas_try_split() error is returned directly without worrying
>>>    about stats updates.
>>
>> You seem to be doing two things at once, a big refactoring where you move stuff
>> about AND changing functionality.
> 
> No function change is done in this patchset. The wording might be
> confusing here, it should be read like:
> 
> After simplification, __split_unmapped_folio() directly calculate and
> update the folio statistics upon a successful split, so An xas_try_split()
> error is returned directly without worrying about stats updates.
> 
> David suggested a change[1] to make it clear:
> Stats fixup is no longer needed for an xas_try_split() error,
> since we now update the stats only after a successful split.
> 
> 
> [1] https://lore.kernel.org/linux-mm/518dedb8-d379-47c3-a4c1-f4afc789f1b4@redhat.com/
> 
>>
>> Can we split this out please? It makes review so much harder.
> 
> I asked Wei to use a single patch for this change, since the original
> code was complicated due to the initial implementation. After my
> recent change (first commit 6c7de9c83)[1], __split_unmmaped_folio()
> can be simplified like Wei did here.

I think it was too much split, agreed.

This patch was a bit hard for me to review, not sure if there would have 
been a better way to split some stuff out more logically.

Most changes just complicated way.

Not sure if splitting out the stats update change or the calculation of 
the number of folios could have been easily done.

-- 
Cheers

David / dhildenb