[RFC] [PATCH 0/1] timers: add_timer should never be called if pending

[RFC] [PATCH 0/1] timers: add_timer should never be called if pending

[Krishna Kumar]

From: Krishna Kumar <krkumar2@in.ibm.com>

(Hopefully this is not something that has been suggested earlier and rejected).

add_timer should never be called on a pending timer - such bugs should be
caught by reports generated by debug kernels. It is the responsibility of the
subsystem users to ensure that timers are not added twice. Using the
add_timer_on interface reduces the code that checks if the timer is pending,
the timer_base change, updating 'expires' two times, etc.

1. Single CPU, Single Timer: Add the same timer serially on one CPU - do this
   many times.
	Laptop (2 way Xeon): Saves 10.8%
		ORG: Time: 35359
		NEW: Time: 31527
	Server (4 way x86-64): Saves 7.9%
		ORG: Time: 4520
		NEW: Time: 4164
2. Single CPU, Multiple Timer: Add different timers serially on one CPU - do
   this many times.
	Laptop (2 way Xeon): Saves 7.7%
		ORG: Time: 133728
		NEW: Time: 144822
	Server (4 way x86-64): Saves 15.7%
		ORG: Time: 69012
		NEW: Time: 58186
3. Many CPU's, Single Timer: Add the same timer in parallel on all CPUs - do
   this many times.
	Laptop (2 way Xeon): Saves 14%
		ORG: Time: 69845
		NEW: Time: 60067
	Server (4 way x86-64): Saves 21.8%
		ORG: Time: 18047
		NEW: Time: 14116
4. Many CPU's, Multiple Timer: Add different timers in parallel on all CPUs -
   do this many times.
	Laptop (2 way Xeon): Saves 47.1%
		ORG: Time: 292173
		NEW: Time: 154485
	Server (4 way x86-64): Saves 8.23
		ORG: Time: 319129
		NEW: Time: 292842

[RFC] [PATCH 1/1]: timers: Change add_timer to use add_timer_on

[Krishna Kumar]

From: Krishna Kumar <krkumar2@in.ibm.com>

timer: Change add_timer to not check for existing timer.

Signed-off-by: Krishna Kumar <krkumar2@in.ibm.com>
---

 include/linux/timer.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -ruNp 2.6.27-rc7-org/include/linux/timer.h 2.6.27-rc7-new/include/linux/timer.h
--- 2.6.27-rc7-org/include/linux/timer.h	2008-09-17 17:40:36.000000000 +0530
+++ 2.6.27-rc7-new/include/linux/timer.h	2008-09-17 17:41:50.000000000 +0530
@@ -4,6 +4,7 @@
 #include <linux/list.h>
 #include <linux/ktime.h>
 #include <linux/stddef.h>
+#include <linux/smp.h>
 #include <linux/debugobjects.h>
 
 struct tvec_base;
@@ -162,8 +163,7 @@ static inline void timer_stats_timer_cle
  */
 static inline void add_timer(struct timer_list *timer)
 {
-	BUG_ON(timer_pending(timer));
-	__mod_timer(timer, timer->expires);
+	add_timer_on(timer, raw_smp_processor_id());
 }
 
 #ifdef CONFIG_SMP

Re: [RFC] [PATCH 1/1]: timers: Change add_timer to use add_timer_on

[Oleg Nesterov]

(add Thomas)

On 09/22, Krishna Kumar wrote:
>
> timer: Change add_timer to not check for existing timer.

Perhaps the changelog should explain why ;)

But, besides the removing BUG_ON(), the patch does something else.

>  static inline void add_timer(struct timer_list *timer)
>  {
> -	BUG_ON(timer_pending(timer));
> -	__mod_timer(timer, timer->expires);
> +	add_timer_on(timer, raw_smp_processor_id());
>  }

This is racy wrt CPU hotplug. The CPU can be cpu_down()'ed right
after raw_smp_processor_id() returns, and before we actually add
the timer.

Another problem is that now it is add_timer() becomes unsafe wrt
concurrent del_timer/del_timer_sync, because add_timer_on() does
not lock the timer, it only locks the target base.

Oleg.