timers/hrtimers: Minor cleanups: Part II

PATCH In review Viresh Kumar 2 participants 25 messages Mar 28, 2014 – Apr 2, 2014 High-resolution timers, timer wheel, clockevents

lore

Share & tools

lore

Viresh Kumar

author

2014-03-28 11:41 UTC #0 lore raw

cover timers/hrtimers: Minor cleanups: Part II

Hi Thomas,

I have got few more cleanups other than what I posted yesterday:
https://lkml.org/lkml/2014/3/26/107

All 30 are pushed here (i.e. Part I and II)

git://git.linaro.org/people/viresh.kumar/linux.git timer-cleanup-for-tglx

Please see if they make sense.

Viresh Kumar (16):
hrtimer: move unlock_hrtimer_base() upwards
hrtimer: reorder code in __remove_hrtimer()
hrtimer: Create hrtimer_get_monoexpires()
hrtimer: remove 'base' parameter from remove_timer() and

    __remove_timer()
  hrtimer: remove 'base' parameter from switch_hrtimer_base()
  hrtimer: remove 'base' parameter from enqueue_hrtimer()
  hrtimer: remove 'base' parameter from hrtimer_{enqueue_}reprogram()
  hrtimer: make switch_hrtimer_base() return void
  hrtimer: make lock_hrtimer_base() return void
  hrtimer: make enqueue_hrtimer() return void
  hrtimer: don't check if timer is queued in __remove_hrtimer()
  hrtimer: rewrite switch_hrtimer_base() to remove extra indentation
    level
  hrtimer: rewrite remove_hrtimer() to remove extra indentation level
  hrtimer: replace base by new_base to get resolution:
    __hrtimer_start_range_ns()

hrtimer: create base_on_this_cpu()
hrtimer: use base->hres_active directly instead of
hrtimer_hres_active()

 include/linux/hrtimer.h |  11 +++
 kernel/hrtimer.c        | 242 +++++++++++++++++++++---------------------------
 2 files changed, 119 insertions(+), 134 deletions(-)

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-28 11:41 UTC #1 lore raw

PATCH 1/16 hrtimer: move unlock_hrtimer_base() upwards

unlock_hrtimer_base() was handled specially at a separate place earlier as lock_hrtimer_base() had separate definitions for SMP and non-SMP cases, but unlock_hrtimer_base() had only a single definition. And so probably it was kept at the end of this #ifdef/endif CONFIG_SMP. But this #ifdef ends right after lock_hrtimer_base()'s definition and so we can move unlock_hrtimer_base() upwards, i.e. closer to its counterparts. This improves readability of the code.

Signed-off-by Viresh Kumar <[email protected]>

---
 kernel/hrtimer.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

kernel/hrtimer.c +6 -9

index e486a14..a710638 100644
@@ -253,6 +253,12 @@ lock_hrtimer_base(const struct hrtimer *timer, unsigned long *flags)
 
 #endif	/* !CONFIG_SMP */
 
+static inline
+void unlock_hrtimer_base(const struct hrtimer *timer, unsigned long *flags)
+{
+	raw_spin_unlock_irqrestore(&timer->base->cpu_base->lock, *flags);
+}
+
 /*
  * Functions for the union type storage format of ktime_t which are
  * too large for inlining:

@@ -801,15 +807,6 @@ static inline void timer_stats_account_hrtimer(struct hrtimer *timer)
 #endif
 }
 
-/*
- * Counterpart to lock_hrtimer_base above:
- */
-static inline
-void unlock_hrtimer_base(const struct hrtimer *timer, unsigned long *flags)
-{
-	raw_spin_unlock_irqrestore(&timer->base->cpu_base->lock, *flags);
-}
-
 /**
  * hrtimer_forward - forward the timer expiry
  * @timer:	hrtimer to forward

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-28 11:42 UTC #2 lore raw

PATCH 2/16 hrtimer: reorder code in __remove_hrtimer()

In reply to Viresh Kumar

This patch reorders code within __remove_hrtimer() routine to achieve this:
- no need to check if timer is the next timer to expire when high resolution mode isn't configured in kernel.
- no need of local variable 'next_timer'
- Validate 'reprogram' and hrtimer_hres_active() first as without these we don't need to check if timer is the next timer to fire.

Signed-off-by Viresh Kumar <[email protected]>

---
 kernel/hrtimer.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

kernel/hrtimer.c +10 ~~-13~~

index a710638..6476152 100644
@@ -887,25 +887,22 @@ static void __remove_hrtimer(struct hrtimer *timer,
 			     struct hrtimer_clock_base *base,
 			     unsigned long newstate, int reprogram)
 {
-	struct timerqueue_node *next_timer;
 	if (!(timer->state & HRTIMER_STATE_ENQUEUED))
 		goto out;
 
-	next_timer = timerqueue_getnext(&base->active);
 	timerqueue_del(&base->active, &timer->node);
-	if (&timer->node == next_timer) {
+
 #ifdef CONFIG_HIGH_RES_TIMERS
-		/* Reprogram the clock event device. if enabled */
-		if (reprogram && hrtimer_hres_active()) {
-			ktime_t expires;
-
-			expires = ktime_sub(hrtimer_get_expires(timer),
-					    base->offset);
-			if (base->cpu_base->expires_next.tv64 == expires.tv64)
-				hrtimer_force_reprogram(base->cpu_base, 1);
-		}
-#endif
+	/* Reprogram the clock event device. if enabled */
+	if (reprogram && hrtimer_hres_active() &&
+	    &timer->node == timerqueue_getnext(&base->active)) {
+		ktime_t expires;
+
+		expires = ktime_sub(hrtimer_get_expires(timer), base->offset);
+		if (base->cpu_base->expires_next.tv64 == expires.tv64)
+			hrtimer_force_reprogram(base->cpu_base, 1);
 	}
+#endif
 	if (!timerqueue_getnext(&base->active))
 		base->cpu_base->active_bases &= ~(1 << base->index);
 out:

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-04-02 05:41 UTC #3 lore raw

Re: [PATCH 02/16] hrtimer: reorder code in __remove_hrtimer()

Quoted 18 lines · Viresh Kumar

kernel/hrtimer.c +1 -6

@@ -887,25 +887,22 @@ static void __remove_hrtimer(struct hrtimer *timer,
                             struct hrtimer_clock_base *base,
                             unsigned long newstate, int reprogram)
 {
-       struct timerqueue_node *next_timer;
        if (!(timer->state & HRTIMER_STATE_ENQUEUED))
                goto out;
 
-       next_timer = timerqueue_getnext(&base->active);
        timerqueue_del(&base->active, &timer->node);
-       if (&timer->node == next_timer) {
+
 #ifdef CONFIG_HIGH_RES_TIMERS
-               /* Reprogram the clock event device. if enabled */
-               if (reprogram && hrtimer_hres_active()) {
-                       ktime_t expires;

kernel/hrtimer.c static void __remove_hrtimer(struct hrtimer *timer, · line 898

Quoted 10 lines

kernel/hrtimer.c +3 -7

-
-                       expires = ktime_sub(hrtimer_get_expires(timer),
-                                           base->offset);
-                       if (base->cpu_base->expires_next.tv64 == expires.tv64)
-                               hrtimer_force_reprogram(base->cpu_base, 1);
-               }
-#endif
+       /* Reprogram the clock event device. if enabled */
+       if (reprogram && hrtimer_hres_active() &&
+           &timer->node == timerqueue_getnext(&base->active)) {

Last comparison here would always fail as we have already removed the timer. Here is a fixup for that, will include that in my V2:

kernel/hrtimer.c +3 -2

index 516c67f..189c525 100644
@@ -886,15 +886,16 @@ static void __remove_hrtimer(struct hrtimer *timer,
                             struct hrtimer_clock_base *base,
                             unsigned long newstate, int reprogram)
 {
+       struct timerqueue_node *next_timer;
        if (!(timer->state & HRTIMER_STATE_ENQUEUED))
                goto out;
 
+       next_timer = timerqueue_getnext(&base->active);
        timerqueue_del(&base->active, &timer->node);
 
 #ifdef CONFIG_HIGH_RES_TIMERS
        /* Reprogram the clock event device. if enabled */
-       if (reprogram && hrtimer_hres_active() &&
-           &timer->node == timerqueue_getnext(&base->active)) {
+       if (reprogram && hrtimer_hres_active() && &timer->node == next_timer) {
                ktime_t expires;
 
                expires = ktime_sub(hrtimer_get_expires(timer), base->offset);

Viresh Kumar

author

2014-03-28 11:42 UTC #4 lore raw

PATCH 3/16 hrtimer: Create hrtimer_get_monoexpires()

In reply to Viresh Kumar

Following code is repeated at many places:

	ktime_sub(hrtimer_get_expires(timer), base->offset);

and so it makes sense to create a separate inlined routine for this.

Signed-off-by Viresh Kumar <[email protected]>

---
 include/linux/hrtimer.h |  6 ++++++
 kernel/hrtimer.c        | 11 +++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

include/linux/hrtimer.h +6 -0

index 6f524db..377023b 100644
@@ -233,6 +233,12 @@ static inline ktime_t hrtimer_get_expires(const struct hrtimer *timer)
 	return timer->node.expires;
 }
 
+static inline ktime_t hrtimer_get_monoexpires(const struct hrtimer *timer,
+		struct hrtimer_clock_base *base)
+{
+	return ktime_sub(hrtimer_get_expires(timer), base->offset);
+}
+
 static inline ktime_t hrtimer_get_softexpires(const struct hrtimer *timer)
 {
 	return timer->_softexpires;

kernel/hrtimer.c +5 -6

index 6476152..675fe9e 100644
@@ -184,7 +184,7 @@ hrtimer_check_target(struct hrtimer *timer, struct hrtimer_clock_base *new_base)
 	if (!new_base->cpu_base->hres_active)
 		return 0;
 
-	expires = ktime_sub(hrtimer_get_expires(timer), new_base->offset);
+	expires = hrtimer_get_monoexpires(timer, new_base);
 	return expires.tv64 <= new_base->cpu_base->expires_next.tv64;
 #else
 	return 0;

@@ -555,7 +555,7 @@ hrtimer_force_reprogram(struct hrtimer_cpu_base *cpu_base, int skip_equal)
 			continue;
 		timer = container_of(next, struct hrtimer, node);
 
-		expires = ktime_sub(hrtimer_get_expires(timer), base->offset);
+		expires = hrtimer_get_monoexpires(timer, base);
 		/*
 		 * clock_was_set() has changed base->offset so the
 		 * result might be negative. Fix it up to prevent a

@@ -589,7 +589,7 @@ static int hrtimer_reprogram(struct hrtimer *timer,
 			     struct hrtimer_clock_base *base)
 {
 	struct hrtimer_cpu_base *cpu_base = &__get_cpu_var(hrtimer_bases);
-	ktime_t expires = ktime_sub(hrtimer_get_expires(timer), base->offset);
+	ktime_t expires = hrtimer_get_monoexpires(timer, base);
 	int res;
 
 	WARN_ON_ONCE(hrtimer_get_expires_tv64(timer) < 0);

@@ -898,7 +898,7 @@ static void __remove_hrtimer(struct hrtimer *timer,
 	    &timer->node == timerqueue_getnext(&base->active)) {
 		ktime_t expires;
 
-		expires = ktime_sub(hrtimer_get_expires(timer), base->offset);
+		expires = hrtimer_get_monoexpires(timer, base);
 		if (base->cpu_base->expires_next.tv64 == expires.tv64)
 			hrtimer_force_reprogram(base->cpu_base, 1);
 	}

@@ -1306,8 +1306,7 @@ retry:
 			if (basenow.tv64 < hrtimer_get_softexpires_tv64(timer)) {
 				ktime_t expires;
 
-				expires = ktime_sub(hrtimer_get_expires(timer),
-						    base->offset);
+				expires = hrtimer_get_monoexpires(timer, base);
 				if (expires.tv64 < 0)
 					expires.tv64 = KTIME_MAX;
 				if (expires.tv64 < expires_next.tv64)

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-28 11:42 UTC #5 lore raw

PATCH 4/16 hrtimer: remove 'base' parameter from remove_timer() and __remove_timer()

In reply to Viresh Kumar

clock 'base' can be obtained easily by doing timer->base and remove_timer()/ __remove_timer() never gets anything else than timer->base as its parameter. Which means, these routines doesn't require this parameter. Remove it.

Signed-off-by Viresh Kumar <[email protected]>

---
 kernel/hrtimer.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

kernel/hrtimer.c +14 ~~-16~~

index 675fe9e..343fe99 100644
@@ -883,10 +883,11 @@ static int enqueue_hrtimer(struct hrtimer *timer,
  * reprogram to zero. This is useful, when the context does a reprogramming
  * anyway (e.g. timer interrupt)
  */
-static void __remove_hrtimer(struct hrtimer *timer,
-			     struct hrtimer_clock_base *base,
-			     unsigned long newstate, int reprogram)
+static void __remove_hrtimer(struct hrtimer *timer, unsigned long newstate,
+			     int reprogram)
 {
+	struct hrtimer_clock_base *base = timer->base;
+
 	if (!(timer->state & HRTIMER_STATE_ENQUEUED))
 		goto out;

@@ -909,11 +910,8 @@ out:
 	timer->state = newstate;
 }
 
-/*
- * remove hrtimer, called with base lock held
- */
-static inline int
-remove_hrtimer(struct hrtimer *timer, struct hrtimer_clock_base *base)
+/* remove hrtimer, called with base lock held */
+static inline int remove_hrtimer(struct hrtimer *timer)
 {
 	if (hrtimer_is_queued(timer)) {
 		unsigned long state;

@@ -929,14 +927,15 @@ remove_hrtimer(struct hrtimer *timer, struct hrtimer_clock_base *base)
 		 */
 		debug_deactivate(timer);
 		timer_stats_hrtimer_clear_start_info(timer);
-		reprogram = base->cpu_base == &__get_cpu_var(hrtimer_bases);
+		reprogram =
+			timer->base->cpu_base == &__get_cpu_var(hrtimer_bases);
 		/*
 		 * We must preserve the CALLBACK state flag here,
 		 * otherwise we could move the timer base in
 		 * switch_hrtimer_base.
 		 */
 		state = timer->state & HRTIMER_STATE_CALLBACK;
-		__remove_hrtimer(timer, base, state, reprogram);
+		__remove_hrtimer(timer, state, reprogram);
 		return 1;
 	}
 	return 0;

@@ -953,7 +952,7 @@ int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 	base = lock_hrtimer_base(timer, &flags);
 
 	/* Remove an active timer from the queue: */
-	ret = remove_hrtimer(timer, base);
+	ret = remove_hrtimer(timer);
 
 	/* Switch the timer base, if necessary: */
 	new_base = switch_hrtimer_base(timer, base, mode & HRTIMER_MODE_PINNED);

@@ -1055,14 +1054,13 @@ EXPORT_SYMBOL_GPL(hrtimer_start);
  */
 int hrtimer_try_to_cancel(struct hrtimer *timer)
 {
-	struct hrtimer_clock_base *base;
 	unsigned long flags;
 	int ret = -1;
 
-	base = lock_hrtimer_base(timer, &flags);
+	lock_hrtimer_base(timer, &flags);
 
 	if (!hrtimer_callback_running(timer))
-		ret = remove_hrtimer(timer, base);
+		ret = remove_hrtimer(timer);
 
 	unlock_hrtimer_base(timer, &flags);

@@ -1218,7 +1216,7 @@ static void __run_hrtimer(struct hrtimer *timer, ktime_t *now)
 	WARN_ON(!irqs_disabled());
 
 	debug_deactivate(timer);
-	__remove_hrtimer(timer, base, HRTIMER_STATE_CALLBACK, 0);
+	__remove_hrtimer(timer, HRTIMER_STATE_CALLBACK, 0);
 	timer_stats_account_hrtimer(timer);
 	fn = timer->function;

@@ -1663,7 +1661,7 @@ static void migrate_hrtimer_list(struct hrtimer_clock_base *old_base,
 		 * timer could be seen as !active and just vanish away
 		 * under us on another CPU
 		 */
-		__remove_hrtimer(timer, old_base, HRTIMER_STATE_MIGRATE, 0);
+		__remove_hrtimer(timer, HRTIMER_STATE_MIGRATE, 0);
 		timer->base = new_base;
 		/*
 		 * Enqueue the timers on the new cpu. This does not

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-28 11:42 UTC #6 lore raw

PATCH 5/16 hrtimer: remove 'base' parameter from switch_hrtimer_base()

In reply to Viresh Kumar

clock 'base' can be obtained easily by doing timer->base and switch_hrtimer_base() never gets anything else than timer->base as its parameter. And so this routines doesn't require this parameter. Remove it.

Signed-off-by Viresh Kumar <[email protected]>

---
 kernel/hrtimer.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

kernel/hrtimer.c +4 -5

index 343fe99..9561336 100644
@@ -195,10 +195,9 @@ hrtimer_check_target(struct hrtimer *timer, struct hrtimer_clock_base *new_base)
  * Switch the timer base to the current CPU when possible.
  */
 static inline struct hrtimer_clock_base *
-switch_hrtimer_base(struct hrtimer *timer, struct hrtimer_clock_base *base,
-		    int pinned)
+switch_hrtimer_base(struct hrtimer *timer, int pinned)
 {
-	struct hrtimer_clock_base *new_base;
+	struct hrtimer_clock_base *new_base, *base = timer->base;
 	struct hrtimer_cpu_base *new_cpu_base;
 	int this_cpu = smp_processor_id();
 	int cpu = get_nohz_timer_target(pinned);

@@ -249,7 +248,7 @@ lock_hrtimer_base(const struct hrtimer *timer, unsigned long *flags)
 	return base;
 }
 
-# define switch_hrtimer_base(t, b, p)	(b)
+# define switch_hrtimer_base(t, p)	(t->base)
 
 #endif	/* !CONFIG_SMP */

@@ -955,7 +954,7 @@ int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 	ret = remove_hrtimer(timer);
 
 	/* Switch the timer base, if necessary: */
-	new_base = switch_hrtimer_base(timer, base, mode & HRTIMER_MODE_PINNED);
+	new_base = switch_hrtimer_base(timer, mode & HRTIMER_MODE_PINNED);
 
 	if (mode & HRTIMER_MODE_REL) {
 		tim = ktime_add_safe(tim, new_base->get_time());

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-28 11:42 UTC #7 lore raw

PATCH 6/16 hrtimer: remove 'base' parameter from enqueue_hrtimer()

In reply to Viresh Kumar

clock 'base' can be obtained easily by doing timer->base and enqueue_hrtimer() never gets anything else than timer->base as its parameter. And so this routines doesn't require this parameter. Remove it.

Signed-off-by Viresh Kumar <[email protected]>

---
 kernel/hrtimer.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

kernel/hrtimer.c +7 -7

index 9561336..d6724b5 100644
@@ -855,9 +855,10 @@ EXPORT_SYMBOL_GPL(hrtimer_forward);
  *
  * Returns 1 when the new timer is the leftmost timer in the tree.
  */
-static int enqueue_hrtimer(struct hrtimer *timer,
-			   struct hrtimer_clock_base *base)
+static int enqueue_hrtimer(struct hrtimer *timer)
 {
+	struct hrtimer_clock_base *base = timer->base;
+
 	debug_activate(timer);
 
 	timerqueue_add(&base->active, &timer->node);

@@ -974,7 +975,7 @@ int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 
 	timer_stats_hrtimer_set_start_info(timer);
 
-	leftmost = enqueue_hrtimer(timer, new_base);
+	leftmost = enqueue_hrtimer(timer);
 
 	/*
 	 * Only allow reprogramming if the new base is on this CPU.

@@ -1207,8 +1208,7 @@ EXPORT_SYMBOL_GPL(hrtimer_get_res);
 
 static void __run_hrtimer(struct hrtimer *timer, ktime_t *now)
 {
-	struct hrtimer_clock_base *base = timer->base;
-	struct hrtimer_cpu_base *cpu_base = base->cpu_base;
+	struct hrtimer_cpu_base *cpu_base = timer->base->cpu_base;
 	enum hrtimer_restart (*fn)(struct hrtimer *);
 	int restart;

@@ -1237,7 +1237,7 @@ static void __run_hrtimer(struct hrtimer *timer, ktime_t *now)
 	 */
 	if (restart != HRTIMER_NORESTART) {
 		BUG_ON(timer->state != HRTIMER_STATE_CALLBACK);
-		enqueue_hrtimer(timer, base);
+		enqueue_hrtimer(timer);
 	}
 
 	WARN_ON_ONCE(!(timer->state & HRTIMER_STATE_CALLBACK));

@@ -1670,7 +1670,7 @@ static void migrate_hrtimer_list(struct hrtimer_clock_base *old_base,
 		 * sort out already expired timers and reprogram the
 		 * event device.
 		 */
-		enqueue_hrtimer(timer, new_base);
+		enqueue_hrtimer(timer);
 
 		/* Clear the migration state bit */
 		timer->state &= ~HRTIMER_STATE_MIGRATE;

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-28 11:42 UTC #8 lore raw

PATCH 7/16 hrtimer: remove 'base' parameter from hrtimer_{enqueue_}reprogram()

In reply to Viresh Kumar

clock 'base' can be obtained easily by doing timer->base and hrtimer_reprogram() & hrtimer_enqueue_reprogram() never gets anything else than timer->base as its parameter. And so these routines doesn't require this parameter. Remove it.

Signed-off-by Viresh Kumar <[email protected]>

---
 kernel/hrtimer.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

kernel/hrtimer.c +6 -9

index d6724b5..98a73d9 100644
@@ -584,11 +584,10 @@ hrtimer_force_reprogram(struct hrtimer_cpu_base *cpu_base, int skip_equal)
  *
  * Called with interrupts disabled and base->cpu_base.lock held
  */
-static int hrtimer_reprogram(struct hrtimer *timer,
-			     struct hrtimer_clock_base *base)
+static int hrtimer_reprogram(struct hrtimer *timer)
 {
 	struct hrtimer_cpu_base *cpu_base = &__get_cpu_var(hrtimer_bases);
-	ktime_t expires = hrtimer_get_monoexpires(timer, base);
+	ktime_t expires = hrtimer_get_monoexpires(timer, timer->base);
 	int res;
 
 	WARN_ON_ONCE(hrtimer_get_expires_tv64(timer) < 0);

@@ -648,10 +647,9 @@ static inline void hrtimer_init_hres(struct hrtimer_cpu_base *base)
  * check happens. The timer gets enqueued into the rbtree. The reprogramming
  * and expiry check is done in the hrtimer_interrupt or in the softirq.
  */
-static inline int hrtimer_enqueue_reprogram(struct hrtimer *timer,
-					    struct hrtimer_clock_base *base)
+static inline int hrtimer_enqueue_reprogram(struct hrtimer *timer)
 {
-	return base->cpu_base->hres_active && hrtimer_reprogram(timer, base);
+	return timer->base->cpu_base->hres_active && hrtimer_reprogram(timer);
 }
 
 static inline ktime_t hrtimer_update_base(struct hrtimer_cpu_base *base)

@@ -730,8 +728,7 @@ static void clock_was_set_delayed(void)
 static inline int hrtimer_hres_active(void) { return 0; }
 static inline int hrtimer_is_hres_enabled(void) { return 0; }
 static inline int hrtimer_switch_to_hres(void) { return 0; }
-static inline int hrtimer_enqueue_reprogram(struct hrtimer *timer,
-					    struct hrtimer_clock_base *base)
+static inline int hrtimer_enqueue_reprogram(struct hrtimer *timer)
 {
 	return 0;
 }

@@ -984,7 +981,7 @@ int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 	 * XXX send_remote_softirq() ?
 	 */
 	if (leftmost && new_base->cpu_base == &__get_cpu_var(hrtimer_bases)
-		&& hrtimer_enqueue_reprogram(timer, new_base)) {
+		&& hrtimer_enqueue_reprogram(timer)) {
 		if (wakeup) {
 			/*
 			 * We need to drop cpu_base->lock to avoid a

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-28 11:42 UTC #9 lore raw

PATCH 8/16 hrtimer: make switch_hrtimer_base() return void

In reply to Viresh Kumar

switch_hrtimer_base() always sets timer->base to the right base and so the caller can obtain it easily. So, this routine doesn't need to return anything.

Signed-off-by Viresh Kumar <[email protected]>

---
 kernel/hrtimer.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

kernel/hrtimer.c +5 -6

index 98a73d9..d98c1ec 100644
@@ -194,8 +194,7 @@ hrtimer_check_target(struct hrtimer *timer, struct hrtimer_clock_base *new_base)
 /*
  * Switch the timer base to the current CPU when possible.
  */
-static inline struct hrtimer_clock_base *
-switch_hrtimer_base(struct hrtimer *timer, int pinned)
+static inline void switch_hrtimer_base(struct hrtimer *timer, int pinned)
 {
 	struct hrtimer_clock_base *new_base, *base = timer->base;
 	struct hrtimer_cpu_base *new_cpu_base;

@@ -217,7 +216,7 @@ again:
 		 * the timer is enqueued.
 		 */
 		if (unlikely(hrtimer_callback_running(timer)))
-			return base;
+			return;
 
 		/* See the comment in lock_timer_base() */
 		timer->base = NULL;

@@ -233,7 +232,6 @@ again:
 		}
 		timer->base = new_base;
 	}
-	return new_base;
 }
 
 #else /* CONFIG_SMP */

@@ -248,7 +246,7 @@ lock_hrtimer_base(const struct hrtimer *timer, unsigned long *flags)
 	return base;
 }
 
-# define switch_hrtimer_base(t, p)	(t->base)
+static inline void switch_hrtimer_base(struct hrtimer *timer, int pinned) {}
 
 #endif	/* !CONFIG_SMP */

@@ -952,7 +950,8 @@ int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 	ret = remove_hrtimer(timer);
 
 	/* Switch the timer base, if necessary: */
-	new_base = switch_hrtimer_base(timer, mode & HRTIMER_MODE_PINNED);
+	switch_hrtimer_base(timer, mode & HRTIMER_MODE_PINNED);
+	new_base = timer->base;
 
 	if (mode & HRTIMER_MODE_REL) {
 		tim = ktime_add_safe(tim, new_base->get_time());

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-28 11:42 UTC #10 lore raw

PATCH 9/16 hrtimer: make lock_hrtimer_base() return void

In reply to Viresh Kumar

lock_hrtimer_base() always returns after taking lock and so timer->base can't change further. So, callers of this routine can simply do timer->base to get the base and so we can make this routine return void.

Signed-off-by Viresh Kumar <[email protected]>

---
 kernel/hrtimer.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

kernel/hrtimer.c +6 ~~-11~~

index d98c1ec..7d85b8f 100644
@@ -149,9 +149,7 @@ static void hrtimer_get_softirq_time(struct hrtimer_cpu_base *base)
  * possible to set timer->base = NULL and drop the lock: the timer remains
  * locked.
  */
-static
-struct hrtimer_clock_base *lock_hrtimer_base(const struct hrtimer *timer,
-					     unsigned long *flags)
+static void lock_hrtimer_base(const struct hrtimer *timer, unsigned long *flags)
 {
 	struct hrtimer_clock_base *base;

@@ -160,7 +158,7 @@ struct hrtimer_clock_base *lock_hrtimer_base(const struct hrtimer *timer,
 		if (likely(base != NULL)) {
 			raw_spin_lock_irqsave(&base->cpu_base->lock, *flags);
 			if (likely(base == timer->base))
-				return base;
+				return;
 			/* The timer has migrated to another CPU: */
 			raw_spin_unlock_irqrestore(&base->cpu_base->lock, *flags);
 		}

@@ -236,14 +234,10 @@ again:
 
 #else /* CONFIG_SMP */
 
-static inline struct hrtimer_clock_base *
+static inline void
 lock_hrtimer_base(const struct hrtimer *timer, unsigned long *flags)
 {
-	struct hrtimer_clock_base *base = timer->base;
-
-	raw_spin_lock_irqsave(&base->cpu_base->lock, *flags);
-
-	return base;
+	raw_spin_lock_irqsave(&timer->base->cpu_base->lock, *flags);
 }
 
 static inline void switch_hrtimer_base(struct hrtimer *timer, int pinned) {}

@@ -944,7 +938,8 @@ int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 	unsigned long flags;
 	int ret, leftmost;
 
-	base = lock_hrtimer_base(timer, &flags);
+	lock_hrtimer_base(timer, &flags);
+	base = timer->base;
 
 	/* Remove an active timer from the queue: */
 	ret = remove_hrtimer(timer);

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-28 11:43 UTC #11 lore raw

PATCH 10/16 hrtimer: make enqueue_hrtimer() return void

In reply to Viresh Kumar

enqueue_hrtimer() routine is called from three places and only one of them effectively uses its return value. Also, by its name enqueue_hrtimer() isn't supposed to return "if the queued timer is the leftmost". So it makes more sense to separate this routine into two parts, first one enqueues a timer and the other one tells if the timer is leftmost or not.

Signed-off-by Viresh Kumar <[email protected]>

---
 include/linux/hrtimer.h |  5 +++++
 kernel/hrtimer.c        | 11 +++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

include/linux/hrtimer.h +5 -0

index 377023b..7ca9fd0 100644
@@ -263,6 +263,11 @@ static inline ktime_t hrtimer_expires_remaining(const struct hrtimer *timer)
 	return ktime_sub(timer->node.expires, timer->base->get_time());
 }
 
+static inline int hrtimer_is_leftmost(struct hrtimer *timer)
+{
+	return &timer->node == timer->base->active.next;
+}
+
 #ifdef CONFIG_HIGH_RES_TIMERS
 struct clock_event_device;

kernel/hrtimer.c +5 -6

index 7d85b8f..2702185 100644
@@ -844,7 +844,7 @@ EXPORT_SYMBOL_GPL(hrtimer_forward);
  *
  * Returns 1 when the new timer is the leftmost timer in the tree.
  */
-static int enqueue_hrtimer(struct hrtimer *timer)
+static void enqueue_hrtimer(struct hrtimer *timer)
 {
 	struct hrtimer_clock_base *base = timer->base;

@@ -858,8 +858,6 @@ static int enqueue_hrtimer(struct hrtimer *timer)
 	 * state of a possibly running callback.
 	 */
 	timer->state |= HRTIMER_STATE_ENQUEUED;
-
-	return (&timer->node == base->active.next);
 }
 
 /*

@@ -936,7 +934,7 @@ int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 {
 	struct hrtimer_clock_base *base, *new_base;
 	unsigned long flags;
-	int ret, leftmost;
+	int ret;
 
 	lock_hrtimer_base(timer, &flags);
 	base = timer->base;

@@ -966,7 +964,7 @@ int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 
 	timer_stats_hrtimer_set_start_info(timer);
 
-	leftmost = enqueue_hrtimer(timer);
+	enqueue_hrtimer(timer);
 
 	/*
 	 * Only allow reprogramming if the new base is on this CPU.

@@ -974,7 +972,8 @@ int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 	 *
 	 * XXX send_remote_softirq() ?
 	 */
-	if (leftmost && new_base->cpu_base == &__get_cpu_var(hrtimer_bases)
+	if (hrtimer_is_leftmost(timer) &&
+	    new_base->cpu_base == &__get_cpu_var(hrtimer_bases)
 		&& hrtimer_enqueue_reprogram(timer)) {
 		if (wakeup) {
 			/*

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-28 11:43 UTC #12 lore raw

PATCH 11/16 hrtimer: don't check if timer is queued in __remove_hrtimer()

In reply to Viresh Kumar

__remove_hrtimer() is called from three locations: remove_hrtimer(), __run_hrtimer() and migrate_hrtimer_list(). And all these guarantee that timer was queued earlier. And so there is no need to check if the timer is queued or not in __remove_hrtimer().

Signed-off-by Viresh Kumar <[email protected]>

---
 kernel/hrtimer.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

kernel/hrtimer.c +1 -5

index 2702185..04f8e44 100644
@@ -875,9 +875,6 @@ static void __remove_hrtimer(struct hrtimer *timer, unsigned long newstate,
 {
 	struct hrtimer_clock_base *base = timer->base;
 
-	if (!(timer->state & HRTIMER_STATE_ENQUEUED))
-		goto out;
-
 	timerqueue_del(&base->active, &timer->node);
 
 #ifdef CONFIG_HIGH_RES_TIMERS

@@ -891,10 +888,9 @@ static void __remove_hrtimer(struct hrtimer *timer, unsigned long newstate,
 			hrtimer_force_reprogram(base->cpu_base, 1);
 	}
 #endif
+	timer->state = newstate;
 	if (!timerqueue_getnext(&base->active))
 		base->cpu_base->active_bases &= ~(1 << base->index);
-out:
-	timer->state = newstate;
 }
 
 /* remove hrtimer, called with base lock held */

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-28 11:43 UTC #13 lore raw

PATCH 12/16 hrtimer: rewrite switch_hrtimer_base() to remove extra indentation level

In reply to Viresh Kumar

Complete bottom part of switch_hrtimer_base() is part of a 'if' block and so all code present in that block has extra indentation level before it. Rewrite it to remove this extra indentation level.

Signed-off-by Viresh Kumar <[email protected]>

---
 kernel/hrtimer.c | 48 ++++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

kernel/hrtimer.c +24 ~~-24~~

index 04f8e44..30efa1c 100644
@@ -203,33 +203,33 @@ again:
 	new_cpu_base = &per_cpu(hrtimer_bases, cpu);
 	new_base = &new_cpu_base->clock_base[base->index];
 
-	if (base != new_base) {
-		/*
-		 * We are trying to move timer to new_base.
-		 * However we can't change timer's base while it is running,
-		 * so we keep it on the same CPU. No hassle vs. reprogramming
-		 * the event source in the high resolution case. The softirq
-		 * code will take care of this when the timer function has
-		 * completed. There is no conflict as we hold the lock until
-		 * the timer is enqueued.
-		 */
-		if (unlikely(hrtimer_callback_running(timer)))
-			return;
+	if (base == new_base)
+		return;
 
-		/* See the comment in lock_timer_base() */
-		timer->base = NULL;
-		raw_spin_unlock(&base->cpu_base->lock);
-		raw_spin_lock(&new_base->cpu_base->lock);
+	/*
+	 * We are trying to move timer to new_base. However we can't change
+	 * timer's base while it is running, so we keep it on the same CPU. No
+	 * hassle vs. reprogramming the event source in the high resolution
+	 * case. The softirq code will take care of this when the timer function
+	 * has completed. There is no conflict as we hold the lock until the
+	 * timer is enqueued.
+	 */
+	if (unlikely(hrtimer_callback_running(timer)))
+		return;
 
-		if (cpu != this_cpu && hrtimer_check_target(timer, new_base)) {
-			cpu = this_cpu;
-			raw_spin_unlock(&new_base->cpu_base->lock);
-			raw_spin_lock(&base->cpu_base->lock);
-			timer->base = base;
-			goto again;
-		}
-		timer->base = new_base;
+	/* See the comment in lock_timer_base() */
+	timer->base = NULL;
+	raw_spin_unlock(&base->cpu_base->lock);
+	raw_spin_lock(&new_base->cpu_base->lock);
+
+	if (cpu != this_cpu && hrtimer_check_target(timer, new_base)) {
+		cpu = this_cpu;
+		raw_spin_unlock(&new_base->cpu_base->lock);
+		raw_spin_lock(&base->cpu_base->lock);
+		timer->base = base;
+		goto again;
 	}
+	timer->base = new_base;
 }
 
 #else /* CONFIG_SMP */

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-28 11:43 UTC #14 lore raw

PATCH 13/16 hrtimer: rewrite remove_hrtimer() to remove extra indentation level

In reply to Viresh Kumar

Complete bottom part of remove_hrtimer() is part of a 'if' block and so all code present in that block has extra indentation level before it. Rewrite it to remove this extra indentation level.

Signed-off-by Viresh Kumar <[email protected]>

---
 kernel/hrtimer.c | 47 ++++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

kernel/hrtimer.c +22 ~~-25~~

index 30efa1c..0ae0fbf 100644
@@ -896,32 +896,29 @@ static void __remove_hrtimer(struct hrtimer *timer, unsigned long newstate,
 /* remove hrtimer, called with base lock held */
 static inline int remove_hrtimer(struct hrtimer *timer)
 {
-	if (hrtimer_is_queued(timer)) {
-		unsigned long state;
-		int reprogram;
+	unsigned long state;
 
-		/*
-		 * Remove the timer and force reprogramming when high
-		 * resolution mode is active and the timer is on the current
-		 * CPU. If we remove a timer on another CPU, reprogramming is
-		 * skipped. The interrupt event on this CPU is fired and
-		 * reprogramming happens in the interrupt handler. This is a
-		 * rare case and less expensive than a smp call.
-		 */
-		debug_deactivate(timer);
-		timer_stats_hrtimer_clear_start_info(timer);
-		reprogram =
-			timer->base->cpu_base == &__get_cpu_var(hrtimer_bases);
-		/*
-		 * We must preserve the CALLBACK state flag here,
-		 * otherwise we could move the timer base in
-		 * switch_hrtimer_base.
-		 */
-		state = timer->state & HRTIMER_STATE_CALLBACK;
-		__remove_hrtimer(timer, state, reprogram);
-		return 1;
-	}
-	return 0;
+	if (!hrtimer_is_queued(timer))
+		return 0;
+
+	/*
+	 * Remove the timer and force reprogramming when high resolution mode is
+	 * active and the timer is on the current CPU. If we remove a timer on
+	 * another CPU, reprogramming is skipped. The interrupt event on this
+	 * CPU is fired and reprogramming happens in the interrupt handler. This
+	 * is a rare case and less expensive than a smp call.
+	 */
+	debug_deactivate(timer);
+	timer_stats_hrtimer_clear_start_info(timer);
+
+	/*
+	 * We must preserve the CALLBACK state flag here, otherwise we could
+	 * move the timer base in switch_hrtimer_base.
+	 */
+	state = timer->state & HRTIMER_STATE_CALLBACK;
+	__remove_hrtimer(timer, state,
+			timer->base->cpu_base == &__get_cpu_var(hrtimer_bases));
+	return 1;
 }
 
 int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-28 11:43 UTC #15 lore raw

PATCH 14/16 hrtimer: replace base by new_base to get resolution: __hrtimer_start_range_ns()

In reply to Viresh Kumar

This code was added long back by following commit:

	commit 06027bdd278a32a84b273e41db68a5db8ffd2bb6
	Author: Ingo Molnar <[email protected]>
	Date:   Tue Feb 14 13:53:15 2006 -0800

	    [PATCH] hrtimer: round up relative start time on low-res arches

Don't know if it was a mistake or was intentional. But probably we must use new_base instead of base here to get resolution. Things might be working smoothly as resolution might be same for both the bases in most of the cases.

Cc Ingo Molnar <[email protected]>

Signed-off-by Viresh Kumar <[email protected]>

---
Also commit log of above commit has this: "This will go away with the GTOD framework". So, should we get this removed?

 kernel/hrtimer.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

kernel/hrtimer.c +2 -3

index 0ae0fbf..5d77f36 100644
@@ -925,12 +925,11 @@ int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 		unsigned long delta_ns, const enum hrtimer_mode mode,
 		int wakeup)
 {
-	struct hrtimer_clock_base *base, *new_base;
+	struct hrtimer_clock_base *new_base;
 	unsigned long flags;
 	int ret;
 
 	lock_hrtimer_base(timer, &flags);
-	base = timer->base;
 
 	/* Remove an active timer from the queue: */
 	ret = remove_hrtimer(timer);

@@ -949,7 +948,7 @@ int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 		 * timeouts. This will go away with the GTOD framework.
 		 */
 #ifdef CONFIG_TIME_LOW_RES
-		tim = ktime_add_safe(tim, base->resolution);
+		tim = ktime_add_safe(tim, new_base->resolution);
 #endif
 	}

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-28 11:43 UTC #16 lore raw

PATCH 15/16 hrtimer: create base_on_this_cpu()

In reply to Viresh Kumar

We had this code at two places to find if a clock base belongs to current CPU:

	base->cpu_base == &__get_cpu_var(hrtimer_bases)

Better to get a inlined routine for that.

Signed-off-by Viresh Kumar <[email protected]>

---
 kernel/hrtimer.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

kernel/hrtimer.c +7 -5

index 5d77f36..83e5f2d 100644
@@ -107,6 +107,10 @@ static inline int hrtimer_clockid_to_base(clockid_t clock_id)
 	return hrtimer_clock_to_base_table[clock_id];
 }
 
+static inline bool base_on_this_cpu(struct hrtimer_clock_base *base)
+{
+	return base->cpu_base == &__get_cpu_var(hrtimer_bases);
+}
 
 /*
  * Get the coarse grained time at the softirq based on xtime and

@@ -916,8 +920,7 @@ static inline int remove_hrtimer(struct hrtimer *timer)
 	 * move the timer base in switch_hrtimer_base.
 	 */
 	state = timer->state & HRTIMER_STATE_CALLBACK;
-	__remove_hrtimer(timer, state,
-			timer->base->cpu_base == &__get_cpu_var(hrtimer_bases));
+	__remove_hrtimer(timer, state, base_on_this_cpu(timer->base));
 	return 1;
 }

@@ -964,9 +967,8 @@ int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 	 *
 	 * XXX send_remote_softirq() ?
 	 */
-	if (hrtimer_is_leftmost(timer) &&
-	    new_base->cpu_base == &__get_cpu_var(hrtimer_bases)
-		&& hrtimer_enqueue_reprogram(timer)) {
+	if (hrtimer_is_leftmost(timer) && base_on_this_cpu(new_base) &&
+	    hrtimer_enqueue_reprogram(timer)) {
 		if (wakeup) {
 			/*
 			 * We need to drop cpu_base->lock to avoid a

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-28 11:43 UTC #17 lore raw

PATCH 16/16 hrtimer: use base->hres_active directly instead of hrtimer_hres_active()

In reply to Viresh Kumar

use base->hres_active directly when we already have a pointer to base instead of calling hrtimer_hres_active(). As that would lead to:

__this_cpu_read(hrtimer_bases.hres_active)

Signed-off-by Viresh Kumar <[email protected]>

---
 kernel/hrtimer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

kernel/hrtimer.c +3 -3

index 83e5f2d..f379821 100644
@@ -883,7 +883,7 @@ static void __remove_hrtimer(struct hrtimer *timer, unsigned long newstate,
 
 #ifdef CONFIG_HIGH_RES_TIMERS
 	/* Reprogram the clock event device. if enabled */
-	if (reprogram && hrtimer_hres_active() &&
+	if (reprogram && base->cpu_base->hres_active &&
 	    &timer->node == timerqueue_getnext(&base->active)) {
 		ktime_t expires;

@@ -1107,7 +1107,7 @@ ktime_t hrtimer_get_next_event(void)
 
 	raw_spin_lock_irqsave(&cpu_base->lock, flags);
 
-	if (!hrtimer_hres_active()) {
+	if (!cpu_base->hres_active) {
 		for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++, base++) {
 			struct hrtimer *timer;
 			struct timerqueue_node *next;

@@ -1437,7 +1437,7 @@ void hrtimer_run_queues(void)
 	struct hrtimer_clock_base *base;
 	int index, gettime = 1;
 
-	if (hrtimer_hres_active())
+	if (cpu_base->hres_active)
 		return;
 
 	for (index = 0; index < HRTIMER_MAX_CLOCK_BASES; index++) {

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-28 12:24 UTC #18 lore raw

Re: [PATCH 16/16] hrtimer: use base->hres_active directly instead of hrtimer_hres_active()

Quoted 10 lines · Viresh Kumar

kernel/hrtimer.c +1 -1

@@ -1107,7 +1107,7 @@ ktime_t hrtimer_get_next_event(void)
 
        raw_spin_lock_irqsave(&cpu_base->lock, flags);
 
-       if (!hrtimer_hres_active()) {
+       if (!cpu_base->hres_active) {
                for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++, base++) {
                        struct hrtimer *timer;
                        struct timerqueue_node *next;

kernel/hrtimer.c void hrtimer_run_queues(void) · line 1443

Quoted 8 lines

kernel/hrtimer.c +1 -1

@@ -1437,7 +1437,7 @@ void hrtimer_run_queues(void)
        struct hrtimer_clock_base *base;
        int index, gettime = 1;
 
-       if (hrtimer_hres_active())
+       if (cpu_base->hres_active)
                return;
 
        for (index = 0; index < HRTIMER_MAX_CLOCK_BASES; index++) {

These two changes are broken.. Would remove these and resend.. My tree is fixed though..

Viresh Kumar

author

2014-03-28 12:26 UTC #19 lore raw

PATCH v2 16/16 hrtimer: use base->hres_active directly instead of hrtimer_hres_active()

In reply to Viresh Kumar

use base->hres_active directly when we already have a pointer to base instead of calling hrtimer_hres_active(). As that would lead to:

__this_cpu_read(hrtimer_bases.hres_active)

Signed-off-by Viresh Kumar <[email protected]>

---
V1->V2: Removed few entries as hres_active isn't available if CONFIG_HIGH_RES_TIMERS isn't enabled.

 kernel/hrtimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/hrtimer.c +1 -1

index 83e5f2d..9e18145 100644
@@ -883,7 +883,7 @@ static void __remove_hrtimer(struct hrtimer *timer, unsigned long newstate,
 
 #ifdef CONFIG_HIGH_RES_TIMERS
 	/* Reprogram the clock event device. if enabled */
-	if (reprogram && hrtimer_hres_active() &&
+	if (reprogram && base->cpu_base->hres_active &&
 	    &timer->node == timerqueue_getnext(&base->active)) {
 		ktime_t expires;

Signature · 1 line

1.7.12.rc2.18.g61b472e

Thomas Gleixner

2014-03-31 13:38 UTC #20 lore raw

In reply to Viresh Kumar

Quoted 2 lines · Viresh Kumar

I have got few more cleanups other than what I posted yesterday:
https://lkml.org/lkml/2014/3/26/107

And those cleanups make the compiler generate worse code at least on x86_64:

   text	     data      bss      dec     hex    filename
   7475	     554       0	8029	1f5d   kernel/hrtimer.o
   7706	     554       0	8260	2044   kernel/hrtimer.o

So just removing parameters and return values because you can get the same information from a datastructure is not necessarily a good thing.

Thanks,

tglx

Viresh Kumar

author

2014-03-31 14:31 UTC #21 lore raw

Quoted 9 lines · Thomas Gleixner

And those cleanups make the compiler generate worse code at least on x86_64:
   text      data      bss      dec     hex    filename
   7475      554       0        8029    1f5d   kernel/hrtimer.o
   7706      554       0        8260    2044   kernel/hrtimer.o
So just removing parameters and return values because you can get the same information from a datastructure is not necessarily a good thing.

Hmm.. Nice.

Okay, I will have another look at patches and do this kind of investigation before sending it next time :) Its been fun going through these frameworks.

How do you want to proceed now? I mean, you will take the other patches (which don't play with function parameters) as is or want me to send a single unified patchset with all the pending patches that I have?

Thanks for your support :)

--
viresh

Thomas Gleixner

2014-03-31 15:51 UTC #22 lore raw

Quoted 11 lines · Viresh Kumar

Quoted 9 lines · Thomas Gleixner
And those cleanups make the compiler generate worse code at least on x86_64:
   text      data      bss      dec     hex    filename
   7475      554       0        8029    1f5d   kernel/hrtimer.o
   7706      554       0        8260    2044   kernel/hrtimer.o
So just removing parameters and return values because you can get the same information from a datastructure is not necessarily a good thing.

Quoted 8 lines

Hmm.. Nice.
Okay, I will have another look at patches and do this kind of investigation before sending it next time :) Its been fun going through these frameworks.
How do you want to proceed now? I mean, you will take the other patches (which don't play with function parameters) as is or want me to send a single unified patchset with all the pending patches that I have?

I still want to go through the lot and review them, but I wont take anything before the end of the merge window.

Thanks,

tglx

Viresh Kumar

author

2014-04-01 06:33 UTC #23 lore raw

In reply to Thomas Gleixner

Quoted 9 lines · Thomas Gleixner

And those cleanups make the compiler generate worse code at least on x86_64:
   text      data      bss      dec     hex    filename
   7475      554       0        8029    1f5d   kernel/hrtimer.o
   7706      554       0        8260    2044   kernel/hrtimer.o
So just removing parameters and return values because you can get the same information from a datastructure is not necessarily a good thing.

So here is the patch by patch analysis (With x86_64_defconfig):

Initial details:
text data bss dec hex filename
7989 408 0 8397 20cd ../bx86/kernel/hrtimer.o

Quoted 4 lines

73a6cd8 hrtimer: replace 'tab' with 'space' after comma ','
04223a8 hrtimer: Coalesce format fragments in printk()
52fac3f hrtimer: call hrtimer_set_expires_range() from hrtimer_set_expires_range_ns() 535a552 hrtimer: use base->index instead of basenum in switch_hrtimer_base()

7989 408 0 8397 20cd ../bx86/kernel/hrtimer.o

Quoted 1 line

f3a2cdd hrtimer: no need to rewrite '1' to hrtimer_hres_enabled

7974 408 0 8382 20be ../bx86/kernel/hrtimer.o

This one actually made a smaller :)

Quoted 1 line

479d66f hrtimer: don't rewrite same value to expires_next in hrtimer_force_reprogram()

7990 408 0 8398 20ce ../bx86/kernel/hrtimer.o

Quoted 1 line

0e134df hrtimer: use base->hres_active directly instead of hrtimer_hres_active()

8006 408 0 8414 20de ../bx86/kernel/hrtimer.o

So, reading from a per-cpu variable is efficient as compared to base->hres_active. In that case this patch can be dropped :)

Quoted 1 line

0030ddd hrtimer: remove dummy definition of hrtimer_force_reprogram()

8006 408 0 8414 20de ../bx86/kernel/hrtimer.o

Quoted 1 line

4a0c909 hrtimer: don't check state of base->hres_active in hrtimer_switch_to_hres()

8022 408 0 8430 20ee ../bx86/kernel/hrtimer.o

This one actually removed some code, how come it is bigger :(

Quoted 1 line

213bc01 hrtimer: remove clock_was_set_delayed() from hrtimer.h

8006 408 0 8414 20de ../bx86/kernel/hrtimer.o

Though this one made it shorter, we probably need to drop it as clock_was_set_delayed() is now used in kernel/time/timekeeping.c

Quoted 1 line

3defaf8 hrtimer: don't emulate notifier call to initialize timer base

8071 408 0 8479 211f ../bx86/kernel/hrtimer.o

I thought this will surely generate a smaller text segment :(

Quoted 1 line

e438576 hrtimer: use ffs() to iterate over valid bits from active_bases

8151 408 0 8559 216f ../bx86/kernel/hrtimer.o

Quoted 5 lines

1ab3d15 timer: simplify CPU_UP_PREPARE notifier code path c5111c9 timer: don't emulate notifier call to initialize timer base a576aa9 hrtimer: move unlock_hrtimer_base() upwards 9ea8619 hrtimer: reorder code in __remove_hrtimer() 2619470 hrtimer: Create hrtimer_get_monoexpires()

8135 408 0 8543 215f ../bx86/kernel/hrtimer.o

Quoted 1 line

b64d746 hrtimer: remove 'base' parameter from remove_timer() and __remove_timer()

8183 408 0 8591 218f ../bx86/kernel/hrtimer.o

Quoted 1 line

ced918e2 hrtimer: remove 'base' parameter from switch_hrtimer_base()

8135 408 0 8543 215f ../bx86/kernel/hrtimer.o

Quoted 1 line

f046106b hrtimer: remove 'base' parameter from enqueue_hrtimer()

8119 408 0 8527 214f ../bx86/kernel/hrtimer.o

Quoted 1 line

b102337 hrtimer: remove 'base' parameter from hrtimer_{enqueue_}reprogram()

8135 408 0 8543 215f ../bx86/kernel/hrtimer.o

So, these are the four patches that have removed parameters from routines. and it started with 8135 and ended up with 8135 :)

Quoted 1 line

d43b5d1 hrtimer: make switch_hrtimer_base() return void

8199 408 0 8607 219f ../bx86/kernel/hrtimer.o

Quoted 1 line

cd08cd9 hrtimer: make lock_hrtimer_base() return void

8199 408 0 8607 219f ../bx86/kernel/hrtimer.o

Quoted 1 line

74a3a18 hrtimer: make enqueue_hrtimer() return void

8167 408 0 8575 217f ../bx86/kernel/hrtimer.o

Quoted 1 line

b5cb057 hrtimer: don't check if timer is queued in __remove_hrtimer()

8135 408 0 8543 215f ../bx86/kernel/hrtimer.o

Quoted 5 lines

b56d3fc hrtimer: rewrite switch_hrtimer_base() to remove extra indentation level fccd30d hrtimer: rewrite remove_hrtimer() to remove extra indentation level d4a6c58 hrtimer: replace base by new_base to get resolution: __hrtimer_start_range_ns() 24bf5c0 hrtimer: create base_on_this_cpu()
3f690eb hrtimer: use base->hres_active directly instead of hrtimer_hres_active()

8135 408 0 8543 215f ../bx86/kernel/hrtimer.o

And finally we ended up at 8135 :)

Viresh Kumar

author

2014-04-01 07:02 UTC #24 lore raw

Quoted 17 lines · Viresh Kumar

So here is the patch by patch analysis (With x86_64_defconfig):
Initial details:
text data bss dec hex filename
7989 408 0 8397 20cd ../bx86/kernel/hrtimer.o
Quoted 4 lines
73a6cd8 hrtimer: replace 'tab' with 'space' after comma ','
04223a8 hrtimer: Coalesce format fragments in printk()
52fac3f hrtimer: call hrtimer_set_expires_range() from hrtimer_set_expires_range_ns() 535a552 hrtimer: use base->index instead of basenum in switch_hrtimer_base()
7989 408 0 8397 20cd ../bx86/kernel/hrtimer.o
Quoted 1 line
f3a2cdd hrtimer: no need to rewrite '1' to hrtimer_hres_enabled
7974 408 0 8382 20be ../bx86/kernel/hrtimer.o
This one actually made a smaller :)

Quoted 7 lines

Quoted 1 line
479d66f hrtimer: don't rewrite same value to expires_next in hrtimer_force_reprogram()
7990 408 0 8398 20ce ../bx86/kernel/hrtimer.o
Quoted 1 line
0e134df hrtimer: use base->hres_active directly instead of hrtimer_hres_active()
8006 408 0 8414 20de ../bx86/kernel/hrtimer.o
So, reading from a per-cpu variable is efficient as compared to base->hres_active. In that case this patch can be dropped :)

Its a bit awkward now. When I keep this patch at this location it exactly generates above numbers, i.e. 7990 -> 8006.

But if I revert this patch over top of all 30 commits I have, it doesn't make a difference. Even applying it again over the revert doesn't make a difference.

There are no other changes to the concerned routine, retrigger_next_event() in my commits..