On Wed, 17 May 2023 at 16:37, Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> wrote: > > 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. Right. > 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. > This depends on the type of algorithm (skciphers/aeads vs shashes/ahashes) but they generally all have a bounded quantum of work before yielding the NEON (and therefore the CPU). > > > 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 Actually, given your explanation above, I think this may not be necessary. Given that on !RT, disabling BH implies disabling preemption and on RT disabling preemption implies disabling BH, it should just be a matter of doing one or the other consistently, depending on IS_ENABLED(CONFIG_PREEMPT_RT) > - #2 disable BH followed by preemption in vfp_sync_hwstate() and > vfp_entry(). > Not sure what to do here, given my answer to #1 In most cases where the VFP code disables BH, it is either because an interrupting softirq may enable the SIMD unit and disable it again, which would result in a FP exception when the interrupted context tries to access the registers, or because such an interruption would corrupt the preserved/restored FP state if it occurs while such a preserve/restore is in progress. Maybe the commit log of patch 62b95a7b44d1a30b3a9 sheds some more light here. > if so, let me prepare something unless you want to :) > Don't let me stop you :-)
