Re: [RFC] [PATCH] Performance of del_timer_sync

[Andrew Morton]

Ingo Molnar <mingo@xxxxxxx> wrote:
>
> 
> * Andrew Morton <akpm@xxxxxxxx> 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,

_

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/