Re: [PATCH 05/10] mm/madvise: factor huge-PMD folio processing

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Wed, Sep 23, 2026 at 01:06:45PM -0400, Gregory Price wrote:
> On Wed, Sep 23, 2026 at 05:43:59PM +0100, Lorenzo Stoakes (ARM) wrote:
> > > +/* Return a locked, referenced folio only when it must be split. */
> >
> > I find it really weird that when it:
> >
> > a. succeeds
> > b. mapped folio is missing/invalid/filtered
> >
> > In both cases it returns NULL.
> >
> > And it's also weirdly returning a folio in a kind of failure case, or it's
> > more like a defer-to-the-rest-of-the-code case I suppose.
> >
> > I wonder if the split could be done as part of the function?
> >
> > Then maybe have it return bool and document that true means it's fully
> > processed (invalid folio cases, success case), false means that it's been
> > split and the rest of the code should continue.
> >
> > Awkward one actually.
>
> Yes this was an awkward one to futz around with.  I took a couple tries
> at it and this is ultimately what fell out and passed the tests.
>
> I think there's some tweaks that could be made here, but I err'd on the
> side of "don't break shit" before I went twiddling.
>
> It is at least easier to understand, but certainly this shows how poorly
> the original code was structured.

Yeah, if this is an intermediate state I won't necessarily insist but it just
feels so odd.

Maybe have a look at it doing the split in the function and see how that
sits?

>
> >
> > > +static struct folio *
> > > +madvise_lru_huge_pmd_locked(pmd_t *pmd, pmd_t orig_pmd,
> > > +		unsigned long addr, unsigned long next, struct mm_walk *walk,
> > > +		struct list_head *folio_list, bool pageout_anon_only)
> > > +{
> > > +	const struct madvise_walk_private *private = walk->private;
> > > +	struct vm_area_struct *vma = walk->vma;
> > > +	struct folio *folio;
> > > +
> > > +	folio = vm_normal_folio_pmd(vma, addr, orig_pmd);
> > > +	if (!folio || folio_is_zone_device(folio))
> > > +		return NULL;
> > > +	if (madvise_lru_folio_is_filtered(folio, pageout_anon_only))
> > > +		return NULL;
> > > +
> > > +	if (next - addr != HPAGE_PMD_SIZE) {
> >
> > NIT: Maybe could define above as:
> >
> > 	const bool spans_pmd = next - addr == HPAGE_PMD_SIZE;
> >
> > And then make this:
> >
> > 	if (!spans_pmd)
> >
>
> seems reasonable. ack
>
> ~Gregory

--
Cheers, Lorenzo