Message-ID: <971994b0-052e-6a93-3949-0b05d2b558e5@arm.com>
Date:   Thu, 25 Apr 2019 18:12:59 +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 v3 3/3] arm64/fpsimd: Don't disable softirq when touching
 FPSIMD/SVE state

Hi Dave,

On 25/04/2019 17:39, Dave Martin wrote:
> On Thu, Apr 25, 2019 at 04:57:26PM +0100, Julien Grall wrote:
>> Hi Dave,
>>
>> On 24/04/2019 14:17, Dave Martin wrote:
>>> On Tue, Apr 23, 2019 at 02:57:19PM +0100, Julien Grall wrote:
>>>> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
>>>> index 5313aa257be6..6168d06bbd20 100644
>>>> --- a/arch/arm64/kernel/fpsimd.c
>>>> +++ b/arch/arm64/kernel/fpsimd.c
>>>> @@ -92,7 +92,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 {, __}get_cpu_fpsimd_context(). 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
>>>> @@ -155,6 +156,56 @@ extern void __percpu *efi_sve_state;
>>>>   #endif /* ! CONFIG_ARM64_SVE */
>>>> +DEFINE_PER_CPU(bool, fpsimd_context_busy);
>>>> +EXPORT_PER_CPU_SYMBOL(fpsimd_context_busy);
>>>> +
>>>> +static void __get_cpu_fpsimd_context(void)
>>>> +{
>>>> +	bool busy = __this_cpu_xchg(fpsimd_context_busy, true);
>>>> +
>>>> +	WARN_ON(busy);
>>>> +}
>>>> +
>>>> +/*
>>>> + * Claim ownership of the CPU FPSIMD context for use by the calling context.
>>>> + *
>>>> + * The caller may freely modify FPSIMD context until *put_cpu_fpsimd_context()
>>>> + * is called.
>>>
>>> Nit: it may be better to say "freely manipulate the FPSIMD context
>>> metadata".
>>>
>>> get_cpu_fpsimd_context() isn't enough to allow the FPSIMD regs to be
>>> safely trashed, because they may still contain live data (or an up to
>>> date copy) for some task.
>>
>> Good point, I will update the comment.
>>
>>>
>>> (For that you also need fpsimd_save_and_flush_cpu_state(), or just use
>>> kernel_neon_begin() instead.)
>>>
>>> [...]
>>>
>>>> @@ -922,6 +971,8 @@ void fpsimd_thread_switch(struct task_struct *next)
>>>>   	if (!system_supports_fpsimd())
>>>>   		return;
>>>> +	__get_cpu_fpsimd_context();
>>>> +
>>>>   	/* Save unsaved fpsimd state, if any: */
>>>>   	fpsimd_save();
>>>> @@ -936,6 +987,8 @@ void fpsimd_thread_switch(struct task_struct *next)
>>>>   	update_tsk_thread_flag(next, TIF_FOREIGN_FPSTATE,
>>>>   			       wrong_task || wrong_cpu);
>>>> +
>>>> +	__put_cpu_fpsimd_context();
>>>
>>> There should be a note in the commit message explaining why these are
>>> here.
>>>
>>> Are they actually needed, other than to keep
>>> WARN_ON(have_cpu_fpsimd_context()) happy elsewhere?
>>
>> It depends on how fpsimd_thread_switch() is called. I will answer more below.
>>
>>>
>>> Does PREEMPT_RT allow non-threaded softirqs to execute while we're in
>>> this code?
>>
>> This has nothing to do with PREEMPT_RT. Softirqs might be executed after
>> handling interrupt (see irq_exit()).
>>
>> A call to preempt_disable() will not be enough to prevent softirqs, you
>> actually need to either mask interrupts or have BH disabled.
>>
>> fpsimd_thread_switch() seems to be only called from the context switch code.
>> AFAICT, interrupt will be masked. Therefore, holding the FPSIMD CPU is not
>> necessary. However...
>>
>>>
>>>
>>> OTOH, if the overall effect on performance remains positive, we can
>>> probably argue that these operations make the code more self-describing
>>> and help guard against mistakes during future maintanence, even if
>>> they're not strictly needed today.
>>
>> .... I think it would help guard against mistakes. The more I haven't seen
>> any performance impact in the benchmark.
> 
> Which generally seems a good thing.  The commit message should explain
> that these are being added for hygiene rather than necessity here,
> though.

I will update the commit message.

> 
>> [...]
>>
>>>> -/*
>>>> - * Save the FPSIMD state to memory and invalidate cpu view.
>>>> - * This function must be called with softirqs (and preemption) disabled.
>>>> - */
>>>> +/* Save the FPSIMD state to memory and invalidate cpu view. */
>>>>   void fpsimd_save_and_flush_cpu_state(void)
>>>>   {
>>>> +	get_cpu_fpsimd_context();
>>>>   	fpsimd_save();
>>>>   	fpsimd_flush_cpu_state();
>>>> +	put_cpu_fpsimd_context();
>>>>   }
>>>
>>> Again, are these added just to keep WARN_ON()s happy?
>>
>> !preemptible() is not sufficient to prevent softirq running. You also need
>> to have either interrupt masked or BH disabled.
> 
> So, why was the code safe before this series?  (In fact, _was_ it safe?)
> 
> AFAICT, we have local_irq_disable() around context switch, which covers
> preempt notifiers (where kvm_arch_vcpu_put_fp() gets called) and
> fpsimd_thread_switch(): this is what prevents softirqs from firing.

That's correct, both callers of the function will have IRQs masked. Also, the 
function fpsimd_save() contained:
     WARN_ON(!in_softirq() && !irqs_disabled());

So we were covered in case of misuse.

> 
> So, while it's clean to have get/put here, I still don't see why they're
> required.
> 
> I think the arguments are basically similar to fpsimd_thread_switch().
> Since fpsimd_save_and_flush_cpu_state() and fpsimd_thread_switch() are
> called from similar contexts, is makes sense to keep them aligned.

Yes, this is for hygiene rather than a real bug (thought the WARN_ON() in 
fpsimd_save() would fire). I will update the message accordingly.

Cheers,

-- 
Julien Grall

Powered by blists - more mailing lists