Messages in this thread Patch in this message | | | Subject | Re: [RFC PATCH] arm64/fpsimd: Don't disable softirq when touching FPSIMD/SVE state | From | Julien Grall <> | Date | Thu, 11 Apr 2019 16:58:41 +0100 |
| |
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...
> > Looks like you inferred what I meant later on anyway. > >> >>> >>>> + >>>> /* >>>> * Call __sve_free() directly only if you know task can't be scheduled >>>> * or preempted. >>>> @@ -213,11 +217,11 @@ static void sve_free(struct task_struct *task) >>>> * thread_struct is known to be up to date, when preparing to enter >>>> * userspace. >>>> * >>>> - * Softirqs (and preemption) must be disabled. >>>> + * Preemption must be disabled. >>> >>> [*] That's not enough: we need to be in kernel_neon_disable()... >>> _enable() (i.e., kernel_neon_busy needs to be true to prevent softirqs >>> messing with the FPSIMD state). >> >> How about not mentioning preemption at all and just say: >> >> "The fpsimd context should be acquired before hand". >> >> This would help if we ever decide to protect critical section differently. > > Yes, or even better, name the function used to do this (i.e., > kernel_neon_disable() or get_cpu_fpsimd_context() or whatever it's > called).
... would make the comments simpler because we would have only one possible case to care.
Cheers,
-- Julien Grall
| |