Re: [PATCH 3/4] mm/mprotect: un-inline folio_pte_batch_flags()

[Pedro Falcato <pfalcato@suse.de>]

On Thu, Mar 19, 2026 at 07:14:38PM +0000, Lorenzo Stoakes (Oracle) wrote:
> On Thu, Mar 19, 2026 at 06:31:07PM +0000, Pedro Falcato wrote:
> > Hoist some previous, important to be inlined checks to the main loop
> > and noinline the entirety of folio_pte_batch_flags(). The loop itself is
> > quite large and whether it is inlined or not should not matter, as we
> > are ideally dealing with larger orders.
> >
> > Signed-off-by: Pedro Falcato <pfalcato@suse.de>
> > ---
> >  mm/mprotect.c | 16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/mm/mprotect.c b/mm/mprotect.c
> > index 8d4fa38a8a26..aa845f5bf14d 100644
> > --- a/mm/mprotect.c
> > +++ b/mm/mprotect.c
> > @@ -103,16 +103,9 @@ bool can_change_pte_writable(struct vm_area_struct *vma, unsigned long addr,
> >  	return can_change_shared_pte_writable(vma, pte);
> >  }
> >
> > -static __always_inline int mprotect_folio_pte_batch(struct folio *folio, pte_t *ptep,
> > +static noinline int mprotect_folio_pte_batch(struct folio *folio, pte_t *ptep,
> 
> Hmm I'm iffy about noinline-ing a 1 line function now?

Well, it's a 1 line function that itself (probably) has an inlined
folio_pte_batch_flags() with a bunch of inlined functions, calls, etc. But yes, I
see your point.

> 
> And now yu're noinlining something that contains an inline (but not guaranteed
> to be function, it's a bit strange overall?
> 
> >  				    pte_t pte, int max_nr_ptes, fpb_t flags)
> >  {
> > -	/* No underlying folio, so cannot batch */
> > -	if (!folio)
> > -		return 1;
> > -
> > -	if (!folio_test_large(folio))
> > -		return 1;
> > -
> >  	return folio_pte_batch_flags(folio, NULL, ptep, &pte, max_nr_ptes, flags);
> >  }
> >
> > @@ -333,7 +326,12 @@ static long change_pte_range(struct mmu_gather *tlb,
> >  				continue;
> >  			}
> >
> 
> ^^^ up here is a mprotect_folio_pte_batch() invocation for the prot_numa case,
> which now won't be doing:

Yep, this is a bug (that sashiko also caught!)

> 
> 	if (!folio)
> 		return 1;
> 
> 	if (!folio_test_large(folio))
> 		return 1;
> 
> At all, so isn't that potentially a pessimisation in itself?
> 
> 
> > -			nr_ptes = mprotect_folio_pte_batch(folio, pte, oldpte, max_nr_ptes, flags);
> > +			/* No underlying folio (or not large), so cannot batch */
> > +			if (likely(!folio || !folio_test_large(folio)))
> 
> We actually sure of this likely()? Is there data to support it? Am not a fan of
> using likely()/unlikey() without something to back it.

I did a small little test yesterday with bpftrace + a kernel build +
filemap_get_folio. I got a staggering majority of order-0 folios, with some
smaller folios spread out across a bunch of orders (up to 8, iirc). Of course
I'm on x86 so I don't have mTHP, etc enabled, so those will all be order-0 or
PMD_ORDER folios (which we won't see here).

> 
> > +				nr_ptes = 1;
> > +			else
> > +				nr_ptes = mprotect_folio_pte_batch(folio, pte, oldpte,
> > +					  max_nr_ptes, flags);
> 
> It's also pretty gross to throw this out into a massive function.
> 
> >
> >  			oldpte = modify_prot_start_ptes(vma, addr, pte, nr_ptes);
> >  			ptent = pte_modify(oldpte, newprot);
> > --
> > 2.53.0
> >
> 
> This is all seems VERY delicate, and subject to somebody else coming along and
> breaking it/causing some of these noinline/__always_inline invocations to make
> things far worse.

Who told you optimization isn't delicate?!

> 
> I also reserve the right to seriously rework this pile of crap software.
> 
> I'd rather we try to find less fragile ways to optimise!

But yes, I understand your point. Hm. I'll need to think about this some more.
There's nothing I would love more than to simply slap a

if (pte_batch_hint(ptep, pte) == 1)
	nr_ptes = 1;

on !contpte archs. I don't see where most of the wins for those architectures
would exist, but apparently they do. Confusing :/

> 
> Maybe there's some steps that are bigger wins than others?

Definitely :) So yeah I'll probably be dropping this patch, or at least
reworking this one a good bit.

-- 
Pedro