On Thu, Sep 2, 2021 at 2:51 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> From: Vlastimil Babka <vbabka@xxxxxxx>
> Subject: mm, slab: make flush_slab() possible to call with irqs enabled
>
> Currently flush_slab() is always called with disabled IRQs if it's needed,
> but the following patches will change that, so add a parameter to control
> IRQ disabling within the function, which only protects the kmem_cache_cpu
> manipulation and not the call to deactivate_slab() which doesn't need it.
I absolutely DETEST this patch.
Code like this is just garbage:
> -static inline void flush_slab(struct kmem_cache *s, struct kmem_cache_cpu *c)
> +static inline void flush_slab(struct kmem_cache *s, struct kmem_cache_cpu *c,
> + bool lock)
....
> + if (lock)
> + local_irq_save(flags);
with actively misleading names ("lock"? WTF? There's no lock there!)
and just disgusting semantics with conditionals etc.
And the thing is, I don't even see that this would be in the least required.
If that function had just been split into two: the first part that
needed to absolutely be called with interrupts disabled, and the
latter part that didn't, then none of this stupid ugly code would be
needed at all.
So flush_slab() could have been split into "that first part that
returned the freelist and page, and then the second part that did the
deactivate_slab() and the stats update.
Then somebody who had interrupts disabled anyway (ie existing cases)
would just do both.
And then the new code that you want to have with interrupts disabled
just for the first part would do the first part with interrupts
disabled, and the second part without.
See? No need for a "flags" field and odd conditional interrupt disables.
And yes, I realize that the "inline" means that *most* of the time,
the compiler will inline things, the "conditional" will become static,
and it will not be a run-time conditional.
But it's the human-readable conditional that is the ugly part here.
Now any sane human that reads that flush_slab() code will see "oh,
sometimes interrupts are disabled, and sometimes they aren't".
Because that is what the code does, at that level.
But no, what's actually going on is that interrupts are *always*
disabled - it's just that sometimes they will have already been
disabled in the caller. So all that misleading and misnamed "lock"
argument does is really "were interrupts already disabled so that I
don't need to disable and re-enable them".
That's what the code actually *does*, but it's not how the code reads,
and it's not how the code is written, or how the argument in question
is named.
So it's all very misleading, and ugly.
I'm going to sleep on this, and maybe tomorrow morning my reaction
will be "whatever, the code probably works".
But more likely, tomorrow morning I will take another look, and still
say "no, this is actually making the code less maintainable and
actively misleading", and just throw the whole slab series out the
window.
Linus
