workqueue and pending

workqueue and pending

[Mikkel Christiansen]

Hi

We're trying to use the workqueue interface for scheduling a delayed 
gargage collector.
Since we only need a single delayed thread we used the DECLEARE_WORK macro.

The problem is that once we call cancel_delayed_work we can't schedule 
work again.

Having looked at the code i noticed that cancel_delayed_work only 
deletes the timer but
doesn't set clear the "pending" bit, thus any call to 
schedule_delayed_work is ignorred.

Example:

static DECLARE_WORK(tft, timeoutfun, NULL)
.
.
.
schedule_delayed_work(&tft, 1*HZ);
cancel_delayed_work(&tft);

<do some work>

tft.pending = 0;   / / if we dont clear pending no work can be scheduled
schedule_delayed_work(&tft, 1*HZ);
return 0;

End example

Setting tft.pending is a rather ugly solution - shouldn't it be cleared by
cancel_delayed_work?, or are we using the interface in the wrong way?

Cheers
    Mikkel

Re: workqueue and pending

[Andrew Morton]

Mikkel Christiansen <mixxel@cs.auc.dk> wrote:
>
> We're trying to use the workqueue interface for scheduling a delayed 
> gargage collector.
> Since we only need a single delayed thread we used the DECLEARE_WORK macro.
> 
> The problem is that once we call cancel_delayed_work we can't schedule 
> work again.

It looks like nobody has tried to do that yet.

> Having looked at the code i noticed that cancel_delayed_work only 
> deletes the timer but
> doesn't set clear the "pending" bit, thus any call to 
> schedule_delayed_work is ignorred.

Does this work?

--- 25/include/linux/workqueue.h~cancel_delayed_work-fix	Mon May  3 16:24:46 2004
+++ 25-akpm/include/linux/workqueue.h	Mon May  3 16:26:01 2004
@@ -7,6 +7,7 @@
 
 #include <linux/timer.h>
 #include <linux/linkage.h>
+#include <linux/bitops.h>
 
 struct workqueue_struct;
 
@@ -75,8 +76,11 @@ extern void init_workqueues(void);
  */
 static inline int cancel_delayed_work(struct work_struct *work)
 {
-	return del_timer_sync(&work->timer);
+	int ret;
+
+	ret = del_timer_sync(&work->timer);
+	clear_bit(0, &work->pending);
+	return ret;
 }
 
 #endif
-

_

Re: workqueue and pending

[Benjamin Herrenschmidt]

>  
> @@ -75,8 +76,11 @@ extern void init_workqueues(void);
>   */
>  static inline int cancel_delayed_work(struct work_struct *work)
>  {
> -	return del_timer_sync(&work->timer);
> +	int ret;
> +
> +	ret = del_timer_sync(&work->timer);
> +	clear_bit(0, &work->pending);
> +	return ret;
>  }
>  

Looks wrong to me. The time may have fired already and queued the
work. Clearing pending is an error in this case since the work is
indeed pending for execution.... 

Ben.

Re: workqueue and pending

[Andrew Morton]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> 
> >  
> > @@ -75,8 +76,11 @@ extern void init_workqueues(void);
> >   */
> >  static inline int cancel_delayed_work(struct work_struct *work)
> >  {
> > -	return del_timer_sync(&work->timer);
> > +	int ret;
> > +
> > +	ret = del_timer_sync(&work->timer);
> > +	clear_bit(0, &work->pending);
> > +	return ret;
> >  }
> >  
> 
> Looks wrong to me. The time may have fired already and queued the
> work. Clearing pending is an error in this case since the work is
> indeed pending for execution.... 

OK...

--- 25/include/linux/workqueue.h~cancel_delayed_work-fix	2004-05-03 20:14:26.796321416 -0700
+++ 25-akpm/include/linux/workqueue.h	2004-05-03 20:15:41.010039216 -0700
@@ -7,6 +7,7 @@
 
 #include <linux/timer.h>
 #include <linux/linkage.h>
+#include <linux/bitops.h>
 
 struct workqueue_struct;
 
@@ -75,8 +76,12 @@ extern void init_workqueues(void);
  */
 static inline int cancel_delayed_work(struct work_struct *work)
 {
-	return del_timer_sync(&work->timer);
+	int ret;
+
+	ret = del_timer_sync(&work->timer);
+	if (ret)
+		clear_bit(0, &work->pending);
+	return ret;
 }
 
 #endif
-

_

Re: workqueue and pending

[Benjamin Herrenschmidt]

>  static inline int cancel_delayed_work(struct work_struct *work)
>  {
> -	return del_timer_sync(&work->timer);
> +	int ret;
> +
> +	ret = del_timer_sync(&work->timer);
> +	if (ret)
> +		clear_bit(0, &work->pending);
> +	return ret;
>  }

Heh, I was trying to figure out a simple way to do that and just
didn't figure out del_timer_sync() would actually have a useful
return value :)

It's probably still good to precise explicitely in the comments
that upon return of cancel_delayed_work(), the work queue might
still be pending and that a flush and whoever called this may
still need a flush_scheduled_work() or flush_workqueue() (provided
it's running in a context where that can sleep)

Ben.

Re: workqueue and pending

[Andrew Morton]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> It's probably still good to precise explicitely in the comments
>  that upon return of cancel_delayed_work(), the work queue might
>  still be pending and that a flush and whoever called this may
>  still need a flush_scheduled_work() or flush_workqueue() (provided
>  it's running in a context where that can sleep)

That function was originally written by a comment fetishist.

/*
 * Kill off a pending schedule_delayed_work().  Note that the work callback
 * function may still be running on return from cancel_delayed_work().  Run
 * flush_scheduled_work() to wait on it.
 */
static inline int cancel_delayed_work(struct work_struct *work)
{
	int ret;

	ret = del_timer_sync(&work->timer);
	if (ret)
		clear_bit(0, &work->pending);
	return ret;
}

Re: workqueue and pending

[Benjamin Herrenschmidt]

On Tue, 2004-05-04 at 14:15, Andrew Morton wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> >
> > It's probably still good to precise explicitely in the comments
> >  that upon return of cancel_delayed_work(), the work queue might
> >  still be pending and that a flush and whoever called this may
> >  still need a flush_scheduled_work() or flush_workqueue() (provided
> >  it's running in a context where that can sleep)
> 
> That function was originally written by a comment fetishist.

Heh, I should have looked at the code =P

Ben.

Re: workqueue and pending

[Mikkel Christiansen]

I tried the patch and it works fine

-Mikkel

Andrew Morton wrote:

>Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>  
>
>>    
>>
>>> 
>>>@@ -75,8 +76,11 @@ extern void init_workqueues(void);
>>>  */
>>> static inline int cancel_delayed_work(struct work_struct *work)
>>> {
>>>-	return del_timer_sync(&work->timer);
>>>+	int ret;
>>>+
>>>+	ret = del_timer_sync(&work->timer);
>>>+	clear_bit(0, &work->pending);
>>>+	return ret;
>>> }
>>> 
>>>      
>>>
>>Looks wrong to me. The time may have fired already and queued the
>>work. Clearing pending is an error in this case since the work is
>>indeed pending for execution.... 
>>    
>>
>
>OK...
>
>--- 25/include/linux/workqueue.h~cancel_delayed_work-fix	2004-05-03 20:14:26.796321416 -0700
>+++ 25-akpm/include/linux/workqueue.h	2004-05-03 20:15:41.010039216 -0700
>@@ -7,6 +7,7 @@
> 
> #include <linux/timer.h>
> #include <linux/linkage.h>
>+#include <linux/bitops.h>
> 
> struct workqueue_struct;
> 
>@@ -75,8 +76,12 @@ extern void init_workqueues(void);
>  */
> static inline int cancel_delayed_work(struct work_struct *work)
> {
>-	return del_timer_sync(&work->timer);
>+	int ret;
>+
>+	ret = del_timer_sync(&work->timer);
>+	if (ret)
>+		clear_bit(0, &work->pending);
>+	return ret;
> }
> 
> #endif
>-
>
>_
>
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/
>  
>