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

[Dave Martin <Dave.Martin@arm.com>]

On Thu, Apr 25, 2019 at 06:12:59PM +0100, Julien Grall wrote:
> 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:

[...]

> >>>(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.

Thanks

[...]

> >>>>-/*
> >>>>- * 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.

OK -- my main worry here was that there might be a bug in mainline.

Adding protection where none existed previously raises the question of
whether we're papering over an existing bug, so it's good to mention in
the commit message if this is not the case.

> >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.

OK, thanks
---Dave