Re: [RFC PATCH] arm64/fpsimd: Don't disable softirq when touching FPSIMD/SVE state
From: Julien Grall
Date: Thu Apr 11 2019 - 12:50:46 EST
Hi Dave,
On 4/11/19 5:34 PM, Dave Martin wrote:
On Thu, Apr 11, 2019 at 04:58:41PM +0100, Julien Grall wrote:
Hi Dave,
On 4/5/19 4:07 PM, Dave Martin wrote:
On Fri, Apr 05, 2019 at 10:02:45AM +0100, Julien Grall wrote:
+#ifdef CONFIG_KERNEL_MODE_NEON
+
/*
* may_use_simd - whether it is allowable at this time to issue SIMD
* instructions or access the SIMD register file
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 5ebe73b69961..b7e5dac26190 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -90,7 +90,8 @@
* To prevent this from racing with the manipulation of the task's FPSIMD state
* from task context and thereby corrupting the state, it is necessary to
* protect any manipulation of a task's fpsimd_state or TIF_FOREIGN_FPSTATE
- * flag with local_bh_disable() unless softirqs are already masked.
+ * flag with kernel_neon_{disable, enable}. This will still allow softirqs to
+ * run but prevent them to use FPSIMD.
*
* For a certain task, the sequence may look something like this:
* - the task gets scheduled in; if both the task's fpsimd_cpu field
@@ -142,6 +143,9 @@ extern void __percpu *efi_sve_state;
#endif /* ! CONFIG_ARM64_SVE */
+static void kernel_neon_disable(void);
+static void kernel_neon_enable(void);
I find these names a bit confusing: _disable() actualy enables FPSIMD/SVE
context access for the current context (i.e., makes it safe).
Since these also disable/enable preemption, perhaps we can align them
with the existing get_cpu()/put_cpu(), something like:
void get_cpu_fpsimd_context();
vold put_cpu_fpsimd_context();
If you consider it's worth adding the checking helper I alluded to
above, it could then be called something like:
bool have_cpu_fpsimd_context();
I am not sure where you suggested a checking helper above. Do you refer to
exposing kernel_neon_busy even for !CONFIG_KERNEL_MODE_NEON?
Hmmm, looks like I got my reply out of order here.
I meant the helper (if any) to check
!preemptible() && !__this_cpu_read(kernel_neon_busy).
I guess you are using && instead of || because some of the caller may not
call get_cpu_fpsimd_context() before but still disable preemption, right?
Wouldn't it be better to have all the user calling get_cpu_fpsimd_context()
and put_cpu_fpsimd_context()?
This has the advantage to uniformize how the way FPSIMD is protected and
also...
My expectation is that all users would have called
get_cpu_fpsimd_context().
This is not the case today (see kvm_arch_vcpu_put_fp), I will look at
protecting it with a call to get_cpu_fpsimd_context().
The reason for writing the check that way is that we can't meaningfully
inspect percpu variables unless we are non-preemptible already. The &&
means we don't do the percpu read at all is the case where preemptible()
is true.
I am not sure to understand why it would be necessary.
this_cpu_read(kernel_neon_busy) should be sufficient here.
If it is set then, preemption is disabled. Or are you worried about user
directly setting kernel_neon_busy instead of calling get_cpu_fpsimd_context?
Or do you think my logic is wrong somewhere? (It's possible...)
I think your logic would not return the correct value. We want
have_cpu_fpsimd_context() to return true if it is not preemptible and
kernel_neon_busy is true. So we would want:
!preemptible() && __this_cpu_read(kernel_neon_busy)
If we speak about the implementation of have_cpu_fpsimd_context(), then
we want:
!preemptible() && __this_cpu_read(kernel_neon_busy)
Cheers,
--
Julien Grall