| Message ID | 20180726150634.cl3wccqur6qhle6p@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RT,v3] arm64: fpsimd: use preemp_disable in addition to local_bh_disable() | expand |
On Thu, 2018-07-26 at 17:06 +0200, Sebastian Andrzej Siewior wrote: > > @@ -1115,6 +1139,7 @@ void kernel_neon_begin(void) > > BUG_ON(!may_use_simd()); > > + preempt_disable(); > local_bh_disable(); > > __this_cpu_write(kernel_neon_busy, true); > @@ -1131,6 +1156,7 @@ void kernel_neon_begin(void) > preempt_disable(); Nit: this preempt_disable() could be removed... > local_bh_enable(); > + preempt_enable(); > } > EXPORT_SYMBOL(kernel_neon_begin); ...instead of adding this one. -Mike
On 2018-07-27 05:17:23 [+0200], Mike Galbraith wrote: > On Thu, 2018-07-26 at 17:06 +0200, Sebastian Andrzej Siewior wrote: > > > > @@ -1115,6 +1139,7 @@ void kernel_neon_begin(void) > > > > BUG_ON(!may_use_simd()); > > > > + preempt_disable(); > > local_bh_disable(); > > > > __this_cpu_write(kernel_neon_busy, true); > > @@ -1131,6 +1156,7 @@ void kernel_neon_begin(void) > > preempt_disable(); > > Nit: this preempt_disable() could be removed... > > > local_bh_enable(); > > + preempt_enable(); > > } > > EXPORT_SYMBOL(kernel_neon_begin); > > ...instead of adding this one. It could. I have currently no idea for the long term solution and this keeps track what is intended to do. It might get replaced with preempt_.*_rt()… > -Mike Sebastian
On Thu, Jul 26, 2018 at 05:06:34PM +0200, Sebastian Andrzej Siewior wrote: > In v4.16-RT I noticed a number of warnings from task_fpsimd_load(). The > code disables BH and expects that it is not preemptible. On -RT the > task remains preemptible but remains the same CPU. This may corrupt the > content of the SIMD registers if the task is preempted during > saving/restoring those registers. > > Add preempt_disable()/enable() to enfore the required semantic on -RT. Does this supersede the local_lock based approach? That would have been nice to have, but if there are open questions about how to do it then I guess something like this patch makes sense as a stopgap solution. > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > This should work. Compiling currently gcc-6 on the box to see what > happens. Since the crypto disables preemption "frequently" and I don't > expect or see anything to worry about. > > arch/arm64/kernel/fpsimd.c | 30 ++++++++++++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) > > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -157,6 +157,15 @@ static void sve_free(struct task_struct > __sve_free(task); > } > > +static void *sve_free_atomic(struct task_struct *task) > +{ > + void *sve_state = task->thread.sve_state; > + > + WARN_ON(test_tsk_thread_flag(task, TIF_SVE)); > + > + task->thread.sve_state = NULL; > + return sve_state; > +} This feels a bit excessive. Since there's only one call site, I'd prefer if the necessary code were simply inlined. We wouldn't need the WARN either in that case, since (IIUC) it's only there to check for accidental misuse of this helper. > /* Offset of FFR in the SVE register dump */ > static size_t sve_ffr_offset(int vl) > @@ -594,6 +603,7 @@ int sve_set_vector_length(struct task_st > * non-SVE thread. > */ > if (task == current) { > + preempt_disable(); > local_bh_disable(); > > task_fpsimd_save(); > @@ -604,8 +614,10 @@ int sve_set_vector_length(struct task_st > if (test_and_clear_tsk_thread_flag(task, TIF_SVE)) > sve_to_fpsimd(task); > > - if (task == current) > + if (task == current) { > local_bh_enable(); > + preempt_enable(); > + } > > /* > * Force reallocation of task SVE state to the correct size > @@ -837,6 +849,7 @@ asmlinkage void do_sve_acc(unsigned int > > sve_alloc(current); > > + preempt_disable(); > local_bh_disable(); I think we should have local helpers for the preempt+local_bh maintenance, since they're needed all over the place in this file. [...] Cheers ---Dave
On 2018-07-27 16:35:59 [+0100], Dave Martin wrote: > On Thu, Jul 26, 2018 at 05:06:34PM +0200, Sebastian Andrzej Siewior wrote: > > In v4.16-RT I noticed a number of warnings from task_fpsimd_load(). The > > code disables BH and expects that it is not preemptible. On -RT the > > task remains preemptible but remains the same CPU. This may corrupt the > > content of the SIMD registers if the task is preempted during > > saving/restoring those registers. > > > > Add preempt_disable()/enable() to enfore the required semantic on -RT. > > Does this supersede the local_lock based approach? Yes, it does. > That would have been nice to have, but if there are open questions about > how to do it then I guess something like this patch makes sense as a > stopgap solution. > > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > > --- > > This should work. Compiling currently gcc-6 on the box to see what > > happens. Since the crypto disables preemption "frequently" and I don't > > expect or see anything to worry about. > > > > arch/arm64/kernel/fpsimd.c | 30 ++++++++++++++++++++++++++++-- > > 1 file changed, 28 insertions(+), 2 deletions(-) > > > > --- a/arch/arm64/kernel/fpsimd.c > > +++ b/arch/arm64/kernel/fpsimd.c > > @@ -157,6 +157,15 @@ static void sve_free(struct task_struct > > __sve_free(task); > > } > > > > +static void *sve_free_atomic(struct task_struct *task) > > +{ > > + void *sve_state = task->thread.sve_state; > > + > > + WARN_ON(test_tsk_thread_flag(task, TIF_SVE)); > > + > > + task->thread.sve_state = NULL; > > + return sve_state; > > +} > > This feels a bit excessive. Since there's only one call site, I'd > prefer if the necessary code were simply inlined. We wouldn't need the > WARN either in that case, since (IIUC) it's only there to check for > accidental misuse of this helper. okay. > > /* Offset of FFR in the SVE register dump */ > > static size_t sve_ffr_offset(int vl) … > I think we should have local helpers for the preempt+local_bh > maintenance, since they're needed all over the place in this > file. okay. > Cheers > ---Dave Sebastian
--- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -157,6 +157,15 @@ static void sve_free(struct task_struct __sve_free(task); } +static void *sve_free_atomic(struct task_struct *task) +{ + void *sve_state = task->thread.sve_state; + + WARN_ON(test_tsk_thread_flag(task, TIF_SVE)); + + task->thread.sve_state = NULL; + return sve_state; +} /* Offset of FFR in the SVE register dump */ static size_t sve_ffr_offset(int vl) @@ -594,6 +603,7 @@ int sve_set_vector_length(struct task_st * non-SVE thread. */ if (task == current) { + preempt_disable(); local_bh_disable(); task_fpsimd_save(); @@ -604,8 +614,10 @@ int sve_set_vector_length(struct task_st if (test_and_clear_tsk_thread_flag(task, TIF_SVE)) sve_to_fpsimd(task); - if (task == current) + if (task == current) { local_bh_enable(); + preempt_enable(); + } /* * Force reallocation of task SVE state to the correct size @@ -837,6 +849,7 @@ asmlinkage void do_sve_acc(unsigned int sve_alloc(current); + preempt_disable(); local_bh_disable(); task_fpsimd_save(); @@ -850,6 +863,7 @@ asmlinkage void do_sve_acc(unsigned int WARN_ON(1); /* SVE access shouldn't have trapped */ local_bh_enable(); + preempt_enable(); } /* @@ -921,10 +935,12 @@ void fpsimd_thread_switch(struct task_st void fpsimd_flush_thread(void) { int vl, supported_vl; + void *mem = NULL; if (!system_supports_fpsimd()) return; + preempt_disable(); local_bh_disable(); memset(¤t->thread.fpsimd_state, 0, sizeof(struct fpsimd_state)); @@ -932,7 +948,7 @@ void fpsimd_flush_thread(void) if (system_supports_sve()) { clear_thread_flag(TIF_SVE); - sve_free(current); + mem = sve_free_atomic(current); /* * Reset the task vector length as required. @@ -968,6 +984,8 @@ void fpsimd_flush_thread(void) set_thread_flag(TIF_FOREIGN_FPSTATE); local_bh_enable(); + preempt_enable(); + kfree(mem); } /* @@ -979,9 +997,11 @@ void fpsimd_preserve_current_state(void) if (!system_supports_fpsimd()) return; + preempt_disable(); local_bh_disable(); task_fpsimd_save(); local_bh_enable(); + preempt_enable(); } /* @@ -1021,6 +1041,7 @@ void fpsimd_restore_current_state(void) if (!system_supports_fpsimd()) return; + preempt_disable(); local_bh_disable(); if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { @@ -1029,6 +1050,7 @@ void fpsimd_restore_current_state(void) } local_bh_enable(); + preempt_enable(); } /* @@ -1041,6 +1063,7 @@ void fpsimd_update_current_state(struct if (!system_supports_fpsimd()) return; + preempt_disable(); local_bh_disable(); current->thread.fpsimd_state.user_fpsimd = *state; @@ -1053,6 +1076,7 @@ void fpsimd_update_current_state(struct fpsimd_bind_to_cpu(); local_bh_enable(); + preempt_enable(); } /* @@ -1115,6 +1139,7 @@ void kernel_neon_begin(void) BUG_ON(!may_use_simd()); + preempt_disable(); local_bh_disable(); __this_cpu_write(kernel_neon_busy, true); @@ -1131,6 +1156,7 @@ void kernel_neon_begin(void) preempt_disable(); local_bh_enable(); + preempt_enable(); } EXPORT_SYMBOL(kernel_neon_begin);
In v4.16-RT I noticed a number of warnings from task_fpsimd_load(). The code disables BH and expects that it is not preemptible. On -RT the task remains preemptible but remains the same CPU. This may corrupt the content of the SIMD registers if the task is preempted during saving/restoring those registers. Add preempt_disable()/enable() to enfore the required semantic on -RT. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- This should work. Compiling currently gcc-6 on the box to see what happens. Since the crypto disables preemption "frequently" and I don't expect or see anything to worry about. arch/arm64/kernel/fpsimd.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)