| Message ID | 20210729105215.2222338-2-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/sve: Delay freeing memory in fpsimd_flush_thread() | expand |
On Thu, Jul 29, 2021 at 12:52:14PM +0200, Sebastian Andrzej Siewior wrote: > fpsimd_flush_thread() invokes kfree() via sve_free() within a preempt disabled > section which is not working on -RT. > > Delay freeing of memory until preemption is enabled again. I'll leave this for Mark Brown to comment on, but it looks reasonable to me. Cheers ---Dave > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > arch/arm64/kernel/fpsimd.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index e57b23f952846..e098f6c67b1de 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -226,6 +226,16 @@ static void sve_free(struct task_struct *task) > __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; > +} > + > /* > * TIF_SVE controls whether a task can use SVE without trapping while > * in userspace, and also the way a task's FPSIMD/SVE state is stored > @@ -1033,6 +1043,7 @@ void fpsimd_thread_switch(struct task_struct *next) > void fpsimd_flush_thread(void) > { > int vl, supported_vl; > + void *mem = NULL; > > if (!system_supports_fpsimd()) > return; > @@ -1045,7 +1056,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. > @@ -1079,6 +1090,7 @@ void fpsimd_flush_thread(void) > } > > put_cpu_fpsimd_context(); > + kfree(mem); > } > > /* > -- > 2.32.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Jul 29, 2021 at 12:52:14PM +0200, Sebastian Andrzej Siewior wrote: > +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 has exactly one caller - why not just inline it there? It'd probably make it easier to follow what's going on.
On 2021-07-29 15:26:16 [+0100], Mark Brown wrote: > On Thu, Jul 29, 2021 at 12:52:14PM +0200, Sebastian Andrzej Siewior wrote: > > > +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 has exactly one caller - why not just inline it there? It'd > probably make it easier to follow what's going on. In case someone makes changes to the non-atomic version which should also happen here. But I can inline it. Sebastian
On Thu, Jul 29, 2021 at 04:39:04PM +0200, Sebastian Andrzej Siewior wrote: > On 2021-07-29 15:26:16 [+0100], Mark Brown wrote: > > On Thu, Jul 29, 2021 at 12:52:14PM +0200, Sebastian Andrzej Siewior wrote: > > > > > +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 has exactly one caller - why not just inline it there? It'd > > probably make it easier to follow what's going on. > > In case someone makes changes to the non-atomic version which should > also happen here. But I can inline it. Not critical, but you could drop a /* defer kfree() while in atomic context */ or similar in the code just to make it absolutely clear why sve_state is temporarily stashed rather than just being freed immediately. The whole function would be small enough that it's probably still reasonably obvious even without the comment, though. Cheers ---Dave
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index e57b23f952846..e098f6c67b1de 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -226,6 +226,16 @@ static void sve_free(struct task_struct *task) __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; +} + /* * TIF_SVE controls whether a task can use SVE without trapping while * in userspace, and also the way a task's FPSIMD/SVE state is stored @@ -1033,6 +1043,7 @@ void fpsimd_thread_switch(struct task_struct *next) void fpsimd_flush_thread(void) { int vl, supported_vl; + void *mem = NULL; if (!system_supports_fpsimd()) return; @@ -1045,7 +1056,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. @@ -1079,6 +1090,7 @@ void fpsimd_flush_thread(void) } put_cpu_fpsimd_context(); + kfree(mem); } /*
fpsimd_flush_thread() invokes kfree() via sve_free() within a preempt disabled section which is not working on -RT. Delay freeing of memory until preemption is enabled again. Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- arch/arm64/kernel/fpsimd.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)