[RFC] [PATCH] Performance of del_timer_sync

[RFC] [PATCH] Performance of del_timer_sync

[Geoff Gustafson]

I started this patch based on profiling an enterprise database application
on a 32p IA64 NUMA machine, where del_timer_sync was one of the top few
functions taking CPU time in the kernel. The problem: even though a timer
can only be scheduled on one CPU at a time, a loop was searching through
each CPU. This is particularly bad on a NUMA machine, but doesn't scale
well on a normal SMP either. With this patch, here were some results:

Before:             32p     4p
     Warm cache   29,000    505
     Cold cache   37,800   1220

After:              32p     4p
     Warm cache       95     88
     Cold cache    1,800    140

[Measurements are CPU cycles spent in a call to del_timer_sync, the average
of 1000 calls. 32p is 16-node NUMA, 4p is SMP.]

I'd be very interested for others to try out this patch, and see what kind
of results you get. Have you also observed del_timer_sync as a hotspot?

Now on to the patch. The basic idea is to remember which 'base' (i.e. CPU)
is currently executing the timer function. Ingo once sent a patch that did
this exact thing, to solve SMP races:
http://marc.theaimsgroup.com/?l=linux-kernel&m=106610555708068&w=2

Linus pointed out that you can't reset timer->running to NULL in
__run_timers because the timer might have been freed by the timer function.
The patch was then dropped. It now seems to me though that we can still get
the performance gains - I call it timer->last_running and it just does not
get reset to NULL. Thus it may be stale, but at least then you only have to
check _one_ CPU for the running timer instead of all of them.

Here are some known issues I'd like people to comment on:

1. How bad is the performance hit of adding a last_running field to
timer_list? I ifdef'd everything with CONFIG_SMP to make sure there's no
impact to uniprocessors, although that may be objectionable for its ugliness.

2. I believe there's a theoretical race; I'm not sure if it could actually
happen in practice. When the timer function gets executed in __run_timers,
the base->lock is briefly unlocked. This gives another CPU that has been
waiting in __mod_timer the opportunity to step in and reschedule the timer.
Then that new scheduled timer has to actually expire (next timer interrupt
at the earliest I believe). Then it has to get around to executing the timer
function. So now two timer functions are overlapping, and timer->last_running
only remembers the second one.

In this situation, the original del_timer_sync code would find the timer
function on the lowest numbered CPU, wait for it to finish, and break out of
the loop. If the function on the higher numbered CPU runs longer, it will
return before it is finished.

With the patch, the code will wait for the most recently executed timer
function to finish. If the older function finishes after the newer one, we
get the original problem. I'd suggest waiting for the most recent one is an
improvement, though not a fix.

If del_timer_sync returns while the timer function is still running, the
caller may do something (like free the timer) that will cause the running
timer function to go awry. Can this really happen? Would it be the timer
function's fault, for running too long?

Comments, results, bug fixes, and improvements welcome!

- Geoff Gustafson

diff -Naur fresh/include/linux/timer.h timer/include/linux/timer.h
--- fresh/include/linux/timer.h	Sat Apr  3 19:37:37 2004
+++ timer/include/linux/timer.h	Tue May  4 14:51:51 2004
@@ -18,10 +18,20 @@
  	unsigned long data;

  	struct tvec_t_base_s *base;
+
+#ifdef CONFIG_SMP
+	struct tvec_t_base_s *last_running;
+#endif
  };

  #define TIMER_MAGIC	0x4b87ad6e

+#ifdef CONFIG_SMP
+#define TIMER_INIT_LASTRUNNING .last_running = NULL,
+#else
+#define TIMER_INIT_LASTRUNNING
+#endif
+
  #define TIMER_INITIALIZER(_function, _expires, _data) {		\
  		.function = (_function),			\
  		.expires = (_expires),				\
@@ -29,6 +39,7 @@
  		.base = NULL,					\
  		.magic = TIMER_MAGIC,				\
  		.lock = SPIN_LOCK_UNLOCKED,			\
+		TIMER_INIT_LASTRUNNING				\
  	}

  /***
@@ -40,6 +51,9 @@
   */
  static inline void init_timer(struct timer_list * timer)
  {
+#ifdef CONFIG_SMP
+	timer->last_running = NULL;
+#endif
  	timer->base = NULL;
  	timer->magic = TIMER_MAGIC;
  	spin_lock_init(&timer->lock);
diff -Naur fresh/kernel/timer.c timer/kernel/timer.c
--- fresh/kernel/timer.c	Sat Apr  3 19:37:59 2004
+++ timer/kernel/timer.c	Tue May  4 11:37:36 2004
@@ -75,6 +75,14 @@
  #endif
  }

+static inline void set_last_running(struct timer_list *timer,
+				    tvec_base_t *base)
+{
+#ifdef CONFIG_SMP
+	timer->last_running = base;
+#endif
+}
+
  /* Fake initialization */
  static DEFINE_PER_CPU(tvec_base_t, tvec_bases) = { SPIN_LOCK_UNLOCKED };

@@ -325,25 +333,22 @@
  int del_timer_sync(struct timer_list *timer)
  {
  	tvec_base_t *base;
-	int i, ret = 0;
+	int ret = 0;

  	check_timer(timer);

  del_again:
  	ret += del_timer(timer);

-	for_each_cpu(i) {
-		base = &per_cpu(tvec_bases, i);
-		if (base->running_timer == timer) {
-			while (base->running_timer == timer) {
-				cpu_relax();
-				preempt_check_resched();
-			}
-			break;
+	base = timer->last_running;
+	if (base) {
+		while (base->running_timer == timer) {
+			cpu_relax();
+			preempt_check_resched();
  		}
  	}
  	smp_rmb();
-	if (timer_pending(timer))
+	if (timer_pending(timer) || timer->last_running != base)
  		goto del_again;

  	return ret;
@@ -418,6 +423,7 @@
  			set_running_timer(base, timer);
  			smp_wmb();
  			timer->base = NULL;
+			set_last_running(timer, base);
  			spin_unlock_irq(&base->lock);
  			fn(data);
  			spin_lock_irq(&base->lock);

Re: [RFC] [PATCH] Performance of del_timer_sync

[Andrew Morton]

Geoff Gustafson <geoff@linux.jf.intel.com> wrote:
>
> I started this patch based on profiling an enterprise database application
>  on a 32p IA64 NUMA machine, where del_timer_sync was one of the top few
>  functions taking CPU time in the kernel.

Do you know where it's being called from?

Re: [RFC] [PATCH] Performance of del_timer_sync

[Geoff Gustafson]

Andrew Morton wrote:
> Geoff Gustafson <geoff@linux.jf.intel.com> wrote:
>> 
>> I started this patch based on profiling an enterprise database
>>  application on a 32p IA64 NUMA machine, where del_timer_sync was
>>  one of the top few functions taking CPU time in the kernel.
> 
> Do you know where it's being called from?

OK, the main sources were:

sys_semtimedop() -> schedule_timeout()

sys_io_getevents() -> read_events() -> clear_timeout()

- Geoff

Re: [RFC] [PATCH] Performance of del_timer_sync

[Andrew Morton]

"Geoff Gustafson" <geoff@linux.jf.intel.com> wrote:
>
> Andrew Morton wrote:
> > Geoff Gustafson <geoff@linux.jf.intel.com> wrote:
> >> 
> >> I started this patch based on profiling an enterprise database
> >>  application on a 32p IA64 NUMA machine, where del_timer_sync was
> >>  one of the top few functions taking CPU time in the kernel.
> > 
> > Do you know where it's being called from?
> 
> OK, the main sources were:
> 
> sys_semtimedop() -> schedule_timeout()
> 
> sys_io_getevents() -> read_events() -> clear_timeout()
> 

OK, thanks.  schedule_timeout()!  Ow.

Ingo, why is this not sufficient?


diff -puN kernel/timer.c~a kernel/timer.c
--- 25/kernel/timer.c~a	2004-05-11 12:10:28.695557600 -0700
+++ 25-akpm/kernel/timer.c	2004-05-11 12:10:42.820410296 -0700
@@ -331,6 +331,8 @@ int del_timer_sync(struct timer_list *ti
 
 del_again:
 	ret += del_timer(timer);
+	if (!ret)
+		return 0;
 
 	for_each_cpu(i) {
 		base = &per_cpu(tvec_bases, i);

_

RE: [RFC] [PATCH] Performance of del_timer_sync

[Chen, Kenneth W]

>>>> Andrew Morton wrote on Tuesday, May 11, 2004 12:11 PM
> > > Geoff Gustafson <geoff@linux.jf.intel.com> wrote:
> > >>
> > >> I started this patch based on profiling an enterprise database
> > >>  application on a 32p IA64 NUMA machine, where del_timer_sync was
> > >>  one of the top few functions taking CPU time in the kernel.
> > >
> > > Do you know where it's being called from?
> >
> > OK, the main sources were:
> >
> > sys_semtimedop() -> schedule_timeout()
> >
> > sys_io_getevents() -> read_events() -> clear_timeout()
> >
>
> OK, thanks.  schedule_timeout()!  Ow.
>
> Ingo, why is this not sufficient?
>
>
> diff -puN kernel/timer.c~a kernel/timer.c
> --- 25/kernel/timer.c~a	2004-05-11 12:10:28.695557600 -0700
> +++ 25-akpm/kernel/timer.c	2004-05-11 12:10:42.820410296 -0700
> @@ -331,6 +331,8 @@ int del_timer_sync(struct timer_list *ti
>
>  del_again:
>  	ret += del_timer(timer);
> +	if (!ret)
> +		return 0;
>
>  	for_each_cpu(i) {
>  		base = &per_cpu(tvec_bases, i);


Hehe ... , first thing we tried :-). But has problem that if timeout
function is running while del_timer_sync() is called, it returns
without waiting.

- Ken

Re: [RFC] [PATCH] Performance of del_timer_sync

[Andrew Morton]

"Chen, Kenneth W" <kenneth.w.chen@intel.com> wrote:
>
> >  del_again:
>  >  	ret += del_timer(timer);
>  > +	if (!ret)
>  > +		return 0;
>  >
>  >  	for_each_cpu(i) {
>  >  		base = &per_cpu(tvec_bases, i);
> 
> 
>  Hehe ... , first thing we tried :-). But has problem that if timeout
>  function is running while del_timer_sync() is called, it returns
>  without waiting.

eh?  If del_timer() returns non-zero, that means that the timer was
successfully deleted before it triggered.

Re: [RFC] [PATCH] Performance of del_timer_sync

[Andrew Morton]

Andrew Morton <akpm@osdl.org> wrote:
>
> "Chen, Kenneth W" <kenneth.w.chen@intel.com> wrote:
> >
> > >  del_again:
> >  >  	ret += del_timer(timer);
> >  > +	if (!ret)
> >  > +		return 0;
> >  >
> >  >  	for_each_cpu(i) {
> >  >  		base = &per_cpu(tvec_bases, i);
> > 
> > 
> >  Hehe ... , first thing we tried :-). But has problem that if timeout
> >  function is running while del_timer_sync() is called, it returns
> >  without waiting.
> 
> eh?  If del_timer() returns non-zero, that means that the timer was
> successfully deleted before it triggered.

s/non-zero/zero/

Re: [RFC] [PATCH] Performance of del_timer_sync

[Ingo Molnar]

* Andrew Morton <akpm@osdl.org> wrote:

> Ingo, why is this not sufficient?
> 
> @@ -331,6 +331,8 @@ int del_timer_sync(struct timer_list *ti
>  
>  del_again:
>  	ret += del_timer(timer);
> +	if (!ret)
> +		return 0;
>  
>  	for_each_cpu(i) {
>  		base = &per_cpu(tvec_bases, i);

it's not sufficient because a timer might be running on another CPU even
if it has not been deleted. We delete a timer prior running it (so that
the timer fn can re-activate the timer). So del_timer_sync() has to
synchronize independently of whether the timer was removed or not.

	Ingo

Re: [RFC] [PATCH] Performance of del_timer_sync

[Andrew Morton]

Ingo Molnar <mingo@elte.hu> wrote:
>
> 
> * Andrew Morton <akpm@osdl.org> wrote:
> 
> > Ingo, why is this not sufficient?
> > 
> > @@ -331,6 +331,8 @@ int del_timer_sync(struct timer_list *ti
> >  
> >  del_again:
> >  	ret += del_timer(timer);
> > +	if (!ret)
> > +		return 0;
> >  
> >  	for_each_cpu(i) {
> >  		base = &per_cpu(tvec_bases, i);
> 
> it's not sufficient because a timer might be running on another CPU even
> if it has not been deleted. We delete a timer prior running it (so that
> the timer fn can re-activate the timer). So del_timer_sync() has to
> synchronize independently of whether the timer was removed or not.
> 

Ah, OK, the timer handler may re-add itself.  Really, that's a bug in the
caller: once they've decided to shoot down the timer the caller should have
made state changes which prevent the handler from re-adding the timer.

Still, too late to change that.

Neither AIO nor schedule_timeout() actually re-add the timer so they don't
need the full treatment, yes?


 25-akpm/fs/aio.c              |    2 +-
 25-akpm/include/linux/timer.h |    2 ++
 25-akpm/kernel/timer.c        |    9 +++++++--
 3 files changed, 10 insertions(+), 3 deletions(-)

diff -puN kernel/timer.c~del-timer-speedup kernel/timer.c
--- 25/kernel/timer.c~del-timer-speedup	2004-05-11 13:05:41.997859088 -0700
+++ 25-akpm/kernel/timer.c	2004-05-11 13:07:32.493061264 -0700
@@ -348,8 +348,13 @@ del_again:
 
 	return ret;
 }
-
 EXPORT_SYMBOL(del_timer_sync);
+
+int del_single_shot_timer(struct timer_struct *timer)
+{
+	if (del_timer(timer))
+		del_timer_sync(timer);
+}
 #endif
 
 static int cascade(tvec_base_t *base, tvec_t *tv, int index)
@@ -1109,7 +1114,7 @@ fastcall signed long __sched schedule_ti
 
 	add_timer(&timer);
 	schedule();
-	del_timer_sync(&timer);
+	del_single_shot_timer(&timer);
 
 	timeout = expire - jiffies;
 
diff -puN include/linux/timer.h~del-timer-speedup include/linux/timer.h
--- 25/include/linux/timer.h~del-timer-speedup	2004-05-11 13:05:42.012856808 -0700
+++ 25-akpm/include/linux/timer.h	2004-05-11 13:07:01.905711248 -0700
@@ -88,8 +88,10 @@ static inline void add_timer(struct time
 
 #ifdef CONFIG_SMP
   extern int del_timer_sync(struct timer_list * timer);
+  extern int del_single_shot_timer(struct timer_struct *timer);
 #else
 # define del_timer_sync(t) del_timer(t)
+# define del_single_shot_timer(t) del_timer(t)
 #endif
 
 extern void init_timers(void);
diff -puN fs/aio.c~del-timer-speedup fs/aio.c
--- 25/fs/aio.c~del-timer-speedup	2004-05-11 13:05:42.028854376 -0700
+++ 25-akpm/fs/aio.c	2004-05-11 13:07:14.591782672 -0700
@@ -788,7 +788,7 @@ static inline void set_timeout(long star
 
 static inline void clear_timeout(struct timeout *to)
 {
-	del_timer_sync(&to->timer);
+	del_single_shot_timer(&to->timer);
 }
 
 static int read_events(struct kioctx *ctx,

_

Re: [RFC] [PATCH] Performance of del_timer_sync

[Christoph Hellwig]

On Tue, May 11, 2004 at 01:11:37PM -0700, Andrew Morton wrote:
> +int del_single_shot_timer(struct timer_struct *timer)
> +{
> +	if (del_timer(timer))
> +		del_timer_sync(timer);
> +}

it's probably better named del_timer_singleshot given the name we gave
to the other timer functions.

Re: [RFC] [PATCH] Performance of del_timer_sync

[Andrew Morton]

Christoph Hellwig <hch@infradead.org> wrote:
>
> On Tue, May 11, 2004 at 01:11:37PM -0700, Andrew Morton wrote:
> > +int del_single_shot_timer(struct timer_struct *timer)
> > +{
> > +	if (del_timer(timer))
> > +		del_timer_sync(timer);
> > +}
> 
> it's probably better named del_timer_singleshot given the name we gave
> to the other timer functions.

Nah, that's ungrammatical.  del_timer_singleshot means "delete a timer in a
single-shot manner".

We have:

"add a timer"
"modify a timer"
"delete a timer"
"delete a timer synchronously"
"delete a single-shot timer"

Re: [RFC] [PATCH] Performance of del_timer_sync

[Ingo Molnar]

* Andrew Morton <akpm@osdl.org> wrote:

> Ah, OK, the timer handler may re-add itself.  Really, that's a bug in
> the caller: once they've decided to shoot down the timer the caller
> should have made state changes which prevent the handler from
> re-adding the timer.
> 
> Still, too late to change that.

yeah.

> Neither AIO nor schedule_timeout() actually re-add the timer so they
> don't need the full treatment, yes?

correct.

> +int del_single_shot_timer(struct timer_struct *timer)
> +{
> +	if (del_timer(timer))
> +		del_timer_sync(timer);
> +}

cool, this looks good to me. It's obviously correct and has a limited
scope. (I'd suggest another name though: del_timer_singleshot(). This i
think fits into the existing naming better: del_timer() and
del_timer_sync(). But no strong feelings either way.)

	Ingo

RE: [RFC] [PATCH] Performance of del_timer_sync

[Chen, Kenneth W]

>>>>> Andrew Morton wrote on Tuesday, May 11, 2004 1:12 PM
> > > Ingo, why is this not sufficient?
> >
> > it's not sufficient because a timer might be running on another CPU even
> > if it has not been deleted. We delete a timer prior running it (so that
> > the timer fn can re-activate the timer). So del_timer_sync() has to
> > synchronize independently of whether the timer was removed or not.
> >
>
> Ah, OK, the timer handler may re-add itself.  Really, that's a bug in the
> caller: once they've decided to shoot down the timer the caller should have
> made state changes which prevent the handler from re-adding the timer.
>
> Still, too late to change that.
>
> Neither AIO nor schedule_timeout() actually re-add the timer so they don't
> need the full treatment, yes?
>
>
> diff -puN kernel/timer.c~del-timer-speedup kernel/timer.c
> --- 25/kernel/timer.c~del-timer-speedup	2004-05-11 13:05:41.997859088 -0700
> +++ 25-akpm/kernel/timer.c	2004-05-11 13:07:32.493061264 -0700
> @@ -348,8 +348,13 @@ del_again:
>
>  	return ret;
>  }
> -
>  EXPORT_SYMBOL(del_timer_sync);
> +
> +int del_single_shot_timer(struct timer_struct *timer)
> +{
> +	if (del_timer(timer))
> +		del_timer_sync(timer);
> +}
>  #endif

I'm confused, isn't the polarity of del_timer() need to be reversed?
Also propagate the return value of del_timer_sync()?

- Ken

Re: [RFC] [PATCH] Performance of del_timer_sync

[Andrew Morton]

"Chen, Kenneth W" <kenneth.w.chen@intel.com> wrote:
>
> > +
>  > +int del_single_shot_timer(struct timer_struct *timer)
>  > +{
>  > +	if (del_timer(timer))
>  > +		del_timer_sync(timer);
>  > +}
>  >  #endif
> 
>  I'm confused, isn't the polarity of del_timer() need to be reversed?

Hey, I didn't compile it, let alone test it!

>  Also propagate the return value of del_timer_sync()?

yup.

If it looks OK, please fix it up, kerneldocify the function and prepare a
real patch?

Re: [RFC] [PATCH] Performance of del_timer_sync

[Ingo Molnar]

* Andrew Morton <akpm@osdl.org> wrote:

> Nah, that's ungrammatical.  del_timer_singleshot means "delete a timer
> in a single-shot manner".
> 
> We have:
> 
> "add a timer"
> "modify a timer"
> "delete a timer"
> "delete a timer synchronously"
> "delete a single-shot timer"

hm, indeed. Miraculously, the existing timer API names are correct
grammatically, so we might as well go for del_single_shot_timer() ...

	Ingo

Re: [RFC] [PATCH] Performance of del_timer_sync

[Ingo Molnar]

* Chen, Kenneth W <kenneth.w.chen@intel.com> wrote:

> > +int del_single_shot_timer(struct timer_struct *timer)
> > +{
> > +	if (del_timer(timer))
> > +		del_timer_sync(timer);
> > +}
> >  #endif
> 
> I'm confused, isn't the polarity of del_timer() need to be reversed?
> Also propagate the return value of del_timer_sync()?

indeed. If the removal didnt succeed then we must make sure there's no
timer fn pending. Btw., in that case del_timer_sync() must not succeed -
it would mean the timer fn re-added the timer, which by definition must
not happen here. So i'd go for:

int del_single_shot_timer(struct timer_struct *timer)
{
	int ret = del_timer(timer);

	if (!ret) {
		ret = del_timer_sync(timer);
		BUG_ON(ret);
	}

	return ret;
}

this should catch illegal uses of del_single_shot_timer().

	Ingo

RE: [RFC] [PATCH] Performance of del_timer_sync

[Chen, Kenneth W]

>>>>> Andrew Morton wrote on Tuesday, May 11, 2004 1:31 PM
> > > +int del_single_shot_timer(struct timer_struct *timer)
> > > +{
> > > +	if (del_timer(timer))
> > > +		del_timer_sync(timer);
> > > +}
> > >  #endif
> >
> > I'm confused, isn't the polarity of del_timer() need to be reversed?
>
> Hey, I didn't compile it, let alone test it!
>
> > Also propagate the return value of del_timer_sync()?
>
> yup.
>
> If it looks OK, please fix it up, kerneldocify the function and prepare
> a real patch?

Looks wonderful, much better than what we had before.  We will consolidate
the comments, run through our test setup and re-post.  Thanks!

- Ken

Re: [RFC] [PATCH] Performance of del_timer_sync

[Andrew Morton]

Ingo Molnar <mingo@elte.hu> wrote:
>
> 
> * Andrew Morton <akpm@osdl.org> wrote:
> 
> > Nah, that's ungrammatical.  del_timer_singleshot means "delete a timer
> > in a single-shot manner".
> > 
> > We have:
> > 
> > "add a timer"
> > "modify a timer"
> > "delete a timer"
> > "delete a timer synchronously"
> > "delete a single-shot timer"
> 
> hm, indeed. Miraculously, the existing timer API names are correct
> grammatically, so we might as well go for del_single_shot_timer() ...
> 

<anal>del_singleshot_timer_sync</anal>

I vote we leave it up to Ken.  But please, not del_timer_kenneth().

Re: [RFC] [PATCH] Performance of del_timer_sync

[Ingo Molnar]

* Andrew Morton <akpm@osdl.org> wrote:

> > > Nah, that's ungrammatical.  del_timer_singleshot means "delete a timer
> > > in a single-shot manner".
> > > 
> > > We have:
> > > 
> > > "add a timer"
> > > "modify a timer"
> > > "delete a timer"
> > > "delete a timer synchronously"
> > > "delete a single-shot timer"
> > 
> > hm, indeed. Miraculously, the existing timer API names are correct
> > grammatically, so we might as well go for del_single_shot_timer() ...
> > 
> 
> <anal>del_singleshot_timer_sync</anal>
> 
> I vote we leave it up to Ken.  But please, not del_timer_kenneth().

yeah. Ken's got a license to name ;)

	Ingo

RE: [RFC] [PATCH] Performance of del_timer_sync

[Chen, Kenneth W]

>>>> Ingo Molnar wrote on Tuesday, May 11, 2004 1:57 PM
> * Andrew Morton <akpm@osdl.org> wrote:
>
> > > > Nah, that's ungrammatical.  del_timer_singleshot means "delete a timer
> > > > in a single-shot manner".
> > > >
> > > > We have:
> > > >
> > > > "add a timer"
> > > > "modify a timer"
> > > > "delete a timer"
> > > > "delete a timer synchronously"
> > > > "delete a single-shot timer"
> > >
> > > hm, indeed. Miraculously, the existing timer API names are correct
> > > grammatically, so we might as well go for del_single_shot_timer() ...
> > >
> >
> > <anal>del_singleshot_timer_sync</anal>
> >
> > I vote we leave it up to Ken.  But please, not del_timer_kenneth().
>
> yeah. Ken's got a license to name ;)

Cool, feeling pretty good here. I'm going to stick with andrew's original
name, and add a big fat comments for the new function ;-)

- Ken

Re: [RFC] [PATCH] Performance of del_timer_sync

[Geoff Gustafson]

Andrew Morton wrote:
 > <anal>del_singleshot_timer_sync</anal>
 >
 > I vote we leave it up to Ken.  But please, not del_timer_kenneth().

Despite your markup language, I thought keeping the 'sync' sounded like
a good idea, since that's the function it's replacing.

Here's a patch. I've done a small amount of testing with a 4p machine.
I've at least seen that the average call to singleshot is down around 90
cycles, pretty similar/expected. We'll test on the original platform and
post results.

- Geoff

* I am in a transitional period w/ email clients and can't seem to use
any of them properly. Sorry for the dups, etc.

diff -Naur fresh/fs/aio.c timer/fs/aio.c
--- fresh/fs/aio.c	Sun May  9 19:32:29 2004
+++ timer/fs/aio.c	Tue May 11 15:14:47 2004
@@ -793,7 +793,7 @@

  static inline void clear_timeout(struct timeout *to)
  {
-	del_timer_sync(&to->timer);
+	del_singleshot_timer_sync(&to->timer);
  }

  static int read_events(struct kioctx *ctx,
diff -Naur fresh/include/linux/timer.h timer/include/linux/timer.h
--- fresh/include/linux/timer.h	Sun May  9 19:32:54 2004
+++ timer/include/linux/timer.h	Tue May 11 14:21:36 2004
@@ -88,8 +88,10 @@

  #ifdef CONFIG_SMP
    extern int del_timer_sync(struct timer_list * timer);
+  extern int del_singleshot_timer_sync(struct timer_list * timer);
  #else
  # define del_timer_sync(t) del_timer(t)
+# define del_singleshot_timer_sync(t) del_timer(t)
  #endif

  extern void init_timers(void);
diff -Naur fresh/kernel/timer.c timer/kernel/timer.c
--- fresh/kernel/timer.c	Sun May  9 19:33:13 2004
+++ timer/kernel/timer.c	Tue May 11 15:08:24 2004
@@ -350,6 +350,35 @@
  }

  EXPORT_SYMBOL(del_timer_sync);
+
+/***
+ * del_singleshot_timer_sync - deactivate a non-recursive timer
+ * @timer: the timer to be deactivated
+ *
+ * This function is an optimization of del_timer_sync for the case where the
+ * caller can guarantee the timer does not reschedule itself in its timer
+ * function.
+ *
+ * Synchronization rules: callers must prevent restarting of the timer,
+ * otherwise this function is meaningless. It must not be called from
+ * interrupt contexts. Upon exit the timer is not queued and the handler
+ * is not running on any CPU.
+ *
+ * The function returns whether it has deactivated a pending timer or not.
+ */
+int del_singleshot_timer_sync(struct timer_list *timer)
+{
+	int ret = del_timer(timer);
+
+	if (!ret) {
+		ret = del_timer_sync(timer);
+		BUG_ON(ret);
+	}
+
+	return ret;
+}
+
+EXPORT_SYMBOL(del_singleshot_timer_sync);
  #endif

  static int cascade(tvec_base_t *base, tvec_t *tv, int index)
@@ -1109,7 +1138,7 @@

  	add_timer(&timer);
  	schedule();
-	del_timer_sync(&timer);
+	del_singleshot_timer_sync(&timer);

  	timeout = expire - jiffies;

Re: [RFC] [PATCH] Performance of del_timer_sync

[Andrew Morton]

Geoff Gustafson <geoff@linux.jf.intel.com> wrote:
>
> Andrew Morton wrote:
>  > <anal>del_singleshot_timer_sync</anal>
>  >
>  > I vote we leave it up to Ken.  But please, not del_timer_kenneth().
> 
> Despite your markup language, I thought keeping the 'sync' sounded like
> a good idea, since that's the function it's replacing.
> 
> Here's a patch. I've done a small amount of testing with a 4p machine.
> I've at least seen that the average call to singleshot is down around 90
> cycles, pretty similar/expected.

Looks good.  Your email client performs bizarre whitespace transformations, so I had to
fix the patch up by hand.

Here's an incremental patch:



diff -puN include/linux/timer.h~del_singleshot_timer_sync-tweaks include/linux/timer.h
--- 25/include/linux/timer.h~del_singleshot_timer_sync-tweaks	Tue May 11 15:59:57 2004
+++ 25-akpm/include/linux/timer.h	Tue May 11 15:59:57 2004
@@ -87,8 +87,8 @@ static inline void add_timer(struct time
 }
 
 #ifdef CONFIG_SMP
-  extern int del_timer_sync(struct timer_list * timer);
-  extern int del_singleshot_timer_sync(struct timer_list * timer);
+  extern int del_timer_sync(struct timer_list *timer);
+  extern int del_singleshot_timer_sync(struct timer_list *timer);
 #else
 # define del_timer_sync(t) del_timer(t)
 # define del_singleshot_timer_sync(t) del_timer(t)
diff -puN kernel/timer.c~del_singleshot_timer_sync-tweaks kernel/timer.c
--- 25/kernel/timer.c~del_singleshot_timer_sync-tweaks	Tue May 11 15:59:57 2004
+++ 25-akpm/kernel/timer.c	Tue May 11 15:59:57 2004
@@ -317,10 +317,16 @@ EXPORT_SYMBOL(del_timer);
  *
  * Synchronization rules: callers must prevent restarting of the timer,
  * otherwise this function is meaningless. It must not be called from
- * interrupt contexts. Upon exit the timer is not queued and the handler
- * is not running on any CPU.
+ * interrupt contexts. The caller must not hold locks which would prevent
+ * completion of the timer's handler.  Upon exit the timer is not queued and
+ * the handler is not running on any CPU.
  *
  * The function returns whether it has deactivated a pending timer or not.
+ *
+ * del_timer_sync() is slow and complicated because it copes with timer
+ * handlers which re-arm the timer (periodic timers).  If the timer handler
+ * is known to not do this (a single shot timer) then use
+ * del_singleshot_timer_sync() instead.
  */
 int del_timer_sync(struct timer_list *timer)
 {
@@ -348,7 +354,6 @@ del_again:
 
 	return ret;
 }
-
 EXPORT_SYMBOL(del_timer_sync);
 
 /***
@@ -361,8 +366,9 @@ EXPORT_SYMBOL(del_timer_sync);
  *
  * Synchronization rules: callers must prevent restarting of the timer,
  * otherwise this function is meaningless. It must not be called from
- * interrupt contexts. Upon exit the timer is not queued and the handler
- * is not running on any CPU.
+ * interrupt contexts. The caller must not hold locks which would prevent
+ * completion of the timer's handler.  Upon exit the timer is not queued and
+ * the handler is not running on any CPU.
  *
  * The function returns whether it has deactivated a pending timer or not.
  */
@@ -377,7 +383,6 @@ int del_singleshot_timer_sync(struct tim
 
 	return ret;
 }
-
 EXPORT_SYMBOL(del_singleshot_timer_sync);
 #endif
 

_