by Julien Grall

[permalink] [raw]

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

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

2019-04-23 11:25:53

by Dave Martin

[permalink] [raw]

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

On Tue, Apr 23, 2019 at 11:59:44AM +0100, Julien Grall wrote:
> 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:

[...]

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

OK, either is good for me.

Cheers
---Dave