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

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

[Lai Jiangshan]

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 <laijs@cn.fujitsu.com>
---
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

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

[Tejun Heo]

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

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

[Lai Jiangshan]

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 <laijs@cn.fujitsu.com>
---
 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

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

[Tejun Heo]

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 <laijs@cn.fujitsu.com>

Applied to wq/for-3.17.

Thanks.

-- 
tejun