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

[Lorenzo Stoakes (Oracle)]

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@xxxxxxx>
> ---
>  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?

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:

	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.

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

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!

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

Thanks, Lorenzo