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

[Wei Yang <richard.weiyang@gmail.com>]

On Fri, Oct 17, 2025 at 03:55:16PM +0100, Lorenzo Stoakes wrote:
>On Fri, Oct 17, 2025 at 04:45:17PM +0200, David Hildenbrand wrote:
>> 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.
>
>I mean as I said to Wei/Zi, my objection is both moving code around and
>making complicated changes that require you to really dig in to figure them
>out _at the same time_.

Is this version better for review?

http://lkml.kernel.org/r/20251014134606.22543-1-richard.weiyang@gmail.com

>
>It's perfectly feasible to send separate patches for non-functional
>changes, I've done that myself, each building on the next to make it clear
>what's going on.
>
>I'm a little surprised at the oppositon to that...
>
>But if you're convinced there's no other way this could be done (bit
>surprising as you're asking 'why this change' several times here) I guess
>I'll sit down and try to decode it.

-- 
Wei Yang
Help you, Help me