Re: [PATCH RT v3] arm64: fpsimd: use preemp_disable in addition to local_bh_disable()
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
> > ---
> > 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
Re: [PATCH RT v3] arm64: fpsimd: use preemp_disable in addition to local_bh_disable()
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
> ---
> 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
Re: [PATCH RT v3] arm64: fpsimd: use preemp_disable in addition to local_bh_disable()
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
Re: [PATCH RT v3] arm64: fpsimd: use preemp_disable in addition to local_bh_disable()
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
[PATCH RT v3] arm64: fpsimd: use preemp_disable in addition to local_bh_disable()
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
---
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;
+}
/* 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);
