[PATCH -mm] workqueue: debug possible endless loop in cancel_rearming_delayed_work

[PATCH -mm] workqueue: debug possible endless loop in cancel_rearming_delayed_work

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

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 <jarkao2@o2.pl>

---

diff -Nurp 2.6.21-rc6-mm1-/kernel/workqueue.c 2.6.21-rc6-mm1/kernel/workqueue.c
--- 2.6.21-rc6-mm1-/kernel/workqueue.c	2007-04-18 20:07:45.000000000 +0200
+++ 2.6.21-rc6-mm1/kernel/workqueue.c	2007-04-18 20:15:44.000000000 +0200
@@ -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);

Re: [PATCH -mm] workqueue: debug possible endless loop in cancel_rearming_delayed_work

[Ingo Molnar]

* Jarek Poplawski <jarkao2@o2.pl> wrote:

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

Re: [PATCH -mm] workqueue: debug possible endless loop in cancel_rearming_delayed_work

[Jarek Poplawski]

On Thu, Apr 19, 2007 at 09:32:22AM +0200, Ingo Molnar wrote:
> 
> * Jarek Poplawski <jarkao2@o2.pl> wrote:
> 
> > +		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?

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.

Re: [PATCH -mm] workqueue: debug possible endless loop in cancel_rearming_delayed_work

[David Chinner]

On Thu, Apr 19, 2007 at 08:54:04AM +0200, Jarek Poplawski wrote:
> 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...

> - 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.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [PATCH -mm] workqueue: debug possible endless loop in cancel_rearming_delayed_work

[David Chinner]

On Thu, Apr 19, 2007 at 09:32:22AM +0200, Ingo Molnar wrote:
> 
> * Jarek Poplawski <jarkao2@o2.pl> wrote:
> 
> > +		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?

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

Cheers,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [PATCH -mm] workqueue: debug possible endless loop in cancel_rearming_delayed_work

[Chuck Ebbert]

Jarek Poplawski wrote:
> 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

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

Re: [PATCH -mm] workqueue: debug possible endless loop in cancel_rearming_delayed_work

[Jarek Poplawski]

On Thu, Apr 19, 2007 at 01:07:11PM -0400, Chuck Ebbert wrote:
> Jarek Poplawski wrote:
> > 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
> 
> 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.

Re: [PATCH -mm] workqueue: debug possible endless loop in cancel_rearming_delayed_work

[Jarek Poplawski]

On Fri, Apr 20, 2007 at 12:46:18AM +1000, David Chinner wrote:
> On Thu, Apr 19, 2007 at 08:54:04AM +0200, Jarek Poplawski wrote:
> > 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...
> 
> > - 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.....

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.

Re: [PATCH -mm] workqueue: debug possible endless loop in cancel_rearming_delayed_work

[David Chinner]

On Fri, Apr 20, 2007 at 10:13:26AM +0200, Jarek Poplawski wrote:
> On Fri, Apr 20, 2007 at 12:46:18AM +1000, David Chinner wrote:
> > On Thu, Apr 19, 2007 at 08:54:04AM +0200, Jarek Poplawski wrote:
> > > 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...
> > 
> > > - 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.....
> 
> 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".

> 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.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

Re: [PATCH -mm] workqueue: debug possible endless loop in cancel_rearming_delayed_work

[Jarek Poplawski]

On Fri, Apr 20, 2007 at 06:53:54PM +1000, David Chinner wrote:
...
> 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!

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

Re: [PATCH -mm] workqueue: debug possible endless loop in cancel_rearming_delayed_work

[David Chinner]

On Fri, Apr 20, 2007 at 12:21:57PM +0200, Jarek Poplawski wrote:
> On Fri, Apr 20, 2007 at 06:53:54PM +1000, David Chinner wrote:
> ...
> > 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."

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

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

> > 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.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group

[PATCH -mm] workqueue: debug possible endless loop in cancel_rearming_delayed_work

[Jarek Poplawski]

(take 2)

I'm not sure we've agreed enough, who'll resubmit, so here
it is with WARN_ON. If it was submited already - forget it.

Jarek P.

--->
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 <jarkao2@o2.pl>

---

diff -Nurp 2.6.21-rc6-mm1-/kernel/workqueue.c 2.6.21-rc6-mm1/kernel/workqueue.c
--- 2.6.21-rc6-mm1-/kernel/workqueue.c	2007-04-18 20:07:45.000000000 +0200
+++ 2.6.21-rc6-mm1/kernel/workqueue.c	2007-04-18 20:15:44.000000000 +0200
@@ -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);
+			WARN_ON(!i--);
+		}
 	}
 }
 EXPORT_SYMBOL(cancel_rearming_delayed_work);

[PATCH] workqueue: cancel_rearming_delayed_work/workqueue usage warning

[Jarek Poplawski]

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

CC: David Chinner <dgc@sgi.com>
CC: Oleg Nesterov <oleg@tv-sign.ru>
Signed-off-by: Jarek Poplawski <jarkao2@o2.pl>

---

diff -Nurp 2.6.21-rc7-/kernel/workqueue.c 2.6.21-rc7/kernel/workqueue.c
--- 2.6.21-rc7-/kernel/workqueue.c	2007-04-18 10:14:16.000000000 +0200
+++ 2.6.21-rc7/kernel/workqueue.c	2007-04-20 13:56:51.000000000 +0200
@@ -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)
 {

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

[Oleg Nesterov]

On 04/20, Jarek Poplawski wrote:
> 
> Here is my proposal to make things clearer:
> (this time on 2.6.21-rc7)
> 
> CC: David Chinner <dgc@sgi.com>
> CC: Oleg Nesterov <oleg@tv-sign.ru>
> Signed-off-by: Jarek Poplawski <jarkao2@o2.pl>
> 
> ---
> 
> diff -Nurp 2.6.21-rc7-/kernel/workqueue.c 2.6.21-rc7/kernel/workqueue.c
> --- 2.6.21-rc7-/kernel/workqueue.c	2007-04-18 10:14:16.000000000 +0200
> +++ 2.6.21-rc7/kernel/workqueue.c	2007-04-20 13:56:51.000000000 +0200
> @@ -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.

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

[Jarek Poplawski]

On Fri, Apr 20, 2007 at 09:23:48PM +0400, Oleg Nesterov wrote:
> On 04/20, Jarek Poplawski wrote:
> > 
> > Here is my proposal to make things clearer:
> > (this time on 2.6.21-rc7)
> > 
> > CC: David Chinner <dgc@sgi.com>
> > CC: Oleg Nesterov <oleg@tv-sign.ru>
> > Signed-off-by: Jarek Poplawski <jarkao2@o2.pl>
> > 
> > ---
> > 
> > diff -Nurp 2.6.21-rc7-/kernel/workqueue.c 2.6.21-rc7/kernel/workqueue.c
> > --- 2.6.21-rc7-/kernel/workqueue.c	2007-04-18 10:14:16.000000000 +0200
> > +++ 2.6.21-rc7/kernel/workqueue.c	2007-04-20 13:56:51.000000000 +0200
> > @@ -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?

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