From: Eric Biggers <[email protected]>
Currently, registering an algorithm with the crypto API always causes a
notification to be posted to the "cryptomgr", which then creates a
kthread to self-test the algorithm. However, if self-tests are disabled
in the kconfig (as is the default option), then this kthread just
notifies waiters that the algorithm has been tested, then exits.
This causes a significant amount of overhead, especially in the kthread
creation and destruction, which is not necessary at all. For example,
in a quick test I found that booting a "minimum" x86_64 kernel with all
the crypto options enabled (except for the self-tests) takes about 400ms
until PID 1 can start. Of that, a full 13ms is spent just doing this
pointless dance, involving a kthread being created, run, and destroyed
over 200 times. That's over 3% of the entire kernel start time.
Fix this by just skipping the creation of the test larval and the
posting of the registration notification entirely, when self-tests are
disabled. Also compile out the unnecessary code in algboss.c.
While this patch is an optimization and not a "fix" per se, I've marked
it as for stable, due to the large improvement it can make to boot time.
Cc: [email protected]
Signed-off-by: Eric Biggers <[email protected]>
---
crypto/algapi.c | 3 ++-
crypto/algboss.c | 11 +++++++----
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/crypto/algapi.c b/crypto/algapi.c
index 5c69ff8e8fa5c..018935cb8417d 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -226,7 +226,8 @@ static struct crypto_larval *crypto_alloc_test_larval(struct crypto_alg *alg)
{
struct crypto_larval *larval;
- if (!IS_ENABLED(CONFIG_CRYPTO_MANAGER))
+ if (!IS_ENABLED(CONFIG_CRYPTO_MANAGER) ||
+ IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS))
return NULL;
larval = crypto_larval_alloc(alg->cra_name,
diff --git a/crypto/algboss.c b/crypto/algboss.c
index eb5fe84efb83e..e6f0443d08048 100644
--- a/crypto/algboss.c
+++ b/crypto/algboss.c
@@ -171,16 +171,18 @@ static int cryptomgr_schedule_probe(struct crypto_larval *larval)
return NOTIFY_OK;
}
+#ifdef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
+static int cryptomgr_schedule_test(struct crypto_alg *alg)
+{
+ return 0;
+}
+#else
static int cryptomgr_test(void *data)
{
struct crypto_test_param *param = data;
u32 type = param->type;
int err = 0;
-#ifdef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
- goto skiptest;
-#endif
-
if (type & CRYPTO_ALG_TESTED)
goto skiptest;
@@ -229,6 +231,7 @@ static int cryptomgr_schedule_test(struct crypto_alg *alg)
err:
return NOTIFY_OK;
}
+#endif /* !CONFIG_CRYPTO_MANAGER_DISABLE_TESTS */
static int cryptomgr_notify(struct notifier_block *this, unsigned long msg,
void *data)
base-commit: f67dd6ce0723ad013395f20a3f79d8a437d3f455
--
2.38.1
> -----Original Message-----
> From: Eric Biggers <[email protected]>
> Sent: Wednesday, November 9, 2022 8:38 PM
> Subject: [PATCH] crypto: avoid unnecessary work when self-tests are
> disabled
>
> Currently, registering an algorithm with the crypto API always causes a
> notification to be posted to the "cryptomgr", which then creates a
> kthread to self-test the algorithm. However, if self-tests are disabled
> in the kconfig (as is the default option), then this kthread just
> notifies waiters that the algorithm has been tested, then exits.
>
> This causes a significant amount of overhead, especially in the kthread
> creation and destruction, which is not necessary at all. For example,
> in a quick test I found that booting a "minimum" x86_64 kernel with all
> the crypto options enabled (except for the self-tests) takes about 400ms
> until PID 1 can start. Of that, a full 13ms is spent just doing this
> pointless dance, involving a kthread being created, run, and destroyed
> over 200 times. That's over 3% of the entire kernel start time.
>
> Fix this by just skipping the creation of the test larval and the
> posting of the registration notification entirely, when self-tests are
> disabled. Also compile out the unnecessary code in algboss.c.
>
...
> +#ifdef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
> +static int cryptomgr_schedule_test(struct crypto_alg *alg)
> +{
> + return 0;
> +}
> +#else
The crypto/kdf_sp800108.c init function currently ignores both
CONFIG_CRYPTO_MANAGER_DISABLE_TESTS and the cryptomgr module's
notests module parameter and always runs its self-test, as described in
https://lore.kernel.org/lkml/MW5PR84MB1842811C4EECC0F4B35B5FB3AB709@MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM/T/#t
Paul reported that taking 262 ms on his system; I measured 1.4 s on
my system.
It'd be nice if a patch series improving how DISABLE_TESTS is honored
would tackle that module too.
On Thu, Nov 10, 2022 at 04:36:17AM +0000, Elliott, Robert (Servers) wrote:
>
> > -----Original Message-----
> > From: Eric Biggers <[email protected]>
> > Sent: Wednesday, November 9, 2022 8:38 PM
> > Subject: [PATCH] crypto: avoid unnecessary work when self-tests are
> > disabled
> >
> > Currently, registering an algorithm with the crypto API always causes a
> > notification to be posted to the "cryptomgr", which then creates a
> > kthread to self-test the algorithm. However, if self-tests are disabled
> > in the kconfig (as is the default option), then this kthread just
> > notifies waiters that the algorithm has been tested, then exits.
> >
> > This causes a significant amount of overhead, especially in the kthread
> > creation and destruction, which is not necessary at all. For example,
> > in a quick test I found that booting a "minimum" x86_64 kernel with all
> > the crypto options enabled (except for the self-tests) takes about 400ms
> > until PID 1 can start. Of that, a full 13ms is spent just doing this
> > pointless dance, involving a kthread being created, run, and destroyed
> > over 200 times. That's over 3% of the entire kernel start time.
> >
> > Fix this by just skipping the creation of the test larval and the
> > posting of the registration notification entirely, when self-tests are
> > disabled. Also compile out the unnecessary code in algboss.c.
> >
> ...
> > +#ifdef CONFIG_CRYPTO_MANAGER_DISABLE_TESTS
> > +static int cryptomgr_schedule_test(struct crypto_alg *alg)
> > +{
> > + return 0;
> > +}
> > +#else
>
> The crypto/kdf_sp800108.c init function currently ignores both
> CONFIG_CRYPTO_MANAGER_DISABLE_TESTS and the cryptomgr module's
> notests module parameter and always runs its self-test, as described in
> https://lore.kernel.org/lkml/MW5PR84MB1842811C4EECC0F4B35B5FB3AB709@MW5PR84MB1842.NAMPRD84.PROD.OUTLOOK.COM/T/#t
>
> Paul reported that taking 262 ms on his system; I measured 1.4 s on
> my system.
>
> It'd be nice if a patch series improving how DISABLE_TESTS is honored
> would tackle that module too.
That should be a separate patch, but yes, it should only run the test if
!IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS), like what everywhere else
does.
- Eric
On Wed, Nov 09, 2022 at 06:37:38PM -0800, Eric Biggers wrote:
> From: Eric Biggers <[email protected]>
>
> Currently, registering an algorithm with the crypto API always causes a
> notification to be posted to the "cryptomgr", which then creates a
> kthread to self-test the algorithm. However, if self-tests are disabled
> in the kconfig (as is the default option), then this kthread just
> notifies waiters that the algorithm has been tested, then exits.
>
> This causes a significant amount of overhead, especially in the kthread
> creation and destruction, which is not necessary at all. For example,
> in a quick test I found that booting a "minimum" x86_64 kernel with all
> the crypto options enabled (except for the self-tests) takes about 400ms
> until PID 1 can start. Of that, a full 13ms is spent just doing this
> pointless dance, involving a kthread being created, run, and destroyed
> over 200 times. That's over 3% of the entire kernel start time.
>
> Fix this by just skipping the creation of the test larval and the
> posting of the registration notification entirely, when self-tests are
> disabled. Also compile out the unnecessary code in algboss.c.
>
> While this patch is an optimization and not a "fix" per se, I've marked
> it as for stable, due to the large improvement it can make to boot time.
>
> Cc: [email protected]
> Signed-off-by: Eric Biggers <[email protected]>
Unfortunately, this patch won't work because it breaks templates.
There should still be a solution that at least avoids having to spawn kthreads,
though...
- Eric