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

[Julien Grall]

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