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

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.