Date:   Tue, 23 Apr 2019 11:59:44 +0100
From:   Julien Grall <julien.grall@....com>
To:     Dave Martin <Dave.Martin@....com>
Cc:     julien.thierry@....com, marc.zyngier@....com,
        catalin.marinas@....com, ard.biesheuvel@...aro.org,
        will.deacon@....com, linux-kernel@...r.kernel.org,
        christoffer.dall@....com, james.morse@....com,
        suzuki.poulose@....com, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 3/3] arm64/fpsimd: Don't disable softirq when touching
 FPSIMD/SVE state

Hi Dave,

On 4/17/19 3:01 PM, Dave Martin wrote:
> On Wed, Apr 17, 2019 at 12:37:57PM +0100, Julien Grall wrote:
>> Hi Dave,
>>
>> On 16/04/2019 13:30, Dave Martin wrote:
>>> On Fri, Apr 12, 2019 at 06:14:20PM +0100, Julien Grall wrote:
> 
> [...]
> 
>>>> +
>>>> +/*
>>>> + * Obtain the CPU FPSIMD context for use by the calling context.
> 
> If we say something like "Claim ownership of the CPU FPSIMD context for
> use by the calling context", this may be easier to reference elsewhere
> (see below).
> 
>>>> + *
>>>> + * The caller may freely modify FPSIMD context until *put_cpu_fpsimd_context()
>>>
>>> Nit: Why *?  This makes it look a bit like get_cpu_fpsimd_context()
>>> returns a pointer and you're saying something about dereferencing that
>>> pointer here.
>>
>> I tend to use * for wildcard. In this context it used to refers to both the
>> double-underscored version and the one without.
> 
> Ah, right.  * makes sense in general, but here it confused me.
> 
>> I can use {,__}put_cpu_fpsimd_context() instead.
> 
> Maybe, but the caller is not supposed to pair get_cpu_fpsimd_context()
> with __put_cpu_fpsimd_context().
> 
> What if we just comment the non-underscore versions.  The purpose of
> the __ versions is then relatively obvious without additional commenting.

I am happy with this solution as well.

[...]

> 
>>>
>>>> +	{
>>>> +		printk("preemptible() = %u kernel_neon_busy = %u\n",
>>>> +		       preemptible(), __this_cpu_read(kernel_neon_busy));
>>>> +		while (1);
>>>> +	}
>>>>   	if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
>>>>   		if (system_supports_sve() && test_thread_flag(TIF_SVE)) {
>>>> @@ -352,7 +413,8 @@ static int __init sve_sysctl_init(void) { return 0; }
>>>>    * task->thread.sve_state.
>>>>    *
>>>>    * Task can be a non-runnable task, or current.  In the latter case,
>>>> - * softirqs (and preemption) must be disabled.
>>>> + * the FPSIMD context must be acquired with get_fpu_fpsimd_context()
>>>> + * before calling this function.
>>
>> I noticed you didn't comment about the usage of get_cpu_fpsimd_context here.
>> Do you want to add a WARN(..) in the function, or just using {,___} here?
> 
> Partly because of things getting repetitive.
> 
> WARN() is not free, so I suggest we don't add any new ones for now.
> 
>  From a comment point of view, if we just say something "the caller must
> have ownership of the cpu FPSIMD context" to refer back to the
> commenting for get_cpu_fpsimd_context(), that should cover the general
> case.

I am fine with this suggestion.

>>
>>>>    * task->thread.sve_state must point to at least sve_state_size(task)
>>>>    * bytes of allocated kernel memory.
>>>>    * task->thread.uw.fpsimd_state must be up to date before calling this
>>>> @@ -379,7 +441,8 @@ static void fpsimd_to_sve(struct task_struct *task)
>>>>    * task->thread.uw.fpsimd_state.
>>>>    *
>>>>    * Task can be a non-runnable task, or current.  In the latter case,
>>>> - * softirqs (and preemption) must be disabled.
>>>> + * the FPSIMD context must be acquired with get_fpu_fpsimd_context()
>>>> + * before calling this function.
>>
>> Same question here.
>>
>> [...]
>>
>>>> @@ -1012,7 +1079,8 @@ void fpsimd_signal_preserve_current_state(void)
>>>>   /*
>>>>    * Associate current's FPSIMD context with this cpu
>>>> - * Preemption must be disabled when calling this function.
>>>> + * The FPSIMD context should be acquired with get_cpu_fpsimd_context()
>>>> + * before calling this function.
>>
>> Same question here.
> 
> Same for these.
> 
>>>>   /*
>>>>    * Invalidate any task's FPSIMD state that is present on this cpu.
>>>> - * This function must be called with softirqs disabled.
>>>> + * The FPSIMD context should be acquired with get_cpu_fpsimd_context()
>>>> + * before calling this function.
>>>>    */
>>>>   static void fpsimd_flush_cpu_state(void)
>>>>   {
>>>> @@ -1125,19 +1194,18 @@ static void fpsimd_flush_cpu_state(void)
>>>>   /*
>>>>    * Save the FPSIMD state to memory and invalidate cpu view.
>>>> - * This function must be called with softirqs (and preemption) disabled.
>>>> + * This function must be called with preemption disabled.
>>>>    */
>>>>   void fpsimd_save_and_flush_cpu_state(void)
>>>>   {
>>>> +	__get_cpu_fpsimd_context();
>>>>   	fpsimd_save();
>>>>   	fpsimd_flush_cpu_state();
>>>> +	__put_cpu_fpsimd_context();
>>>
>>> It may be cleaner to avoid the assumption about preemption already being
>>> disabled here.  fpsimd_thread_switch() is rather a special case, but for
>>> this one is this really used on a hot path that justifies the assumption?
>>
>> It is currently only called with preemption disabled. So I thought it would
>> be better to avoid disabling preemption again. But I am happy to use the
>> non-__ version if you think it is better.
> 
> Hmmm, this works either way.  Since this is not fast-path and has an
> external caller, it might be worth adding a WARN_ON(preemptible()) here
> if you want to stick with the __ functions.

As it is not a fastpath, I will use the non-underscore version.

Cheers,

-- 
Julien Grall

Powered by blists - more mailing lists