[PATCH] workqueue: lock cwq access in drain_workqueue

Thomas Tuttle

author

2011-09-09 15:22 UTC #0 lore raw

PATCH 1/1 workqueue: lock cwq access in drain_workqueue

Take cwq->gcwq->lock to avoid racing between drain_workqueue checking to make sure the workqueues are empty and cwq_dec_nr_in_flight decrementing and then incrementing nr_active when it activates a delayed work.

We discovered this when a corner case in one of our drivers resulted in us trying to destroy a workqueue in which the remaining work would always requeue itself again in the same workqueue. We would hit this race condition and trip the BUG_ON on workqueue.c:3080.

Patch is against HEAD as of Fri Sep 9 15:16:09 UTC 2011 (e4e436e0bd480668834fe6849a52c5397b7be4fb).

Signed-off-by Thomas Tuttle <[email protected]>

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

kernel/workqueue.c +7 -1

index 25fb1b0..d610ced 100644
@@ -2412,8 +2412,14 @@ reflush:
 
 	for_each_cwq_cpu(cpu, wq) {
 		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
+		int cwq_flushed;
 
-		if (!cwq->nr_active && list_empty(&cwq->delayed_works))
+		spin_lock_irq(&cwq->gcwq->lock);
+		cwq_flushed = !cwq->nr_active
+			      && list_empty(&cwq->delayed_works);
+		spin_unlock_irq(&cwq->gcwq->lock);
+
+		if (cwq_flushed)
 			continue;
 
 		if (++flush_cnt == 10 ||

Signature · 1 line

1.7.3.1

Thomas Tuttle

author

2011-09-09 23:01 UTC #1 lore raw

PATCH v2 1/1 workqueue: lock cwq access in drain_workqueue

Take cwq->gcwq->lock to avoid racing between drain_workqueue checking to make sure the workqueues are empty and cwq_dec_nr_in_flight decrementing and then incrementing nr_active when it activates a delayed work.

We discovered this when a corner case in one of our drivers resulted in us trying to destroy a workqueue in which the remaining work would always requeue itself again in the same workqueue. We would hit this race condition and trip the BUG_ON on workqueue.c:3080.

Signed-off-by Thomas Tuttle <[email protected]>

---
Updated to use bool instead of int (d'oh), and CCed maintainer.

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

kernel/workqueue.c +7 -1

index 25fb1b0..0c2e585 100644
@@ -2412,8 +2412,14 @@ reflush:
 
 	for_each_cwq_cpu(cpu, wq) {
 		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
+		bool cwq_flushed;
 
-		if (!cwq->nr_active && list_empty(&cwq->delayed_works))
+		spin_lock_irq(&cwq->gcwq->lock);
+		cwq_flushed = !cwq->nr_active
+			      && list_empty(&cwq->delayed_works);
+		spin_unlock_irq(&cwq->gcwq->lock);
+
+		if (cwq_flushed)
 			continue;
 
 		if (++flush_cnt == 10 ||

Signature · 1 line

1.7.3.1

Tejun Heo

maintainer

2011-09-11 01:35 UTC #2 lore raw

Hello,

Quoted 4 lines · Thomas Tuttle

Take cwq->gcwq->lock to avoid racing between drain_workqueue checking to make sure the workqueues are empty and cwq_dec_nr_in_flight decrementing and then incrementing nr_active when it activates a delayed work.

Nice catch. Just few minor nits below.

Quoted 17 lines

We discovered this when a corner case in one of our drivers resulted in us trying to destroy a workqueue in which the remaining work would always requeue itself again in the same workqueue. We would hit this race condition and trip the BUG_ON on workqueue.c:3080.
Signed-off-by Thomas Tuttle <[email protected]>
---
Updated to use bool instead of int (d'oh), and CCed maintainer.
 kernel/workqueue.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)
kernel/workqueue.c +0 -0
index 25fb1b0..0c2e585 100644

kernel/workqueue.c reflush: · line 2415

Quoted 4 lines

kernel/workqueue.c +1 -0

@@ -2412,8 +2412,14 @@ reflush:
 
 	for_each_cwq_cpu(cpu, wq) {
 		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
+		bool cwq_flushed;

Maybe "drained" would be better?

kernel/workqueue.c reflush: · line 2419

Quoted 4 lines

kernel/workqueue.c +3 -1

-		if (!cwq->nr_active && list_empty(&cwq->delayed_works))
+		spin_lock_irq(&cwq->gcwq->lock);
+		cwq_flushed = !cwq->nr_active
+			      && list_empty(&cwq->delayed_works);

and then this should fit inside 80 column, right?

Thanks.

Signature · 1 line

tejun

Thomas Tuttle

author

2011-09-11 03:27 UTC #3 lore raw

PATCH v3 1/1 workqueue: lock cwq access in drain_workqueue

Take cwq->gcwq->lock to avoid racing between drain_workqueue checking to make sure the workqueues are empty and cwq_dec_nr_in_flight decrementing and then incrementing nr_active when it activates a delayed work.

We discovered this when a corner case in one of our drivers resulted in us trying to destroy a workqueue in which the remaining work would always requeue itself again in the same workqueue. We would hit this race condition and trip the BUG_ON on workqueue.c:3080.

Signed-off-by Thomas Tuttle <[email protected]>

---
Renamed "cwq_flushed" to "drained" as requested and rebased against current HEAD (d0a77454c70d0449a5f87087deb8f0cb15145e90).

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

kernel/workqueue.c +6 -1

index 25fb1b0..1783aab 100644
@@ -2412,8 +2412,13 @@ reflush:
 
 	for_each_cwq_cpu(cpu, wq) {
 		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
+		bool drained;
 
-		if (!cwq->nr_active && list_empty(&cwq->delayed_works))
+		spin_lock_irq(&cwq->gcwq->lock);
+		drained = !cwq->nr_active && list_empty(&cwq->delayed_works);
+		spin_unlock_irq(&cwq->gcwq->lock);
+
+		if (drained)
 			continue;
 
 		if (++flush_cnt == 10 ||

Signature · 1 line

1.7.3.1

Tejun Heo

maintainer Acked-by on v3 1/1

2011-09-11 03:30 UTC #4 lore raw

Hello,

Quoted 11 lines · Thomas Tuttle

Take cwq->gcwq->lock to avoid racing between drain_workqueue checking to make sure the workqueues are empty and cwq_dec_nr_in_flight decrementing and then incrementing nr_active when it activates a delayed work.
We discovered this when a corner case in one of our drivers resulted in us trying to destroy a workqueue in which the remaining work would always requeue itself again in the same workqueue. We would hit this race condition and trip the BUG_ON on workqueue.c:3080.
Signed-off-by Thomas Tuttle <[email protected]>

Acked-by Tejun Heo <[email protected]>

Can you please also add "Cc: [email protected]" and send it to Andrew Morton <[email protected]>?

Thank you very much.

Signature · 1 line

tejun

Thomas Tuttle

author

2011-09-11 03:41 UTC #5 lore raw

PATCH v3 1/1 workqueue: lock cwq access in drain_workqueue

Take cwq->gcwq->lock to avoid racing between drain_workqueue checking to make sure the workqueues are empty and cwq_dec_nr_in_flight decrementing and then incrementing nr_active when it activates a delayed work.

We discovered this when a corner case in one of our drivers resulted in us trying to destroy a workqueue in which the remaining work would always requeue itself again in the same workqueue. We would hit this race condition and trip the BUG_ON on workqueue.c:3080.

Signed-off-by Thomas Tuttle <[email protected]>

Acked-by Tejun Heo <[email protected]>

Cc [email protected]

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

kernel/workqueue.c +6 -1

index 25fb1b0..1783aab 100644
@@ -2412,8 +2412,13 @@ reflush:
 
 	for_each_cwq_cpu(cpu, wq) {
 		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
+		bool drained;
 
-		if (!cwq->nr_active && list_empty(&cwq->delayed_works))
+		spin_lock_irq(&cwq->gcwq->lock);
+		drained = !cwq->nr_active && list_empty(&cwq->delayed_works);
+		spin_unlock_irq(&cwq->gcwq->lock);
+
+		if (drained)
 			continue;
 
 		if (++flush_cnt == 10 ||

Signature · 4 lines

1.7.3.1


----- End forwarded message -----

Tejun Heo

maintainer

2011-09-11 03:48 UTC #6 lore raw

Quoted 5 lines · Thomas Tuttle

Take cwq->gcwq->lock to avoid racing between drain_workqueue checking to make sure the workqueues are empty and cwq_dec_nr_in_flight decrementing and then incrementing nr_active when it activates a delayed work.

Quoted 8 lines

We discovered this when a corner case in one of our drivers resulted in us trying to destroy a workqueue in which the remaining work would always requeue itself again in the same workqueue. We would hit this race condition and trip the BUG_ON on workqueue.c:3080.
Signed-off-by Thomas Tuttle <[email protected]>
Acked-by Tejun Heo <[email protected]>
Cc [email protected]

Andrew, can you please route this patch through -mm? korg is still down and wq is unlikely to receive many more patches in this cycle.

Thank you.

Signature · 1 line

tejun