padata: Separate cpumasks for cb_cpus and parallel workers

PATCH In review Dan Kruchinin 2 participants 3 messages Jun 29, 2010 – Jun 30, 2010

Subsystems (3)

Crypto API Padata parallel execution mechanism Pcrypt parallel crypto engine

lore

Share & tools

lore

Dan Kruchinin

author

2010-06-29 16:34 UTC #0 lore raw

PATCH 1/2 padata: Separate cpumasks for cb_cpus and parallel workers

1) Two separate cpumasks for parallel and serial workers
2) padata_alloc does not takes cpumask as its argument, instead it uses a default one. __padata_alloc takes two cpumasks as its
arguments: one for parallel workers and another one for serial.
3) I made padata_cpu_hash a public function to reduce code size(in several places in pcrypt.c there is near the same code(for getting cpu hash) as in padata_cpu_hash)

crypto/pcrypt.c +20 ~~-32~~

index 247178c..20f3e47 100644
@@ -33,33 +33,29 @@ static struct workqueue_struct *decwq;
 
 struct pcrypt_instance_ctx {
 	struct crypto_spawn spawn;
-	unsigned int tfm_count;
 };
 
 struct pcrypt_aead_ctx {
 	struct crypto_aead *child;
-	unsigned int cb_cpu;
+	int cb_cpu;
+	unsigned int tfm_count;
 };
 
-static int pcrypt_do_parallel(struct padata_priv *padata, unsigned int *cb_cpu,
-			      struct padata_instance *pinst)
+static int pcrypt_do_parallel(struct padata_priv *padata,
+                              struct pcrypt_aead_ctx *ctx,
+                              struct padata_instance *pinst)
 {
-	unsigned int cpu_index, cpu, i;
-
-	cpu = *cb_cpu;
-
-	if (cpumask_test_cpu(cpu, cpu_active_mask))
-			goto out;
-
-	cpu_index = cpu % cpumask_weight(cpu_active_mask);
+	int cpu;
 
-	cpu = cpumask_first(cpu_active_mask);
-	for (i = 0; i < cpu_index; i++)
-		cpu = cpumask_next(cpu, cpu_active_mask);
+	cpu = ctx->cb_cpu;
+	if (cpu < 0) {
+		struct cpumask mask;
 
-	*cb_cpu = cpu;
+		padata_get_cb_cpumask(pinst, &mask);
+		cpu = padata_cpu_hash(&mask, ctx->tfm_count);
+		ctx->cb_cpu = cpu;
+	}
 
-out:
 	return padata_do_parallel(pinst, padata, cpu);
 }

@@ -142,7 +138,7 @@ static int pcrypt_aead_encrypt(struct aead_request *req)
 			       req->cryptlen, req->iv);
 	aead_request_set_assoc(creq, req->assoc, req->assoclen);
 
-	err = pcrypt_do_parallel(padata, &ctx->cb_cpu, pcrypt_enc_padata);
+	err = pcrypt_do_parallel(padata, ctx, pcrypt_enc_padata);
 	if (err)
 		return err;
 	else

@@ -186,7 +182,7 @@ static int pcrypt_aead_decrypt(struct aead_request *req)
 			       req->cryptlen, req->iv);
 	aead_request_set_assoc(creq, req->assoc, req->assoclen);
 
-	err = pcrypt_do_parallel(padata, &ctx->cb_cpu, pcrypt_dec_padata);
+	err = pcrypt_do_parallel(padata, ctx, pcrypt_dec_padata);
 	if (err)
 		return err;
 	else

@@ -232,7 +228,7 @@ static int pcrypt_aead_givencrypt(struct aead_givcrypt_request *req)
 	aead_givcrypt_set_assoc(creq, areq->assoc, areq->assoclen);
 	aead_givcrypt_set_giv(creq, req->giv, req->seq);
 
-	err = pcrypt_do_parallel(padata, &ctx->cb_cpu, pcrypt_enc_padata);
+	err = pcrypt_do_parallel(padata, ctx, pcrypt_enc_padata);
 	if (err)
 		return err;
 	else

@@ -243,20 +239,12 @@ static int pcrypt_aead_givencrypt(struct aead_givcrypt_request *req)
 
 static int pcrypt_aead_init_tfm(struct crypto_tfm *tfm)
 {
-	int cpu, cpu_index;
 	struct crypto_instance *inst = crypto_tfm_alg_instance(tfm);
-	struct pcrypt_instance_ctx *ictx = crypto_instance_ctx(inst);
 	struct pcrypt_aead_ctx *ctx = crypto_tfm_ctx(tfm);
 	struct crypto_aead *cipher;
 
-	ictx->tfm_count++;
-
-	cpu_index = ictx->tfm_count % cpumask_weight(cpu_active_mask);
-
-	ctx->cb_cpu = cpumask_first(cpu_active_mask);
-	for (cpu = 0; cpu < cpu_index; cpu++)
-		ctx->cb_cpu = cpumask_next(ctx->cb_cpu, cpu_active_mask);
-
+	ctx->tfm_count++;
+	ctx->cb_cpu = -1;
 	cipher = crypto_spawn_aead(crypto_instance_ctx(inst));
 
 	if (IS_ERR(cipher))

@@ -394,11 +382,11 @@ static int __init pcrypt_init(void)
 		goto err_destroy_encwq;
 
 
-	pcrypt_enc_padata = padata_alloc(cpu_possible_mask, encwq);
+	pcrypt_enc_padata = padata_alloc(encwq);
 	if (!pcrypt_enc_padata)
 		goto err_destroy_decwq;
 
-	pcrypt_dec_padata = padata_alloc(cpu_possible_mask, decwq);
+	pcrypt_dec_padata = padata_alloc(decwq);
 	if (!pcrypt_dec_padata)
 		goto err_free_padata;

include/linux/padata.h +61 ~~-30~~

148 lines not on this page · raw message

kernel/padata.c +282 ~~-130~~

737 lines not on this page · raw message

Signature · 2 lines

W.B.R.
Dan Kruchinin

Steffen Klassert

maintainer

2010-06-30 11:51 UTC #1 lore raw

Hi,

I like the idea to have separate cpumasks for parallel and serial workers. I had not yet time to test your patches, but I'll do so as soon as possible. I've added some comments to your patch below.

Quoted 7 lines · Dan Kruchinin

1) Two separate cpumasks for parallel and serial workers
2) padata_alloc does not takes cpumask as its argument, instead it uses a default one. __padata_alloc takes two cpumasks as its
arguments: one for parallel workers and another one for serial.
3) I made padata_cpu_hash a public function to reduce code size(in several places in pcrypt.c there is near the same code(for getting cpu hash) as in padata_cpu_hash)

Please add a Signed-off-by line to your patches.

[snip]

Quoted 7 lines

static int pcrypt_aead_init_tfm(struct crypto_tfm *tfm) {

crypto/pcrypt.c +0 -2

-	int cpu, cpu_index;
 	struct crypto_instance *inst = crypto_tfm_alg_instance(tfm);
-	struct pcrypt_instance_ctx *ictx = crypto_instance_ctx(inst);
 	struct pcrypt_aead_ctx *ctx = crypto_tfm_ctx(tfm);
 	struct crypto_aead *cipher;

crypto/pcrypt.c static int pcrypt_aead_givencrypt(struct aead_givcrypt_request *req) · line 246

Quoted 10 lines

crypto/pcrypt.c +1 -8

 
-	ictx->tfm_count++;
-
-	cpu_index = ictx->tfm_count % cpumask_weight(cpu_active_mask);
-
-	ctx->cb_cpu = cpumask_first(cpu_active_mask);
-	for (cpu = 0; cpu < cpu_index; cpu++)
-		ctx->cb_cpu = cpumask_next(ctx->cb_cpu, cpu_active_mask);
-
+	ctx->tfm_count++;

Having a transformation count on the transformation context itself does not make too much sense. pcrypt_aead_init_tfm() is called once for each transformation, so all transformations have a count of one. The transformation count has to stay at the instance context.

[snip]

kernel/padata.c static void padata_reorder(struct parallel_data *pd) · line 345

Quoted 10 lines

kernel/padata.c +4 -4

 	/*
-	 * The next object that needs serialization might have arrived to
-	 * the reorder queues in the meantime, we will be called again
-	 * from the timer function if noone else cares for it.
-	 */
+     * The next object that needs serialization might have arrived to
+     * the reorder queues in the meantime, we will be called again
+     * from the timer function if noone else cares for it.
+     */

You replaced the tabs by some whitespaces in the lines above, this changed the code indent of some lines of this comment.

[snip]

Quoted 6 lines

static int __padata_add_cpu(struct padata_instance *pinst, int cpu) {
kernel/padata.c +1 -1
-	struct parallel_data *pd;
+	struct parallel_data *pd = NULL;
 
 	if (cpumask_test_cpu(cpu, cpu_active_mask)) {

kernel/padata.c out: · line 651

Quoted 10 lines

kernel/padata.c +3 -3

-		pd = padata_alloc_pd(pinst, pinst->cpumask);
+		pd = padata_alloc_pd(pinst,
+				pinst->cpumask.pcpu, pinst->cpumask.cbcpu);
 		if (!pd)
 			return -ENOMEM;
-
-		padata_replace(pinst, pd);
 	}
 
+	padata_replace(pinst, pd);

This crashes if the cpu you add is not in the active cpumask, as you pass a NULL pointer to padata_replace() then.

Some minor general things. It is quite helpfull to run checkpatch on your patches, this helps to spot some codingstyle issues. Also reading Documentation/CodingStyle is helpfull to get an idea of the recommended codingstyle.

The rest of the patch looks quite good.

Thanks a lot,

Steffen

Steffen Klassert

maintainer

2010-06-30 13:01 UTC #2 lore raw

In reply to Dan Kruchinin

Quoted 8 lines · Dan Kruchinin

1) Two separate cpumasks for parallel and serial workers
2) padata_alloc does not takes cpumask as its argument, instead it uses a default one. __padata_alloc takes two cpumasks as its
arguments: one for parallel workers and another one for serial.
3) I made padata_cpu_hash a public function to reduce code size(in several places in pcrypt.c there is near the same code(for getting cpu hash) as in padata_cpu_hash)

Plead keep padata_cpu_hash() static to padata. If this becomes part of the padata API it's not that easy to change it anymore. You could add a similar function to pcrypt and use this function there wehenver it's needed.