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

["David Hildenbrand (Arm)" <david@kernel.org>]

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

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

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

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

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

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

-- 
Cheers,

David