padata: cpumasks

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

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

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

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.

---
 kernel/padata.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index 526f9ea..4287868 100644
@@ -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.

---
 kernel/padata.c |   24 +++++++-----------------
 1 files changed, 7 insertions(+), 17 deletions(-)

index 4287868..6a51945 100644
@@ -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.

---
 kernel/padata.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 6a51945..7f895e2 100644
@@ -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.

---
 crypto/pcrypt.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

index 7153a50..794c172 100644
@@ -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

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