2014-07-16 06:55:48

by Lai Jiangshan

Subject: [PATCH] workqueue: detach rescuer from pool until the last

In 51697d393922 ("workqueue: use generic attach/detach routine for rescuers"),
The rescuer detaches itself from the pool before put_pwq() so that
the put_unbound_pool() will not destroy the rescuer-attached pool.

It is unnecessary, worker_detach_from_pool() can be used as the last
statement to access to the pool as the same as the regular workers,
put_unbound_pool() will wait for it to detach and then free the pool.

So we move the worker_detach_from_pool() down, make it coincide with
the regular workers.

Signed-off-by: Lai Jiangshan <[email protected]>
---
This old logic is introduced by the same-purpose but un-accepted patch:
https://lkml.org/lkml/2014/4/12/26

In the old patch, put_unbound_pool() still synchronizes with the workers
exiting via kthread_stop() which means put_unbound_pool() can't destroy the
rescuer-attached pool, so the put_pwq() should be the last statement
for the rescuer.

And after several versions of the patchset, the put_unbound_pool() was
re-implemented and it can wait any un-detached worker including the
rescuers, but the above mentioned old logic is still kept in the code.

The old logic is still correct now, but not grace nor necessary.

kernel/workqueue.c | 12 +++++-------
1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index a791a8c..dfd9f68 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2300,11 +2300,6 @@ repeat:
move_linked_works(work, scheduled, &n);

process_scheduled_works(rescuer);
- spin_unlock_irq(&pool->lock);
-
- worker_detach_from_pool(rescuer, pool);
-
- spin_lock_irq(&pool->lock);

/*
* Put the reference grabbed by send_mayday(). @pool won't
@@ -2322,8 +2317,11 @@ repeat:
wake_up_worker(pool);

rescuer->pool = NULL;
- spin_unlock(&pool->lock);
- spin_lock(&wq_mayday_lock);
+ spin_unlock_irq(&pool->lock);
+
+ worker_detach_from_pool(rescuer, pool);
+
+ spin_lock_irq(&wq_mayday_lock);
}

spin_unlock_irq(&wq_mayday_lock);
--
1.7.4.4

2014-07-18 22:52:05

by Tejun Heo

Subject: Re: [PATCH] workqueue: detach rescuer from pool until the last

Hello,

On Wed, Jul 16, 2014 at 02:56:50PM +0800, Lai Jiangshan wrote:
> @@ -2300,11 +2300,6 @@ repeat:
> move_linked_works(work, scheduled, &n);
>
> process_scheduled_works(rescuer);
> - spin_unlock_irq(&pool->lock);
> -
> - worker_detach_from_pool(rescuer, pool);
> -
> - spin_lock_irq(&pool->lock);
>
> /*
> * Put the reference grabbed by send_mayday(). @pool won't
> @@ -2322,8 +2317,11 @@ repeat:
> wake_up_worker(pool);
>
> rescuer->pool = NULL;
> - spin_unlock(&pool->lock);
> - spin_lock(&wq_mayday_lock);
> + spin_unlock_irq(&pool->lock);
> +
> + worker_detach_from_pool(rescuer, pool);
> +
> + spin_lock_irq(&wq_mayday_lock);

Can you please also update the comment above put_pwq() and explain
above detach that @pool will stay around till detach is complete?

Thanks.

--
tejun

2014-07-22 05:02:45

by Lai Jiangshan

Subject: [PATCH v2] workqueue: detach rescuer from pool until the last

In 51697d393922 ("workqueue: use generic attach/detach routine for rescuers"),
The rescuer detaches itself from the pool before put_pwq() so that
the put_unbound_pool() will not destroy the rescuer-attached pool.

It is unnecessary, worker_detach_from_pool() can be used as the last
statement to access to the pool as the same as the regular workers,
put_unbound_pool() will wait for it to detach and then free the pool.

So we move the worker_detach_from_pool() down, make it coincide with
the regular workers.

Signed-off-by: Lai Jiangshan <[email protected]>
---
kernel/workqueue.c | 14 ++++++--------
1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index bf837b0..347ac5b 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2252,15 +2252,10 @@ repeat:
move_linked_works(work, scheduled, &n);

process_scheduled_works(rescuer);
- spin_unlock_irq(&pool->lock);
-
- worker_detach_from_pool(rescuer, pool);
-
- spin_lock_irq(&pool->lock);

/*
* Put the reference grabbed by send_mayday(). @pool won't
- * go away while we're holding its lock.
+ * go away while we're still attached to it.
*/
put_pwq(pwq);

@@ -2273,8 +2268,11 @@ repeat:
wake_up_worker(pool);

rescuer->pool = NULL;
- spin_unlock(&pool->lock);
- spin_lock(&wq_mayday_lock);
+ spin_unlock_irq(&pool->lock);
+
+ worker_detach_from_pool(rescuer, pool);
+
+ spin_lock_irq(&wq_mayday_lock);
}

spin_unlock_irq(&wq_mayday_lock);
--
1.7.4.4

2014-07-22 15:27:31

by Tejun Heo

Subject: Re: [PATCH v2] workqueue: detach rescuer from pool until the last

On Tue, Jul 22, 2014 at 01:03:47PM +0800, Lai Jiangshan wrote:
> In 51697d393922 ("workqueue: use generic attach/detach routine for rescuers"),
> The rescuer detaches itself from the pool before put_pwq() so that
> the put_unbound_pool() will not destroy the rescuer-attached pool.
>
> It is unnecessary, worker_detach_from_pool() can be used as the last
> statement to access to the pool as the same as the regular workers,
> put_unbound_pool() will wait for it to detach and then free the pool.
>
> So we move the worker_detach_from_pool() down, make it coincide with
> the regular workers.
>
> Signed-off-by: Lai Jiangshan <[email protected]>

Applied to wq/for-3.17.

Thanks.

--
tejun