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