[PATCH 00/16] timers/hrtimers: Minor cleanups: Part II

[PATCH 00/16] timers/hrtimers: Minor cleanups: Part II

[Viresh Kumar]

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(-)

-- 
1.7.12.rc2.18.g61b472e

[PATCH 01/16] hrtimer: move unlock_hrtimer_base() upwards

[Viresh Kumar]

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 <viresh.kumar@linaro.org>
---
 kernel/hrtimer.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index e486a14..a710638 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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
-- 
1.7.12.rc2.18.g61b472e

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

[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 <viresh.kumar@linaro.org>
---
 kernel/hrtimer.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index a710638..6476152 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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:
-- 
1.7.12.rc2.18.g61b472e

[PATCH 03/16] hrtimer: Create hrtimer_get_monoexpires()

[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 <viresh.kumar@linaro.org>
---
 include/linux/hrtimer.h |  6 ++++++
 kernel/hrtimer.c        | 11 +++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index 6f524db..377023b 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -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;
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 6476152..675fe9e 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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)
-- 
1.7.12.rc2.18.g61b472e

[PATCH 04/16] hrtimer: remove 'base' parameter from remove_timer() and __remove_timer()

[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 <viresh.kumar@linaro.org>
---
 kernel/hrtimer.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 675fe9e..343fe99 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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
-- 
1.7.12.rc2.18.g61b472e

[PATCH 05/16] hrtimer: remove 'base' parameter from switch_hrtimer_base()

[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 <viresh.kumar@linaro.org>
---
 kernel/hrtimer.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 343fe99..9561336 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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());
-- 
1.7.12.rc2.18.g61b472e

[PATCH 06/16] hrtimer: remove 'base' parameter from enqueue_hrtimer()

[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 <viresh.kumar@linaro.org>
---
 kernel/hrtimer.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 9561336..d6724b5 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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;
-- 
1.7.12.rc2.18.g61b472e

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

[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 <viresh.kumar@linaro.org>
---
 kernel/hrtimer.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index d6724b5..98a73d9 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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
-- 
1.7.12.rc2.18.g61b472e

[PATCH 08/16] hrtimer: make switch_hrtimer_base() return void

[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 <viresh.kumar@linaro.org>
---
 kernel/hrtimer.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 98a73d9..d98c1ec 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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());
-- 
1.7.12.rc2.18.g61b472e

[PATCH 09/16] hrtimer: make lock_hrtimer_base() return void

[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 <viresh.kumar@linaro.org>
---
 kernel/hrtimer.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index d98c1ec..7d85b8f 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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);
-- 
1.7.12.rc2.18.g61b472e

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

[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 <viresh.kumar@linaro.org>
---
 include/linux/hrtimer.h |  5 +++++
 kernel/hrtimer.c        | 11 +++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index 377023b..7ca9fd0 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -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;
 
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 7d85b8f..2702185 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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) {
 			/*
-- 
1.7.12.rc2.18.g61b472e

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

[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 <viresh.kumar@linaro.org>
---
 kernel/hrtimer.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 2702185..04f8e44 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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 */
-- 
1.7.12.rc2.18.g61b472e

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

[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 <viresh.kumar@linaro.org>
---
 kernel/hrtimer.c | 48 ++++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 04f8e44..30efa1c 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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 */
-- 
1.7.12.rc2.18.g61b472e

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

[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 <viresh.kumar@linaro.org>
---
 kernel/hrtimer.c | 47 ++++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 25 deletions(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 30efa1c..0ae0fbf 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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,
-- 
1.7.12.rc2.18.g61b472e

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

[Viresh Kumar]

This code was added long back by following commit:

	commit 06027bdd278a32a84b273e41db68a5db8ffd2bb6
	Author: Ingo Molnar <mingo@elte.hu>
	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 <mingo@redhat.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
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(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 0ae0fbf..5d77f36 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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
 	}
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH 15/16] hrtimer: create base_on_this_cpu()

[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 <viresh.kumar@linaro.org>
---
 kernel/hrtimer.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 5d77f36..83e5f2d 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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
-- 
1.7.12.rc2.18.g61b472e

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

[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 <viresh.kumar@linaro.org>
---
 kernel/hrtimer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 83e5f2d..f379821 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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++) {
-- 
1.7.12.rc2.18.g61b472e

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

[Viresh Kumar]

On 28 March 2014 17:11, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> @@ -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++) {

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

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

[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 <viresh.kumar@linaro.org>
---
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(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 83e5f2d..9e18145 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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;
 
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 00/16] timers/hrtimers: Minor cleanups: Part II

[Thomas Gleixner]

On Fri, 28 Mar 2014, Viresh Kumar wrote:
> 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

Re: [PATCH 00/16] timers/hrtimers: Minor cleanups: Part II

[Viresh Kumar]

On 31 March 2014 19:08, Thomas Gleixner <tglx@linutronix.de> wrote:
> 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

Re: [PATCH 00/16] timers/hrtimers: Minor cleanups: Part II

[Thomas Gleixner]

On Mon, 31 Mar 2014, Viresh Kumar wrote:
> On 31 March 2014 19:08, Thomas Gleixner <tglx@linutronix.de> wrote:
> > 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?

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

Re: [PATCH 00/16] timers/hrtimers: Minor cleanups: Part II

[Viresh Kumar]

On 31 March 2014 19:08, Thomas Gleixner <tglx@linutronix.de> wrote:
> 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

> 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

> 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 :)

> 479d66f hrtimer: don't rewrite same value to expires_next in hrtimer_force_reprogram()
   7990    408      0   8398   20ce ../bx86/kernel/hrtimer.o
> 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 :)

> 0030ddd hrtimer: remove dummy definition of hrtimer_force_reprogram()
   8006    408      0   8414   20de ../bx86/kernel/hrtimer.o
> 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 :(

> 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

> 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 :(

> e438576 hrtimer: use ffs() to iterate over valid bits from active_bases
   8151    408      0   8559   216f ../bx86/kernel/hrtimer.o
> 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

> b64d746 hrtimer: remove 'base' parameter from remove_timer() and __remove_timer()
   8183    408      0   8591   218f ../bx86/kernel/hrtimer.o
> ced918e2 hrtimer: remove 'base' parameter from switch_hrtimer_base()
   8135    408      0   8543   215f ../bx86/kernel/hrtimer.o
> f046106b hrtimer: remove 'base' parameter from enqueue_hrtimer()
   8119    408      0   8527   214f ../bx86/kernel/hrtimer.o
> 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 :)

> d43b5d1 hrtimer: make switch_hrtimer_base() return void
   8199    408      0   8607   219f ../bx86/kernel/hrtimer.o
> cd08cd9 hrtimer: make lock_hrtimer_base() return void
   8199    408      0   8607   219f ../bx86/kernel/hrtimer.o
> 74a3a18 hrtimer: make enqueue_hrtimer() return void
   8167    408      0   8575   217f ../bx86/kernel/hrtimer.o
> b5cb057 hrtimer: don't check if timer is queued in __remove_hrtimer()
   8135    408      0   8543   215f ../bx86/kernel/hrtimer.o
> 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 :)

Re: [PATCH 00/16] timers/hrtimers: Minor cleanups: Part II

[Viresh Kumar]

On 1 April 2014 12:03, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 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
>
>> 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
>
>> 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 :)
>
>> 479d66f hrtimer: don't rewrite same value to expires_next in hrtimer_force_reprogram()
>    7990    408      0   8398   20ce ../bx86/kernel/hrtimer.o
>> 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..

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

[Viresh Kumar]

On 28 March 2014 17:11, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c

> @@ -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)) {

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:

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 516c67f..189c525 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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);