Thomas:
The major difference -- in fact, almost the only difference -- between
del_timer() and try_to_del_timer_sync() is that try_to_del_timer_sync
returns a special code (-1) if the timer couldn't be deleted because it
is currently running, whereas del_timer doesn't check this.
Furthermore, the "_sync" in the name suggests that
try_to_del_timer_sync will wait until a running timer has finished,
which it clearly does not do.
Despite these facts, the kerneldoc for try_to_del_timer_sync states
that it must not be called in interrupt context. Why not? Isn't that
advice simply wrong?
With this in mind, would there be any objection if I renamed it to
try_to_del_timer(), removed the comment forbidding it to be used in
interrupt context, and made it available even on non-SMP builds?
Alan Stern
P.S.: The only other difference is that del_timer calls
timer_stats_timer_clear_start_info. Why doesn't try_to_del_timer_sync
do the same thing?
On Fri, 26 Jun 2009 15:50:54 -0400 (EDT) Alan Stern <[email protected]> wrote:
> Thomas:
I'm not Thomas, but I play one on TV.
> The major difference -- in fact, almost the only difference -- between
> del_timer() and try_to_del_timer_sync() is that try_to_del_timer_sync
> returns a special code (-1) if the timer couldn't be deleted because it
> is currently running, whereas del_timer doesn't check this.
And del_timer() is heaps faster against a not-pending timer. I have a
vague memory that there are some callsites which do this quite a lot.
And try_to_del_timer_sync() forgot to do timer_stats_timer_clear_start_info().
> Furthermore, the "_sync" in the name suggests that
> try_to_del_timer_sync will wait until a running timer has finished,
> which it clearly does not do.
yup.
> Despite these facts, the kerneldoc for try_to_del_timer_sync states
> that it must not be called in interrupt context. Why not? Isn't that
> advice simply wrong?
: commit fd450b7318b75343fd76b3d95416853e34e72c95
: Author: Oleg Nesterov <[email protected]>
: AuthorDate: Thu Jun 23 00:08:59 2005 -0700
: Commit: Linus Torvalds <[email protected]>
: CommitDate: Thu Jun 23 09:45:16 2005 -0700
:
: [PATCH] timers: introduce try_to_del_timer_sync()
:
: This patch splits del_timer_sync() into 2 functions. The new one,
: try_to_del_timer_sync(), returns -1 when it hits executing timer.
:
: It can be used in interrupt context, or when the caller hold locks which
: can prevent completion of the timer's handler.
:
: NOTE. Currently it can't be used in interrupt context in UP case, because
: ->running_timer is used only with CONFIG_SMP.
:
: Should the need arise, it is possible to kill #ifdef CONFIG_SMP in
: set_running_timer(), it is cheap.
:
The changelog is somewhat vodka-fogged, but there is a bit of a problem
there.
> With this in mind, would there be any objection if I renamed it to
> try_to_del_timer(), removed the comment forbidding it to be used in
> interrupt context, and made it available even on non-SMP builds?
Sounds sane to me, if the set_running_timer() change is also made.
> Alan Stern
>
> P.S.: The only other difference is that del_timer calls
> timer_stats_timer_clear_start_info. Why doesn't try_to_del_timer_sync
> do the same thing?
This could be a day-one bug in
: commit 82f67cd9fca8c8762c15ba7ed0d5747588c1e221
: Author: Ingo Molnar <[email protected]>
: AuthorDate: Fri Feb 16 01:28:13 2007 -0800
: Commit: Linus Torvalds <[email protected]>
: CommitDate: Fri Feb 16 08:13:59 2007 -0800
:
: [PATCH] Add debugging feature /proc/timer_stat
timer-stats omits accumulation for del_timer_sync() also.
On Wed, 1 Jul 2009, Andrew Morton wrote:
> On Fri, 26 Jun 2009 15:50:54 -0400 (EDT) Alan Stern <[email protected]> wrote:
>
> > Thomas:
>
> I'm not Thomas, but I play one on TV.
>
> > The major difference -- in fact, almost the only difference -- between
> > del_timer() and try_to_del_timer_sync() is that try_to_del_timer_sync
> > returns a special code (-1) if the timer couldn't be deleted because it
> > is currently running, whereas del_timer doesn't check this.
>
> And del_timer() is heaps faster against a not-pending timer. I have a
> vague memory that there are some callsites which do this quite a lot.
>
> And try_to_del_timer_sync() forgot to do timer_stats_timer_clear_start_info().
>
> > Furthermore, the "_sync" in the name suggests that
> > try_to_del_timer_sync will wait until a running timer has finished,
> > which it clearly does not do.
>
> yup.
>
> > Despite these facts, the kerneldoc for try_to_del_timer_sync states
> > that it must not be called in interrupt context. Why not? Isn't that
> > advice simply wrong?
>
> : commit fd450b7318b75343fd76b3d95416853e34e72c95
> : Author: Oleg Nesterov <[email protected]>
> : AuthorDate: Thu Jun 23 00:08:59 2005 -0700
> : Commit: Linus Torvalds <[email protected]>
> : CommitDate: Thu Jun 23 09:45:16 2005 -0700
> :
> : [PATCH] timers: introduce try_to_del_timer_sync()
> :
> : This patch splits del_timer_sync() into 2 functions. The new one,
> : try_to_del_timer_sync(), returns -1 when it hits executing timer.
> :
> : It can be used in interrupt context, or when the caller hold locks which
> : can prevent completion of the timer's handler.
> :
> : NOTE. Currently it can't be used in interrupt context in UP case, because
> : ->running_timer is used only with CONFIG_SMP.
> :
> : Should the need arise, it is possible to kill #ifdef CONFIG_SMP in
> : set_running_timer(), it is cheap.
> :
>
> The changelog is somewhat vodka-fogged, but there is a bit of a problem
> there.
Okay, thanks. That makes sense.
> > With this in mind, would there be any objection if I renamed it to
> > try_to_del_timer(), removed the comment forbidding it to be used in
> > interrupt context, and made it available even on non-SMP builds?
>
> Sounds sane to me, if the set_running_timer() change is also made.
It turns out I probably don't need the enhanced functionality after
all. So never mind for now...
Alan Stern
On 07/02, Alan Stern wrote:
> On Wed, 1 Jul 2009, Andrew Morton wrote:
>
> > On Fri, 26 Jun 2009 15:50:54 -0400 (EDT) Alan Stern <[email protected]> wrote:
> >
> > > Thomas:
> >
> > I'm not Thomas, but I play one on TV.
> >
> > > The major difference -- in fact, almost the only difference -- between
> > > del_timer() and try_to_del_timer_sync() is that try_to_del_timer_sync
> > > returns a special code (-1) if the timer couldn't be deleted because it
> > > is currently running, whereas del_timer doesn't check this.
> >
> > And del_timer() is heaps faster against a not-pending timer. I have a
> > vague memory that there are some callsites which do this quite a lot.
> >
> > And try_to_del_timer_sync() forgot to do timer_stats_timer_clear_start_info().
> >
> > > Furthermore, the "_sync" in the name suggests that
> > > try_to_del_timer_sync will wait until a running timer has finished,
> > > which it clearly does not do.
> >
> > yup.
Yes, try_to_del_timer_sync() never waits exactly because it fails if the
timer is running.
> > > Despite these facts, the kerneldoc for try_to_del_timer_sync states
> > > that it must not be called in interrupt context. Why not? Isn't that
> > > advice simply wrong?
> >
> > : commit fd450b7318b75343fd76b3d95416853e34e72c95
> > : Author: Oleg Nesterov <[email protected]>
> > : AuthorDate: Thu Jun 23 00:08:59 2005 -0700
> > : Commit: Linus Torvalds <[email protected]>
> > : CommitDate: Thu Jun 23 09:45:16 2005 -0700
> > :
> > : [PATCH] timers: introduce try_to_del_timer_sync()
> > :
> > : This patch splits del_timer_sync() into 2 functions. The new one,
> > : try_to_del_timer_sync(), returns -1 when it hits executing timer.
> > :
> > : It can be used in interrupt context, or when the caller hold locks which
> > : can prevent completion of the timer's handler.
> > :
> > : NOTE. Currently it can't be used in interrupt context in UP case, because
> > : ->running_timer is used only with CONFIG_SMP.
> > :
> > : Should the need arise, it is possible to kill #ifdef CONFIG_SMP in
> > : set_running_timer(), it is cheap.
> > :
> >
> > The changelog is somewhat vodka-fogged, but there is a bit of a problem
> > there.
Yeah. try_to_del_timer_sync() should not be used in interrupt context
because in UP case it is equal to del_timer(), this is not what we want.
But with CONFIG_SMP it can work from any context.
> Okay, thanks. That makes sense.
>
> > > With this in mind, would there be any objection if I renamed it to
> > > try_to_del_timer(),
Not sure I understand why try_to_del_timer is better...
try_to_del_timer_sync() means: try to del_timer_sync(), that is why
"_sync" ;)
But I don't really care.
> removed the comment forbidding it to be used in
> > > interrupt context, and made it available even on non-SMP builds?
> >
> > Sounds sane to me, if the set_running_timer() change is also made.
Yes, set_running_timer() should be changed, and
# define try_to_del_timer_sync(t) del_timer(t)
in timer.h should be killed. I think this makes sense.
Oleg.