Date:	Wed, 3 Dec 2014 11:40:11 +1100
From:	NeilBrown <neilb@...e.de>
To:	Tejun Heo <tj@...nel.org>
Cc:	Jan Kara <jack@...e.cz>, Lai Jiangshan <laijs@...fujitsu.com>,
	Dongsu Park <dongsu.park@...fitbricks.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH - v3?] workqueue: allow rescuer thread to do more work.

On Tue, 2 Dec 2014 15:43:04 -0500 Tejun Heo <tj@...nel.org> wrote:

> Hello,
> 
> On Tue, Nov 18, 2014 at 03:27:54PM +1100, NeilBrown wrote:
> > @@ -2253,26 +2253,36 @@ repeat:
> >  					struct pool_workqueue, mayday_node);
> >  		struct worker_pool *pool = pwq->pool;
> >  		struct work_struct *work, *n;
> > +		int still_needed;
> >  
> >  		__set_current_state(TASK_RUNNING);
> > -		list_del_init(&pwq->mayday_node);
> > -
> > -		spin_unlock_irq(&wq_mayday_lock);
> > -
> > -		worker_attach_to_pool(rescuer, pool);
> > -
> > -		spin_lock_irq(&pool->lock);
> > -		rescuer->pool = pool;
> > -
> > +		spin_lock(&pool->lock);
> >  		/*
> >  		 * Slurp in all works issued via this workqueue and
> >  		 * process'em.
> >  		 */
> >  		WARN_ON_ONCE(!list_empty(&rescuer->scheduled));
> > +		still_needed = need_to_create_worker(pool);
> >  		list_for_each_entry_safe(work, n, &pool->worklist, entry)
> >  			if (get_work_pwq(work) == pwq)
> >  				move_linked_works(work, scheduled, &n);
> >  
> > +		if (!list_empty(scheduled))
> > +			still_needed = 1;
> > +		if (still_needed) {
> > +			list_move_tail(&pwq->mayday_node, &wq->maydays);
> > +			get_pwq(pwq);
> > +		} else
> > +			/* We can let go of this one now */
> > +			list_del_init(&pwq->mayday_node);
> 
> This seems rather convoluted.

Agreed.

>     Why are we testing this before
> executing the work item?  Can't we do this after? 

To execute the work item, we need to drop the locks.
To perform the "list_move_tail" and the "get_pwq" we need to hold both locks.

It seems easier to do the test while holding the required locks.

>    Isn't that -
> whether the wq still needs rescuing after rescuer went through it once
> - what we wanna know anyway?  e.g. something like the following.
> 
> 	for_each_pwq_on_mayday_list {
> 		try to fetch work items from pwq->pool;
> 		if (none was fetched)
> 			goto remove_pwq;
> 
> 		execute the fetched work items;
> 
> 		if (need_to_create_worker()) {
> 			move the pwq to the tail;
> 			continue;
> 		}
> 
> 	remove_pwq:
> 		remove the pwq;
> 	}

That looks nice.  I have a little difficulty matching the code to that
outline.
In particular I need to hold the pool->lock to call put_pwq() and after
calling put_pwq() it isn't clear that I have a safe reference to pool so that
it is safe to de-reference it... unless I always
attach_to_pool/detach_from_pool.
But to do that I have to drop the locks at awkward times.

Please correct me if I'm wrong, but it looks like I have to call
worker_attach_to_pool() or I cannot safely call put_pwq().
I then have to call worker_detach_from_pool() as the last step.

I need to hold wq_mayday_lock to either move the pwq to the end of the mayday
list or to remove it, and I have to drop that lock to call
process_scheduled_works and after that I cannot retake wq_mayday_lock without
first dropping pool->lock.

The net result is that if I try to map the code on to your outline, it
becomes even more convoluted due to the various locking/refcounting issues.

The best I can do is below (much the same as before).  If someone else can do
better I'd be very happy to see it.


> 
> > +
> > +		spin_unlock(&pool->lock);
> > +		spin_unlock_irq(&wq_mayday_lock);
> > +
> > +		worker_attach_to_pool(rescuer, pool);
> > +
> > +		spin_lock_irq(&pool->lock);
> > +		rescuer->pool = pool;
> >  		process_scheduled_works(rescuer);
> >  
> >  		/*
> > @@ -2293,7 +2303,7 @@ repeat:
> >  		spin_unlock_irq(&pool->lock);
> >  
> >  		worker_detach_from_pool(rescuer, pool);
> > -
> > +		cond_resched();
> 
> Also, why this addition?  process_one_work() already has
> cond_resched_rcu_qs().

Just being over-cautious I suppose.  I agree that isn't needed.

> 
> Thanks.
> 

Thanks,
NeilBrown


From: NeilBrown <neilb@...e.de>
Date: Tue, 18 Nov 2014 15:23:07 +1100
Subject: [PATCH] workqueue: allow rescuer thread to do more work.

When there is serious memory pressure, all workers in a pool could be
blocked, and a new thread cannot be created because it requires memory
allocation.

In this situation a WQ_MEM_RECLAIM workqueue will wake up the
rescuer thread to do some work.

The rescuer will only handle requests that are already on ->worklist.
If max_requests is 1, that means it will handle a single request.

The rescuer will be woken again in 100ms to handle another max_requests
requests.

I've seen a machine (running a 3.0 based "enterprise" kernel) with
thousands of requests queued for xfslogd, which has a max_requests of
1, and is needed for retiring all 'xfs' write requests.  When one of
the worker pools gets into this state, it progresses extremely slowly
and possibly never recovers (only waited an hour or two).

With this patch we leave a pool_workqueue on mayday list
until it is clearly no longer in need of assistance.  This allows
all requests to be handled in a timely fashion.

The code is a bit awkward due to the need to hold both wq_mayday_lock
and pool->lock at the same time, and due to the lock ordering imposed
on them.  In particular we move work items from the ->worklist to the
rescuer list while holding both locks because we need pool->lock
to do the move, and need wq_mayday_lock to manipulate the mayday list
after we have found out if there was anything to do.

'need_to_create_worker()' is called *before* moving work items off
pool->worklist as an empty worklist will make it return false, but
after the move_linked_works() calls and before the
process_scheduled_works() call, an empty worklist does not indicate
that there is no work to do.

We keep each pool_workqueue on the mayday list until
need_to_create_worker() is false, and no work for this workqueue is
found in the pool.

As the main rescuer loop now iterates an arbitrary number of time,
cond_resched() was inserted to avoid imposing excessive latencies.

I have tested this in combination with a (hackish) patch which forces
all work items to be handled by the rescuer thread.  In that context
it significantly improves performance.  A similar patch for a 3.0
kernel significantly improved performance on a heavy work load.

Signed-off-by: NeilBrown <neilb@...e.de>

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index caedde34ee7f..fc9a2771fc0d 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2253,26 +2253,36 @@ repeat:
 					struct pool_workqueue, mayday_node);
 		struct worker_pool *pool = pwq->pool;
 		struct work_struct *work, *n;
+		int still_needed;
 
 		__set_current_state(TASK_RUNNING);
-		list_del_init(&pwq->mayday_node);
-
-		spin_unlock_irq(&wq_mayday_lock);
-
-		worker_attach_to_pool(rescuer, pool);
-
-		spin_lock_irq(&pool->lock);
-		rescuer->pool = pool;
-
+		spin_lock(&pool->lock);
 		/*
 		 * Slurp in all works issued via this workqueue and
 		 * process'em.
 		 */
 		WARN_ON_ONCE(!list_empty(&rescuer->scheduled));
+		still_needed = need_to_create_worker(pool);
 		list_for_each_entry_safe(work, n, &pool->worklist, entry)
 			if (get_work_pwq(work) == pwq)
 				move_linked_works(work, scheduled, &n);
 
+		if (list_empty(scheduled))
+			still_needed = 0;
+		if (still_needed) {
+			list_move_tail(&pwq->mayday_node, &wq->maydays);
+			get_pwq(pwq);
+		} else
+			/* We can let go of this one now */
+			list_del_init(&pwq->mayday_node);
+
+		spin_unlock(&pool->lock);
+		spin_unlock_irq(&wq_mayday_lock);
+
+		worker_attach_to_pool(rescuer, pool);
+
+		spin_lock_irq(&pool->lock);
+		rescuer->pool = pool;
 		process_scheduled_works(rescuer);
 
 		/*

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists