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

[David Hildenbrand (Arm)]

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

I tend to agree (and I think I made a similar point in previous
discussions around mprotect() performance).

There is the use case for userspace jits etc to call mprotect() on
individual pages. I suspected that TLB flushing and syscall overhead
would overshadow most micro-optimizations. :)

[...]

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

Agreed.

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

Agreed.

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


I think the usage of noinline for micro-performance optimization is
really questionable and should be avoided at all costs.

The folio_pte_patch() stuff likely really should just be a set of
mm/util.c helpers that specialize on the flags only to make the inner
loop as efficient as possible.

-- 
Cheers,

David