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
