Subject: Re: [PATCH/RFC] workqueue: allow rescuer thread to do more work.

On Wed, 29 Oct 2014 10:32:10 -0400 Tejun Heo <[email protected]> wrote:

> Hello, Neil.
>
> On Wed, Oct 29, 2014 at 05:26:08PM +1100, NeilBrown wrote:
> > Hi Tejun,
> > I haven't tested this patch yet so this really is an 'RFC'.
> > In general ->nr_active should only be accessed under the pool->lock,
> > but a miss-read here will at most cause a very occasional 100ms delay so
> > shouldn't be a big problem. The only thread likely to change ->nr_active is
> > this thread, so such a delay would be extremely unlikely.
> >
> > Thanks,
> > NeilBrown
> >
> >
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index 09b685daee3d..d0a8b101c5d9 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -2244,16 +2244,18 @@ repeat:
> > spin_lock_irq(&pool->lock);
> > rescuer->pool = pool;
> >
> > - /*
> > - * Slurp in all works issued via this workqueue and
> > - * process'em.
> > - */
> > - WARN_ON_ONCE(!list_empty(&rescuer->scheduled));
> > - list_for_each_entry_safe(work, n, &pool->worklist, entry)
> > - if (get_work_pwq(work) == pwq)
> > - move_linked_works(work, scheduled, &n);
> > + do {
> > + /*
> > + * Slurp in all works issued via this workqueue and
> > + * process'em.
> > + */
> > + WARN_ON_ONCE(!list_empty(&rescuer->scheduled));
> > + list_for_each_entry_safe(work, n, &pool->worklist, entry)
> > + if (get_work_pwq(work) == pwq)
> > + move_linked_works(work, scheduled, &n);
> >
> > - process_scheduled_works(rescuer);
> > + process_scheduled_works(rescuer);
> > + } while (need_more_worker(pool) && pwq->nr_active);
>
> need_more_worker(pool) is always true for unbound pools as long as
> there are work items queued, so the above condition may stay true
> longer than it needs to.

Because ->nr_running isn't maintained for WORKER_UNBOUND (which is part of
WORKER_NOT_RUNNING) - got it.

> Given that workder depletion is pool-wide
> event, maybe it'd make sense to trigger rescuers immediately while
> workers are in short supply? e.g. while there's a manager stuck in
> maybe_create_worker() with the mayday timer already triggered?

So what if I change "need_more_worker" to "need_to_create_worker" ?
Then it will stop as soon as there in an idle worker thread.
That is the condition that keeps maybe_create_worker() looping.
??

Thanks,
NeilBrown

Attachments:

(No filename) (828.00 B)
OpenPGP digital signature

2014-11-04 14:22:46

by Tejun Heo

[permalink] [raw]

Subject: Re: [PATCH/RFC] workqueue: allow rescuer thread to do more work.

by Tejun Heo

[permalink] [raw]

Subject: Re: [PATCH - v3?] workqueue: allow rescuer thread to do more work.

So, something like the following. Only compile tested. I'll test it
and post proper patches w/ due credits.

Thanks.

Index: work/kernel/workqueue.c
===================================================================
--- work.orig/kernel/workqueue.c
+++ work/kernel/workqueue.c
@@ -1804,8 +1804,8 @@ static void pool_mayday_timeout(unsigned
struct worker_pool *pool = (void *)__pool;
struct work_struct *work;

- spin_lock_irq(&wq_mayday_lock); /* for wq->maydays */
- spin_lock(&pool->lock);
+ spin_lock_irq(&pool->lock);
+ spin_lock(&wq_mayday_lock); /* for wq->maydays */

if (need_to_create_worker(pool)) {
/*
@@ -1818,8 +1818,8 @@ static void pool_mayday_timeout(unsigned
send_mayday(work);
}

- spin_unlock(&pool->lock);
- spin_unlock_irq(&wq_mayday_lock);
+ spin_unlock(&wq_mayday_lock);
+ spin_unlock_irq(&pool->lock);

mod_timer(&pool->mayday_timer, jiffies + MAYDAY_INTERVAL);
}
@@ -2248,12 +2248,29 @@ repeat:
* Slurp in all works issued via this workqueue and
* process'em.
*/
- WARN_ON_ONCE(!list_empty(&rescuer->scheduled));
+ WARN_ON_ONCE(!list_empty(scheduled));
list_for_each_entry_safe(work, n, &pool->worklist, entry)
if (get_work_pwq(work) == pwq)
move_linked_works(work, scheduled, &n);

- process_scheduled_works(rescuer);
+ if (!list_empty(scheduled)) {
+ process_scheduled_works(rescuer);
+
+ /*
+ * The above execution of rescued work items could
+ * have created more to rescue through
+ * pwq_activate_first_delayed() or chained
+ * queueing. Let's put @pwq back on mayday list so
+ * that such back-to-back work items, which may be
+ * being used to relieve memory pressure, don't
+ * incur MAYDAY_INTERVAL delay inbetween.
+ */
+ if (need_to_create_worker(pool)) {
+ spin_lock(&wq_mayday_lock);
+ list_move_tail(&pwq->mayday_node, &wq->maydays);
+ spin_unlock(&wq_mayday_lock);
+ }
+ }

/*
* Put the reference grabbed by send_mayday(). @pool won't

--
tejun

2014-12-03 22:31:25

by Dongsu Park

[permalink] [raw]

Subject: Re: [PATCH - v3?] workqueue: allow rescuer thread to do more work.

Hi Tejun,

On 03.12.2014 13:02, Tejun Heo wrote:
> So, something like the following. Only compile tested. I'll test it
> and post proper patches w/ due credits.

I have been already satisfied with Neil's patch,
but your patch looks indeed a lot cleaner, I like it.
I just compiled and tested it shortly, which seems to work.
Though there's one nitpick. (see below)

> Thanks.
>
> Index: work/kernel/workqueue.c
> ===================================================================
> --- work.orig/kernel/workqueue.c
> +++ work/kernel/workqueue.c
> @@ -1804,8 +1804,8 @@ static void pool_mayday_timeout(unsigned
> struct worker_pool *pool = (void *)__pool;
> struct work_struct *work;
>
> - spin_lock_irq(&wq_mayday_lock); /* for wq->maydays */
> - spin_lock(&pool->lock);
> + spin_lock_irq(&pool->lock);
> + spin_lock(&wq_mayday_lock); /* for wq->maydays */
>
> if (need_to_create_worker(pool)) {
> /*
> @@ -1818,8 +1818,8 @@ static void pool_mayday_timeout(unsigned
> send_mayday(work);
> }
>
> - spin_unlock(&pool->lock);
> - spin_unlock_irq(&wq_mayday_lock);
> + spin_unlock(&wq_mayday_lock);
> + spin_unlock_irq(&pool->lock);
>
> mod_timer(&pool->mayday_timer, jiffies + MAYDAY_INTERVAL);
> }
> @@ -2248,12 +2248,29 @@ repeat:
> * Slurp in all works issued via this workqueue and
> * process'em.
> */
> - WARN_ON_ONCE(!list_empty(&rescuer->scheduled));
> + WARN_ON_ONCE(!list_empty(scheduled));
> list_for_each_entry_safe(work, n, &pool->worklist, entry)
> if (get_work_pwq(work) == pwq)
> move_linked_works(work, scheduled, &n);
>
> - process_scheduled_works(rescuer);
> + if (!list_empty(scheduled)) {
> + process_scheduled_works(rescuer);
> +
> + /*
> + * The above execution of rescued work items could
> + * have created more to rescue through
> + * pwq_activate_first_delayed() or chained
> + * queueing. Let's put @pwq back on mayday list so
> + * that such back-to-back work items, which may be
> + * being used to relieve memory pressure, don't
> + * incur MAYDAY_INTERVAL delay inbetween.
> + */
> + if (need_to_create_worker(pool)) {
> + spin_lock(&wq_mayday_lock);

Does it need to call get_pwq(pwq), doesn't it?

Thanks,
Dongsu

> + list_move_tail(&pwq->mayday_node, &wq->maydays);
> + spin_unlock(&wq_mayday_lock);
> + }
> + }
>
> /*
> * Put the reference grabbed by send_mayday(). @pool won't
>
> --
> tejun

2014-12-04 00:58:12

by Lai Jiangshan

[permalink] [raw]

Subject: Re: [PATCH - v3?] workqueue: allow rescuer thread to do more work.

On 12/04/2014 02:02 AM, Tejun Heo wrote:
> So, something like the following. Only compile tested. I'll test it
> and post proper patches w/ due credits.
>
> Thanks.
>
> Index: work/kernel/workqueue.c
> ===================================================================
> --- work.orig/kernel/workqueue.c
> +++ work/kernel/workqueue.c
> @@ -1804,8 +1804,8 @@ static void pool_mayday_timeout(unsigned
> struct worker_pool *pool = (void *)__pool;
> struct work_struct *work;
>
> - spin_lock_irq(&wq_mayday_lock); /* for wq->maydays */
> - spin_lock(&pool->lock);
> + spin_lock_irq(&pool->lock);
> + spin_lock(&wq_mayday_lock); /* for wq->maydays */
>
> if (need_to_create_worker(pool)) {
> /*
> @@ -1818,8 +1818,8 @@ static void pool_mayday_timeout(unsigned
> send_mayday(work);
> }
>
> - spin_unlock(&pool->lock);
> - spin_unlock_irq(&wq_mayday_lock);
> + spin_unlock(&wq_mayday_lock);
> + spin_unlock_irq(&pool->lock);
>
> mod_timer(&pool->mayday_timer, jiffies + MAYDAY_INTERVAL);
> }
> @@ -2248,12 +2248,29 @@ repeat:
> * Slurp in all works issued via this workqueue and
> * process'em.
> */
> - WARN_ON_ONCE(!list_empty(&rescuer->scheduled));
> + WARN_ON_ONCE(!list_empty(scheduled));
> list_for_each_entry_safe(work, n, &pool->worklist, entry)
> if (get_work_pwq(work) == pwq)
> move_linked_works(work, scheduled, &n);
>
> - process_scheduled_works(rescuer);
> + if (!list_empty(scheduled)) {
> + process_scheduled_works(rescuer);
> +
> + /*
> + * The above execution of rescued work items could
> + * have created more to rescue through
> + * pwq_activate_first_delayed() or chained
> + * queueing. Let's put @pwq back on mayday list so
> + * that such back-to-back work items, which may be
> + * being used to relieve memory pressure, don't
> + * incur MAYDAY_INTERVAL delay inbetween.
> + */
> + if (need_to_create_worker(pool)) {
> + spin_lock(&wq_mayday_lock);
> + list_move_tail(&pwq->mayday_node, &wq->maydays);

how about the pwq is empty now? and ...

> + spin_unlock(&wq_mayday_lock);
> + }
> + }
>
> /*
> * Put the reference grabbed by send_mayday(). @pool won't

and the pwq is scheduled free here>
>

2014-12-04 01:19:15

On Mon, 8 Dec 2014 12:40:52 -0500 Tejun Heo <[email protected]> wrote:

> On Thu, Dec 04, 2014 at 10:12:23AM -0500, Tejun Heo wrote:
> > From: NeilBrown <[email protected]>
> >
> > 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.
> >
> > 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.
> >
> > 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.
> >
> > Thanks to Jan Kara for some design ideas, and to Dongsu Park for
> > some comments and testing.
> >
> > tj: Inverted the lock order between wq_mayday_lock and pool->lock with
> > a preceding patch and simplified this patch. Added comment and
> > updated changelog accordingly. Dongsu spotted missing get_pwq()
> > in the simplified code.
> >
> > Cc: Dongsu Park <[email protected]>
> > Cc: Jan Kara <[email protected]>
> > Cc: Lai Jiangshan <[email protected]>
> > Signed-off-by: NeilBrown <[email protected]>
> > Signed-off-by: Tejun Heo <[email protected]>
>
> Too late for for-3.18-fixes. Applied the two patches to wq/for-3.19.

I've just run my synthetic test which forces most works to be handled by the
rescuer thread. With these patches it works more smoothly than without.
And importantly: it still works :-)

Thanks,
NeilBrown

Attachments:

(No filename) (811.00 B)
OpenPGP digital signature