Skip to contentSkip to conversation

workqueue: debug possible endless loop in cancel_rearming_delayed_work

PATCH In review Jarek Poplawski 5 participants 14 messages Apr 19, 2007 – Apr 23, 2007 Workqueue

lore
Share & tools
lore

Jarek Poplawski

author
#0 lore raw

PATCH 1/1 workqueue: debug possible endless loop in cancel_rearming_delayed_work

Hi,

IMHO cancel_rearming_delayed_work is dangerous place:

- it assumes a work function always rearms (with no exception), which probably isn't explained enough now (but anyway should be checked in such loops);

- probably possible (theoretical) scenario: a few work functions rearm themselves with very short, equal times; before flush_workqueue ends, their timers are already fired, so cancel_delayed_work has nothing to do.

Maybe this patch could check, if I'm not dreaming...

PS: of course the counter value below is a question of taste

Signed-off-by Jarek Poplawski <[email protected]>

---

kernel/workqueue.c +4 -1
diff -Nurp 2.6.21-rc6-mm1-/kernel/workqueue.c 2.6.21-rc6-mm1/kernel/workqueue.c
@@ -557,9 +557,12 @@ void cancel_rearming_delayed_work(struct
 	/* Was it ever queued ? */
 	if (cwq != NULL) {
 		struct workqueue_struct *wq = cwq->wq;
+		int i = 1000;
 
-		while (!cancel_delayed_work(dwork))
+		while (!cancel_delayed_work(dwork)) {
 			flush_workqueue(wq);
+			BUG_ON(!i--);
+		}
 	}
 }
 EXPORT_SYMBOL(cancel_rearming_delayed_work);
#1 lore raw

kernel/workqueue.c void cancel_rearming_delayed_work(struct · line 565

Quoted 7 lines · Jarek Poplawski
kernel/workqueue.c +4 -1
+		int i = 1000;
 
-		while (!cancel_delayed_work(dwork))
+		while (!cancel_delayed_work(dwork)) {
 			flush_workqueue(wq);
+			BUG_ON(!i--);
+		}

if then make it a WARN_ON(). But ... dont we have the softlockup detector for such cases? Does CONFIG_DETECT_SOFTLOCKUP=y give you enough information?

Ingo

author
#2 lore raw
Quoted 5 lines · Ingo Molnar
Quoted 2 lines · Jarek Poplawski
kernel/workqueue.c +1 -0
+		int i = 1000;
Quoted 9 lines

kernel/workqueue.c void cancel_rearming_delayed_work(struct · line 565

Quoted 5 lines
kernel/workqueue.c +3 -1
-		while (!cancel_delayed_work(dwork))
+		while (!cancel_delayed_work(dwork)) {
 			flush_workqueue(wq);
+			BUG_ON(!i--);
+		}

if then make it a WARN_ON(). But ... dont we have the softlockup detector for such cases? Does CONFIG_DETECT_SOFTLOCKUP=y give you enough information?

Maybe it was my testing, but no softlockup detection triggered here. I'll try to wait longer - and if no result - I'll resubmit tomorrow.

On the other hand, I think the return of cancel_delayed_work is too much depending on probability here (it may look like shooting to running targets), so it could be hard to repeat (and debugging not necessarily on at the time). As a matter of fact I intentionally made it for -mm, to see if the problem really exists, because then, another solution is needed.

And of course WARN_ON should be better - if only it's messages were more reliably saved in logs after rebooting.

Jarek P.

#3 lore raw
In reply to Ingo Molnar
Quoted 5 lines · Ingo Molnar
Quoted 2 lines · Jarek Poplawski
kernel/workqueue.c +1 -0
+		int i = 1000;
Quoted 9 lines

kernel/workqueue.c void cancel_rearming_delayed_work(struct · line 565

Quoted 5 lines
kernel/workqueue.c +3 -1
-		while (!cancel_delayed_work(dwork))
+		while (!cancel_delayed_work(dwork)) {
 			flush_workqueue(wq);
+			BUG_ON(!i--);
+		}

if then make it a WARN_ON(). But ... dont we have the softlockup detector for such cases? Does CONFIG_DETECT_SOFTLOCKUP=y give you enough information?

No, you won't get a soft lockup - flush_workqueue() has a schedule() call in the middle of it.

Cheers,

Dave.

Signature · 3 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
#4 lore raw
In reply to Jarek Poplawski
Quoted 3 lines · Jarek Poplawski

Hi,

IMHO cancel_rearming_delayed_work is dangerous place:

Agreed - I spent a couple of hours today learning why it can only be used on work functions that always rearm...

Quoted 8 lines

- it assumes a work function always rearms (with no exception), which probably isn't explained enough now (but anyway should be checked in such loops);

- probably possible (theoretical) scenario: a few work functions rearm themselves with very short, equal times; before flush_workqueue ends, their timers are already fired, so cancel_delayed_work has nothing to do.

Easier than that - have a work function that rearms only if there's more work to do in the future. You only arm the timer when you have work to do, and it only rearms if there's more work to do in the future (e.g. rotating expiry lists).

i.e. while there's more work to do, you need to call cancel_rearming_delayed_work() to stop it reliably, but if you race with the work function not restarting itself, you hang.....

Cheers,

Dave.

Signature · 3 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
author
#5 lore raw
Quoted 17 lines · David Chinner
Quoted 3 lines · Jarek Poplawski

Hi,

IMHO cancel_rearming_delayed_work is dangerous place:

Agreed - I spent a couple of hours today learning why it can only be used on work functions that always rearm...

Quoted 8 lines

- it assumes a work function always rearms (with no exception), which probably isn't explained enough now (but anyway should be checked in such loops);

- probably possible (theoretical) scenario: a few work functions rearm themselves with very short, equal times; before flush_workqueue ends, their timers are already fired, so cancel_delayed_work has nothing to do.

Quoted 8 lines

Easier than that - have a work function that rearms only if there's more work to do in the future. You only arm the timer when you have work to do, and it only rearms if there's more work to do in the future (e.g. rotating expiry lists).

i.e. while there's more work to do, you need to call cancel_rearming_delayed_work() to stop it reliably, but if you race with the work function not restarting itself, you hang.....

I'm not sure I correctly get your point, but according to this comment:

" * cancel_rearming_delayed_work - reliably kill off a delayed keventd work whose handler rearms the delayed work."

there is a question, whether a function that "rearms only if"
- "rearms". It seems the author of this comment didn't think so and it was obvious to him/her cancel_rearming_delayed_work wasn't intended for this case. At first I thought it's only a language question - now, I see it's probably logical, too.

Cheers,
Jarek P.

#6 lore raw
Quoted 27 lines · Jarek Poplawski
Quoted 17 lines · David Chinner
Quoted 3 lines · Jarek Poplawski

Hi,

IMHO cancel_rearming_delayed_work is dangerous place:

Agreed - I spent a couple of hours today learning why it can only be used on work functions that always rearm...

Quoted 8 lines

- it assumes a work function always rearms (with no exception), which probably isn't explained enough now (but anyway should be checked in such loops);

- probably possible (theoretical) scenario: a few work functions rearm themselves with very short, equal times; before flush_workqueue ends, their timers are already fired, so cancel_delayed_work has nothing to do.

Quoted 8 lines

Easier than that - have a work function that rearms only if there's more work to do in the future. You only arm the timer when you have work to do, and it only rearms if there's more work to do in the future (e.g. rotating expiry lists).

i.e. while there's more work to do, you need to call cancel_rearming_delayed_work() to stop it reliably, but if you race with the work function not restarting itself, you hang.....

Quoted 8 lines

I'm not sure I correctly get your point, but according to this comment:

" * cancel_rearming_delayed_work - reliably kill off a delayed keventd work whose handler rearms the delayed work."

there is a question, whether a function that "rearms only if"
- "rearms".

Right. Given that the bug I was initially trying to solve was a race killing off a handler that was rearming itself, that comment says to me "this is the right thing to do".

Quoted 4 lines

It seems the author of this comment didn't think
so and it was obvious to him/her cancel_rearming_delayed_work wasn't intended for this case. At first I thought it's only a language question - now, I see it's probably logical, too.

Yes, after spending another two hours working out why my fix was then hanging in cancel_rearming_delayed_work() I was a little bit annoyed at the now obviously misleading comment. Five minutes later I'd fixed the bug properly. A better comment would have saved me two hours of wasted time.....

Cheers,

Dave.

Signature · 3 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
author
#7 lore raw

On Fri, Apr 20, 2007 at 06:53:54PM +1000, David Chinner wrote:
...

Quoted 3 lines

Yes, after spending another two hours working out why my fix was then hanging in cancel_rearming_delayed_work() I was a little bit annoyed at the now obviously misleading comment. Five minutes later

I agree with your feelings, but strictly speaking it's not "obviously misleading":

" * cancel_rearming_delayed_work - reliably kill off a delayed keventd work whose handler rearms the delayed work."

And all your rearmed works were, probably, reliably killed!

Quoted 2 lines

I'd fixed the bug properly. A better comment would have saved me two hours of wasted time.....

It seems to be an Achilles' heel of linux...

Regards,
Jarek P.

#8 lore raw
Quoted 11 lines · Jarek Poplawski

On Fri, Apr 20, 2007 at 06:53:54PM +1000, David Chinner wrote:
...

Quoted 3 lines

Yes, after spending another two hours working out why my fix was then hanging in cancel_rearming_delayed_work() I was a little bit annoyed at the now obviously misleading comment. Five minutes later

I agree with your feelings, but strictly speaking it's not "obviously misleading":

" * cancel_rearming_delayed_work - reliably kill off a delayed keventd work whose handler rearms the delayed work."

" This is not reliable if the handler rearms itself conditionally and it is restarted by external means."

Quoted 1 line

And all your rearmed works were, probably, reliably killed!

Yeah, it's the ones that weren't that we the problem ;)

Quoted 4 lines
Quoted 2 lines

I'd fixed the bug properly. A better comment would have saved me two hours of wasted time.....

It seems to be an Achilles' heel of linux...

Too true....

Cheers,

Dave.

Signature · 3 lines
Dave Chinner
Principal Engineer
SGI Australian Software Group
author
#9 lore raw

PATCH 1/1 workqueue: cancel_rearming_delayed_work/workqueue usage warning

Here is my proposal to make things clearer:
(this time on 2.6.21-rc7)

CC David Chinner <[email protected]>
CC Oleg Nesterov <[email protected]>
Signed-off-by Jarek Poplawski <[email protected]>

---

kernel/workqueue.c +4 -0
diff -Nurp 2.6.21-rc7-/kernel/workqueue.c 2.6.21-rc7/kernel/workqueue.c
@@ -662,6 +662,8 @@ EXPORT_SYMBOL(flush_scheduled_work);
  * cancel_rearming_delayed_workqueue - reliably kill off a delayed work whose handler rearms the delayed work.
  * @wq:   the controlling workqueue structure
  * @dwork: the delayed work struct
+ *
+ * WARNING: use only with handlers, which rearm unconditionally with delay > 0
  */
 void cancel_rearming_delayed_workqueue(struct workqueue_struct *wq,
 				       struct delayed_work *dwork)

@@ -674,6 +676,8 @@ EXPORT_SYMBOL(cancel_rearming_delayed_wo
 /**
  * cancel_rearming_delayed_work - reliably kill off a delayed keventd work whose handler rearms the delayed work.
  * @dwork: the delayed work struct
+ *
+ * WARNING: use only with handlers, which rearm unconditionally with delay > 0
  */
 void cancel_rearming_delayed_work(struct delayed_work *dwork)
 {
#10 lore raw

Re: [PATCH] workqueue: cancel_rearming_delayed_work/workqueue usage warning

Quoted 14 lines · Jarek Poplawski

Here is my proposal to make things clearer:
(this time on 2.6.21-rc7)

CC David Chinner <[email protected]>
CC Oleg Nesterov <[email protected]>
Signed-off-by Jarek Poplawski <[email protected]>

---

diff -Nurp 2.6.21-rc7-/kernel/workqueue.c 2.6.21-rc7/kernel/workqueue.c

kernel/workqueue.c EXPORT_SYMBOL(flush_scheduled_work); · line 667

Quoted 6 lines
kernel/workqueue.c +2 -0
@@ -662,6 +662,8 @@ EXPORT_SYMBOL(flush_scheduled_work);
  * cancel_rearming_delayed_workqueue - reliably kill off a delayed work whose handler rearms the delayed work.
  * @wq:   the controlling workqueue structure
  * @dwork: the delayed work struct
+ *
+ * WARNING: use only with handlers, which rearm unconditionally with delay > 0
  */

Nit: it is ok if the work re-arms itself with delay == 0 "sometimes". What we need is that the handler use delay > 0 eventually.

I'd suggest to re-diff against -mm tree. I don't think this patch can find its way to the soon to be released 2.6.21.

Oleg.

author
#11 lore raw

Re: [PATCH] workqueue: cancel_rearming_delayed_work/workqueue usage warning

Quoted 11 lines · Oleg Nesterov
Quoted 10 lines · Jarek Poplawski

Here is my proposal to make things clearer:
(this time on 2.6.21-rc7)

CC David Chinner <[email protected]>
CC Oleg Nesterov <[email protected]>
Signed-off-by Jarek Poplawski <[email protected]>

---

Quoted 13 lines

kernel/workqueue.c EXPORT_SYMBOL(flush_scheduled_work); · line 667

Quoted 10 lines

diff -Nurp 2.6.21-rc7-/kernel/workqueue.c 2.6.21-rc7/kernel/workqueue.c

kernel/workqueue.c +2 -0
@@ -662,6 +662,8 @@ EXPORT_SYMBOL(flush_scheduled_work);
  * cancel_rearming_delayed_workqueue - reliably kill off a delayed work whose handler rearms the delayed work.
  * @wq:   the controlling workqueue structure
  * @dwork: the delayed work struct
+ *
+ * WARNING: use only with handlers, which rearm unconditionally with delay > 0
  */

Nit: it is ok if the work re-arms itself with delay == 0 "sometimes". What we need is that the handler use delay > 0 eventually.

Probably it would be shorter to write it yourself, because I'm not sure of your intentions: so, you think we can call this function with such a work?

Quoted 3 lines

I'd suggest to re-diff against -mm tree. I don't think this patch can find its way to the soon to be released 2.6.21.

The current thread seems to confirm my initial fear, this function is not explained enough, so it should help to remove errors as soon as possible from the current code. And I hope this functions will work other way soon...

Jarek P.

#12 lore raw
In reply to Jarek Poplawski
Quoted 8 lines · Jarek Poplawski

Hi,

IMHO cancel_rearming_delayed_work is dangerous place:

- it assumes a work function always rearms (with no exception), which probably isn't explained enough now (but anyway should be checked in such loops);

Quoted 8 lines

- probably possible (theoretical) scenario: a few work functions rearm themselves with very short, equal times; before flush_workqueue ends, their timers are already fired, so cancel_delayed_work has nothing to do.

Maybe this patch could check, if I'm not dreaming...

PS: of course the counter value below is a question of taste

Okay, an easy test for it: insmod netconsole ; rmmod netconsole

In 2.6.20.x it loops forever and cancel_rearming_delayed_work() is part of the trace...

author
#13 lore raw
Quoted 14 lines · Chuck Ebbert
Quoted 13 lines · Jarek Poplawski

Hi,

IMHO cancel_rearming_delayed_work is dangerous place:

- it assumes a work function always rearms (with no exception), which probably isn't explained enough now (but anyway should be checked in such loops);

- probably possible (theoretical) scenario: a few work functions rearm themselves with very short, equal times; before flush_workqueue ends, their timers are already fired, so cancel_delayed_work has nothing to do.

Quoted 8 lines
Quoted 3 lines

Maybe this patch could check, if I'm not dreaming...

PS: of course the counter value below is a question of taste

Okay, an easy test for it: insmod netconsole ; rmmod netconsole

In 2.6.20.x it loops forever and cancel_rearming_delayed_work() is part of the trace...

Sure, but the dreaming test is more needed for the second point - I mean - if it's theoretical only? The first scenario is in my opinion assured logically - there was only a question, whether it could be detected by other means - and it seems not.

Cheers,
Jarek P.