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

From: Julien Grall
Date: Tue Apr 23 2019 - 06:59:51 EST


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