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

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

On Fri, Mar 20, 2026 at 12:01:09PM +0100, David Hildenbrand (Arm) wrote:
> On 3/20/26 11:36, Lorenzo Stoakes (Oracle) wrote:
> > On Thu, Mar 19, 2026 at 10:41:54PM +0100, David Hildenbrand (Arm) wrote:
> >>
> >>>
> >>> 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?
> >>
> >> What we can do is, collect similar folio_pte_batch_*() variants and
> >> centralize them in mm/utils.c.
> >
> > I'm not sure that addresses any of the comments above?
>
> Well, the point is that you still end up with an optimized variant
> regarding flags, and you don't have to play games in this file with
> "inline".

Right, I mean maybe we're talking across purposes here, if the variants are say
generalised specific sets of behaviour rather than 'mprotect folio pte batch' or
'mremap folio pte batch' or whatever then that's fine.

>
> >
> > Putting logic specific to components of mm away from where those components
> > are and into mm/util.c seems like a complete regression in terms of
> > fragility and code separation.
>
> My point is that in mm/rmap.c we have another caller that passes
> FPB_RESPECT_WRITE | FPB_RESPECT_SOFT_DIRTY, that could use the same
> optimized function.
>
> For folio_pte_batch_flags() we primarily care about propagating the
> flags such that __pte_batch_clear_ignored() will become as short and
> small as possible.
>
> How much that matters in practice? For fork() and unmap() it was
> measurable. So I'd assume for mprotect() it would matter as well.

Yeah I think this was a misunderstanding, that's fine.

>
> >
> > And for what reason would you want to do that? To force a noinline of an
> > inline and people 'just have to know' that's why you randomly separated the
> > two?
>
> Again, I don't want the noinline. But providing a single optimized
> instance for two users that care about the same flags makes perfect
> sense to me.

Yup, that's fine! I agree with you now I realise this was a
misunderstanding :p

>
> >
> > Doesn't sound appealing overall.
> >
> > I'd rather we find a way to implement the batching such that it doesn't
> > exhibit bad inlining decisions in the first place.
> >
> > I mean mprotect_folio_batch() having !folio, !folio_test_large() checks
> > only there is already silly, we should have a _general_ function that does
> > optimisations like that.
> >
> > Isn't the issue rather than folio_pte_batch_flags() shouldn't be an inline
> > function in internal.h but rather a function in mm/util.c?
>
> No, we want specialized code for fork and zapping. That's the whole
> purpose: optimize out the flag checks in the loop.

Yeah, I think something isn't quite working right there though if we're
seeing measureable improvements by doing:

static noinline unsigned int blahdy_blah()
{
	...
	return folio_pte_batch_flags(...);
}

But we need to figure out exactly what via actual perf analysis, as perf is
an area that totally confounds developer expectation and 'going with your
gut' is a perfect way of doing completely the wrong thing.

(And I've repeatedly seen otherwise smart developers do this OVER and OVER
again, it's like a honey trap for the clever).

Not saying any of that applies to you or Pedro :) just a general point.

I don't _love_ that function being inline like that, but I will cede to the
data, but right now it seems, at least for I guess order-0 folios that it's
not quite doing what we want.

Anyway we're agreed the best way forwards here, for now, is to specialise
order-0 which just seems like an all-round win, and maybe the rest is not
as much of a contributor?

>
> >
> >>
> >> For
> >>
> >> 	nr_ptes = mprotect_folio_pte_batch(folio, pte, oldpte,
> >> 					   max_nr_ptes, /* flags = */ 0)
> >>
> >> We might just be able to use folio_pte_batch()?
> >
> > Yup.
> >
> >>
> >> For the other variant (soft-dirt+write) we'd have to create a helper like
> >>
> >> 	folio_pte_batch_sd_w() [better name suggestion welcome]
> >>
> >> That will reduce the code footprint overall I guess.
> >
> > I mean yeah that's a terrible name so obviously it'd have to be something
> > better.
> >
> > But again, this seems pretty stupid, now we're writing a bunch of duplicate
> > per-case code to force noinline because we made the central function inline
> > no?
> >
> > That's also super fragile, because an engineer might later decide that
> > pattern is horrible and fix it, and we regress this.
> >
> > But I mean overall, is the perf here really all that important? Are people
> > really that dependent on mprotect() et al. performing brilliantly fast?
>
> For basic primitives like mprotect/zap/fork I think yes. For other stuff
> like rmap.c, maybe not.

Well on big ranges of mprotect() it could be, and I know often databases
like to do this kind of thing potentially, so yeah sure.

But more so the microbenchmark stuff of *a million protect() invocations*
is not something to optimise for so much.

Rather I'd say mprotect() over larger ranges is what we should look to.

Fork of course is utterly critical despite fork being both a terrible
mistake and a great idea at the exact same time in OS development history
:)

>
> >
> > Couldn't we do this with any mm interface and end up making efforts that
> > degrade code quality, increase fragility for dubious benefit?
>
> I don't see a big issue here like you do.

As I've said to Pedro elsewhere here, I guess my concern is nuanced:

So if we introduce stuff like carefully chosen __always_inline or noinline
or other things that have characteristics like:

- They're beneficial for the code AS-IS.
- They're based on compiler codegen that can easily be altered by other
  changes.
- It is not obvious how other changes to the code might break them.

We are asking for trouble - because people WILL change that code and WILL
break that, OR a possibly worse outcome - something like a noinline sticks
around when it makes sense, but everybody's scared to remove it + _doesn't
know why it's there_ - so it becomes a part of 'oh yeah we don't touch
that' lore that exists for a lot of 'weird' stuff in the kernel.

Then it might end up actually _worsening_ the performance in future
accidentally because nobody dare touch it.

Or another hellish future is one in which such things cause bot perf
regression reports for otherwise fine series, on microoptimisations we're
not even clear matter, and cause developers to have to spend hours figuring
out how to avoid them, meanwhile potentially making it even more difficult
to understand why the code is the way it is.

So what is the solution?

1. Focus on the changes that are NOT brittle like this, e.g. special casing
   order-0 is fine, adding profile/benchmark-proven likely()/unlikely(),
   etc. - these are not things that have the above characteristics and are
   just wins.

2. For cases where things MIGHT have the characteristics listed above,
   avoid the issue by abstracting it as much as possible, adding lengthily
   comments and making it as hard as possible to screw it up/misunderstand
   it.

3. Often times perf issues coming up might be an indication that the
   underlying mechanism is itself not well abstracted/already adding
   unnecessary complexity that manifests in perf issues, so in that case -
   rework first.

mm/mprotect.c is a good example of case 3 I think in that it's a big ball
of mud with overly long functions (e.g. change_pte_range(),
do_mprotect_pkey()) that likely refactoring code actually see perf gains
just from the compiler not having to heuristically determine inlining/other
optimisations due to functions being in smalle rchunks.

Anwyay in this case, we can pull out an example of approach 3 (the softleaf
specialisation), and approach 1 (order-0) handling easily, and defer
considering taking approach 2 if it makes sense to later, if we get most of
the wins by doing the former 2 things.

>
> --
> Cheers,
>
> David

Cheers, Lorenzo