padata: fixes/cleanups

PATCH In review Steffen Klassert 3 participants 15 messages Apr 30, 2010 – May 3, 2010 Padata parallel execution mechanism

lore

Share & tools

lore

Andrew Morton

2010-04-30 16:47 UTC #1 lore raw

In reply to Steffen Klassert

Quoted 3 lines · On Thu, 29 Apr 2010 14:36:36 +0200 Steffen Klassert…

This pachset is based on Andrew's padata review. It applies agains the cryptodev-2.6 tree. Btw. how to route future padata patches upstream, still via cryptodev-2.6?

That's OK by me. I'll often grab stuff (especially if I was cc'ed on it) and will drop it again if I see a subsystem maintainer took it. People use this to delegate the resend-to-sleepy-maintainer job to myself. Herbert is less sleepy than many ;)

Andrew Morton

2010-04-30 17:08 UTC #8 lore raw

Re: [PATCH 6/8] padata: Use a timer to handle the reorder queues

In reply to Steffen Klassert

Quoted 2 lines · On Thu, 29 Apr 2010 14:43:37 +0200 Steffen Klassert…

padata_get_next had a bogus check that returned always true, so the try_again loop in padata_reorder was never taken.

A better changelog would have told us what this "bogus check" _is_.

Quoted 1 line

This can lead to object leaks in some rare cases.

And a better changelog would describe those leaks!

Quoted 3 lines

This patch
implements a timer that processes the reorder queues if noone else does it in appropriate time.

Under what circumstances would "noone else do it in appropriate time"? Would that be a bug, or what?

Quoted 5 lines

kernel/padata.c +0 -1

@@ -273,13 +274,22 @@ try_again:
 
 	spin_unlock_bh(&pd->lock);
 
-	if (atomic_read(&pd->reorder_objects))

kernel/padata.c try_again: · line 284

Quoted 10 lines

kernel/padata.c +5 -2

-		goto try_again;
+	if (atomic_read(&pd->reorder_objects)
+			&& !(pinst->flags & PADATA_RESET))
+		mod_timer(&pd->timer, jiffies + HZ);
+	else
+		del_timer(&pd->timer);
 
-out:
 	return;
 }

I'd feel more comfortable if the above was in the locked region. Is there a race whereby another CPU can set pd->reorder_objects, but we forgot to arm the timer?

Steffen Klassert

author maintainer

2010-04-30 17:29 UTC #14 lore raw

PATCH 8/8 padata: Add some code comments

In reply to Steffen Klassert

Signed-off-by Steffen Klassert <[email protected]>

---
 include/linux/padata.h |   53 ++++++++++++++++++++++++++++++++++++++++++++++++
 kernel/padata.c        |   46 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 97 insertions(+), 2 deletions(-)

include/linux/padata.h +53 -0

index 64836a6..e8aac0f 100644
@@ -26,6 +26,17 @@
 #include <linux/list.h>
 #include <linux/timer.h>
 
+/*
+ * struct padata_priv -  Embedded to the users data structure.
+ *
+ * @list: List entry, to attach to the padata lists.
+ * @pd: Pointer to the internal control structure.
+ * @cb_cpu: Callback cpu for serializatioon.
+ * @seq_nr: Sequence number of the parallelized data object.
+ * @info: Used to pass information from the parallel to the serial function.
+ * @parallel: Parallel execution function.
+ * @serial: Serial complete function.
+ */
 struct padata_priv {
 	struct list_head	list;
 	struct parallel_data	*pd;

@@ -36,11 +47,29 @@ struct padata_priv {
 	void                    (*serial)(struct padata_priv *padata);
 };
 
+/*
+ * struct padata_list
+ *
+ * @list: List head.
+ * @lock: List lock.
+ */
 struct padata_list {
 	struct list_head        list;
 	spinlock_t              lock;
 };
 
+/*
+ * struct padata_queue - The percpu padata queues.
+ *
+ * @parallel: List to wait for parallelization.
+ * @reorder: List to wait for reordering after parallel processing.
+ * @serial: List to wait for serialization after reordering.
+ * @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 {
 	struct padata_list	parallel;
 	struct padata_list	reorder;

@@ -52,6 +81,20 @@ struct padata_queue {
 	int			cpu_index;
 };
 
+/*
+ * struct parallel_data - Internal control structure, covers everything
+ * that depends on the cpumask in use.
+ *
+ * @pinst: padata instance.
+ * @queue: percpu padata queues.
+ * @seq_nr: The sequence number that will be attached to the next object.
+ * @reorder_objects: Number of objects waiting in the reorder queues.
+ * @refcnt: Number of objects holding a reference on this parallel_data.
+ * @max_seq_nr:  Maximal used sequence number.
+ * @cpumask: cpumask in use.
+ * @lock: Reorder lock.
+ * @timer: Reorder timer.
+ */
 struct parallel_data {
 	struct padata_instance	*pinst;
 	struct padata_queue	*queue;

@@ -64,6 +107,16 @@ struct parallel_data {
 	struct timer_list       timer;
 };
 
+/*
+ * struct padata_instance - The overall control structure.
+ *
+ * @cpu_notifier: cpu hotplug notifier.
+ * @wq: The workqueue in use.
+ * @pd: The internal control structure.
+ * @cpumask: User supplied cpumask.
+ * @lock: padata instance lock.
+ * @flags: padata flags.
+ */
 struct padata_instance {
 	struct notifier_block   cpu_notifier;
 	struct workqueue_struct *wq;

kernel/padata.c +44 -2

103 lines not on this page · raw message

Signature · 1 line

1.5.6.5

Steffen Klassert

author maintainer

2010-04-30 17:29 UTC #13 lore raw

Re: [PATCH 7/8] padata: Flush the padata queues actively

In reply to Andrew Morton

Quoted 12 lines · Andrew Morton

Quoted 9 lines · On Thu, 29 Apr 2010 14:44:26 +0200 Steffen Klassert…

kernel/padata.c +9 -0

+static void padata_flush_queues(struct parallel_data *pd)
+{
+	int cpu;
+	struct padata_queue *queue;
+
+	for_each_cpu(cpu, pd->cpumask) {
+		queue = per_cpu_ptr(pd->queue, cpu);
+		flush_work(&queue->pwork);
+	}

Quoted 9 lines

kernel/padata.c static void padata_free_pd(struct parallel_data *pd) · line 433
Quoted 5 lines
kernel/padata.c +5 -0
+
+	del_timer_sync(&pd->timer);
+
+	if (atomic_read(&pd->reorder_objects))
+		padata_reorder(pd);
padata_reorder() can fail to do anything, if someone else is holding pd->lock. What happens then?

padata does not accept new objects for parallelization if padata_flush_queues is called. The way of the data objects throught the padata queues is

--> parallelization queue -> reorder queue -> serialization queue -->

So padata_flush_queues processes the objects in the parallelization queue by doing flush_work(&queue->pwork). Then we delete the timer and wait on a potentially running timer function. We are not accepting new objects and the parallelization queue is empty, so the lock must be free then.

Quoted 9 lines

kernel/padata.c static void padata_free_pd(struct parallel_data *pd) · line 441
Quoted 7 lines
kernel/padata.c +6 -0
+	for_each_cpu(cpu, pd->cpumask) {
+		queue = per_cpu_ptr(pd->queue, cpu);
+		flush_work(&queue->swork);
+	}
+	BUG_ON(atomic_read(&pd->refcnt) != 0);
+}
Are we safe against cpu hot-unplug in this code?

padata_flush_queues is called after a call to get_online_cpus in all but one cases. I just noticed that I forgot to add the get_online_cpus/put_online_cpus in padata_free. I'll update the get_online_cpus/put_online_cpus patch accordingly, then it should be save in all cases.

Steffen Klassert

author maintainer

2010-04-30 18:02 UTC #0 lore raw

cover padata: fixes/cleanups

This pachset is based on Andrew's padata review. It applies agains the cryptodev-2.6 tree. Btw. how to route future padata patches upstream, still via cryptodev-2.6?

Steffen

Steffen Klassert

author maintainer

2010-04-30 18:04 UTC #5 lore raw

PATCH 4/8 padata: Initialize the padata queues only for the used cpus

padata_alloc_pd set up queues for all possible cpus. This patch changes this to set up the queues just for the used cpus.

Signed-off-by Steffen Klassert <[email protected]>

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

kernel/padata.c +5 -9

index 5fa6ba6..fc9f19a 100644
@@ -358,17 +358,15 @@ static struct parallel_data *padata_alloc_pd(struct padata_instance *pinst,
 	if (!alloc_cpumask_var(&pd->cpumask, GFP_KERNEL))
 		goto err_free_queue;
 
-	for_each_possible_cpu(cpu) {
+	cpumask_and(pd->cpumask, cpumask, cpu_active_mask);
+
+	for_each_cpu(cpu, pd->cpumask) {
 		queue = per_cpu_ptr(pd->queue, cpu);
 
 		queue->pd = pd;
 
-		if (cpumask_test_cpu(cpu, cpumask)
-		    && cpumask_test_cpu(cpu, cpu_active_mask)) {
-			queue->cpu_index = cpu_index;
-			cpu_index++;
-		} else
-			queue->cpu_index = -1;
+		queue->cpu_index = cpu_index;
+		cpu_index++;
 
 		INIT_LIST_HEAD(&queue->reorder.list);
 		INIT_LIST_HEAD(&queue->parallel.list);

@@ -382,8 +380,6 @@ static struct parallel_data *padata_alloc_pd(struct padata_instance *pinst,
 		atomic_set(&queue->num_obj, 0);
 	}
 
-	cpumask_and(pd->cpumask, cpumask, cpu_active_mask);
-
 	num_cpus = cpumask_weight(pd->cpumask);
 	pd->max_seq_nr = (MAX_SEQ_NR / num_cpus) * num_cpus - 1;

Signature · 1 line

1.5.6.5

Steffen Klassert

author maintainer

2010-04-30 18:04 UTC #3 lore raw

PATCH 2/8 padata: cpu hotplug code should depend on CONFIG_HOTPLUG_CPU

In reply to Steffen Klassert

This patch makes the padata cpu hotplug code dependend on CONFIG_HOTPLUG_CPU.

Signed-off-by Steffen Klassert <[email protected]>

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

kernel/padata.c +7 -6

index 5b44d0f..1209a17 100644
@@ -570,6 +570,7 @@ void padata_stop(struct padata_instance *pinst)
 }
 EXPORT_SYMBOL(padata_stop);
 
+#ifdef CONFIG_HOTPLUG_CPU
 static int padata_cpu_callback(struct notifier_block *nfb,
 			       unsigned long action, void *hcpu)
 {

@@ -621,6 +622,7 @@ static int padata_cpu_callback(struct notifier_block *nfb,
 
 	return NOTIFY_OK;
 }
+#endif
 
 /*
  * padata_alloc - allocate and initialize a padata instance

@@ -631,7 +633,6 @@ static int padata_cpu_callback(struct notifier_block *nfb,
 struct padata_instance *padata_alloc(const struct cpumask *cpumask,
 				     struct workqueue_struct *wq)
 {
-	int err;
 	struct padata_instance *pinst;
 	struct parallel_data *pd;

@@ -654,18 +655,16 @@ struct padata_instance *padata_alloc(const struct cpumask *cpumask,
 
 	pinst->flags = 0;
 
+#ifdef CONFIG_HOTPLUG_CPU
 	pinst->cpu_notifier.notifier_call = padata_cpu_callback;
 	pinst->cpu_notifier.priority = 0;
-	err = register_hotcpu_notifier(&pinst->cpu_notifier);
-	if (err)
-		goto err_free_cpumask;
+	register_hotcpu_notifier(&pinst->cpu_notifier);
+#endif
 
 	mutex_init(&pinst->lock);
 
 	return pinst;
 
-err_free_cpumask:
-	free_cpumask_var(pinst->cpumask);
 err_free_pd:
 	padata_free_pd(pd);
 err_free_inst:

@@ -689,7 +688,9 @@ void padata_free(struct padata_instance *pinst)
 	while (atomic_read(&pinst->pd->refcnt) != 0)
 		yield();
 
+#ifdef CONFIG_HOTPLUG_CPU
 	unregister_hotcpu_notifier(&pinst->cpu_notifier);
+#endif
 	padata_free_pd(pinst->pd);
 	free_cpumask_var(pinst->cpumask);
 	kfree(pinst);

Signature · 1 line

1.5.6.5

Steffen Klassert

author maintainer

2010-04-30 18:05 UTC #2 lore raw

PATCH 1/8 padata: Dont scale the parallel objects with the cpus

In reply to Steffen Klassert

Scaling the maximum number of objects in the parallel codepath can lead to out of memory problems on bigsmp machines.

Signed-off-by Steffen Klassert <[email protected]>

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

kernel/padata.c +1 -1

index 70b5d5e..5b44d0f 100644
@@ -29,7 +29,7 @@
 #include <linux/rcupdate.h>
 
 #define MAX_SEQ_NR INT_MAX - NR_CPUS
-#define MAX_OBJ_NUM 10000 * NR_CPUS
+#define MAX_OBJ_NUM 1000
 
 static int padata_index_to_cpu(struct parallel_data *pd, int cpu_index)
 {

Signature · 1 line

1.5.6.5

Steffen Klassert

author maintainer

2010-04-30 18:06 UTC #4 lore raw

PATCH 3/8 padata: Remove superfluous might_sleep

In reply to Steffen Klassert

might_sleep() was placed before mutex_lock() in some places. We remove them because mutex_lock() does might_sleep() too.

Signed-off-by Steffen Klassert <[email protected]>

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

kernel/padata.c +0 ~~-10~~

index 1209a17..5fa6ba6 100644
@@ -443,8 +443,6 @@ int padata_set_cpumask(struct padata_instance *pinst,
 	struct parallel_data *pd;
 	int err = 0;
 
-	might_sleep();
-
 	mutex_lock(&pinst->lock);
 
 	pd = padata_alloc_pd(pinst, cpumask);

@@ -489,8 +487,6 @@ int padata_add_cpu(struct padata_instance *pinst, int cpu)
 {
 	int err;
 
-	might_sleep();
-
 	mutex_lock(&pinst->lock);
 
 	cpumask_set_cpu(cpu, pinst->cpumask);

@@ -527,8 +523,6 @@ int padata_remove_cpu(struct padata_instance *pinst, int cpu)
 {
 	int err;
 
-	might_sleep();
-
 	mutex_lock(&pinst->lock);
 
 	cpumask_clear_cpu(cpu, pinst->cpumask);

@@ -547,8 +541,6 @@ EXPORT_SYMBOL(padata_remove_cpu);
  */
 void padata_start(struct padata_instance *pinst)
 {
-	might_sleep();
-
 	mutex_lock(&pinst->lock);
 	pinst->flags |= PADATA_INIT;
 	mutex_unlock(&pinst->lock);

@@ -562,8 +554,6 @@ EXPORT_SYMBOL(padata_start);
  */
 void padata_stop(struct padata_instance *pinst)
 {
-	might_sleep();
-
 	mutex_lock(&pinst->lock);
 	pinst->flags &= ~PADATA_INIT;
 	mutex_unlock(&pinst->lock);

Signature · 1 line

1.5.6.5

Steffen Klassert

author maintainer

2010-04-30 18:18 UTC #7 lore raw

PATCH 6/8 padata: Use a timer to handle the reorder queues

In reply to Steffen Klassert

padata_get_next had a bogus check that returned always true, so the try_again loop in padata_reorder was never taken. This can lead to object leaks in some rare cases. This patch implements a timer that processes the reorder queues if noone else does it in appropriate time.

Signed-off-by Steffen Klassert <[email protected]>

---
 include/linux/padata.h |    2 ++
 kernel/padata.c        |   25 ++++++++++++++++++-------
 2 files changed, 20 insertions(+), 7 deletions(-)

include/linux/padata.h +2 -0

index 51611da..64836a6 100644
@@ -24,6 +24,7 @@
 #include <linux/workqueue.h>
 #include <linux/spinlock.h>
 #include <linux/list.h>
+#include <linux/timer.h>
 
 struct padata_priv {
 	struct list_head	list;

@@ -60,6 +61,7 @@ struct parallel_data {
 	unsigned int		max_seq_nr;
 	cpumask_var_t		cpumask;
 	spinlock_t              lock;
+	struct timer_list       timer;
 };
 
 struct padata_instance {

kernel/padata.c +18 -7

index 82958e0..6d7ea48 100644
@@ -231,7 +231,8 @@ static struct padata_priv *padata_get_next(struct parallel_data *pd)
 		goto out;
 	}
 
-	if (next_nr % num_cpus == next_queue->cpu_index) {
+	queue = per_cpu_ptr(pd->queue, smp_processor_id());
+	if (queue->cpu_index == next_queue->cpu_index) {
 		padata = ERR_PTR(-ENODATA);
 		goto out;
 	}

@@ -247,9 +248,8 @@ static void padata_reorder(struct parallel_data *pd)
 	struct padata_queue *queue;
 	struct padata_instance *pinst = pd->pinst;
 
-try_again:
 	if (!spin_trylock_bh(&pd->lock))
-		goto out;
+		return;
 
 	while (1) {
 		padata = padata_get_next(pd);

@@ -258,8 +258,9 @@ try_again:
 			break;
 
 		if (PTR_ERR(padata) == -ENODATA) {
+			del_timer(&pd->timer);
 			spin_unlock_bh(&pd->lock);
-			goto out;
+			return;
 		}
 
 		queue = per_cpu_ptr(pd->queue, padata->cb_cpu);

@@ -273,13 +274,22 @@ try_again:
 
 	spin_unlock_bh(&pd->lock);
 
-	if (atomic_read(&pd->reorder_objects))
-		goto try_again;
+	if (atomic_read(&pd->reorder_objects)
+			&& !(pinst->flags & PADATA_RESET))
+		mod_timer(&pd->timer, jiffies + HZ);
+	else
+		del_timer(&pd->timer);
 
-out:
 	return;
 }
 
+static void padata_reorder_timer(unsigned long arg)
+{
+	struct parallel_data *pd = (struct parallel_data *)arg;
+
+	padata_reorder(pd);
+}
+
 static void padata_serial_worker(struct work_struct *work)
 {
 	struct padata_queue *queue;

@@ -383,6 +393,7 @@ static struct parallel_data *padata_alloc_pd(struct padata_instance *pinst,
 	num_cpus = cpumask_weight(pd->cpumask);
 	pd->max_seq_nr = (MAX_SEQ_NR / num_cpus) * num_cpus - 1;
 
+	setup_timer(&pd->timer, padata_reorder_timer, (unsigned long)pd);
 	atomic_set(&pd->seq_nr, -1);
 	atomic_set(&pd->reorder_objects, 0);
 	atomic_set(&pd->refcnt, 0);

Signature · 1 line

1.5.6.5

Steffen Klassert

author maintainer

2010-04-30 18:18 UTC #6 lore raw

PATCH 5/8 padata: Use get_online_cpus/put_online_cpus

In reply to Steffen Klassert

This patch puts get_online_cpus/put_online_cpus around the places we modify the padata cpumask to ensure that no cpu goes offline during this operation.

Signed-off-by Steffen Klassert <[email protected]>

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

kernel/padata.c +13 -0

index fc9f19a..82958e0 100644
@@ -441,6 +441,8 @@ int padata_set_cpumask(struct padata_instance *pinst,
 
 	mutex_lock(&pinst->lock);
 
+	get_online_cpus();
+
 	pd = padata_alloc_pd(pinst, cpumask);
 	if (!pd) {
 		err = -ENOMEM;

@@ -452,6 +454,8 @@ int padata_set_cpumask(struct padata_instance *pinst,
 	padata_replace(pinst, pd);
 
 out:
+	put_online_cpus();
+
 	mutex_unlock(&pinst->lock);
 
 	return err;

@@ -485,8 +489,10 @@ int padata_add_cpu(struct padata_instance *pinst, int cpu)
 
 	mutex_lock(&pinst->lock);
 
+	get_online_cpus();
 	cpumask_set_cpu(cpu, pinst->cpumask);
 	err = __padata_add_cpu(pinst, cpu);
+	put_online_cpus();
 
 	mutex_unlock(&pinst->lock);

@@ -521,8 +527,10 @@ int padata_remove_cpu(struct padata_instance *pinst, int cpu)
 
 	mutex_lock(&pinst->lock);
 
+	get_online_cpus();
 	cpumask_clear_cpu(cpu, pinst->cpumask);
 	err = __padata_remove_cpu(pinst, cpu);
+	put_online_cpus();
 
 	mutex_unlock(&pinst->lock);

@@ -626,6 +634,8 @@ struct padata_instance *padata_alloc(const struct cpumask *cpumask,
 	if (!pinst)
 		goto err;
 
+	get_online_cpus();
+
 	pd = padata_alloc_pd(pinst, cpumask);
 	if (!pd)
 		goto err_free_inst;

@@ -647,6 +657,8 @@ struct padata_instance *padata_alloc(const struct cpumask *cpumask,
 	register_hotcpu_notifier(&pinst->cpu_notifier);
 #endif
 
+	put_online_cpus();
+
 	mutex_init(&pinst->lock);
 
 	return pinst;

@@ -655,6 +667,7 @@ err_free_pd:
 	padata_free_pd(pd);
 err_free_inst:
 	kfree(pinst);
+	put_online_cpus();
 err:
 	return NULL;
 }

Signature · 1 line

1.5.6.5

Steffen Klassert

author maintainer

2010-04-30 18:18 UTC #9 lore raw

Re: [PATCH 6/8] padata: Use a timer to handle the reorder queues

In reply to Andrew Morton

Quoted 11 lines · Andrew Morton

Quoted 2 lines · On Thu, 29 Apr 2010 14:43:37 +0200 Steffen Klassert…
padata_get_next had a bogus check that returned always true, so the try_again loop in padata_reorder was never taken.
A better changelog would have told us what this "bogus check" _is_.
Quoted 1 line
This can lead to object leaks in some rare cases.
And a better changelog would describe those leaks!

I'll try to write a better one and resent.

Quoted 8 lines

Quoted 4 lines
This patch
implements a timer that processes the reorder queues if noone else does it in appropriate time.
Under what circumstances would "noone else do it in appropriate time"? Would that be a bug, or what?

We need to ensure that only one cpu can work on dequeueing of the reorder queue the time. Calculating in which percpu reorder queue the next object will arrive takes some time. A spinlock would be highly contended. Also it is not clear in which order the objects arrive to the reorder queues. So a cpu could wait to get the lock just to notice that there is nothing to do at the moment. Therefore we use a trylock and let the holder of the lock care for all the objects enqueued during the holdtime of the lock.

The timer is to handle a race that appears with the trylock. If cpu1 queues an object to the reorder queue while cpu2 holds the pd->lock but left the while loop in padata_reorder already, cpu2 can't care for this object but cpu1 exits because it can't get the lock. Usually the next cpu that takes the lock cares for this object too. We need the timer just if this object was the last one that arrives to the reorder queues. The timer function sends it out in this case.

Quoted 12 lines

kernel/padata.c +5 -2

@@ -273,13 +274,22 @@ try_again:
 
 	spin_unlock_bh(&pd->lock);
 
-	if (atomic_read(&pd->reorder_objects))
-		goto try_again;
+	if (atomic_read(&pd->reorder_objects)
+			&& !(pinst->flags & PADATA_RESET))
+		mod_timer(&pd->timer, jiffies + HZ);
+	else
+		del_timer(&pd->timer);

Quoted 8 lines

kernel/padata.c try_again: · line 284
Quoted 3 lines
kernel/padata.c +0 -1
-out:
 	return;
 }
I'd feel more comfortable if the above was in the locked region. Is there a race whereby another CPU can set pd->reorder_objects, but we forgot to arm the timer?

We could hit the race that the timer handles, if we move this into the lock.

cpu1 cpu2

spin_trylock_bh()

      |
      |
      |
test pd->reorder_objects == 0
delete timer
      |
hardinterrupt
      |                         set pd->reorder_objects == 1
      |                         enqueue object
      |                         spin_trylock_bh() busy
      |                         exit
      |

spin_unlock_bh()

Steffen Klassert

author maintainer

2010-04-30 18:19 UTC #11 lore raw

PATCH 7/8 padata: Flush the padata queues actively

In reply to Steffen Klassert

yield was used to wait until all references of the internal control structure in use are dropped before it is freed. This patch implements padata_flush_queues which actively flushes the padata percpu queues in this case.

Signed-off-by Steffen Klassert <[email protected]>

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

kernel/padata.c +25 -8

index 6d7ea48..ec6b8b7 100644
@@ -417,6 +417,29 @@ static void padata_free_pd(struct parallel_data *pd)
 	kfree(pd);
 }
 
+static void padata_flush_queues(struct parallel_data *pd)
+{
+	int cpu;
+	struct padata_queue *queue;
+
+	for_each_cpu(cpu, pd->cpumask) {
+		queue = per_cpu_ptr(pd->queue, cpu);
+		flush_work(&queue->pwork);
+	}
+
+	del_timer_sync(&pd->timer);
+
+	if (atomic_read(&pd->reorder_objects))
+		padata_reorder(pd);
+
+	for_each_cpu(cpu, pd->cpumask) {
+		queue = per_cpu_ptr(pd->queue, cpu);
+		flush_work(&queue->swork);
+	}
+
+	BUG_ON(atomic_read(&pd->refcnt) != 0);
+}
+
 static void padata_replace(struct padata_instance *pinst,
 			   struct parallel_data *pd_new)
 {

@@ -428,11 +451,7 @@ static void padata_replace(struct padata_instance *pinst,
 
 	synchronize_rcu();
 
-	while (atomic_read(&pd_old->refcnt) != 0)
-		yield();
-
-	flush_workqueue(pinst->wq);
-
+	padata_flush_queues(pd_old);
 	padata_free_pd(pd_old);
 
 	pinst->flags &= ~PADATA_RESET;

@@ -695,12 +714,10 @@ void padata_free(struct padata_instance *pinst)
 
 	synchronize_rcu();
 
-	while (atomic_read(&pinst->pd->refcnt) != 0)
-		yield();
-
 #ifdef CONFIG_HOTPLUG_CPU
 	unregister_hotcpu_notifier(&pinst->cpu_notifier);
 #endif
+	padata_flush_queues(pinst->pd);
 	padata_free_pd(pinst->pd);
 	free_cpumask_var(pinst->cpumask);
 	kfree(pinst);

Signature · 1 line

1.5.6.5

Andrew Morton

2010-04-30 18:54 UTC #12 lore raw

Re: [PATCH 7/8] padata: Flush the padata queues actively

Quoted 4 lines · On Thu, 29 Apr 2010 14:44:26 +0200 Steffen Klassert…

kernel/padata.c +4 -0

+static void padata_flush_queues(struct parallel_data *pd)
+{
+	int cpu;
+	struct padata_queue *queue;

kernel/padata.c static void padata_free_pd(struct parallel_data *pd) · line 433

Quoted 10 lines

kernel/padata.c +10 -0

+
+	for_each_cpu(cpu, pd->cpumask) {
+		queue = per_cpu_ptr(pd->queue, cpu);
+		flush_work(&queue->pwork);
+	}
+
+	del_timer_sync(&pd->timer);
+
+	if (atomic_read(&pd->reorder_objects))
+		padata_reorder(pd);

padata_reorder() can fail to do anything, if someone else is holding pd->lock. What happens then?

kernel/padata.c static void padata_free_pd(struct parallel_data *pd) · line 441

Quoted 6 lines

kernel/padata.c +6 -0

+	for_each_cpu(cpu, pd->cpumask) {
+		queue = per_cpu_ptr(pd->queue, cpu);
+		flush_work(&queue->swork);
+	}
+	BUG_ON(atomic_read(&pd->refcnt) != 0);
+}

Are we safe against cpu hot-unplug in this code?

Herbert Xu

2010-05-03 03:38 UTC #10 lore raw

Re: [PATCH 6/8] padata: Use a timer to handle the reorder queues

In reply to Steffen Klassert

Quoted 7 lines · Steffen Klassert

Quoted 6 lines · Andrew Morton
Quoted 2 lines · On Thu, 29 Apr 2010 14:43:37 +0200 Steffen Klassert…
padata_get_next had a bogus check that returned always true, so the try_again loop in padata_reorder was never taken.

Quoted 7 lines

Quoted 5 lines
A better changelog would have told us what this "bogus check" _is_.
Quoted 1 line
This can lead to object leaks in some rare cases.
And a better changelog would describe those leaks!
I'll try to write a better one and resent.

I've picked up patches 1-5.

Thanks!

Signature · 4 lines

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt