[PATCH] cancel_delayed_work: use del_timer() instead of del_timer_sync()

[PATCH] cancel_delayed_work: use del_timer() instead of del_timer_sync()

[Oleg Nesterov]

del_timer_sync() buys nothing for cancel_delayed_work(), but it is less
efficient since it locks the timer unconditionally, and may wait for the
completion of the delayed_work_timer_fn().

cancel_delayed_work() == 0 means:

	before this patch:
		work->func may still be running or queued

	after this patch:
		work->func may still be running or queued, or
		delayed_work_timer_fn->__queue_work() in progress.

		The latter doesn't differ from the caller's POV,
		delayed_work_timer_fn() is called with _PENDING
		bit set.

cancel_delayed_work() == 1 with this patch adds a new possibility:

	delayed_work->work was cancelled, but delayed_work_timer_fn
	is still running (this is only possible for the re-arming
	works on single-threaded workqueue).

	In this case the timer was re-started by work->func(), nobody
	else can do this. This in turn means that delayed_work_timer_fn
	has already passed __queue_work() (and wont't touch delayed_work)
	because nobody else can queue delayed_work->work.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- OLD/include/linux/workqueue.h~2_CDW	2007-04-05 12:20:35.000000000 +0400
+++ OLD/include/linux/workqueue.h	2007-04-25 01:48:24.000000000 +0400
@@ -152,14 +152,15 @@ extern void cancel_work_sync(struct work
 
 /*
  * 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_workqueue() or cancel_work_sync() to wait on it.
+ * function may still be running on return from cancel_delayed_work(), unless
+ * it returns 1 and the work doesn't re-arm itself. Run flush_workqueue() or
+ * cancel_work_sync() to wait on it.
  */
 static inline int cancel_delayed_work(struct delayed_work *work)
 {
 	int ret;
 
-	ret = del_timer_sync(&work->timer);
+	ret = del_timer(&work->timer);
 	if (ret)
 		work_clear_pending(&work->work);
 	return ret;

Re: [PATCH] cancel_delayed_work: use del_timer() instead of del_timer_sync()

[David Howells]

Oleg Nesterov <oleg@tv-sign.ru> wrote:

> del_timer_sync() buys nothing for cancel_delayed_work(), but it is less
> efficient since it locks the timer unconditionally, and may wait for the
> completion of the delayed_work_timer_fn().

Okay, this patch seems to work for me, though I'm not using any of the other
related patches that are in the -mm tree (I modified this patch to compile
directly on top of Linus's vanilla tree).

Acked-By: David Howells <dhowells@redhat.com>

Re: [PATCH] cancel_delayed_work: use del_timer() instead of del_timer_sync()

[Jarek Poplawski]

On Wed, Apr 25, 2007 at 01:50:34AM +0400, Oleg Nesterov wrote:
> del_timer_sync() buys nothing for cancel_delayed_work(), but it is less
> efficient since it locks the timer unconditionally, and may wait for the
> completion of the delayed_work_timer_fn().

I'm not sure what is the main aim of this patch. It seems this
change cannot do any harm, but anyway it could change a few
things, e.g. with current version of cancel_rearming_delayed_work
some flush_workqueue could be done needlessly, before the work
is queued from timer. It's not a big deal here, but if anybody
did something like this without loop - it could matter.

So, probably a lot of current code should be checked, before
applying and I doubt the gain is worth of this. Maybe, for
safety, make this with new name as an alternative and
deprecate the current version?

Regards,
Jarek P.

Re: [PATCH] cancel_delayed_work: use del_timer() instead of del_timer_sync()

[Oleg Nesterov]

On 04/25, Jarek Poplawski wrote:
>
> On Wed, Apr 25, 2007 at 01:50:34AM +0400, Oleg Nesterov wrote:
> > del_timer_sync() buys nothing for cancel_delayed_work(), but it is less
> > efficient since it locks the timer unconditionally, and may wait for the
> > completion of the delayed_work_timer_fn().
> 
> I'm not sure what is the main aim of this patch.

optimization

>                                                  It seems this
> change cannot do any harm, but anyway it could change a few
> things, e.g. with current version of cancel_rearming_delayed_work
> some flush_workqueue could be done needlessly, before the work
> is queued from timer.

I don't think so... Could you clarify?

>                       It's not a big deal here, but if anybody
> did something like this without loop - it could matter.
> 
> So, probably a lot of current code should be checked, before
> applying and I doubt the gain is worth of this. Maybe, for
> safety, make this with new name as an alternative and
> deprecate the current version?

This change should not make any visible difference for the callers,
otherwise it is buggy.

Oleg.

Re: [PATCH] cancel_delayed_work: use del_timer() instead of del_timer_sync()

[Jarek Poplawski]

On Wed, Apr 25, 2007 at 04:52:14PM +0400, Oleg Nesterov wrote:
> On 04/25, Jarek Poplawski wrote:
> >
> > On Wed, Apr 25, 2007 at 01:50:34AM +0400, Oleg Nesterov wrote:
> > > del_timer_sync() buys nothing for cancel_delayed_work(), but it is less
> > > efficient since it locks the timer unconditionally, and may wait for the
> > > completion of the delayed_work_timer_fn().
> > 
> > I'm not sure what is the main aim of this patch.
> 
> optimization
> 
> >                                                  It seems this
> > change cannot do any harm, but anyway it could change a few
> > things, e.g. with current version of cancel_rearming_delayed_work
> > some flush_workqueue could be done needlessly, before the work
> > is queued from timer.
> 
> I don't think so... Could you clarify?

With a code like:

if (!cancel_delayed_work(dwork))
	flush_workqueue(wq);
	
if cancel_ returns 0, and there is _queue_work in progress,
flush_ will be done once, after this work is queued.

After the patch, and the same situation flush_ also runs
one time, but maybe without the work in a queue.

So, if there is no more loops, there could be difference,
and even if very unprobable, something could stop working
after such change.

> 
> >                       It's not a big deal here, but if anybody
> > did something like this without loop - it could matter.
> > 
> > So, probably a lot of current code should be checked, before
> > applying and I doubt the gain is worth of this. Maybe, for
> > safety, make this with new name as an alternative and
> > deprecate the current version?
> 
> This change should not make any visible difference for the callers,
> otherwise it is buggy.

IMHO, there is the same visible difference,
as between del_timer and del_timer_sync.

Regards,
Jarek P.

Re: [PATCH] cancel_delayed_work: use del_timer() instead of del_timer_sync()

[Oleg Nesterov]

On 04/26, Jarek Poplawski wrote:
>
> On Wed, Apr 25, 2007 at 04:52:14PM +0400, Oleg Nesterov wrote:
> > 
> > >                                                  It seems this
> > > change cannot do any harm, but anyway it could change a few
> > > things, e.g. with current version of cancel_rearming_delayed_work
> > > some flush_workqueue could be done needlessly, before the work
> > > is queued from timer.
> > 
> > I don't think so... Could you clarify?
> 
> With a code like:
> 
> if (!cancel_delayed_work(dwork))
> 	flush_workqueue(wq);
> 	
> if cancel_ returns 0, and there is _queue_work in progress,
> flush_ will be done once, after this work is queued.
> 
> After the patch, and the same situation flush_ also runs
> one time, but maybe without the work in a queue.

First, this is very unlikely event, and the behaviour is correct,
do you agree?

Even in this case, it is very unlikely that flush_cpu_workqueue()
will take cwq->lock before extremely short delayed_work_timer_fn.

Even if it does, work->func() likely will be completed when the
caller of flush_workqueue() will be woken by run_workquue() and
gets CPU.

Please also look at http://marc.info/?t=117699337200022&r=1

> > >                       It's not a big deal here, but if anybody
> > > did something like this without loop - it could matter.
> > > 
> > > So, probably a lot of current code should be checked, before
> > > applying and I doubt the gain is worth of this. Maybe, for
> > > safety, make this with new name as an alternative and
> > > deprecate the current version?
> > 
> > This change should not make any visible difference for the callers,
> > otherwise it is buggy.
> 
> IMHO, there is the same visible difference,
> as between del_timer and del_timer_sync.

Jarek, please, could you be more explicite ? del_timer() and
del_timer_sync() are different in many ways. What exactly will
impact the user of cancel_delaye_work ?

Oleg.

Re: [PATCH] cancel_delayed_work: use del_timer() instead of del_timer_sync()

[Jarek Poplawski]

On Thu, Apr 26, 2007 at 07:29:53PM +0400, Oleg Nesterov wrote:
> On 04/26, Jarek Poplawski wrote:
...
> > > This change should not make any visible difference for the callers,
> > > otherwise it is buggy.
> > 
> > IMHO, there is the same visible difference,
> > as between del_timer and del_timer_sync.
> 
> Jarek, please, could you be more explicite ? del_timer() and
> del_timer_sync() are different in many ways. What exactly will
> impact the user of cancel_delaye_work ?

OK, I changed my mind. Now, I think it's very probable
this should matter...

According to workqueue.h:
> /*
>  * 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_workqueue() or cancel_work_sync() to wait on it.
>  */
> static inline int cancel_delayed_work(struct delayed_work *work)

So, we can do something like this:

cancel_delayed_work(dwork);
flush_workqueue(wq);
kfree(some_obj_used_by_dwork_func);

And this is enough to work with not rearming work.

But no more after this patch...

So, I think, your proposal should be alternative version,
and current version should stay, so we have a choice.
Just like del_timer and del_timer_sync.

Jarek P.

Re: [PATCH] cancel_delayed_work: use del_timer() instead of del_timer_sync()

[Oleg Nesterov]

On 04/27, Jarek Poplawski wrote:
>
> According to workqueue.h:
> > /*
> >  * 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_workqueue() or cancel_work_sync() to wait on it.
> >  */
> > static inline int cancel_delayed_work(struct delayed_work *work)
> 
> So, we can do something like this:
> 
> cancel_delayed_work(dwork);
> flush_workqueue(wq);
> kfree(some_obj_used_by_dwork_func);
> 
> And this is enough to work with not rearming work.
> 
> But no more after this patch...

Yes, you are right, and so this patch is wrong.

This is even documented in the changelog, with this change cancel_delayed_work()
may return while the queueing is in progress. However, in that case we can not
rely on flush_workqueue/cancel_work_sync.

Thanks a lot!

Oleg.