Re: [PATCH] timers: consider slack value in mod_timer()

[Yong Zhang]

On Tue, May 24, 2011 at 8:13 PM, Sebastian Andrzej Siewior
<sebastian@xxxxxxxxxxxxx> wrote:
> * Yong Zhang | 2011-05-24 15:54:17 [+0800]:
>
>>> diff --git a/kernel/timer.c b/kernel/timer.c
>>> index fd61986..bf09726 100644
>>> --- a/kernel/timer.c
>>> +++ b/kernel/timer.c
>>> @@ -804,6 +804,8 @@ int mod_timer(struct timer_list *timer, unsigned long expires)
>>> ?? ?? ?? ?? ?? ?? ?? ??return 1;
>>>
>>> ?? ?? ?? ??expires = apply_slack(timer, expires);
>>
>>So, why not move above line up, then we can use the recalculated
>>expires?
>
> We leave often before apply_slack() kicks in. From printks() it looks
> like we leave more often in first "return 1" than in the second. Moving
> that line up would lead to more __mode_timer() calls.

Hmmm, so the reason is for a timer whose timer->slack is not set
explicitly. when we recalculate expires, we will get different value
sometimes.

Could you please try the attached patch(webmail will mangle it)

Thanks,
Yong



-- 
Only stand for myself
From 02fd23b0baf6f683e898927ed014cf7d0e8d5a90 Mon Sep 17 00:00:00 2001
From: Yong Zhang <yong.zhang0@xxxxxxxxx>
Date: Wed, 25 May 2011 16:20:17 +0800
Subject: [PATCH] timer: avoid recount slack for same-expire pending timer

mod_timer has a optimized way for same-expire pending timer,
but after we introduced slack, timer->expires will be
recount based on timer->slack.
So the current 'if (timer_pending(timer) && timer->expires == expires)'
will be true only when 'timer->slack >= 0', but for the case where
timer->slack is not defined explicitly, that check will fail sometimes.

Reported-by: Sebastian Andrzej Siewior <sebastian@xxxxxxxxxxxxx>
Signed-off-by: Yong Zhang <yong.zhang0@xxxxxxxxx>
Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
---
 kernel/timer.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/timer.c b/kernel/timer.c
index fd61986..73af53c 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -749,6 +749,10 @@ unsigned long apply_slack(struct timer_list *timer, unsigned long expires)
 	unsigned long expires_limit, mask;
 	int bit;
 
+	/* no need to account slack again for a same-expire pending timer */
+	if (timer_pending(timer) && time_after_eq(timer->expires, expires))
+		return timer->expires;
+
 	expires_limit = expires;
 
 	if (timer->slack >= 0) {
@@ -795,6 +799,8 @@ unsigned long apply_slack(struct timer_list *timer, unsigned long expires)
  */
 int mod_timer(struct timer_list *timer, unsigned long expires)
 {
+	expires = apply_slack(timer, expires);
+
 	/*
 	 * This is a common optimization triggered by the
 	 * networking code - if the timer is re-modified
@@ -803,8 +809,6 @@ int mod_timer(struct timer_list *timer, unsigned long expires)
 	if (timer_pending(timer) && timer->expires == expires)
 		return 1;
 
-	expires = apply_slack(timer, expires);
-
 	return __mod_timer(timer, expires, false, TIMER_NOT_PINNED);
 }
 EXPORT_SYMBOL(mod_timer);
-- 
1.7.4.1