lkml.org 
[lkml]   [2019]   [Apr]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH] arm64/fpsimd: Don't disable softirq when touching FPSIMD/SVE state
From
Date
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

\
 
 \ /
  Last update: 2019-04-11 18:51    [W:0.229 / U:0.772 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site