arm64/sve: Two PREEMPT_RT related arm64 fixes.

arm64/sve: Two PREEMPT_RT related arm64 fixes.

[Sebastian Andrzej Siewior]

Those two popped up while testing RT on arm64.

Sebastian



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] arm64/sve: Delay freeing memory in fpsimd_flush_thread()

[Sebastian Andrzej Siewior]

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(-)

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

Re: [PATCH] arm64/sve: Delay freeing memory in fpsimd_flush_thread()

[Dave Martin]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/sve: Delay freeing memory in fpsimd_flush_thread()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 423 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/sve: Delay freeing memory in fpsimd_flush_thread()

[Sebastian Andrzej Siewior]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/sve: Delay freeing memory in fpsimd_flush_thread()

[Dave Martin]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] arm64/sve: Make kernel FPU protection RT friendly

[Sebastian Andrzej Siewior]

Non RT kernels need to protect FPU against preemption and bottom half
processing. This is achieved by disabling bottom halves via
local_bh_disable() which implictly disables preemption.

On RT kernels this protection mechanism is not sufficient because
local_bh_disable() does not disable preemption. It serializes bottom half
related processing via a CPU local lock.

As bottom halves are running always in thread context on RT kernels
disabling preemption is the proper choice as it implicitly prevents bottom
half processing.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/arm64/kernel/fpsimd.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index e098f6c67b1de..a208514bd69a9 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -177,10 +177,19 @@ static void __get_cpu_fpsimd_context(void)
  *
  * The double-underscore version must only be called if you know the task
  * can't be preempted.
+ *
+ * On RT kernels local_bh_disable() is not sufficient because it only
+ * serializes soft interrupt related sections via a local lock, but stays
+ * preemptible. Disabling preemption is the right choice here as bottom
+ * half processing is always in thread context on RT kernels so it
+ * implicitly prevents bottom half processing as well.
  */
 static void get_cpu_fpsimd_context(void)
 {
-	local_bh_disable();
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		local_bh_disable();
+	else
+		preempt_disable();
 	__get_cpu_fpsimd_context();
 }
 
@@ -201,7 +210,10 @@ static void __put_cpu_fpsimd_context(void)
 static void put_cpu_fpsimd_context(void)
 {
 	__put_cpu_fpsimd_context();
-	local_bh_enable();
+	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
+		local_bh_enable();
+	else
+		preempt_enable();
 }
 
 static bool have_cpu_fpsimd_context(void)
-- 
2.32.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/sve: Make kernel FPU protection RT friendly

[Dave Martin]

On Thu, Jul 29, 2021 at 12:52:15PM +0200, Sebastian Andrzej Siewior wrote:
> Non RT kernels need to protect FPU against preemption and bottom half
> processing. This is achieved by disabling bottom halves via
> local_bh_disable() which implictly disables preemption.
> 
> On RT kernels this protection mechanism is not sufficient because
> local_bh_disable() does not disable preemption. It serializes bottom half
> related processing via a CPU local lock.
> 
> As bottom halves are running always in thread context on RT kernels
> disabling preemption is the proper choice as it implicitly prevents bottom
> half processing.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  arch/arm64/kernel/fpsimd.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index e098f6c67b1de..a208514bd69a9 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -177,10 +177,19 @@ static void __get_cpu_fpsimd_context(void)
>   *
>   * The double-underscore version must only be called if you know the task
>   * can't be preempted.
> + *
> + * On RT kernels local_bh_disable() is not sufficient because it only
> + * serializes soft interrupt related sections via a local lock, but stays
> + * preemptible. Disabling preemption is the right choice here as bottom
> + * half processing is always in thread context on RT kernels so it
> + * implicitly prevents bottom half processing as well.
>   */
>  static void get_cpu_fpsimd_context(void)
>  {
> -	local_bh_disable();
> +	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> +		local_bh_disable();
> +	else
> +		preempt_disable();

Is this wrongly abstracted for RT?

The requirement here is that the code should temporarily be
nonpreemptible by anything except hardirq context.

Having to do this conditional everywhere that is required feels fragile.
Is a similar thing needed anywhere else?

If bh (as a preempting context) doesn't exist on RT, then can't
local_bh_disable() just suppress all preemption up to but excluding
hardirq?  Would anything break?

[...]

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/sve: Make kernel FPU protection RT friendly

[Sebastian Andrzej Siewior]

On 2021-07-29 14:54:59 [+0100], Dave Martin wrote:
> > index e098f6c67b1de..a208514bd69a9 100644
> > --- a/arch/arm64/kernel/fpsimd.c
> > +++ b/arch/arm64/kernel/fpsimd.c
> > @@ -177,10 +177,19 @@ static void __get_cpu_fpsimd_context(void)
> >   *
> >   * The double-underscore version must only be called if you know the task
> >   * can't be preempted.
> > + *
> > + * On RT kernels local_bh_disable() is not sufficient because it only
> > + * serializes soft interrupt related sections via a local lock, but stays
> > + * preemptible. Disabling preemption is the right choice here as bottom
> > + * half processing is always in thread context on RT kernels so it
> > + * implicitly prevents bottom half processing as well.
> >   */
> >  static void get_cpu_fpsimd_context(void)
> >  {
> > -	local_bh_disable();
> > +	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> > +		local_bh_disable();
> > +	else
> > +		preempt_disable();
> 
> Is this wrongly abstracted for RT?

No, we want to keep BH preemptible. Say your NAPI callback is busy for
the next 200us and your RT task needs the CPU now.

> The requirement here is that the code should temporarily be
> nonpreemptible by anything except hardirq context.

That is what I assumed.

> Having to do this conditional everywhere that is required feels fragile.
> Is a similar thing needed anywhere else?

pssst. I wisper now so that the other don't hear us. If you look at
arch/x86/include/asm/fpu/api.h and search for fpregs_lock() then you
find the same pattern. Even some of the comments look similar. And
please don't look up the original commit :)
x86 restores the FPU registers on return to userland (not immediately on
context switch) and requires the same kind of synchronisation/
protection regarding other tasks and crypto in softirq. So it should be
more the same thing that arm64 does here.

> If bh (as a preempting context) doesn't exist on RT, then can't
> local_bh_disable() just suppress all preemption up to but excluding
> hardirq?  Would anything break?

Yes. A lot. Starting with spin_lock_bh() itself because it does:
	local_bh_disable();
	spin_lock()

and with disabled preemption you can't do spin_lock() and you have to
because the owner may be preempted. The next thing is that kmalloc() and
friends won't work in a local_bh_disable() section for the same reason.
The list goes on.

> [...]
> 
> Cheers
> ---Dave

Sebastian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/sve: Make kernel FPU protection RT friendly

[Dave Martin]

On Thu, Jul 29, 2021 at 04:17:48PM +0200, Sebastian Andrzej Siewior wrote:
> On 2021-07-29 14:54:59 [+0100], Dave Martin wrote:
> > > index e098f6c67b1de..a208514bd69a9 100644
> > > --- a/arch/arm64/kernel/fpsimd.c
> > > +++ b/arch/arm64/kernel/fpsimd.c
> > > @@ -177,10 +177,19 @@ static void __get_cpu_fpsimd_context(void)
> > >   *
> > >   * The double-underscore version must only be called if you know the task
> > >   * can't be preempted.
> > > + *
> > > + * On RT kernels local_bh_disable() is not sufficient because it only
> > > + * serializes soft interrupt related sections via a local lock, but stays
> > > + * preemptible. Disabling preemption is the right choice here as bottom
> > > + * half processing is always in thread context on RT kernels so it
> > > + * implicitly prevents bottom half processing as well.
> > >   */
> > >  static void get_cpu_fpsimd_context(void)
> > >  {
> > > -	local_bh_disable();
> > > +	if (!IS_ENABLED(CONFIG_PREEMPT_RT))
> > > +		local_bh_disable();
> > > +	else
> > > +		preempt_disable();
> > 
> > Is this wrongly abstracted for RT?
> 
> No, we want to keep BH preemptible. Say your NAPI callback is busy for
> the next 200us and your RT task needs the CPU now.
> 
> > The requirement here is that the code should temporarily be
> > nonpreemptible by anything except hardirq context.
> 
> That is what I assumed.
> 
> > Having to do this conditional everywhere that is required feels fragile.
> > Is a similar thing needed anywhere else?
> 
> pssst. I wisper now so that the other don't hear us. If you look at
> arch/x86/include/asm/fpu/api.h and search for fpregs_lock() then you
> find the same pattern. Even some of the comments look similar. And
> please don't look up the original commit :)
> x86 restores the FPU registers on return to userland (not immediately on
> context switch) and requires the same kind of synchronisation/
> protection regarding other tasks and crypto in softirq. So it should be
> more the same thing that arm64 does here.

That rather suggests to me that it is worth factoring this and giving it
a name, precisely because irrespectively of CONFIG_PREEMPT_RT, we need to
make sure that to task swtich _and_ no bh runs on the same cpu.  The
problem seems to be that the local_bh_disable() API doesn't express the
difference between wanting to prevent local bh processing and wanting to
prevent local bh _and_ task switch.

So, could this be wrapped up and called something like:

preempt_and_local_bh_disable()
...
local_bh_and_preempt_enable()?

I do wonder whether there are other places making the same assumption
about the local_irq > local_bh > preempt hierarchy that have been
missed...

> > If bh (as a preempting context) doesn't exist on RT, then can't
> > local_bh_disable() just suppress all preemption up to but excluding
> > hardirq?  Would anything break?
> 
> Yes. A lot. Starting with spin_lock_bh() itself because it does:
> 	local_bh_disable();
> 	spin_lock()
> 
> and with disabled preemption you can't do spin_lock() and you have to
> because the owner may be preempted. The next thing is that kmalloc() and
> friends won't work in a local_bh_disable() section for the same reason.

Couldn't this be solved with a trylock loop that re-enables bh (and
preemption) on the sleeping path?  But that may still be trying to
achieve something that doesn't make sense given the goals of
PREEMPT_RT(?)

> The list goes on.
> 
> Sebastian

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/sve: Make kernel FPU protection RT friendly

[Sebastian Andrzej Siewior]

On 2021-07-29 16:34:22 [+0100], Dave Martin wrote:
> 
> That rather suggests to me that it is worth factoring this and giving it
> a name, precisely because irrespectively of CONFIG_PREEMPT_RT, we need to
> make sure that to task swtich _and_ no bh runs on the same cpu.  The
> problem seems to be that the local_bh_disable() API doesn't express the
> difference between wanting to prevent local bh processing and wanting to
> prevent local bh _and_ task switch.
> 
> So, could this be wrapped up and called something like:
> 
> preempt_and_local_bh_disable()
> ...
> local_bh_and_preempt_enable()?

We don't disable both on RT. It is preemption on RT and BH + implicit
preemption on non-RT.
The difference is that on RT a softirq may have been preempted and in
this case get_cpu_fpsimd_context() won't force its completion. However
since get_cpu_fpsimd_context() disables preemption on RT it won't be
preempted in the section where the SIMD registers are modified. And the
softirq does not run on HARDIRQ-exit but in task context so it is okay.

But I get what you mean. I'm just not sure regarding the naming since
the code behaves the same on x86 and arm64 here.

> I do wonder whether there are other places making the same assumption
> about the local_irq > local_bh > preempt hierarchy that have been
> missed...

Based on memory we had a few cases of those while cleaning up
in_atomic(), in_softirq() and friends.

> > > If bh (as a preempting context) doesn't exist on RT, then can't
> > > local_bh_disable() just suppress all preemption up to but excluding
> > > hardirq?  Would anything break?
> > 
> > Yes. A lot. Starting with spin_lock_bh() itself because it does:
> > 	local_bh_disable();
> > 	spin_lock()
> > 
> > and with disabled preemption you can't do spin_lock() and you have to
> > because the owner may be preempted. The next thing is that kmalloc() and
> > friends won't work in a local_bh_disable() section for the same reason.
> 
> Couldn't this be solved with a trylock loop that re-enables bh (and
> preemption) on the sleeping path?  But that may still be trying to
> achieve something that doesn't make sense given the goals of
> PREEMPT_RT(?)

What about
 spin_lock_bh(a);
 spin_lock_bh(b);

? And then still you can't kmalloc() in a spin_lock_bh() section if you
disable preemption as part of local_bh_disable( if you disable
preemption as part of local_bh_disable()).

> Cheers
> ---Dave

Sebastian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/sve: Make kernel FPU protection RT friendly

[Sebastian Andrzej Siewior]

On 2021-07-29 18:00:31 [+0200], To Dave Martin wrote:
> 
> But I get what you mean. I'm just not sure regarding the naming since
> the code behaves the same on x86 and arm64 here.

Assuming that everyone follows this pattern what about
  fpregs_lock()
  fpregs_unlock()

?

Sebastian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/sve: Make kernel FPU protection RT friendly

[Dave Martin]

On Thu, Jul 29, 2021 at 06:07:56PM +0200, Sebastian Andrzej Siewior wrote:
> On 2021-07-29 18:00:31 [+0200], To Dave Martin wrote:
> > 
> > But I get what you mean. I'm just not sure regarding the naming since
> > the code behaves the same on x86 and arm64 here.
> 
> Assuming that everyone follows this pattern what about
>   fpregs_lock()
>   fpregs_unlock()

I'm not sure about the "fpregs".  This is really about CPU-local
resources that are contended between softirq and task context.

Some arches might not to use fp in softirq context and then their fp
regs wouldn't need this; others might have other resources that aren't
"fp" regs, but are contended in the same way.


My "local_bh" was meaning purely softirqs running on this CPU.  This was
the original meaning of "local" in this API IIUC.  This is one reason
why they must disable preemption: "local" is meaningless if preemption
is enabled, since otherwise we might randomly migrate between CPUs.

I guess the "local" was preserved in the naming on PREEMPT_RT to reduce
the amount of noise that would have resulted from a treewide rename, but
this word seems confusing if there is no CPU-localness involved.


In this particular case, we really do want to bind ourselves onto the
current CPU and disable softirqs on this CPU -- if they continue to run
elsewhere, that's just fine.

What do you think about:

get_bh_cpu_context()
put_bh_cpu_context()

or something along those lines?

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/sve: Make kernel FPU protection RT friendly

[Sebastian Andrzej Siewior]

On 2021-07-29 17:32:27 [+0100], Dave Martin wrote:
> On Thu, Jul 29, 2021 at 06:07:56PM +0200, Sebastian Andrzej Siewior wrote:
> > On 2021-07-29 18:00:31 [+0200], To Dave Martin wrote:
> > > 
> > > But I get what you mean. I'm just not sure regarding the naming since
> > > the code behaves the same on x86 and arm64 here.
> > 
> > Assuming that everyone follows this pattern what about
> >   fpregs_lock()
> >   fpregs_unlock()
> 
> I'm not sure about the "fpregs".  This is really about CPU-local
> resources that are contended between softirq and task context.
> 
> Some arches might not to use fp in softirq context and then their fp
> regs wouldn't need this; others might have other resources that aren't
> "fp" regs, but are contended in the same way.

if FP / SIMD is used in crypto then it is likely that they will aim for
softirq for ipsec reasons. Wireguard, dm-crypt perform crypto in process
context if I remember correctly.

> My "local_bh" was meaning purely softirqs running on this CPU.  This was
> the original meaning of "local" in this API IIUC.  This is one reason
> why they must disable preemption: "local" is meaningless if preemption
> is enabled, since otherwise we might randomly migrate between CPUs.

You assume that BH also disable preemption which is an implementation
detail. On RT local_bh_disable() disables BH on the current CPU. The
task won't migrate to another CPU while preempted and another task, which
invokes local_bh_disable() on the same CPU, will wait until the previous
local_bh_disable() section completes.
So all semantics which are expected by local_bh_disable() are preserved
in RT - except for the part where preemption is disabled.

> I guess the "local" was preserved in the naming on PREEMPT_RT to reduce
> the amount of noise that would have resulted from a treewide rename, but
> this word seems confusing if there is no CPU-localness involved.

As I explained above, the BH on the local/current CPU is disabled as in
no softirq is currently running on this CPU. The context however remains
preemptible so there is no need for a rename.

> In this particular case, we really do want to bind ourselves onto the
> current CPU and disable softirqs on this CPU -- if they continue to run
> elsewhere, that's just fine.
> 
> What do you think about:
> 
> get_bh_cpu_context()
> put_bh_cpu_context()
> 
> or something along those lines?

So you are not buying the fpregs_lock()? Then I don't need to reach for
the simd_lock() or such from the shelf?
The problem I have with _bh_ is that on RT the BH/softirq context is not
forced to complete if preempted as it would be the case with
local_bh_disable(). So that is misleading.
It is just that it happens not to matter in this case and it is
sufficient on RT to disable preemption. It would be wrong to disable BH
and preemption on RT (but it might be okay / needed in other cases).

powerpc for instance has
	arch/powerpc/crypto/crc32c-vpmsum_glue.c
doing doing crc32c with ALTIVEC (simd). They only disable preemption and
their crypto_simd_usable() (the generic version of it) forbids an
invocation in the softirq context.
So matter how I look at it, it really comes down to the specific SIMD
usage on an architecture.

> Cheers
> ---Dave

Sebastian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/sve: Make kernel FPU protection RT friendly

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 717 bytes --]

On Thu, Jul 29, 2021 at 12:52:15PM +0200, Sebastian Andrzej Siewior wrote:
> Non RT kernels need to protect FPU against preemption and bottom half
> processing. This is achieved by disabling bottom halves via
> local_bh_disable() which implictly disables preemption.
> 
> On RT kernels this protection mechanism is not sufficient because
> local_bh_disable() does not disable preemption. It serializes bottom half
> related processing via a CPU local lock.
> 
> As bottom halves are running always in thread context on RT kernels
> disabling preemption is the proper choice as it implicitly prevents bottom
> half processing.

I think someone sent a very similar looking patch in the past week or
two?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/sve: Make kernel FPU protection RT friendly

[Sebastian Andrzej Siewior]

On 2021-07-29 15:22:10 [+0100], Mark Brown wrote:
> I think someone sent a very similar looking patch in the past week or
> two?

Not that I' aware of.

Sebastian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64/sve: Make kernel FPU protection RT friendly

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 461 bytes --]

On Thu, Jul 29, 2021 at 04:41:11PM +0200, Sebastian Andrzej Siewior wrote:
> On 2021-07-29 15:22:10 [+0100], Mark Brown wrote:
> > I think someone sent a very similar looking patch in the past week or
> > two?
> 
> Not that I' aware of.

Hrm, right - I can't find it (even internally but I guess it must've
been some internal thing).  Anyway

Reviewed-by: Mark Brown <broonie@kernel.org>

though it does feel like we should have a helper for this.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel