diff mbox series

[v2,1/6] crypto: optimize algorithm registration when self-tests disabled

Message ID 20221110081346.336046-2-ebiggers@kernel.org (mailing list archive)
State Changes Requested
Delegated to: Herbert Xu
Headers show
Series crypto: reduce overhead when self-tests disabled | expand

Commit Message

Eric Biggers Nov. 10, 2022, 8:13 a.m. UTC 
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.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/algapi.c | 151 ++++++++++++++++++++++++++----------------------
 1 file changed, 82 insertions(+), 69 deletions(-)


base-commit: f67dd6ce0723ad013395f20a3f79d8a437d3f455

Comments

Eric Biggers Nov. 10, 2022, 7:42 p.m. UTC | #1 
On Thu, Nov 10, 2022 at 12:13:41AM -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.
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  crypto/algapi.c | 151 ++++++++++++++++++++++++++----------------------
>  1 file changed, 82 insertions(+), 69 deletions(-)

FYI, I realized that this patch breaks CRYPTO_MSG_ALG_LOADED (it isn't always
sent now).  So I'll have to send a new version at least for that.

- Eric
Herbert Xu Nov. 11, 2022, 4:06 a.m. UTC | #2 
Eric Biggers <ebiggers@kernel.org> wrote:
>
> @@ -432,17 +448,16 @@ int crypto_register_alg(struct crypto_alg *alg)
>                return err;
> 
>        down_write(&crypto_alg_sem);
> -       larval = __crypto_register_alg(alg);
> -       test_started = static_key_enabled(&crypto_boot_test_finished);
> +       larval = __crypto_register_alg(alg, &algs_to_put);
>        if (!IS_ERR_OR_NULL(larval))
> -               larval->test_started = test_started;
> +               larval->test_started = static_key_enabled(&crypto_boot_test_finished);
>        up_write(&crypto_alg_sem);
> 
> -       if (IS_ERR_OR_NULL(larval))
> +       if (IS_ERR(larval))
>                return PTR_ERR(larval);
> -
> -       if (test_started)
> +       if (larval && larval->test_started)
>                crypto_wait_for_test(larval);
> +       crypto_remove_final(&algs_to_put);
>        return 0;
> }

Is it safe to test larval->test_started instead of the local
test_started? This value could change from false to true behind
your back.

Cheers,
diff mbox series

Patch

diff --git a/crypto/algapi.c b/crypto/algapi.c
index 5c69ff8e8fa5c..8bbbd5dbbe157 100644
--- a/crypto/algapi.c
+++ b/crypto/algapi.c
@@ -222,12 +222,62 @@  void crypto_remove_spawns(struct crypto_alg *alg, struct list_head *list,
 }
 EXPORT_SYMBOL_GPL(crypto_remove_spawns);
 
+static void crypto_alg_finish_registration(struct crypto_alg *alg,
+					   bool fulfill_requests,
+					   struct list_head *algs_to_put)
+{
+	struct crypto_alg *q;
+
+	list_for_each_entry(q, &crypto_alg_list, cra_list) {
+		if (q == alg)
+			continue;
+
+		if (crypto_is_moribund(q))
+			continue;
+
+		if (crypto_is_larval(q)) {
+			struct crypto_larval *larval = (void *)q;
+
+			/*
+			 * Check to see if either our generic name or
+			 * specific name can satisfy the name requested
+			 * by the larval entry q.
+			 */
+			if (strcmp(alg->cra_name, q->cra_name) &&
+			    strcmp(alg->cra_driver_name, q->cra_name))
+				continue;
+
+			if (larval->adult)
+				continue;
+			if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
+				continue;
+
+			if (fulfill_requests && crypto_mod_get(alg))
+				larval->adult = alg;
+			else
+				larval->adult = ERR_PTR(-EAGAIN);
+
+			continue;
+		}
+
+		if (strcmp(alg->cra_name, q->cra_name))
+			continue;
+
+		if (strcmp(alg->cra_driver_name, q->cra_driver_name) &&
+		    q->cra_priority > alg->cra_priority)
+			continue;
+
+		crypto_remove_spawns(q, algs_to_put, alg);
+	}
+}
+
 static struct crypto_larval *crypto_alloc_test_larval(struct crypto_alg *alg)
 {
 	struct crypto_larval *larval;
 
-	if (!IS_ENABLED(CONFIG_CRYPTO_MANAGER))
-		return NULL;
+	if (!IS_ENABLED(CONFIG_CRYPTO_MANAGER) ||
+	    IS_ENABLED(CONFIG_CRYPTO_MANAGER_DISABLE_TESTS))
+		return NULL; /* No self-test needed */
 
 	larval = crypto_larval_alloc(alg->cra_name,
 				     alg->cra_flags | CRYPTO_ALG_TESTED, 0);
@@ -248,7 +298,8 @@  static struct crypto_larval *crypto_alloc_test_larval(struct crypto_alg *alg)
 	return larval;
 }
 
-static struct crypto_larval *__crypto_register_alg(struct crypto_alg *alg)
+static struct crypto_larval *
+__crypto_register_alg(struct crypto_alg *alg, struct list_head *algs_to_put)
 {
 	struct crypto_alg *q;
 	struct crypto_larval *larval;
@@ -259,9 +310,6 @@  static struct crypto_larval *__crypto_register_alg(struct crypto_alg *alg)
 
 	INIT_LIST_HEAD(&alg->cra_users);
 
-	/* No cheating! */
-	alg->cra_flags &= ~CRYPTO_ALG_TESTED;
-
 	ret = -EEXIST;
 
 	list_for_each_entry(q, &crypto_alg_list, cra_list) {
@@ -288,12 +336,17 @@  static struct crypto_larval *__crypto_register_alg(struct crypto_alg *alg)
 
 	list_add(&alg->cra_list, &crypto_alg_list);
 
-	if (larval)
+	crypto_stats_init(alg);
+
+	if (larval) {
+		/* No cheating! */
+		alg->cra_flags &= ~CRYPTO_ALG_TESTED;
+
 		list_add(&larval->alg.cra_list, &crypto_alg_list);
-	else
+	} else {
 		alg->cra_flags |= CRYPTO_ALG_TESTED;
-
-	crypto_stats_init(alg);
+		crypto_alg_finish_registration(alg, true, algs_to_put);
+	}
 
 out:
 	return larval;
@@ -341,7 +394,10 @@  void crypto_alg_tested(const char *name, int err)
 
 	alg->cra_flags |= CRYPTO_ALG_TESTED;
 
-	/* Only satisfy larval waiters if we are the best. */
+	/*
+	 * If a higher-priority implementation of the same algorithm is
+	 * currently being tested, then don't fulfill request larvals.
+	 */
 	best = true;
 	list_for_each_entry(q, &crypto_alg_list, cra_list) {
 		if (crypto_is_moribund(q) || !crypto_is_larval(q))
@@ -356,47 +412,7 @@  void crypto_alg_tested(const char *name, int err)
 		}
 	}
 
-	list_for_each_entry(q, &crypto_alg_list, cra_list) {
-		if (q == alg)
-			continue;
-
-		if (crypto_is_moribund(q))
-			continue;
-
-		if (crypto_is_larval(q)) {
-			struct crypto_larval *larval = (void *)q;
-
-			/*
-			 * Check to see if either our generic name or
-			 * specific name can satisfy the name requested
-			 * by the larval entry q.
-			 */
-			if (strcmp(alg->cra_name, q->cra_name) &&
-			    strcmp(alg->cra_driver_name, q->cra_name))
-				continue;
-
-			if (larval->adult)
-				continue;
-			if ((q->cra_flags ^ alg->cra_flags) & larval->mask)
-				continue;
-
-			if (best && crypto_mod_get(alg))
-				larval->adult = alg;
-			else
-				larval->adult = ERR_PTR(-EAGAIN);
-
-			continue;
-		}
-
-		if (strcmp(alg->cra_name, q->cra_name))
-			continue;
-
-		if (strcmp(alg->cra_driver_name, q->cra_driver_name) &&
-		    q->cra_priority > alg->cra_priority)
-			continue;
-
-		crypto_remove_spawns(q, &list, alg);
-	}
+	crypto_alg_finish_registration(alg, best, &list);
 
 complete:
 	complete_all(&test->completion);
@@ -423,7 +439,7 @@  EXPORT_SYMBOL_GPL(crypto_remove_final);
 int crypto_register_alg(struct crypto_alg *alg)
 {
 	struct crypto_larval *larval;
-	bool test_started;
+	LIST_HEAD(algs_to_put);
 	int err;
 
 	alg->cra_flags &= ~CRYPTO_ALG_DEAD;
@@ -432,17 +448,16 @@  int crypto_register_alg(struct crypto_alg *alg)
 		return err;
 
 	down_write(&crypto_alg_sem);
-	larval = __crypto_register_alg(alg);
-	test_started = static_key_enabled(&crypto_boot_test_finished);
+	larval = __crypto_register_alg(alg, &algs_to_put);
 	if (!IS_ERR_OR_NULL(larval))
-		larval->test_started = test_started;
+		larval->test_started = static_key_enabled(&crypto_boot_test_finished);
 	up_write(&crypto_alg_sem);
 
-	if (IS_ERR_OR_NULL(larval))
+	if (IS_ERR(larval))
 		return PTR_ERR(larval);
-
-	if (test_started)
+	if (larval && larval->test_started)
 		crypto_wait_for_test(larval);
+	crypto_remove_final(&algs_to_put);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(crypto_register_alg);
@@ -619,6 +634,7 @@  int crypto_register_instance(struct crypto_template *tmpl,
 	struct crypto_larval *larval;
 	struct crypto_spawn *spawn;
 	u32 fips_internal = 0;
+	LIST_HEAD(algs_to_put);
 	int err;
 
 	err = crypto_check_alg(&inst->alg);
@@ -650,7 +666,7 @@  int crypto_register_instance(struct crypto_template *tmpl,
 
 	inst->alg.cra_flags |= (fips_internal & CRYPTO_ALG_FIPS_INTERNAL);
 
-	larval = __crypto_register_alg(&inst->alg);
+	larval = __crypto_register_alg(&inst->alg, &algs_to_put);
 	if (IS_ERR(larval))
 		goto unlock;
 	else if (larval)
@@ -662,15 +678,12 @@  int crypto_register_instance(struct crypto_template *tmpl,
 unlock:
 	up_write(&crypto_alg_sem);
 
-	err = PTR_ERR(larval);
-	if (IS_ERR_OR_NULL(larval))
-		goto err;
-
-	crypto_wait_for_test(larval);
-	err = 0;
-
-err:
-	return err;
+	if (IS_ERR(larval))
+		return PTR_ERR(larval);
+	if (larval)
+		crypto_wait_for_test(larval);
+	crypto_remove_final(&algs_to_put);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(crypto_register_instance);