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

[Dave Martin]

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.

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

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.

> >Now I look at the diff, I think after all that
> >
> >	WARN_ON(preemptible());
> >	__get_cpu_fpsimd_context();
> >
> >	...
> >
> >	__put_cpu_fpsimd_context();
> >
> >is preferable.  The purpose of this function is to free up the FPSIMD
> >regs for use by the kernel, so it makes no sense to call it with
> >preemption enabled: the regs could spontaneously become live again due
> >to a context switch.  So we shouldn't encourage misuse by making the
> >function "safe" to call with preemption enabled.
> 
> Ok, I will switch back to the underscore version and add a WARN_ON(...).

Thanks.

[...]

Cheers
---Dave