Messages in this thread | | | Subject | Re: [PATCH v2 3/3] arm64/fpsimd: Don't disable softirq when touching FPSIMD/SVE state | From | Julien Grall <> | Date | Tue, 23 Apr 2019 11:59:44 +0100 |
| |
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
| |