Re: Preemp-RT kernel builds from 6.3 on fails on Xilinx Zynq 7Z010

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2023-05-17 13:05:32 [+0200], Ard Biesheuvel wrote:
> For context, these changes give a substantial performance boost on the
> RX path to IPsec or WireGuard using SIMD based software crypto.
> 
> > …
> > > Please take a look, and confirm whether or not we still need to drop
> > > the get_cpu() call from vfp_sync_hwstate()
> >
> > Yes, it needs to be dealt with. At the time vfp_sync_hwstate() the
> > context is fully preemptible. So that get_cpu() needs to be removed
> > before local_bh_disable(). But…
> >
> > If I understand the logic right, then the VFP state is saved on context
> > switch and the FPU access is disabled but the content remains and is in
> > sync with vfp_current_hw_state. Upon first usage (after the context
> > switch) in userland there is an exception at which point the access to
> > the FPU is enabled (FPEXC_EX) and the FPU content is loaded unless it is
> > in sync as per vfp_current_hw_state.
> >
> 
> Exactly. On UP we lazily preserve the user space FP state, and on SMP
> we always preserve it on a context switch. Then, only when user space
> actually tries to use the SIMD context, the correct data is copied in.
> That way, we don't take the performance hit for tasks that are blocked
> in the kernel.
> 
> > That means it relies on disabled preemption while operating on
> > vfp_current_hw_state.
> 
> In general, this code relies on preserving/restoring the VFP context
> to/from memory to be a critical section, as it needs to run to
> completion once it is started.
> 
> > On PREEMPT_RT softirqs are only handled in process
> > context (for instance at the end of the threaded interrupt). Therefore
> > it is sufficient to disable preemption instead of BH. It would need
> > something like x86's fpregs_lock().
> 
> I think this is not the only issue, to be honest. We cannot preempt
> tasks while they are using the SIMD unit in kernel mode, as the kernel
> mode context is never preserved/restored. So we at least need to
> disable preemption again in kernel_neon_begin() [which now relies on
> BH disable to disable preemption but as you point out, that is only
> the case on !RT]
> 
> Given this, I wonder whether supporting kernel mode NEON on PREEMPT_RT
> is worth it to begin with. AIUI, the alternative is to disable both
> preemption and BH when touching the VFP state.

Only preemption on RT. So it is debatable if it makes sense to use NEON
on RT. The preempt-off section is limited to PAGE_SIZE due the scatter/
gather walk if I remember correctly.

> > Otherwise vfp_entry() can get preempted after decisions based on
> > vfp_current_hw_state have been made. Also kernel_neon_begin() could get
> > preempted after enabling FPU. I think based on current logic, after
> > kernel_neon_begin() a task can get preempted on PREEMPT_RT and since
> > vfp_current_hw_state is NULL the registers won't be saved and a zero set
> > of registers will be loaded once the neon task gets back on the CPU (it
> > seems that VFP exceptions in kernel mode are treated the same way as
> > those in user mode).
> >
> > What do you think?
> >
> 
> Indeed. kernel_neon_begin() no longer disabling preemption is
> definitely wrong on PREEMPT_RT. The question is whether we want to
> untangle this or just make PREEMPT_RT and KERNEL_MODE_NEON mutually
> exclusive. This, by itself, makes quite a lot of sense, actually,
> given that on actual 32-bit hardware, the SIMD speedup is not that
> spectacular (the AES and other crypto instructions, which do give an
> order of magnitude speedup are only implemented on 64-bit cores, which
> usually run a 64-bit kernel). So if real-time behavior is a
> requirement, using ordinary crypto implemented in C (which does not
> require preemption to be disabled) is likely preferred anyway.

I have no opinion which can be backed up by numbers. So I have no
problem to go along with it and disable KERNEL_MODE_NEON since it is
simpler and the benefit is questionable.

So patch
- #1 KERNEL_MODE_NEON depends on !RT
- #2 disable BH followed by preemption in vfp_sync_hwstate() and
  vfp_entry().

if so, let me prepare something unless you want to :)

Sebastian