[PATCH] arm64/sve: Make kernel FPU protection RT friendly

[Dave Martin]

On Thu, Jul 29, 2021 at 04:17:48PM +0200, Sebastian Andrzej Siewior wrote:
> On 2021-07-29 14:54:59 [+0100], Dave Martin wrote:
> > > index e098f6c67b1de..a208514bd69a9 100644
> > > --- a/arch/arm64/kernel/fpsimd.c
> > > +++ b/arch/arm64/kernel/fpsimd.c
> > > @@ -177,10 +177,19 @@ static void __get_cpu_fpsimd_context(void)
> > >   *
> > >   * The double-underscore version must only be called if you know the task
> > >   * can't be preempted.
> > > + *
> > > + * On RT kernels local_bh_disable() is not sufficient because it only
> > > + * serializes soft interrupt related sections via a local lock, but stays
> > > + * preemptible. Disabling preemption is the right choice here as bottom
> > > + * half processing is always in thread context on RT kernels so it
> > > + * implicitly prevents bottom half processing as well.
> > >   */
> > >  static void get_cpu_fpsimd_context(void)
> > >  {
> > > -	local_bh_disable();
> > > +	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> > > +		local_bh_disable();
> > > +	else
> > > +		preempt_disable();
> > 
> > Is this wrongly abstracted for RT?
> 
> No, we want to keep BH preemptible. Say your NAPI callback is busy for
> the next 200us and your RT task needs the CPU now.
> 
> > The requirement here is that the code should temporarily be
> > nonpreemptible by anything except hardirq context.
> 
> That is what I assumed.
> 
> > Having to do this conditional everywhere that is required feels fragile.
> > Is a similar thing needed anywhere else?
> 
> pssst. I wisper now so that the other don't hear us. If you look at
> arch/x86/include/asm/fpu/api.h and search for fpregs_lock() then you
> find the same pattern. Even some of the comments look similar. And
> please don't look up the original commit :)
> x86 restores the FPU registers on return to userland (not immediately on
> context switch) and requires the same kind of synchronisation/
> protection regarding other tasks and crypto in softirq. So it should be
> more the same thing that arm64 does here.

That rather suggests to me that it is worth factoring this and giving it
a name, precisely because irrespectively of CONFIG_PREEMPT_RT, we need to
make sure that to task swtich _and_ no bh runs on the same cpu.  The
problem seems to be that the local_bh_disable() API doesn't express the
difference between wanting to prevent local bh processing and wanting to
prevent local bh _and_ task switch.

So, could this be wrapped up and called something like:

preempt_and_local_bh_disable()
...
local_bh_and_preempt_enable()?

I do wonder whether there are other places making the same assumption
about the local_irq > local_bh > preempt hierarchy that have been
missed...

> > If bh (as a preempting context) doesn't exist on RT, then can't
> > local_bh_disable() just suppress all preemption up to but excluding
> > hardirq?  Would anything break?
> 
> Yes. A lot. Starting with spin_lock_bh() itself because it does:
> 	local_bh_disable();
> 	spin_lock()
> 
> and with disabled preemption you can't do spin_lock() and you have to
> because the owner may be preempted. The next thing is that kmalloc() and
> friends won't work in a local_bh_disable() section for the same reason.

Couldn't this be solved with a trylock loop that re-enables bh (and
preemption) on the sleeping path?  But that may still be trying to
achieve something that doesn't make sense given the goals of
PREEMPT_RT(?)

> The list goes on.
> 
> Sebastian

Cheers
---Dave