[PATCH] crypto: testmgr - Only disable migration in crypto_disable_simd_for_test()

[PATCH] crypto: testmgr - Only disable migration in crypto_disable_simd_for_test()

[Sebastian Andrzej Siewior]

crypto_disable_simd_for_test() disables preemption in order to receive a
stable per-CPU variable which it needs to modify in order to alter
crypto_simd_usable() results.

This can also be achived by migrate_disable() which forbidds CPU
migrations but allows the task to be preempted. The latter is important
for PREEMPT_RT since operation like skcipher_walk_first() may allocate
memory which must not happen with disabled preemption on PREEMPT_RT.

Use migrate_disable() in crypto_disable_simd_for_test() to achieve a
stable per-CPU pointer.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 crypto/testmgr.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -1061,14 +1061,14 @@ static void generate_random_testvec_conf
 
 static void crypto_disable_simd_for_test(void)
 {
-	preempt_disable();
+	migrate_disable();
 	__this_cpu_write(crypto_simd_disabled_for_test, true);
 }
 
 static void crypto_reenable_simd_for_test(void)
 {
 	__this_cpu_write(crypto_simd_disabled_for_test, false);
-	preempt_enable();
+	migrate_enable();
 }
 
 /*

Re: [PATCH] crypto: testmgr - Only disable migration in crypto_disable_simd_for_test()

[Eric Biggers]

Hi Sebastian,

On Tue, Sep 28, 2021 at 01:54:01PM +0200, Sebastian Andrzej Siewior wrote:
> crypto_disable_simd_for_test() disables preemption in order to receive a
> stable per-CPU variable which it needs to modify in order to alter
> crypto_simd_usable() results.
> 
> This can also be achived by migrate_disable() which forbidds CPU
> migrations but allows the task to be preempted. The latter is important
> for PREEMPT_RT since operation like skcipher_walk_first() may allocate
> memory which must not happen with disabled preemption on PREEMPT_RT.
> 
> Use migrate_disable() in crypto_disable_simd_for_test() to achieve a
> stable per-CPU pointer.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

The comment above the definition of migrate_disable() in include/linux/preempt.h
claims that it is a temporary workaround.

Is there a better way to do this that should be used instead?

- Eric

Re: [PATCH] crypto: testmgr - Only disable migration in crypto_disable_simd_for_test()

[Sebastian Andrzej Siewior]

On 2021-10-05 11:17:16 [-0700], Eric Biggers wrote:
> Hi Sebastian,
Hi,

> The comment above the definition of migrate_disable() in include/linux/preempt.h
> claims that it is a temporary workaround.

It claims that, yes. I think this is due to the scheduler's limitation
and should not encourage to use this over long sections.

> Is there a better way to do this that should be used instead?

An alternative might be to move the whole test into a kworker which is
bound to a specific CPU. So even without disabling preemption/ migration
the per-CPU pointer would remain stable.

> - Eric

Sebastian

Re: [PATCH] crypto: testmgr - Only disable migration in crypto_disable_simd_for_test()

[Eric Biggers]

On Thu, Oct 07, 2021 at 11:42:24AM +0200, Sebastian Andrzej Siewior wrote:
> On 2021-10-05 11:17:16 [-0700], Eric Biggers wrote:
> > Hi Sebastian,
> Hi,
> 
> > The comment above the definition of migrate_disable() in include/linux/preempt.h
> > claims that it is a temporary workaround.
> 
> It claims that, yes. I think this is due to the scheduler's limitation
> and should not encourage to use this over long sections.
> 
> > Is there a better way to do this that should be used instead?
> 
> An alternative might be to move the whole test into a kworker which is
> bound to a specific CPU. So even without disabling preemption/ migration
> the per-CPU pointer would remain stable.
> 

It's only individual test cases that need to use the SIMD disabled flag, not the
whole test.  An algorithm test could involve 100 test cases, any subset of which
use the SIMD disabled flag and the others don't.  So the assumption is that this
can be a relatively fine-grained knob.  Executing different individual test
cases on different threads sounds painful.  Your simple change to use
migrate_disable() looks much better, but I'm not sure how to reconcile that with
the claim that migrate_disable() is a temporary workaround.

- Eric

Re: [PATCH] crypto: testmgr - Only disable migration in crypto_disable_simd_for_test()

[Sebastian Andrzej Siewior]

On 2021-10-07 10:53:03 [-0700], Eric Biggers wrote:
> On Thu, Oct 07, 2021 at 11:42:24AM +0200, Sebastian Andrzej Siewior wrote:
> > An alternative might be to move the whole test into a kworker which is
> > bound to a specific CPU. So even without disabling preemption/ migration
> > the per-CPU pointer would remain stable.
> > 
> 
> It's only individual test cases that need to use the SIMD disabled flag, not the
> whole test.  An algorithm test could involve 100 test cases, any subset of which
> use the SIMD disabled flag and the others don't.  So the assumption is that this
> can be a relatively fine-grained knob.  Executing different individual test
> cases on different threads sounds painful.  Your simple change to use
> migrate_disable() looks much better, but I'm not sure how to reconcile that with
> the claim that migrate_disable() is a temporary workaround.

Now that you bring it up, I was under the impression that the
"SIMD-disabled" flag is only changed by the tcrypt module not by the
individual tests which are performed at the algorithm lookup time. If it
is the latter than yes, it will be painful. If it is just tcrypt then
you could wrap the whole test-suite (at module init's time) into the
kworker and bind it to one CPU).

Maybe Peter can bring light into the temporary workaround but it does
look like simplest thing here.

> - Eric

Sebastian

Re: [PATCH] crypto: testmgr - Only disable migration in crypto_disable_simd_for_test()

[Herbert Xu]

On Thu, Oct 07, 2021 at 09:03:32PM +0200, Sebastian Andrzej Siewior wrote:
>
> Now that you bring it up, I was under the impression that the
> "SIMD-disabled" flag is only changed by the tcrypt module not by the
> individual tests which are performed at the algorithm lookup time. If it
> is the latter than yes, it will be painful. If it is just tcrypt then
> you could wrap the whole test-suite (at module init's time) into the
> kworker and bind it to one CPU).
> 
> Maybe Peter can bring light into the temporary workaround but it does
> look like simplest thing here.

Let's use migrate_disable for now.  If something better comes along
we can always change again.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: testmgr - Only disable migration in crypto_disable_simd_for_test()

[Herbert Xu]

On Tue, Sep 28, 2021 at 01:54:01PM +0200, Sebastian Andrzej Siewior wrote:
> crypto_disable_simd_for_test() disables preemption in order to receive a
> stable per-CPU variable which it needs to modify in order to alter
> crypto_simd_usable() results.
> 
> This can also be achived by migrate_disable() which forbidds CPU
> migrations but allows the task to be preempted. The latter is important
> for PREEMPT_RT since operation like skcipher_walk_first() may allocate
> memory which must not happen with disabled preemption on PREEMPT_RT.
> 
> Use migrate_disable() in crypto_disable_simd_for_test() to achieve a
> stable per-CPU pointer.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  crypto/testmgr.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt