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

[Sebastian Andrzej Siewior]

On 2021-07-29 16:34:22 [+0100], Dave Martin wrote:
> 
> 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()?

We don't disable both on RT. It is preemption on RT and BH + implicit
preemption on non-RT.
The difference is that on RT a softirq may have been preempted and in
this case get_cpu_fpsimd_context() won't force its completion. However
since get_cpu_fpsimd_context() disables preemption on RT it won't be
preempted in the section where the SIMD registers are modified. And the
softirq does not run on HARDIRQ-exit but in task context so it is okay.

But I get what you mean. I'm just not sure regarding the naming since
the code behaves the same on x86 and arm64 here.

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

Based on memory we had a few cases of those while cleaning up
in_atomic(), in_softirq() and friends.

> > > 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(?)

What about
 spin_lock_bh(a);
 spin_lock_bh(b);

? And then still you can't kmalloc() in a spin_lock_bh() section if you
disable preemption as part of local_bh_disable( if you disable
preemption as part of local_bh_disable()).

> Cheers
> ---Dave

Sebastian