| Message ID | 20221110023738.147128-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: avoid unnecessary work when self-tests are disabled | expand |
> -----Original Message----- > From: Eric Biggers <ebiggers@kernel.org> > 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 <ebiggers@kernel.org> > > 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 <ebiggers@google.com> > > 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: stable@vger.kernel.org > Signed-off-by: Eric Biggers <ebiggers@google.com> 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
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)