This patchset contains the following changes:
1. Add a flag to mark the padata instance as invalid.
We will use this to mark the padata instace as invalid
if the padata cpumask contains no active cpu.
2. Make padata_stop to block until the padata instance is
unused. So it's save to change the cpumask after a call to
padata_stop.
3. Add a possibility to handle empty padata cpumasks.
The first three patches address a bug, found by Dan Kruchinin.
When the padata cpumask does not intersect with the active
cpumask we get a division by zero in padata_alloc_pd and we
end up with a useless padata instance. It's not possible to
trigger this bug by now, but Dan has patches that let the
cpumask change from userspace. So this bug has to be fixed
before these patches can go in.
4. Make padata_do_parallel to return zero on success.
To return -EINPROGRESS on success was considered to be odd,
so we change the API to return zero instead. Users are updated
accordingly.
5. Simplify the serialization mechanism.
As it is, we go through all the percpu reorder queues to calculate
the sequence number of the next object that needs serialization.
This patch avoids this and minimalizes the accesses of foreign
memory. This gives some performance inprovements, in particular
on machines with many cpus.
6. Update the padata documentation.
Steffen
This patch introduces the PADATA_INVALID flag which is
checked on padata start. This will be used to mark a padata
instance as invalid, if the padata cpumask does not intersect
with the active cpumask. we change padata_start to return an
error if the PADATA_INVALID is set. Also we adapt the only
padata user, pcrypt to this change.
Signed-off-by: Steffen Klassert <[email protected]>
---
crypto/pcrypt.c | 19 ++++++++++++++-----
include/linux/padata.h | 3 ++-
kernel/padata.c | 18 ++++++++++++++++--
3 files changed, 32 insertions(+), 8 deletions(-)
diff --git a/crypto/pcrypt.c b/crypto/pcrypt.c
index 247178c..71ae2b2 100644
--- a/crypto/pcrypt.c
+++ b/crypto/pcrypt.c
@@ -385,6 +385,7 @@ static struct crypto_template pcrypt_tmpl = {
static int __init pcrypt_init(void)
{
+ int err = -ENOMEM;
encwq = create_workqueue("pencrypt");
if (!encwq)
goto err;
@@ -400,14 +401,22 @@ static int __init pcrypt_init(void)
pcrypt_dec_padata = padata_alloc(cpu_possible_mask, decwq);
if (!pcrypt_dec_padata)
- goto err_free_padata;
+ goto err_free_enc_padata;
- padata_start(pcrypt_enc_padata);
- padata_start(pcrypt_dec_padata);
+ err = padata_start(pcrypt_enc_padata);
+ if (err)
+ goto err_free_dec_padata;
+
+ err = padata_start(pcrypt_dec_padata);
+ if (err)
+ goto err_free_dec_padata;
return crypto_register_template(&pcrypt_tmpl);
-err_free_padata:
+err_free_dec_padata:
+ padata_free(pcrypt_dec_padata);
+
+err_free_enc_padata:
padata_free(pcrypt_enc_padata);
err_destroy_decwq:
@@ -417,7 +426,7 @@ err_destroy_encwq:
destroy_workqueue(encwq);
err:
- return -ENOMEM;
+ return err;
}
static void __exit pcrypt_exit(void)
diff --git a/include/linux/padata.h b/include/linux/padata.h
index 8d84062..e4c17f9 100644
--- a/include/linux/padata.h
+++ b/include/linux/padata.h
@@ -126,6 +126,7 @@ struct padata_instance {
u8 flags;
#define PADATA_INIT 1
#define PADATA_RESET 2
+#define PADATA_INVALID 4
};
extern struct padata_instance *padata_alloc(const struct cpumask *cpumask,
@@ -138,6 +139,6 @@ extern int padata_set_cpumask(struct padata_instance *pinst,
cpumask_var_t cpumask);
extern int padata_add_cpu(struct padata_instance *pinst, int cpu);
extern int padata_remove_cpu(struct padata_instance *pinst, int cpu);
-extern void padata_start(struct padata_instance *pinst);
+extern int padata_start(struct padata_instance *pinst);
extern void padata_stop(struct padata_instance *pinst);
#endif
diff --git a/kernel/padata.c b/kernel/padata.c
index ff8de1b..e7d723a 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -485,6 +485,11 @@ static void padata_flush_queues(struct parallel_data *pd)
BUG_ON(atomic_read(&pd->refcnt) != 0);
}
+static void __padata_start(struct padata_instance *pinst)
+{
+ pinst->flags |= PADATA_INIT;
+}
+
/* Replace the internal control stucture with a new one. */
static void padata_replace(struct padata_instance *pinst,
struct parallel_data *pd_new)
@@ -619,11 +624,20 @@ EXPORT_SYMBOL(padata_remove_cpu);
*
* @pinst: padata instance to start
*/
-void padata_start(struct padata_instance *pinst)
+int padata_start(struct padata_instance *pinst)
{
+ int err = 0;
+
mutex_lock(&pinst->lock);
- pinst->flags |= PADATA_INIT;
+
+ if (pinst->flags & PADATA_INVALID)
+ err =-EINVAL;
+
+ __padata_start(pinst);
+
mutex_unlock(&pinst->lock);
+
+ return err;
}
EXPORT_SYMBOL(padata_start);
--
1.5.6.5
This patch fixes a bug when the padata cpumask does not
intersect with the active cpumask. In this case we get a
division by zero in padata_alloc_pd and we end up with a
useless padata instance. Padata can end up with an empty
cpumask for two reasons:
1. A user removed the last cpu that belongs to the padata
cpumask and the active cpumask.
2. The last cpu that belongs to the padata cpumask and the
active cpumask goes offline.
We introduce a function padata_validate_cpumask to check if the padata
cpumask does intersect with the active cpumask. If the cpumasks do not
intersect we mark the instance as invalid, so it can't be used. We do not
allocate the cpumask dependend recources in this case. This fixes the
division by zero and keeps the padate instance in a consistent state.
It's not possible to trigger this bug by now because the only padata user,
pcrypt uses always the possible cpumask.
Reported-by: Dan Kruchinin <[email protected]>
Signed-off-by: Steffen Klassert <[email protected]>
---
kernel/padata.c | 61 +++++++++++++++++++++++++++++++++++++++++++++----------
1 files changed, 50 insertions(+), 11 deletions(-)
diff --git a/kernel/padata.c b/kernel/padata.c
index 9e18dfa..57ec4eb 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -516,12 +516,27 @@ static void padata_replace(struct padata_instance *pinst,
synchronize_rcu();
- padata_flush_queues(pd_old);
- padata_free_pd(pd_old);
+ if (pd_old) {
+ padata_flush_queues(pd_old);
+ padata_free_pd(pd_old);
+ }
pinst->flags &= ~PADATA_RESET;
}
+/* If cpumask contains no active cpu, we mark the instance as invalid. */
+static bool padata_validate_cpumask(struct padata_instance *pinst,
+ const struct cpumask *cpumask)
+{
+ if (!cpumask_intersects(cpumask, cpu_active_mask)) {
+ pinst->flags |= PADATA_INVALID;
+ return false;
+ }
+
+ pinst->flags &= ~PADATA_INVALID;
+ return true;
+}
+
/**
* padata_set_cpumask - set the cpumask that padata should use
*
@@ -531,11 +546,18 @@ static void padata_replace(struct padata_instance *pinst,
int padata_set_cpumask(struct padata_instance *pinst,
cpumask_var_t cpumask)
{
- struct parallel_data *pd;
+ int valid;
int err = 0;
+ struct parallel_data *pd = NULL;
mutex_lock(&pinst->lock);
+ valid = padata_validate_cpumask(pinst, cpumask);
+ if (!valid) {
+ __padata_stop(pinst);
+ goto out_replace;
+ }
+
get_online_cpus();
pd = padata_alloc_pd(pinst, cpumask);
@@ -544,10 +566,14 @@ int padata_set_cpumask(struct padata_instance *pinst,
goto out;
}
+out_replace:
cpumask_copy(pinst->cpumask, cpumask);
padata_replace(pinst, pd);
+ if (valid)
+ __padata_start(pinst);
+
out:
put_online_cpus();
@@ -567,6 +593,9 @@ static int __padata_add_cpu(struct padata_instance *pinst, int cpu)
return -ENOMEM;
padata_replace(pinst, pd);
+
+ if (padata_validate_cpumask(pinst, pinst->cpumask))
+ __padata_start(pinst);
}
return 0;
@@ -597,9 +626,16 @@ EXPORT_SYMBOL(padata_add_cpu);
static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
{
- struct parallel_data *pd;
+ struct parallel_data *pd = NULL;
if (cpumask_test_cpu(cpu, cpu_online_mask)) {
+
+ if (!padata_validate_cpumask(pinst, pinst->cpumask)) {
+ __padata_stop(pinst);
+ padata_replace(pinst, pd);
+ goto out;
+ }
+
pd = padata_alloc_pd(pinst, pinst->cpumask);
if (!pd)
return -ENOMEM;
@@ -607,6 +643,7 @@ static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
padata_replace(pinst, pd);
}
+out:
return 0;
}
@@ -732,7 +769,7 @@ struct padata_instance *padata_alloc(const struct cpumask *cpumask,
struct workqueue_struct *wq)
{
struct padata_instance *pinst;
- struct parallel_data *pd;
+ struct parallel_data *pd = NULL;
pinst = kzalloc(sizeof(struct padata_instance), GFP_KERNEL);
if (!pinst)
@@ -740,12 +777,14 @@ struct padata_instance *padata_alloc(const struct cpumask *cpumask,
get_online_cpus();
- pd = padata_alloc_pd(pinst, cpumask);
- if (!pd)
+ if (!alloc_cpumask_var(&pinst->cpumask, GFP_KERNEL))
goto err_free_inst;
- if (!alloc_cpumask_var(&pinst->cpumask, GFP_KERNEL))
- goto err_free_pd;
+ if (padata_validate_cpumask(pinst, cpumask)) {
+ pd = padata_alloc_pd(pinst, cpumask);
+ if (!pd)
+ goto err_free_mask;
+ }
rcu_assign_pointer(pinst->pd, pd);
@@ -767,8 +806,8 @@ struct padata_instance *padata_alloc(const struct cpumask *cpumask,
return pinst;
-err_free_pd:
- padata_free_pd(pd);
+err_free_mask:
+ free_cpumask_var(pinst->cpumask);
err_free_inst:
kfree(pinst);
put_online_cpus();
--
1.5.6.5
This patch makes padata_stop to block until the padata
instance is unused. Also we split padata_stop to a locked
and a unlocked version. This is in preparation to be able
to change the cpumask after a call to patata stop.
Signed-off-by: Steffen Klassert <[email protected]>
---
kernel/padata.c | 24 ++++++++++++++++--------
1 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/kernel/padata.c b/kernel/padata.c
index e7d723a..9e18dfa 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -490,6 +490,20 @@ static void __padata_start(struct padata_instance *pinst)
pinst->flags |= PADATA_INIT;
}
+static void __padata_stop(struct padata_instance *pinst)
+{
+ if (!(pinst->flags & PADATA_INIT))
+ return;
+
+ pinst->flags &= ~PADATA_INIT;
+
+ synchronize_rcu();
+
+ get_online_cpus();
+ padata_flush_queues(pinst->pd);
+ put_online_cpus();
+}
+
/* Replace the internal control stucture with a new one. */
static void padata_replace(struct padata_instance *pinst,
struct parallel_data *pd_new)
@@ -649,7 +663,7 @@ EXPORT_SYMBOL(padata_start);
void padata_stop(struct padata_instance *pinst)
{
mutex_lock(&pinst->lock);
- pinst->flags &= ~PADATA_INIT;
+ __padata_stop(pinst);
mutex_unlock(&pinst->lock);
}
EXPORT_SYMBOL(padata_stop);
@@ -770,17 +784,11 @@ EXPORT_SYMBOL(padata_alloc);
*/
void padata_free(struct padata_instance *pinst)
{
- padata_stop(pinst);
-
- synchronize_rcu();
-
#ifdef CONFIG_HOTPLUG_CPU
unregister_hotcpu_notifier(&pinst->cpu_notifier);
#endif
- get_online_cpus();
- padata_flush_queues(pinst->pd);
- put_online_cpus();
+ padata_stop(pinst);
padata_free_pd(pinst->pd);
free_cpumask_var(pinst->cpumask);
kfree(pinst);
--
1.5.6.5
To return -EINPROGRESS on success in padata_do_parallel was
considered to be odd. This patch changes this to return zero
on success. Also the only user of padata, pcrypt is adapted to
convert a return of zero to -EINPROGRESS within the crypto layer.
This also removes the pcrypt fallback if padata_do_parallel
was called on a not running padata instance as we can't handle it
anymore. This fallback was unused, so it's save to remove it.
Signed-off-by: Steffen Klassert <[email protected]>
---
crypto/pcrypt.c | 18 ++++++------------
kernel/padata.c | 11 +++++------
2 files changed, 11 insertions(+), 18 deletions(-)
diff --git a/crypto/pcrypt.c b/crypto/pcrypt.c
index 71ae2b2..6036b6d 100644
--- a/crypto/pcrypt.c
+++ b/crypto/pcrypt.c
@@ -143,10 +143,8 @@ static int pcrypt_aead_encrypt(struct aead_request *req)
aead_request_set_assoc(creq, req->assoc, req->assoclen);
err = pcrypt_do_parallel(padata, &ctx->cb_cpu, pcrypt_enc_padata);
- if (err)
- return err;
- else
- err = crypto_aead_encrypt(creq);
+ if (!err)
+ return -EINPROGRESS;
return err;
}
@@ -187,10 +185,8 @@ static int pcrypt_aead_decrypt(struct aead_request *req)
aead_request_set_assoc(creq, req->assoc, req->assoclen);
err = pcrypt_do_parallel(padata, &ctx->cb_cpu, pcrypt_dec_padata);
- if (err)
- return err;
- else
- err = crypto_aead_decrypt(creq);
+ if (!err)
+ return -EINPROGRESS;
return err;
}
@@ -233,10 +229,8 @@ static int pcrypt_aead_givencrypt(struct aead_givcrypt_request *req)
aead_givcrypt_set_giv(creq, req->giv, req->seq);
err = pcrypt_do_parallel(padata, &ctx->cb_cpu, pcrypt_enc_padata);
- if (err)
- return err;
- else
- err = crypto_aead_givencrypt(creq);
+ if (!err)
+ return -EINPROGRESS;
return err;
}
diff --git a/kernel/padata.c b/kernel/padata.c
index 57ec4eb..ae8defc 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -111,10 +111,13 @@ int padata_do_parallel(struct padata_instance *pinst,
pd = rcu_dereference(pinst->pd);
- err = 0;
+ err = -EINVAL;
if (!(pinst->flags & PADATA_INIT))
goto out;
+ if (!cpumask_test_cpu(cb_cpu, pd->cpumask))
+ goto out;
+
err = -EBUSY;
if ((pinst->flags & PADATA_RESET))
goto out;
@@ -122,11 +125,7 @@ int padata_do_parallel(struct padata_instance *pinst,
if (atomic_read(&pd->refcnt) >= MAX_OBJ_NUM)
goto out;
- err = -EINVAL;
- if (!cpumask_test_cpu(cb_cpu, pd->cpumask))
- goto out;
-
- err = -EINPROGRESS;
+ err = 0;
atomic_inc(&pd->refcnt);
padata->pd = pd;
padata->cb_cpu = cb_cpu;
--
1.5.6.5
This patch updates the padata documentation to the changed
API of padata_start/padata_stop and padata_do parallel.
Signed-off-by: Steffen Klassert <[email protected]>
---
Documentation/padata.txt | 21 +++++++++++----------
1 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/Documentation/padata.txt b/Documentation/padata.txt
index 269d7d0..3d77d09 100644
--- a/Documentation/padata.txt
+++ b/Documentation/padata.txt
@@ -22,12 +22,15 @@ actually be done; it should be a multithreaded queue, naturally.
There are functions for enabling and disabling the instance:
- void padata_start(struct padata_instance *pinst);
+ int padata_start(struct padata_instance *pinst);
void padata_stop(struct padata_instance *pinst);
-These functions literally do nothing beyond setting or clearing the
-"padata_start() was called" flag; if that flag is not set, other functions
-will refuse to work.
+These functions are setting or clearing the "PADATA_INIT" flag;
+if that flag is not set, other functions will refuse to work.
+padata_start returns zero on success (flag set) or -EINVAL if the
+padata cpumask contains no active cpu (flag not set).
+padata_stop clears the flag and blocks until the padata instance
+is unused.
The list of CPUs to be used can be adjusted with these functions:
@@ -63,12 +66,10 @@ The submission of work is done with:
The pinst and padata structures must be set up as described above; cb_cpu
specifies which CPU will be used for the final callback when the work is
done; it must be in the current instance's CPU mask. The return value from
-padata_do_parallel() is a little strange; zero is an error return
-indicating that the caller forgot the padata_start() formalities. -EBUSY
-means that somebody, somewhere else is messing with the instance's CPU
-mask, while -EINVAL is a complaint about cb_cpu not being in that CPU mask.
-If all goes well, this function will return -EINPROGRESS, indicating that
-the work is in progress.
+padata_do_parallel() is zero on success, indicating that the work is in
+progress. -EBUSY means that somebody, somewhere else is messing with the
+instance's CPU mask, while -EINVAL is a complaint about cb_cpu not being
+in that CPU mask or about a not running instance.
Each task submitted to padata_do_parallel() will, in turn, be passed to
exactly one call to the above-mentioned parallel() function, on one CPU, so
--
1.5.6.5
We count the number of processed objects on a percpu basis,
so we need to go through all the percpu reorder queues to calculate
the sequence number of the next object that needs serialization.
This patch changes this to count the number of processed objects
global. So we can calculate the sequence number and the percpu
reorder queue of the next object that needs serialization without
searching through the percpu reorder queues. This avoids some
accesses to memory of foreign cpus.
Signed-off-by: Steffen Klassert <[email protected]>
---
include/linux/padata.h | 6 ++--
kernel/padata.c | 71 +++++++++++++-----------------------------------
2 files changed, 22 insertions(+), 55 deletions(-)
diff --git a/include/linux/padata.h b/include/linux/padata.h
index e4c17f9..8844b85 100644
--- a/include/linux/padata.h
+++ b/include/linux/padata.h
@@ -67,7 +67,6 @@ struct padata_list {
* @pwork: work struct for parallelization.
* @swork: work struct for serialization.
* @pd: Backpointer to the internal control structure.
- * @num_obj: Number of objects that are processed by this cpu.
* @cpu_index: Index of the cpu.
*/
struct padata_queue {
@@ -77,7 +76,6 @@ struct padata_queue {
struct work_struct pwork;
struct work_struct swork;
struct parallel_data *pd;
- atomic_t num_obj;
int cpu_index;
};
@@ -93,6 +91,7 @@ struct padata_queue {
* @max_seq_nr: Maximal used sequence number.
* @cpumask: cpumask in use.
* @lock: Reorder lock.
+ * @processed: Number of already processed objects.
* @timer: Reorder timer.
*/
struct parallel_data {
@@ -103,7 +102,8 @@ struct parallel_data {
atomic_t refcnt;
unsigned int max_seq_nr;
cpumask_var_t cpumask;
- spinlock_t lock;
+ spinlock_t lock ____cacheline_aligned;
+ unsigned int processed;
struct timer_list timer;
};
diff --git a/kernel/padata.c b/kernel/padata.c
index ae8defc..450d67d 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -170,79 +170,47 @@ EXPORT_SYMBOL(padata_do_parallel);
*/
static struct padata_priv *padata_get_next(struct parallel_data *pd)
{
- int cpu, num_cpus, empty, calc_seq_nr;
- int seq_nr, next_nr, overrun, next_overrun;
+ int cpu, num_cpus;
+ int next_nr, next_index;
struct padata_queue *queue, *next_queue;
struct padata_priv *padata;
struct padata_list *reorder;
- empty = 0;
- next_nr = -1;
- next_overrun = 0;
- next_queue = NULL;
-
num_cpus = cpumask_weight(pd->cpumask);
- for_each_cpu(cpu, pd->cpumask) {
- queue = per_cpu_ptr(pd->queue, cpu);
- reorder = &queue->reorder;
-
- /*
- * Calculate the seq_nr of the object that should be
- * next in this reorder queue.
- */
- overrun = 0;
- calc_seq_nr = (atomic_read(&queue->num_obj) * num_cpus)
- + queue->cpu_index;
-
- if (unlikely(calc_seq_nr > pd->max_seq_nr)) {
- calc_seq_nr = calc_seq_nr - pd->max_seq_nr - 1;
- overrun = 1;
- }
-
- if (!list_empty(&reorder->list)) {
- padata = list_entry(reorder->list.next,
- struct padata_priv, list);
-
- seq_nr = padata->seq_nr;
- BUG_ON(calc_seq_nr != seq_nr);
- } else {
- seq_nr = calc_seq_nr;
- empty++;
- }
-
- if (next_nr < 0 || seq_nr < next_nr
- || (next_overrun && !overrun)) {
- next_nr = seq_nr;
- next_overrun = overrun;
- next_queue = queue;
- }
+ /*
+ * Calculate the percpu reorder queue and the sequence
+ * number of the next object.
+ */
+ next_nr = pd->processed;
+ next_index = next_nr % num_cpus;
+ cpu = padata_index_to_cpu(pd, next_index);
+ next_queue = per_cpu_ptr(pd->queue, cpu);
+
+ if (unlikely(next_nr > pd->max_seq_nr)) {
+ next_nr = next_nr - pd->max_seq_nr - 1;
+ next_index = next_nr % num_cpus;
+ cpu = padata_index_to_cpu(pd, next_index);
+ next_queue = per_cpu_ptr(pd->queue, cpu);
+ pd->processed = 0;
}
padata = NULL;
- if (empty == num_cpus)
- goto out;
-
reorder = &next_queue->reorder;
if (!list_empty(&reorder->list)) {
padata = list_entry(reorder->list.next,
struct padata_priv, list);
- if (unlikely(next_overrun)) {
- for_each_cpu(cpu, pd->cpumask) {
- queue = per_cpu_ptr(pd->queue, cpu);
- atomic_set(&queue->num_obj, 0);
- }
- }
+ BUG_ON(next_nr != padata->seq_nr);
spin_lock(&reorder->lock);
list_del_init(&padata->list);
atomic_dec(&pd->reorder_objects);
spin_unlock(&reorder->lock);
- atomic_inc(&next_queue->num_obj);
+ pd->processed++;
goto out;
}
@@ -430,7 +398,6 @@ static struct parallel_data *padata_alloc_pd(struct padata_instance *pinst,
INIT_WORK(&queue->pwork, padata_parallel_worker);
INIT_WORK(&queue->swork, padata_serial_worker);
- atomic_set(&queue->num_obj, 0);
}
num_cpus = cpumask_weight(pd->cpumask);
--
1.5.6.5
On Wed, Jul 07, 2010 at 03:29:15PM +0200, Steffen Klassert wrote:
> This patchset contains the following changes:
All applied to cryptodev. Thanks Steffen!
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt