workqueue: allow rescuer thread to do more work.

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

kernel/workqueue.c +3 -8

index 09b685daee3d..d0a8b101c5d9 100644
@@ -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

Hello, Neil.

Quoted 37 lines · NeilBrown

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

kernel/workqueue.c +9 -8

index 09b685daee3d..d0a8b101c5d9 100644
@@ -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);
 

kernel/workqueue.c repeat: · line 2258

Quoted 3 lines

kernel/workqueue.c +2 -1

-		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.

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?

Quoted 4 lines

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. ??

Quoted 9 lines · NeilBrown

Quoted 4 lines

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. ??

Yeah, that'd be a better condition and can work out. Can you please write up a patch to do that and do some synthetic tests excercising the code path? Also please cc Lai Jiangshan <[email protected]> when posting the patch.

From de9aadd6fb742ea8acce4245a27946d3f233ab7f Mon Sep 17 00:00:00 2001

Hi Tejun & Neil,

Quoted 11 lines · Tejun Heo

Quoted 9 lines · NeilBrown

Quoted 4 lines

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. ??

Quoted 4 lines

Yeah, that'd be a better condition and can work out. Can you please write up a patch to do that and do some synthetic tests excercising the code path? Also please cc Lai Jiangshan <[email protected]> when posting the patch.

This issue looks exactly like what I've encountered occasionally in our test setup. (with a custom kernel based on 3.12, MD/raid1, dm-multipath, etc.) When a system suffers from high memory pressure, and at the same time underlying devices of RAID arrays are repeatedly removed and re-added, then sometimes the whole system gets locked up on a worker pool's lock. So I had to fix our custom MD code to allocate a separate ordered workqueue with WQ_MEM_RECLAIM, apart from md_wq or md_misc_wq.
Then the lockup seemed to have disappeared.

Now that I read the Neil's patch, which looks like an ultimate solution to the problem I have seen. I'm really looking forward to seeing this change in mainline.

How about the attached patch? Based on the Neil's patch, I replaced need_more_worker() with need_to_create_worker() as Tejun suggested.

Test is running with this patch, which seems to be working for now. But I'm going to observe the test result carefully for a few more days.

Regards,
Dongsu

----

Quoted 1 line

From de9aadd6fb742ea8acce4245a27946d3f233ab7f Mon Sep 17 00:00:00 2001

From: Dongsu Park <[email protected]>
Date: Wed, 5 Nov 2014 17:28:07 +0100
Subject: [RFC PATCH] workqueue: allow rescuer thread to do more work

Original commit message 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).

So if, after handling everything on worklist, there is again something on worklist (counted in nr_active), and if the queue is still congested, keep processing instead of waiting for the next wake-up.
====

Dongsu Park: replaced need_more_worker() with need_to_create_worker(), as suggested by Tejun.

Signed-off-by Dongsu Park <[email protected]>

Link https://lkml.org/lkml/2014/10/29/55

Cc Tejun Heo <[email protected]>

Cc Lai Jiangshan <[email protected]>

Original-by NeilBrown <[email protected]>

Signed-off-by NeilBrown <[email protected]>

---
 kernel/workqueue.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

kernel/workqueue.c +13 -10

index 09b685d..4d20225 100644
@@ -2244,16 +2244,19 @@ 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);
-
-		process_scheduled_works(rescuer);
+		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);
+		} while (need_to_create_worker(pool) && pwq->nr_active);
 
 		/*
 		 * Put the reference grabbed by send_mayday().  @pool won't

Quoted 53 lines · Dongsu Park

Hi Tejun & Neil,

Quoted 11 lines · Tejun Heo

Quoted 9 lines · NeilBrown

Quoted 4 lines

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. ??

Quoted 4 lines

Yeah, that'd be a better condition and can work out. Can you please write up a patch to do that and do some synthetic tests excercising the code path? Also please cc Lai Jiangshan <[email protected]> when posting the patch.

This issue looks exactly like what I've encountered occasionally in our test setup. (with a custom kernel based on 3.12, MD/raid1, dm-multipath, etc.) When a system suffers from high memory pressure, and at the same time underlying devices of RAID arrays are repeatedly removed and re-added, then sometimes the whole system gets locked up on a worker pool's lock. So I had to fix our custom MD code to allocate a separate ordered workqueue with WQ_MEM_RECLAIM, apart from md_wq or md_misc_wq.
Then the lockup seemed to have disappeared.

Now that I read the Neil's patch, which looks like an ultimate solution to the problem I have seen. I'm really looking forward to seeing this change in mainline.

How about the attached patch? Based on the Neil's patch, I replaced need_more_worker() with need_to_create_worker() as Tejun suggested.

Test is running with this patch, which seems to be working for now. But I'm going to observe the test result carefully for a few more days.

Regards,
Dongsu

----

Quoted 1 line

From de9aadd6fb742ea8acce4245a27946d3f233ab7f Mon Sep 17 00:00:00 2001

From: Dongsu Park <[email protected]>
Date: Wed, 5 Nov 2014 17:28:07 +0100
Subject: [RFC PATCH] workqueue: allow rescuer thread to do more work

Original commit message 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.

Quoted 8 lines

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 also observed this problem by review when I was developing
the per-pwq-worklist patchset which has a side-affect that it also naturally fix the problem.

However, it is nothing about correctness and I made promise to Frederic Weisbecker for working on unbound pool for power-saving, then the per-pwq-worklist patchset is put off. So I have to ack it.

Quoted 58 lines

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).

So if, after handling everything on worklist, there is again something on worklist (counted in nr_active), and if the queue is still congested, keep processing instead of waiting for the next wake-up.
====

Dongsu Park: replaced need_more_worker() with need_to_create_worker(), as suggested by Tejun.

Signed-off-by Dongsu Park <[email protected]>

Link https://lkml.org/lkml/2014/10/29/55

Cc Tejun Heo <[email protected]>

Cc Lai Jiangshan <[email protected]>

Original-by NeilBrown <[email protected]>

Signed-off-by NeilBrown <[email protected]>

---
 kernel/workqueue.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

kernel/workqueue.c +13 -10

index 09b685d..4d20225 100644
@@ -2244,16 +2244,19 @@ 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);
-
-		process_scheduled_works(rescuer);
+		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);
+		} while (need_to_create_worker(pool) && pwq->nr_active);
 

kernel/workqueue.c repeat: · line 2262

Quoted 2 lines

kernel/workqueue.c +0 -0

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

Quoted 64 lines · On Fri, 7 Nov 2014 11:03:40 +0800 Lai Jiangshan <[email protected]> wrote:

Quoted 53 lines · Dongsu Park

Hi Tejun & Neil,

Quoted 11 lines · Tejun Heo

Quoted 9 lines · NeilBrown

Quoted 4 lines

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. ??

Quoted 4 lines

Yeah, that'd be a better condition and can work out. Can you please write up a patch to do that and do some synthetic tests excercising the code path? Also please cc Lai Jiangshan <[email protected]> when posting the patch.

This issue looks exactly like what I've encountered occasionally in our test setup. (with a custom kernel based on 3.12, MD/raid1, dm-multipath, etc.) When a system suffers from high memory pressure, and at the same time underlying devices of RAID arrays are repeatedly removed and re-added, then sometimes the whole system gets locked up on a worker pool's lock. So I had to fix our custom MD code to allocate a separate ordered workqueue with WQ_MEM_RECLAIM, apart from md_wq or md_misc_wq.
Then the lockup seemed to have disappeared.

Now that I read the Neil's patch, which looks like an ultimate solution to the problem I have seen. I'm really looking forward to seeing this change in mainline.

How about the attached patch? Based on the Neil's patch, I replaced need_more_worker() with need_to_create_worker() as Tejun suggested.

Test is running with this patch, which seems to be working for now. But I'm going to observe the test result carefully for a few more days.

Regards,
Dongsu

----

Quoted 1 line

From de9aadd6fb742ea8acce4245a27946d3f233ab7f Mon Sep 17 00:00:00 2001

From: Dongsu Park <[email protected]>
Date: Wed, 5 Nov 2014 17:28:07 +0100
Subject: [RFC PATCH] workqueue: allow rescuer thread to do more work

Original commit message 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.

Quoted 8 lines

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.

Quoted 7 lines

I also observed this problem by review when I was developing
the per-pwq-worklist patchset which has a side-affect that it also naturally fix the problem.

However, it is nothing about correctness and I made promise to Frederic Weisbecker for working on unbound pool for power-saving, then the per-pwq-worklist patchset is put off. So I have to ack it.

Thanks!
However testing showed that the patch isn't quite right.
The test on ->nr_active is not correct. I was meaning to test "are there any requests that have been activated but not yet serviced", but this test only covers the first half.

If a queue allows a number of active requests (max_active > 1), and several are blocked waiting for something (e.g. more memory), then max_active will be positive even though there is no useful work for the rescuer thread to do - so it will spin.

Jan Kara and I came up with a different patch which testing has shown is quite successful. However it makes changes to when mayday_clear_cpu() is set, and that isn't relevant in the current kernel.

I've ported the patch to -mainline, but haven't really tested it properly (just compile tested so far).
That version is below.

From: NeilBrown <[email protected]>
Subject: workqueue: Make rescuer thread process more works

Currently workqueue rescuer thread processes at most max_active works from a workqueue before it goes back to sleep for 100 ms. Especially for workqueues with low max_active this leads to rescuer being very slow and when queued work is blocking reclaim it leads to machine taking very long time (minutes or more) to recover from a situation when new workers cannot be created.

Fix the problem by going through worklist until either new worker is created or all no new works can be found.

We remove and re-add the pool_workqueue to the mayday list so that each pool_workqueue so that no one pool_workqueue can starve the others.

Signed-off-by Jan Kara <[email protected]>

Signed-off-by NeilBrown <[email protected]>

kernel/workqueue.c +0 -0

index 09b685daee3d..19ecee70e3e9 100644

Quoted 91 lines · NeilBrown

Quoted 64 lines · On Fri, 7 Nov 2014 11:03:40 +0800 Lai Jiangshan <[email protected]> wrote:

Quoted 53 lines · Dongsu Park

Hi Tejun & Neil,

Quoted 11 lines · Tejun Heo

Quoted 9 lines · NeilBrown

Quoted 4 lines

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. ??

Quoted 4 lines

Yeah, that'd be a better condition and can work out. Can you please write up a patch to do that and do some synthetic tests excercising the code path? Also please cc Lai Jiangshan <[email protected]> when posting the patch.

This issue looks exactly like what I've encountered occasionally in our test setup. (with a custom kernel based on 3.12, MD/raid1, dm-multipath, etc.) When a system suffers from high memory pressure, and at the same time underlying devices of RAID arrays are repeatedly removed and re-added, then sometimes the whole system gets locked up on a worker pool's lock. So I had to fix our custom MD code to allocate a separate ordered workqueue with WQ_MEM_RECLAIM, apart from md_wq or md_misc_wq.
Then the lockup seemed to have disappeared.

Now that I read the Neil's patch, which looks like an ultimate solution to the problem I have seen. I'm really looking forward to seeing this change in mainline.

How about the attached patch? Based on the Neil's patch, I replaced need_more_worker() with need_to_create_worker() as Tejun suggested.

Test is running with this patch, which seems to be working for now. But I'm going to observe the test result carefully for a few more days.

Regards,
Dongsu

----

Quoted 1 line

From de9aadd6fb742ea8acce4245a27946d3f233ab7f Mon Sep 17 00:00:00 2001

From: Dongsu Park <[email protected]>
Date: Wed, 5 Nov 2014 17:28:07 +0100
Subject: [RFC PATCH] workqueue: allow rescuer thread to do more work

Original commit message 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.

Quoted 8 lines

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.

Quoted 7 lines

I also observed this problem by review when I was developing
the per-pwq-worklist patchset which has a side-affect that it also naturally fix the problem.

However, it is nothing about correctness and I made promise to Frederic Weisbecker for working on unbound pool for power-saving, then the per-pwq-worklist patchset is put off. So I have to ack it.

Thanks!
However testing showed that the patch isn't quite right.
The test on ->nr_active is not correct. I was meaning to test "are there any requests that have been activated but not yet serviced", but this test only covers the first half.

If a queue allows a number of active requests (max_active > 1), and several are blocked waiting for something (e.g. more memory), then max_active will be positive even though there is no useful work for the rescuer thread to do - so it will spin.

Jan Kara and I came up with a different patch which testing has shown is quite successful. However it makes changes to when mayday_clear_cpu() is set, and that isn't relevant in the current kernel.

I've ported the patch to -mainline, but haven't really tested it properly (just compile tested so far).
That version is below.

...

Quoted 24 lines

From: NeilBrown <[email protected]>
Subject: workqueue: Make rescuer thread process more works

Currently workqueue rescuer thread processes at most max_active works from a workqueue before it goes back to sleep for 100 ms. Especially for workqueues with low max_active this leads to rescuer being very slow and when queued work is blocking reclaim it leads to machine taking very long time (minutes or more) to recover from a situation when new workers cannot be created.

Fix the problem by going through worklist until either new worker is created or all no new works can be found.

We remove and re-add the pool_workqueue to the mayday list so that each pool_workqueue so that no one pool_workqueue can starve the others.

Signed-off-by Jan Kara <[email protected]>

Signed-off-by NeilBrown <[email protected]>

kernel/workqueue.c +0 -0

index 09b685daee3d..19ecee70e3e9 100644

kernel/workqueue.c repeat: · line 2260

Quoted 8 lines

kernel/workqueue.c +4 -0

@@ -2253,6 +2253,10 @@ repeat:
 			if (get_work_pwq(work) == pwq)
 				move_linked_works(work, scheduled, &n);
 
+		if (!list_empty(scheduled) && need_to_create_worker(pool))
+			/* Try again, in case more requests get added */
+			if (list_empty(&pwq->mayday_node))
+				list_add_tail(&pwq->mayday_node, &wq->maydays);
 		process_scheduled_works(rescuer);

This is certainly missing locking - we need to hold wq_mayday_lock when
changing wq->maydays list. Otherwise the patch looks good to me.

Honza

kernel/workqueue.c +0 -5

index 09b685daee3d..f2db6073c498 100644
@@ -2235,11 +2235,6 @@ repeat:
 		struct work_struct *work, *n;
 
 		__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;

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.

Thanks to Jan Kara for some design ideas, and to Dongsu Park for some comments and testing.

Cc Dongsu Park <[email protected]>

Cc Jan Kara <[email protected]>

Signed-off-by NeilBrown <[email protected]>

kernel/workqueue.c +1 -7

index caedde34ee7f..4baa7b8b7e0f 100644
@@ -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);

kernel/workqueue.c +16 -0

 		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);
+
+		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);
 

kernel/workqueue.c +1 -1

 		/*

@@ -2293,7 +2303,7 @@ repeat:
 		spin_unlock_irq(&pool->lock);
 
 		worker_detach_from_pool(rescuer, pool);
-
+		cond_resched();
 		spin_lock_irq(&wq_mayday_lock);
 	}
 

Quoted 82 lines · NeilBrown

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.

Thanks to Jan Kara for some design ideas, and to Dongsu Park for some comments and testing.

Cc Dongsu Park <[email protected]>

Cc Jan Kara <[email protected]>

Signed-off-by NeilBrown <[email protected]>

kernel/workqueue.c +2 -9

index caedde34ee7f..4baa7b8b7e0f 100644
@@ -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));

kernel/workqueue.c repeat: · line 2265

Quoted 1 line

kernel/workqueue.c +1 -0

+		still_needed = need_to_create_worker(pool);

This line of code will cause the rescuer busy-loop even no work-item pending on the workqueue.

kernel/workqueue.c repeat: · line 2271

Quoted 7 lines

kernel/workqueue.c +2 -0

 		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;

Quoted 26 lines

kernel/workqueue.c +15 -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);
+
+		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();
 		spin_lock_irq(&wq_mayday_lock);
 	}
 

kernel/workqueue.c +3 -9

@@ -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)

kernel/workqueue.c +8 -0

+
+		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);
 
 		/*

Hello,

Quoted 27 lines · NeilBrown

(quoted diff) +3 -9

@@ -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);
 

Review repeat: · line 2277

Quoted 8 lines

(quoted diff) +8 -0

+		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.

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

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;
	}

Quoted 15 lines

(quoted diff) +8 -0

+
+		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);
 

Review repeat: · line 2306

Quoted 3 lines

(quoted diff) +1 -1

 		worker_detach_from_pool(rescuer, pool);
-
+		cond_resched();

Also, why this addition? process_one_work() already has cond_resched_rcu_qs().

Thanks.

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.

Quoted 4 lines

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.

Quoted 15 lines

	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.

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
===================================================================

kernel/workqueue.c +9 -6

@@ -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);
+
+			/*

kernel/workqueue.c +4 -0

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

Signature · 2 lines

-- 
tejun

Hi Tejun,

Quoted 2 lines · Tejun Heo

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)

Quoted 47 lines

Thanks.

Index: work/kernel/workqueue.c
===================================================================

kernel/workqueue.c +13 -6

@@ -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

kernel/workqueue.c repeat: · line 2269

Quoted 6 lines

kernel/workqueue.c +6 -0

+			 * 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

Quoted 10 lines

kernel/workqueue.c +4 -0

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

Signature · 2 lines

-- 
tejun

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
===================================================================

kernel/workqueue.c +10 -6

@@ -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

kernel/workqueue.c repeat: · line 2270

Quoted 3 lines

kernel/workqueue.c +3 -0

+			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 ...

kernel/workqueue.c repeat: · line 2276

Quoted 6 lines

kernel/workqueue.c +3 -0

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

and the pwq is scheduled free here>

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]>

Quoted 42 lines · Tejun Heo

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.

Quoted 5 lines

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.

Currently, pool->lock nests inside pool->lock. There's no inherent reason for this order. The only place where the two locks are held together is pool_mayday_timeout() and it just got decided that way.

This nesting order turns out to complicate things with the planned rescuer_thread() update. Let's invert them. This doesn't cause any behavior differences.

Signed-off-by Tejun Heo <[email protected]>

Cc NeilBrown <[email protected]>

Cc Dongsu Park <[email protected]>

Cc Lai Jiangshan <[email protected]>

---
 kernel/workqueue.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

kernel/workqueue.c +4 -4

@@ -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);
 }