This is my third attempt to send padata cpumasks patchset.
The patchset includes fixes of all unclear things Steffen noted in previous two patchsets.
Changes:
1) Make two cpumasks in padata instead of one. The first cpumask is used by parallel workers and
another is used by the workers doing serialization. Two distinguish cpumasks perform to build
configuration where CPUs used by parallel and serial workers aren't intersect. It significantly
improves performance.
Each padata instance now includes notifier chain which can be used by users interested in instance's
cpumask(serial or parallel) change. If one of cpumask is changed an event is generated.
2) Add sysfs primitives to padata. Each padata instance contains kobject which can be embedded to any
proper sysfs hierarchy. Padata kobject can be used to change or show serial or parallel cpumask.
3) Add sysfs representation to pcrypt. Pcrypt now creates /sys/kernel/pcrypt/[pencrypt|pdecrypt] during
module loading phase. pencrypt and pdecrypt directories are represented by kobjects of padata instances
that belongs to pencrypt and pdecrypt respectively. Using this sysfs interface user can change and read
serial and parallel cpumasks of both instances.
--
W.B.R.
Dan Kruchinin
On Wed, Jul 14, 2010 at 02:29:51PM +0400, Dan Kruchinin wrote:
> This is my third attempt to send padata cpumasks patchset.
> The patchset includes fixes of all unclear things Steffen noted in previous two patchsets.
> Changes:
>
> 1) Make two cpumasks in padata instead of one. The first cpumask is used by parallel workers and
> another is used by the workers doing serialization. Two distinguish cpumasks perform to build
> configuration where CPUs used by parallel and serial workers aren't intersect. It significantly
> improves performance.
> Each padata instance now includes notifier chain which can be used by users interested in instance's
> cpumask(serial or parallel) change. If one of cpumask is changed an event is generated.
>
> 2) Add sysfs primitives to padata. Each padata instance contains kobject which can be embedded to any
> proper sysfs hierarchy. Padata kobject can be used to change or show serial or parallel cpumask.
>
> 3) Add sysfs representation to pcrypt. Pcrypt now creates /sys/kernel/pcrypt/[pencrypt|pdecrypt] during
> module loading phase. pencrypt and pdecrypt directories are represented by kobjects of padata instances
> that belongs to pencrypt and pdecrypt respectively. Using this sysfs interface user can change and read
> serial and parallel cpumasks of both instances.
All applied. Thanks!
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Mon, Jul 19, 2010 at 02:04:26PM +0800, Herbert Xu wrote:
>
> All applied. Thanks!
Hm, I was just about to write some comments on these patches,
they still have some issues. For example, handling empty cpumasks
is broken again (NULL pointer dereference in padata_replace).
The cpu_index is zero for all cpus now, so can we leak objects
on cpu hotplug etc. Also I'm not that happy with some of the
API changes. I'll try to fix this up with some patches.
Steffen
On Mon, Jul 19, 2010 at 08:40:50AM +0200, Steffen Klassert wrote:
> On Mon, Jul 19, 2010 at 02:04:26PM +0800, Herbert Xu wrote:
> >
> > All applied. Thanks!
>
> Hm, I was just about to write some comments on these patches,
> they still have some issues. For example, handling empty cpumasks
> is broken again (NULL pointer dereference in padata_replace).
> The cpu_index is zero for all cpus now, so can we leak objects
> on cpu hotplug etc. Also I'm not that happy with some of the
> API changes. I'll try to fix this up with some patches.
OK I'll take your fixes on top when you're ready.
Thanks!
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
This patchset contains the following fixes:
1. padata - Fix a potential hang of the parallel worker threads on
cpu hotplug.
2. padata - Fix two NULL pointer dereference crashes if one of the
padata cpumasks is empty. The crashes can be triggered by doing
echo 0 > /sys/kernel/pcrypt/pencrypt/[parallel_cpumask/serial_cpumask]
3. padata - Fix a division by zero crash if the parallel cpumask
contains no active cpu. Can be triggered by doing
echo 0 > /sys/kernel/pcrypt/pencrypt/parallel_cpumask
4. pcrypt - Fix division by zero crash if the callback cpumask
is empty. Can be triggered by doing
echo 0 > /sys/kernel/pcrypt/pencrypt/serial_cpumask
I ran a script that does cpumask changes and cpu hotplugs as fast as
possible while sending IPsec packets on maximal rate with pcrypt.
It showed no furter crashes overnight, so I hope I've got them all.
padata API corrections and some minor fixes/cleanups are comming later.
Steffen
The counting of the cpu index got lost with a recent commit.
This patch restores it. This fixes a hang of the parallel worker
threads on cpu hotplug.
Signed-off-by: Steffen Klassert <[email protected]>
---
kernel/padata.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/kernel/padata.c b/kernel/padata.c
index 526f9ea..4287868 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -408,6 +408,7 @@ static void padata_init_pqueues(struct parallel_data *pd)
pqueue = per_cpu_ptr(pd->pqueue, cpu);
pqueue->pd = pd;
pqueue->cpu_index = cpu_index;
+ cpu_index++;
__padata_list_init(&pqueue->reorder);
__padata_list_init(&pqueue->parallel);
--
1.5.6.5
The cpumask separation work assumes the cpumask dependend recources
present regardless of valid or invalid cpumasks. With this patch
we allocate the cpumask dependend recources in any case. This fixes
two NULL pointer dereference crashes in padata_replace and in
padata_get_cpumask.
Signed-off-by: Steffen Klassert <[email protected]>
---
kernel/padata.c | 24 +++++++-----------------
1 files changed, 7 insertions(+), 17 deletions(-)
diff --git a/kernel/padata.c b/kernel/padata.c
index 4287868..6a51945 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -417,7 +417,7 @@ static void padata_init_pqueues(struct parallel_data *pd)
}
num_cpus = cpumask_weight(pd->cpumask.pcpu);
- pd->max_seq_nr = (MAX_SEQ_NR / num_cpus) * num_cpus - 1;
+ pd->max_seq_nr = num_cpus ? (MAX_SEQ_NR / num_cpus) * num_cpus - 1 : 0;
}
/* Allocate and initialize the internal cpumask dependend resources. */
@@ -527,21 +527,19 @@ static void padata_replace(struct padata_instance *pinst,
rcu_assign_pointer(pinst->pd, pd_new);
synchronize_rcu();
- if (!pd_old)
- goto out;
- padata_flush_queues(pd_old);
if (!cpumask_equal(pd_old->cpumask.pcpu, pd_new->cpumask.pcpu))
notification_mask |= PADATA_CPU_PARALLEL;
if (!cpumask_equal(pd_old->cpumask.cbcpu, pd_new->cpumask.cbcpu))
notification_mask |= PADATA_CPU_SERIAL;
+ padata_flush_queues(pd_old);
padata_free_pd(pd_old);
+
if (notification_mask)
blocking_notifier_call_chain(&pinst->cpumask_change_notifier,
notification_mask, pinst);
-out:
pinst->flags &= ~PADATA_RESET;
}
@@ -673,6 +671,7 @@ int __padata_set_cpumasks(struct padata_instance *pinst,
struct parallel_data *pd = NULL;
mutex_lock(&pinst->lock);
+ get_online_cpus();
valid = padata_validate_cpumask(pinst, pcpumask);
if (!valid) {
@@ -681,20 +680,16 @@ int __padata_set_cpumasks(struct padata_instance *pinst,
}
valid = padata_validate_cpumask(pinst, cbcpumask);
- if (!valid) {
+ if (!valid)
__padata_stop(pinst);
- goto out_replace;
- }
-
- get_online_cpus();
+out_replace:
pd = padata_alloc_pd(pinst, pcpumask, cbcpumask);
if (!pd) {
err = -ENOMEM;
goto out;
}
-out_replace:
cpumask_copy(pinst->cpumask.pcpu, pcpumask);
cpumask_copy(pinst->cpumask.cbcpu, cbcpumask);
@@ -705,7 +700,6 @@ out_replace:
out:
put_online_cpus();
-
mutex_unlock(&pinst->lock);
return err;
@@ -776,11 +770,8 @@ static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
if (cpumask_test_cpu(cpu, cpu_online_mask)) {
if (!padata_validate_cpumask(pinst, pinst->cpumask.pcpu) ||
- !padata_validate_cpumask(pinst, pinst->cpumask.cbcpu)) {
+ !padata_validate_cpumask(pinst, pinst->cpumask.cbcpu))
__padata_stop(pinst);
- padata_replace(pinst, pd);
- goto out;
- }
pd = padata_alloc_pd(pinst, pinst->cpumask.pcpu,
pinst->cpumask.cbcpu);
@@ -790,7 +781,6 @@ static int __padata_remove_cpu(struct padata_instance *pinst, int cpu)
padata_replace(pinst, pd);
}
-out:
return 0;
}
--
1.5.6.5
Now that we allow to change the cpumasks from userspace, we have
to check for valid cpumasks in padata_do_parallel. This patch adds
the necessary check. This fixes a division by zero crash if the
parallel cpumask contains no active cpu.
Signed-off-by: Steffen Klassert <[email protected]>
---
kernel/padata.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/padata.c b/kernel/padata.c
index 6a51945..7f895e2 100644
--- a/kernel/padata.c
+++ b/kernel/padata.c
@@ -114,7 +114,7 @@ int padata_do_parallel(struct padata_instance *pinst,
pd = rcu_dereference(pinst->pd);
err = -EINVAL;
- if (!(pinst->flags & PADATA_INIT))
+ if (!(pinst->flags & PADATA_INIT) || pinst->flags & PADATA_INVALID)
goto out;
if (!cpumask_test_cpu(cb_cpu, pd->cpumask.cbcpu))
--
1.5.6.5
If the callback cpumask is empty, we crash with a division by zero
when we try to calculate a callback cpu. So we don't update the callback
cpu in pcrypt_do_parallel if the callback cpumask is empty.
Signed-off-by: Steffen Klassert <[email protected]>
---
crypto/pcrypt.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/crypto/pcrypt.c b/crypto/pcrypt.c
index 7153a50..794c172 100644
--- a/crypto/pcrypt.c
+++ b/crypto/pcrypt.c
@@ -82,6 +82,9 @@ static int pcrypt_do_parallel(struct padata_priv *padata, unsigned int *cb_cpu,
if (cpumask_test_cpu(cpu, cpumask->mask))
goto out;
+ if (!cpumask_weight(cpumask->mask))
+ goto out;
+
cpu_index = cpu % cpumask_weight(cpumask->mask);
cpu = cpumask_first(cpumask->mask);
--
1.5.6.5
On Tue, Jul 20, 2010 at 08:47:36AM +0200, Steffen Klassert wrote:
> This patchset contains the following fixes:
All applied. 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