Re: [RFC PATCH] arm64/fpsimd: Don't disable softirq when touching FPSIMD/SVE state

[Julien Grall <julien.grall@arm.com>]

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