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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Fri, Oct 17, 2025 at 10:26:13AM -0400, 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:

I made this assessment based on David saying 'why are you making this change?'
repeatedly.

That doesn't sound like a refactoring, but if you're claiming these are all
invalid...

>
> 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.

Right isn't that a substantive change?

>
>
> [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.
>
>
>
> [1] https://lore.kernel.org/all/20250718023000.4044406-7-ziy@nvidia.com/
>

I mean I am not sure why it is so problematic to split up changes so we dont' do
a bunch of complicated/not complicated things all at once which is harder to
review, more bug prone and makes bisecting harder.

I've done series which consist of a _series_ of 'no functional change' patches
becuase they logically fell out that way.

But if you're really confident this is functional-only and is a simple and
sensible change I can take another look.

Thanks, Lorenzo