cancel_delayed_work and its use of del_timer_sync

cancel_delayed_work and its use of del_timer_sync

[Dmitry Torokhov]

Hi Oleg,

I noticed that you went back and forth on using del_timer() vs.
del_timer_sync() in cancel_delayed_work(), finally settling on using
del_timer_sync(). I must say that the fact that cancel_delayed_work()
might sleep (given that there exists cancel_delayed_work_sync) catches a
few drivers writers by surprise. Moreover it makes delayed works
unsuitable in cases when we do want to cancel (so we can reschedule)
work in an interrupt context.

My question is if sleeping in cancel_delayed_work can be avoided or if
it is possible to have a reschedule_delayed_work() taht would do a
"soft" cancel and [re]submit the work. In the use cases I am concerned
about we don't really care if work is not reliably cancelled, we just
need to be able to schedule it earlier if it has already been scheduled
for execution in some point in the future.

Thanks!

-- 
Dmitry

Re: cancel_delayed_work and its use of del_timer_sync

[Oleg Nesterov]

Hi Dmitry,

(add Roland and Stefan)

On 08/23, Dmitry Torokhov wrote:
>
> I noticed that you went back and forth on using del_timer() vs.
> del_timer_sync() in cancel_delayed_work(), finally settling on using
> del_timer_sync().

Yes. del_timer() can't work when the user does something like

	cancel_delayed_work(dw);
	flush_workqueue();

	kfree(dw);

Perhaps we can add __cancel_delayed_work() which uses del_timer(), most
callers don't really need del_timer_sync. I dunno.

> I must say that the fact that cancel_delayed_work()
> might sleep

Just in case, it can spin, but not sleep

> (given that there exists cancel_delayed_work_sync) catches a
> few drivers writers by surprise. Moreover it makes delayed works
> unsuitable in cases when we do want to cancel (so we can reschedule)
> work in an interrupt context.

Yep. it is not useable from irq. But only because nobody removed
"#ifdef CONFIG_SMP" from set_running_timer(). I guess I should send a
patch finally.

> it is possible to have a reschedule_delayed_work() taht would do a
> "soft" cancel and [re]submit the work. In the use cases I am concerned
> about we don't really care if work is not reliably cancelled, we just
> need to be able to schedule it earlier if it has already been scheduled
> for execution in some point in the future.

Well. this depends on how "soft" should be that cancel.

Consider the auto-rearming delayed work, its work->func() calls
queue_delayed_work(self, BIG_DELAY). The caller of requeue_work(SMALL_DELAY)
preempts cwq->thread right after it sets _PENDING.

Now, what should requeue_work() do ? Even if the requeue_work() and
work->func() run on different CPUs, in this case requeue_ must spin.

So. It is easy to create requeue_work() which never sleeps/spins, but
it can return the error in case it hits the queueing in progress.

Is it OK?

And another question, should it cancel (without sleep/spin) this dwork
if the timer has expired, the work is pending, but its ->func() has not
started yet?

Oleg.

Re: cancel_delayed_work and its use of del_timer_sync

[Dmitry Torokhov]

On Mon, Aug 24, 2009 at 07:49:10PM +0200, Oleg Nesterov wrote:
> On 08/23, Dmitry Torokhov wrote:
> 
> > it is possible to have a reschedule_delayed_work() taht would do a
> > "soft" cancel and [re]submit the work. In the use cases I am concerned
> > about we don't really care if work is not reliably cancelled, we just
> > need to be able to schedule it earlier if it has already been scheduled
> > for execution in some point in the future.
> 
> Well. this depends on how "soft" should be that cancel.
> 
> Consider the auto-rearming delayed work, its work->func() calls
> queue_delayed_work(self, BIG_DELAY). The caller of requeue_work(SMALL_DELAY)
> preempts cwq->thread right after it sets _PENDING.
> 
> Now, what should requeue_work() do ? Even if the requeue_work() and
> work->func() run on different CPUs, in this case requeue_ must spin.
> 
> So. It is easy to create requeue_work() which never sleeps/spins, but
> it can return the error in case it hits the queueing in progress.
> 
> Is it OK?
> 
> And another question, should it cancel (without sleep/spin) this dwork
> if the timer has expired, the work is pending, but its ->func() has not
> started yet?
>

It depends... In most cases we have a delayed work scheduled with a
BIG_DELAY and then interrupt comes and we want to reschedule the delayed
work so it can be executed right away. So, in this particular case, we
don't need to cancel a work that is being scheduled at this particular
moment. On the other hand there was a driver that wanted the same
semantics for delayed works as timers... I think that if work is already
scheduled for execution we could let it go and not try to cancel it.

-- 
Dmitry

Re: cancel_delayed_work and its use of del_timer_sync

[Oleg Nesterov]

Damn, forgot to update CC, re-sending...

Hi Dmitry,

(add Roland and Stefan)

On 08/23, Dmitry Torokhov wrote:
>
> I noticed that you went back and forth on using del_timer() vs.
> del_timer_sync() in cancel_delayed_work(), finally settling on using
> del_timer_sync().

Yes. del_timer() can't work when the user does something like

	cancel_delayed_work(dw);
	flush_workqueue();

	kfree(dw);

Perhaps we can add __cancel_delayed_work() which uses del_timer(), most
callers don't really need del_timer_sync. I dunno.

> I must say that the fact that cancel_delayed_work()
> might sleep

Just in case, it can spin, but not sleep

> (given that there exists cancel_delayed_work_sync) catches a
> few drivers writers by surprise. Moreover it makes delayed works
> unsuitable in cases when we do want to cancel (so we can reschedule)
> work in an interrupt context.

Yep. it is not useable from irq. But only because nobody removed
"#ifdef CONFIG_SMP" from set_running_timer(). I guess I should send a
patch finally.

> it is possible to have a reschedule_delayed_work() taht would do a
> "soft" cancel and [re]submit the work. In the use cases I am concerned
> about we don't really care if work is not reliably cancelled, we just
> need to be able to schedule it earlier if it has already been scheduled
> for execution in some point in the future.

Well. this depends on how "soft" should be that cancel.

Consider the auto-rearming delayed work, its work->func() calls
queue_delayed_work(self, BIG_DELAY). The caller of requeue_work(SMALL_DELAY)
preempts cwq->thread right after it sets _PENDING.

Now, what should requeue_work() do ? Even if the requeue_work() and
work->func() run on different CPUs, in this case requeue_ must spin.

So. It is easy to create requeue_work() which never sleeps/spins, but
it can return the error in case it hits the queueing in progress.

Is it OK?

And another question, should it cancel (without sleep/spin) this dwork
if the timer has expired, the work is pending, but its ->func() has not
started yet?

Oleg.