timers/hrtimers: Minor cleanups

PATCH Superseded Viresh Kumar 3 participants 32 messages Mar 26, 2014 – Mar 27, 2014 High-resolution timers, timer wheel, clockevents

lore

Share & tools

lore

Viresh Kumar

author

2014-03-26 11:21 UTC #0 lore raw

cover timers/hrtimers: Minor cleanups

Hi Thomas,

I am going through this piece of code to complete my 'cpusets.quiesce' work. While going through code I accumulated these patches which are mostly code cleanups and shouldn't have much functional change.

Thanks for applying yesterdays cleanups :)

Viresh Kumar (14):
hrtimer: replace 'tab' with 'space' after comma ','
hrtimer: Coalesce format fragments in printk()
hrtimer: call hrtimer_set_expires_range() from
hrtimer_set_expires_range_ns()
hrtimer: use base->index instead of basenum in switch_hrtimer_base()
hrtimer: no need to rewrite '1' to hrtimer_hres_enabled
hrtimer: don't rewrite same value to expires_next in
hrtimer_force_reprogram()
hrtimer: use base->hres_active directly instead of
hrtimer_hres_active()
hrtimer: remove dummy definition of hrtimer_force_reprogram()
hrtimer: don't check state of base->hres_active in
hrtimer_switch_to_hres()
hrtimer: remove clock_was_set_delayed() from hrtimer.h
hrtimer: remove active_bases field from struct hrtimer_cpu_base
hrtimer: don't emulate notifier call to initialize timer base
timer: simplify CPU_UP_PREPARE notifier code path
timer: don't emulate notifier call to initialize timer base

 include/linux/hrtimer.h | 10 +---------
 kernel/hrtimer.c        | 40 ++++++++++++++--------------------------
 kernel/timer.c          | 12 ++----------
 3 files changed, 17 insertions(+), 45 deletions(-)

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-26 11:22 UTC #1 lore raw

PATCH 1/14 hrtimer: replace 'tab' with 'space' after 'comma'

Currently we have a 'tab' here instead of 'space' after 'comma'. Replace it with 'space'.

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

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

kernel/hrtimer.c +1 -1

index d55092c..a1120a0 100644
@@ -128,7 +128,7 @@ static void hrtimer_get_softirq_time(struct hrtimer_cpu_base *base)
 	base->clock_base[HRTIMER_BASE_MONOTONIC].softirq_time = mono;
 	base->clock_base[HRTIMER_BASE_BOOTTIME].softirq_time = boot;
 	base->clock_base[HRTIMER_BASE_TAI].softirq_time =
-				ktime_add(xtim,	ktime_set(tai_offset, 0));
+				ktime_add(xtim, ktime_set(tai_offset, 0));
 }
 
 /*

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-26 11:25 UTC #2 lore raw

PATCH 2/14 hrtimer: Coalesce format fragments in printk()

In reply to Viresh Kumar

Breaking format fragments into multiple lines hits readability of code. Even if it goes over 80 column width, its better to keep them together.

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

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

kernel/hrtimer.c +2 -2

index a1120a0..95243f2 100644
@@ -695,8 +695,8 @@ static int hrtimer_switch_to_hres(void)
 
 	if (tick_init_highres()) {
 		local_irq_restore(flags);
-		printk(KERN_WARNING "Could not switch to high resolution "
-				    "mode on CPU %d\n", cpu);
+		printk(KERN_WARNING "Could not switch to high resolution mode on CPU %d\n",
+			cpu);
 		return 0;
 	}
 	base->hres_active = 1;

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-26 11:22 UTC #3 lore raw

PATCH 3/14 hrtimer: call hrtimer_set_expires_range() from hrtimer_set_expires_range_ns()

In reply to Viresh Kumar

hrtimer_set_expires_range() and hrtimer_set_expires_range_ns() have almost same implementations and so we can call hrtimer_set_expires_range() from hrtimer_set_expires_range_ns() internally, instead of duplicating code.

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

---
 include/linux/hrtimer.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

include/linux/hrtimer.h +1 -2

index e7a8d3f..17c08ca 100644
@@ -207,8 +207,7 @@ static inline void hrtimer_set_expires_range(struct hrtimer *timer, ktime_t time
 
 static inline void hrtimer_set_expires_range_ns(struct hrtimer *timer, ktime_t time, unsigned long delta)
 {
-	timer->_softexpires = time;
-	timer->node.expires = ktime_add_safe(time, ns_to_ktime(delta));
+	hrtimer_set_expires_range(timer, time, ns_to_ktime(delta));
 }
 
 static inline void hrtimer_set_expires_tv64(struct hrtimer *timer, s64 tv64)

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-26 11:22 UTC #4 lore raw

PATCH 4/14 hrtimer: use base->index instead of basenum in switch_hrtimer_base()

In reply to Viresh Kumar

In switch_hrtimer_base() we have created a local variable basenum which is set to base->index. This variable is used at only one place. It makes code more readable if we remove this variable use base->index directly.

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

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

kernel/hrtimer.c +1 -2

index 95243f2..b0bcc10 100644
@@ -202,11 +202,10 @@ switch_hrtimer_base(struct hrtimer *timer, struct hrtimer_clock_base *base,
 	struct hrtimer_cpu_base *new_cpu_base;
 	int this_cpu = smp_processor_id();
 	int cpu = get_nohz_timer_target(pinned);
-	int basenum = base->index;
 
 again:
 	new_cpu_base = &per_cpu(hrtimer_bases, cpu);
-	new_base = &new_cpu_base->clock_base[basenum];
+	new_base = &new_cpu_base->clock_base[base->index];
 
 	if (base != new_base) {
 		/*

Signature · 1 line

1.7.12.rc2.18.g61b472e

Srivatsa S. Bhat

2014-03-26 11:43 UTC #5 lore raw

Re: [PATCH 04/14] hrtimer: use base->index instead of basenum in switch_hrtimer_base()

Quoted 4 lines · Viresh Kumar

In switch_hrtimer_base() we have created a local variable basenum which is set to base->index. This variable is used at only one place. It makes code more readable if we remove this variable use base->index directly.

No, this doesn't look right. Note that the code can re-execute the assignment to new_base, by jumping to the 'again' label. See below.

kernel/hrtimer.c switch_hrtimer_base(struct hrtimer *timer, struct hrtimer_clock_base *base, · line 209

Quoted 13 lines

kernel/hrtimer.c +1 -2

@@ -202,11 +202,10 @@ switch_hrtimer_base(struct hrtimer *timer, struct hrtimer_clock_base *base,
 	struct hrtimer_cpu_base *new_cpu_base;
 	int this_cpu = smp_processor_id();
 	int cpu = get_nohz_timer_target(pinned);
-	int basenum = base->index;
 
 again:
 	new_cpu_base = &per_cpu(hrtimer_bases, cpu);
-	new_base = &new_cpu_base->clock_base[basenum];
+	new_base = &new_cpu_base->clock_base[base->index];

Further down, timer->base can be altered (and set to NULL too). So if we jump back to 'again', we'll end up in trouble. So I think its important to cache the value in basenum and use it.

kernel/hrtimer.c switch_hrtimer_base(struct hrtimer *timer, struct hrtimer_clock_base *base, · line 211

Quoted 3 lines

kernel/hrtimer.c +0 -0

 	if (base != new_base) {
 		/*

Regards,
Srivatsa S. Bhat

Viresh Kumar

author

2014-03-26 14:08 UTC #6 lore raw

Re: [LNG] Re: [PATCH 04/14] hrtimer: use base->index instead of basenum in switch_hrtimer_base()

Quoted 6 lines · Srivatsa S. Bhat

Quoted 1 line
+ new_base = &new_cpu_base->clock_base[base->index];
Further down, timer->base can be altered (and set to NULL too). So if we jump back to 'again', we'll end up in trouble. So I think its important to cache the value in basenum and use it.

base is a parameter to this function and never changes. So base->index is guaranteed to be valid and same during a functions call.

Thomas Gleixner

2014-03-26 17:31 UTC #7 lore raw

Re: [PATCH 04/14] hrtimer: use base->index instead of basenum in switch_hrtimer_base()

In reply to Srivatsa S. Bhat

Quoted 18 lines · Srivatsa S. Bhat

Quoted 4 lines · Viresh Kumar
In switch_hrtimer_base() we have created a local variable basenum which is set to base->index. This variable is used at only one place. It makes code more readable if we remove this variable use base->index directly.
No, this doesn't look right. Note that the code can re-execute the assignment to new_base, by jumping to the 'again' label. See below.
Quoted 8 lines
kernel/hrtimer.c +0 -1
@@ -202,11 +202,10 @@ switch_hrtimer_base(struct hrtimer *timer, struct hrtimer_clock_base *base,
 	struct hrtimer_cpu_base *new_cpu_base;
 	int this_cpu = smp_processor_id();
 	int cpu = get_nohz_timer_target(pinned);
-	int basenum = base->index;
 

Quoted 10 lines

kernel/hrtimer.c switch_hrtimer_base(struct hrtimer *timer, struct hrtimer_clock_base *base, · line 209
Quoted 5 lines
kernel/hrtimer.c +1 -1
 again:
 	new_cpu_base = &per_cpu(hrtimer_bases, cpu);
-	new_base = &new_cpu_base->clock_base[basenum];
+	new_base = &new_cpu_base->clock_base[base->index];
 
Further down, timer->base can be altered (and set to NULL too). So if we jump back to 'again', we'll end up in trouble. So I think its important to cache the value in basenum and use it.

That's irrelevant. base is not changing.

Srivatsa S. Bhat

2014-03-26 17:58 UTC #8 lore raw

Re: [PATCH 04/14] hrtimer: use base->index instead of basenum in switch_hrtimer_base()

Quoted 30 lines · Thomas Gleixner

Quoted 18 lines · Srivatsa S. Bhat
Quoted 4 lines · Viresh Kumar
In switch_hrtimer_base() we have created a local variable basenum which is set to base->index. This variable is used at only one place. It makes code more readable if we remove this variable use base->index directly.
No, this doesn't look right. Note that the code can re-execute the assignment to new_base, by jumping to the 'again' label. See below.
Quoted 8 lines
kernel/hrtimer.c +0 -1
@@ -202,11 +202,10 @@ switch_hrtimer_base(struct hrtimer *timer, struct hrtimer_clock_base *base,
 	struct hrtimer_cpu_base *new_cpu_base;
 	int this_cpu = smp_processor_id();
 	int cpu = get_nohz_timer_target(pinned);
-	int basenum = base->index;
 
Quoted 10 lines
kernel/hrtimer.c switch_hrtimer_base(struct hrtimer *timer, struct hrtimer_clock_base *base, · line 209
Quoted 5 lines
kernel/hrtimer.c +1 -1
 again:
 	new_cpu_base = &per_cpu(hrtimer_bases, cpu);
-	new_base = &new_cpu_base->clock_base[basenum];
+	new_base = &new_cpu_base->clock_base[base->index];
 
Further down, timer->base can be altered (and set to NULL too). So if we jump back to 'again', we'll end up in trouble. So I think its important to cache the value in basenum and use it.

Quoted 2 lines

That's irrelevant. base is not changing.

Sorry, I missed that :-(

Regards,
Srivatsa S. Bhat

Viresh Kumar

author

2014-03-26 11:22 UTC #9 lore raw

PATCH 5/14 hrtimer: no need to rewrite '1' to hrtimer_hres_enabled

In reply to Viresh Kumar

High Resolution feature can be enabled/disabled from bootargs if we have a string 'highres=' followed by 'on' or 'off'. The default value of this variable is '1'. When 'on' is passed as bootarg, we don't have to overwrite this variable by '1'.

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

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

kernel/hrtimer.c +1 -3

index b0bcc10..e5d81ee 100644
@@ -503,9 +503,7 @@ static int __init setup_hrtimer_hres(char *str)
 {
 	if (!strcmp(str, "off"))
 		hrtimer_hres_enabled = 0;
-	else if (!strcmp(str, "on"))
-		hrtimer_hres_enabled = 1;
-	else
+	else if (strcmp(str, "on"))
 		return 0;
 	return 1;
 }

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-26 11:22 UTC #10 lore raw

PATCH 6/14 hrtimer: don't rewrite same value to expires_next in hrtimer_force_reprogram()

In reply to Viresh Kumar

We have just checked that expires_next.tv64 == cpu_base->expires_next.tv64, and in this case we shouldn't rewrite the same value again. Rewrite code to fix this.

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

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

kernel/hrtimer.c +3 -3

index e5d81ee..32d1504 100644
@@ -561,11 +561,11 @@ hrtimer_force_reprogram(struct hrtimer_cpu_base *cpu_base, int skip_equal)
 			expires_next = expires;
 	}
 
-	if (skip_equal && expires_next.tv64 == cpu_base->expires_next.tv64)
+	if (expires_next.tv64 != cpu_base->expires_next.tv64)
+		cpu_base->expires_next.tv64 = expires_next.tv64;
+	else if (skip_equal)
 		return;
 
-	cpu_base->expires_next.tv64 = expires_next.tv64;
-
 	if (cpu_base->expires_next.tv64 != KTIME_MAX)
 		tick_program_event(cpu_base->expires_next, 1);
 }

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-26 11:22 UTC #11 lore raw

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

In reply to Viresh Kumar

retrigger_next_event() is defined within #ifdef CONFIG_HIGH_RES_TIMERS and we already have pointer to base available. So it makes more sense to simply use base->hres_active instead of doing this by calling hrtimer_hres_active():

__this_cpu_read(hrtimer_bases.hres_active)

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

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

kernel/hrtimer.c +1 -1

index 32d1504..cb485b9 100644
@@ -667,7 +667,7 @@ static void retrigger_next_event(void *arg)
 {
 	struct hrtimer_cpu_base *base = &__get_cpu_var(hrtimer_bases);
 
-	if (!hrtimer_hres_active())
+	if (!base->hres_active)
 		return;
 
 	raw_spin_lock(&base->lock);

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-26 11:24 UTC #12 lore raw

PATCH 8/14 hrtimer: remove dummy definition of hrtimer_force_reprogram()

In reply to Viresh Kumar

hrtimer_force_reprogram() is only called from parts of kernel which are defined within #ifdef CONFIG_HIGH_RES_TIMERS and so its empty definition is never used. Remove it.

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

---
 kernel/hrtimer.c | 2 --
 1 file changed, 2 deletions(-)

kernel/hrtimer.c +0 -2

index cb485b9..5b3b212 100644
@@ -728,8 +728,6 @@ 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 void
-hrtimer_force_reprogram(struct hrtimer_cpu_base *base, int skip_equal) { }
 static inline int hrtimer_enqueue_reprogram(struct hrtimer *timer,
 					    struct hrtimer_clock_base *base)
 {

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-26 11:22 UTC #13 lore raw

PATCH 9/14 hrtimer: don't check state of base->hres_active in hrtimer_switch_to_hres()

In reply to Viresh Kumar

Caller of hrtimer_switch_to_hres(), i.e. hrtimer_run_pending(), has already verified this by calling hrtimer_hres_active() and so we don't need to do it again in hrtimer_switch_to_hres().

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

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

kernel/hrtimer.c +0 -3

index 5b3b212..e6e1255 100644
@@ -685,9 +685,6 @@ static int hrtimer_switch_to_hres(void)
 	struct hrtimer_cpu_base *base = &per_cpu(hrtimer_bases, cpu);
 	unsigned long flags;
 
-	if (base->hres_active)
-		return 1;
-
 	local_irq_save(flags);
 
 	if (tick_init_highres()) {

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-26 11:22 UTC #14 lore raw

PATCH 10/14 hrtimer: remove clock_was_set_delayed() from hrtimer.h

In reply to Viresh Kumar

clock_was_set_delayed() is called from only hrtimer.c and so should be marked static. Along with that its declaration and dummy definition must be removed from hrtimer.h.

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

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

include/linux/hrtimer.h +0 -5

index 17c08ca..6f524db 100644
@@ -288,8 +288,6 @@ extern void hrtimer_peek_ahead_timers(void);
 # define MONOTONIC_RES_NSEC	HIGH_RES_NSEC
 # define KTIME_MONOTONIC_RES	KTIME_HIGH_RES
 
-extern void clock_was_set_delayed(void);
-
 #else
 
 # define MONOTONIC_RES_NSEC	LOW_RES_NSEC

@@ -310,9 +308,6 @@ static inline int hrtimer_is_hres_active(struct hrtimer *timer)
 {
 	return 0;
 }
-
-static inline void clock_was_set_delayed(void) { }
-
 #endif
 
 extern void clock_was_set(void);

kernel/hrtimer.c +2 -1

index e6e1255..1e4eedb 100644
@@ -715,7 +715,7 @@ static DECLARE_WORK(hrtimer_work, clock_was_set_work);
  * Called from timekeeping and resume code to reprogramm the hrtimer
  * interrupt device on all cpus.
  */
-void clock_was_set_delayed(void)
+static void clock_was_set_delayed(void)
 {
 	schedule_work(&hrtimer_work);
 }

@@ -732,6 +732,7 @@ static inline int hrtimer_enqueue_reprogram(struct hrtimer *timer,
 }
 static inline void hrtimer_init_hres(struct hrtimer_cpu_base *base) { }
 static inline void retrigger_next_event(void *arg) { }
+static inline void clock_was_set_delayed(void) { }
 
 #endif /* CONFIG_HIGH_RES_TIMERS */

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-26 11:22 UTC #15 lore raw

PATCH 11/14 hrtimer: remove active_bases field from struct hrtimer_cpu_base

In reply to Viresh Kumar

Active_bases field of struct hrtimer_cpu_base is used at only one place, i.e. hrtimer_interrupt() and at that place too we can easily use timerqueue_getnext() instead to achieve the same result. I don't think this will have any performance degradation issues and so removing this field.

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

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

include/linux/hrtimer.h +0 -2

index 6f524db..cb6a315 100644
@@ -165,7 +165,6 @@ enum  hrtimer_base_type {
  * struct hrtimer_cpu_base - the per cpu clock bases
  * @lock:		lock protecting the base and associated clock bases
  *			and timers
- * @active_bases:	Bitfield to mark bases with active timers
  * @clock_was_set:	Indicates that clock was set from irq context.
  * @expires_next:	absolute time of the next event which was scheduled
  *			via clock_set_next_event()

@@ -179,7 +178,6 @@ enum  hrtimer_base_type {
  */
 struct hrtimer_cpu_base {
 	raw_spinlock_t			lock;
-	unsigned int			active_bases;
 	unsigned int			clock_was_set;
 #ifdef CONFIG_HIGH_RES_TIMERS
 	ktime_t				expires_next;

kernel/hrtimer.c +2 -6

index 1e4eedb..f14d861 100644
@@ -865,7 +865,6 @@ static int enqueue_hrtimer(struct hrtimer *timer,
 	debug_activate(timer);
 
 	timerqueue_add(&base->active, &timer->node);
-	base->cpu_base->active_bases |= 1 << base->index;
 
 	/*
 	 * HRTIMER_STATE_ENQUEUED is or'ed to the current state to preserve the

@@ -909,8 +908,6 @@ static void __remove_hrtimer(struct hrtimer *timer,
 		}
 #endif
 	}
-	if (!timerqueue_getnext(&base->active))
-		base->cpu_base->active_bases &= ~(1 << base->index);
 out:
 	timer->state = newstate;
 }

@@ -1284,14 +1281,13 @@ retry:
 	cpu_base->expires_next.tv64 = KTIME_MAX;
 
 	for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) {
-		struct hrtimer_clock_base *base;
+		struct hrtimer_clock_base *base = cpu_base->clock_base + i;
 		struct timerqueue_node *node;
 		ktime_t basenow;
 
-		if (!(cpu_base->active_bases & (1 << i)))
+		if (!timerqueue_getnext(&base->active))
 			continue;
 
-		base = cpu_base->clock_base + i;
 		basenow = ktime_add(now, base->offset);
 
 		while ((node = timerqueue_getnext(&base->active))) {

Signature · 1 line

1.7.12.rc2.18.g61b472e

Thomas Gleixner

2014-03-26 17:28 UTC #16 lore raw

Re: [PATCH 11/14] hrtimer: remove active_bases field from struct hrtimer_cpu_base

Quoted 4 lines · Viresh Kumar

Active_bases field of struct hrtimer_cpu_base is used at only one place, i.e. hrtimer_interrupt() and at that place too we can easily use timerqueue_getnext() instead to achieve the same result. I don't think this will have any performance degradation issues and so removing this field.

Instead of removing it we should actually use ffs and avoid the whole looping. That was the intention in the first place, but I never wrote the patch...

Thanks,

tglx

Viresh Kumar

author

2014-03-27 04:30 UTC #17 lore raw

Re: [PATCH 11/14] hrtimer: remove active_bases field from struct hrtimer_cpu_base

Quoted 3 lines · Thomas Gleixner

Instead of removing it we should actually use ffs and avoid the whole looping. That was the intention in the first place, but I never wrote the patch...

I thought about that and then using ffs for a field of which only 4 bits are useful didn't looked too convincing to me :)

But, probably it will make things slightly better in case this routine is heavily used.

Viresh Kumar

author

2014-03-26 11:23 UTC #18 lore raw

PATCH 12/14 hrtimer: don't emulate notifier call to initialize timer base

In reply to Viresh Kumar

In hrtimers_init() we need to call init_hrtimers_cpu() for boot CPU. For this, currently we are emulating a call to hotplug notifier. Probably this was done initially to get rid of code redundancy. But this sequence always called a single routine, i.e. init_hrtimers_cpu(), and so calling that routine directly would be better. This would get rid of emulating a notifier call, few typecasts and the extra steps we are doing in notifier callback.

So, this patch calls init_hrtimers_cpu() directly from hrtimers_init().

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

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

kernel/hrtimer.c +1 -2

index f14d861..39dbdbd 100644
@@ -1756,8 +1756,7 @@ static struct notifier_block hrtimers_nb = {
 
 void __init hrtimers_init(void)
 {
-	hrtimer_cpu_notify(&hrtimers_nb, (unsigned long)CPU_UP_PREPARE,
-			  (void *)(long)smp_processor_id());
+	init_hrtimers_cpu(smp_processor_id());
 	register_cpu_notifier(&hrtimers_nb);
 #ifdef CONFIG_HIGH_RES_TIMERS
 	open_softirq(HRTIMER_SOFTIRQ, run_hrtimer_softirq);

Signature · 1 line

1.7.12.rc2.18.g61b472e

Srivatsa S. Bhat

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

Re: [PATCH 12/14] hrtimer: don't emulate notifier call to initialize timer base

Quoted 9 lines · Viresh Kumar

In hrtimers_init() we need to call init_hrtimers_cpu() for boot CPU. For this, currently we are emulating a call to hotplug notifier. Probably this was done initially to get rid of code redundancy. But this sequence always called a single routine, i.e. init_hrtimers_cpu(), and so calling that routine directly would be better. This would get rid of emulating a notifier call, few typecasts and the extra steps we are doing in notifier callback.
So, this patch calls init_hrtimers_cpu() directly from hrtimers_init().

I don't think this is such a good idea. Open-coding a part of that callback in the init routine can lead to loop-holes down the road: what if someone changes or adds something to the CPU_UP_PREPARE switch-case, and forgets to do the same in the init-routine?

It is more comforting to know that there is just one single place where CPU hotplug operations are handled (hrtimer_cpu_notify). That, in turn is good for reliability because it makes it easier to write bug-free code.

Regards,
Srivatsa S. Bhat

Quoted 20 lines

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

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

kernel/hrtimer.c +1 -2

index f14d861..39dbdbd 100644
@@ -1756,8 +1756,7 @@ static struct notifier_block hrtimers_nb = {
 
 void __init hrtimers_init(void)
 {
-	hrtimer_cpu_notify(&hrtimers_nb, (unsigned long)CPU_UP_PREPARE,
-			  (void *)(long)smp_processor_id());
+	init_hrtimers_cpu(smp_processor_id());
 	register_cpu_notifier(&hrtimers_nb);
 #ifdef CONFIG_HIGH_RES_TIMERS
 	open_softirq(HRTIMER_SOFTIRQ, run_hrtimer_softirq);

Viresh Kumar

author

2014-03-26 14:17 UTC #20 lore raw

Re: [LNG] Re: [PATCH 12/14] hrtimer: don't emulate notifier call to initialize timer base

Quoted 2 lines · Srivatsa S. Bhat

I don't think this is such a good idea. Open-coding a part of that callback in the init routine can lead to loop-holes down the road:

We think that we are open-coding part of that callback here because it is implemented that way on the first design.

Rather, we should have a common routine which should do all the work required when a CPU comes up. And any modification should be done to that code.

Quoted 3 lines

what if someone
changes or adds something to the CPU_UP_PREPARE switch-case, and forgets to do the same in the init-routine?

This is not a driver which only 2-3 people use. This part is so well reviewed by so many highly smart people that this should never happen. And if it happens than its nothing but a review mistake.

Quoted 3 lines

It is more comforting to know that there is just one single place where CPU hotplug operations are handled (hrtimer_cpu_notify). That, in turn is good for reliability because it makes it easier to write bug-free code.

And for me that single place is: init_hrtimers_cpu() :)

Viresh Kumar

author

2014-03-26 11:23 UTC #21 lore raw

PATCH 13/14 timer: simplify CPU_UP_PREPARE notifier code path

In reply to Viresh Kumar

Currently we are returning notifier_from_errno() from CPU_UP_PREPARE notifier when we detect an error while calling init_timers_cpu(). notifier_from_errno() already has enough checks within to do something similar. And so we can call it directly without checking if there was an error or not.

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

---
 kernel/timer.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

kernel/timer.c +1 -3

index 1d35dda..4360edc 100644
@@ -1646,9 +1646,7 @@ static int timer_cpu_notify(struct notifier_block *self,
 	case CPU_UP_PREPARE:
 	case CPU_UP_PREPARE_FROZEN:
 		err = init_timers_cpu(cpu);
-		if (err < 0)
-			return notifier_from_errno(err);
-		break;
+		return notifier_from_errno(err);
 #ifdef CONFIG_HOTPLUG_CPU
 	case CPU_DEAD:
 	case CPU_DEAD_FROZEN:

Signature · 1 line

1.7.12.rc2.18.g61b472e

Srivatsa S. Bhat

Reviewed-by on v1 13/14

2014-03-26 12:47 UTC #22 lore raw

Re: [PATCH 13/14] timer: simplify CPU_UP_PREPARE notifier code path

Quoted 6 lines · Viresh Kumar

Currently we are returning notifier_from_errno() from CPU_UP_PREPARE notifier when we detect an error while calling init_timers_cpu(). notifier_from_errno() already has enough checks within to do something similar. And so we can call it directly without checking if there was an error or not.
Signed-off-by Viresh Kumar <[email protected]>

Reviewed-by Srivatsa S. Bhat <[email protected]>

Regards,
Srivatsa S. Bhat

Quoted 20 lines

---
 kernel/timer.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

kernel/timer.c +1 -3

index 1d35dda..4360edc 100644
@@ -1646,9 +1646,7 @@ static int timer_cpu_notify(struct notifier_block *self,
 	case CPU_UP_PREPARE:
 	case CPU_UP_PREPARE_FROZEN:
 		err = init_timers_cpu(cpu);
-		if (err < 0)
-			return notifier_from_errno(err);
-		break;
+		return notifier_from_errno(err);
 #ifdef CONFIG_HOTPLUG_CPU
 	case CPU_DEAD:
 	case CPU_DEAD_FROZEN:

Viresh Kumar

author

2014-03-26 11:23 UTC #23 lore raw

PATCH 14/14 timer: don't emulate notifier call to initialize timer base

In reply to Viresh Kumar

In init_timers() we need to call init_timers_cpu() for boot CPU. For this, currently we are emulating a call to hotplug notifier. Probably this was done initially to get rid of code redundancy. But this sequence always called a single routine, i.e. init_timers_cpu(), and so calling that routine directly would be better. This would get rid of emulating a notifier call, few typecasts and the extra steps we are doing in notifier callback.

So, this patch calls init_timers_cpu() directly from init_timers().

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

---
 kernel/timer.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

kernel/timer.c +1 -7

index 4360edc..d13eb56 100644
@@ -1666,15 +1666,9 @@ static struct notifier_block timers_nb = {
 
 void __init init_timers(void)
 {
-	int err;
-
 	/* ensure there are enough low bits for flags in timer->base pointer */
 	BUILD_BUG_ON(__alignof__(struct tvec_base) & TIMER_FLAG_MASK);
-
-	err = timer_cpu_notify(&timers_nb, (unsigned long)CPU_UP_PREPARE,
-			       (void *)(long)smp_processor_id());
-	BUG_ON(err != NOTIFY_OK);
-
+	BUG_ON(init_timers_cpu(smp_processor_id()));
 	init_timer_stats();
 	register_cpu_notifier(&timers_nb);
 	open_softirq(TIMER_SOFTIRQ, run_timer_softirq);

Signature · 1 line

1.7.12.rc2.18.g61b472e

Srivatsa S. Bhat

2014-03-26 12:44 UTC #24 lore raw

Re: [PATCH 14/14] timer: don't emulate notifier call to initialize timer base

Quoted 9 lines · Viresh Kumar

In init_timers() we need to call init_timers_cpu() for boot CPU. For this, currently we are emulating a call to hotplug notifier. Probably this was done initially to get rid of code redundancy. But this sequence always called a single routine, i.e. init_timers_cpu(), and so calling that routine directly would be better. This would get rid of emulating a notifier call, few typecasts and the extra steps we are doing in notifier callback.
So, this patch calls init_timers_cpu() directly from init_timers().

Same here: I don't think this is a good idea, for the same reason I gave for patch 12.

Regards,
Srivatsa S. Bhat

Quoted 27 lines

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

---
 kernel/timer.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

kernel/timer.c +1 -7

index 4360edc..d13eb56 100644
@@ -1666,15 +1666,9 @@ static struct notifier_block timers_nb = {
 
 void __init init_timers(void)
 {
-	int err;
-
 	/* ensure there are enough low bits for flags in timer->base pointer */
 	BUILD_BUG_ON(__alignof__(struct tvec_base) & TIMER_FLAG_MASK);
-
-	err = timer_cpu_notify(&timers_nb, (unsigned long)CPU_UP_PREPARE,
-			       (void *)(long)smp_processor_id());
-	BUG_ON(err != NOTIFY_OK);
-
+	BUG_ON(init_timers_cpu(smp_processor_id()));
 	init_timer_stats();
 	register_cpu_notifier(&timers_nb);
 	open_softirq(TIMER_SOFTIRQ, run_timer_softirq);

Viresh Kumar

author

2014-03-27 05:23 UTC #25 lore raw

PATCH 1/2 hrtimer: don't add clock-base to active_bases if already present

In reply to Viresh Kumar

If active_bases already has entry for a particular clock type, then we don't need to rewrite it while queuing a hrtimer.

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

---
Initially I thought of doing this but then thought better remove active_bases completely and so didn't sent this one. Now it might find some place for itself :).

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

kernel/hrtimer.c +2 -1

index da351ad..acfef5f 100644
@@ -864,8 +864,9 @@ static int enqueue_hrtimer(struct hrtimer *timer,
 {
 	debug_activate(timer);
 
+	if (!timerqueue_getnext(&base->active))
+		base->cpu_base->active_bases |= 1 << base->index;
 	timerqueue_add(&base->active, &timer->node);
-	base->cpu_base->active_bases |= 1 << base->index;
 
 	/*
 	 * HRTIMER_STATE_ENQUEUED is or'ed to the current state to preserve the

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-27 05:23 UTC #26 lore raw

PATCH v2 2/2 hrtimer: use __ffs() to iterate over valid bits from active_bases

Currently we are iterating over all possible (currently four) bits of active_bases to see if corresponding clock bases are active. This is good enough for cases where 3 or 4 bases are used but if only 1 or 2 are used then it makes more sense to use __ffs() to find the right bit directly.

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

---
V1->V2: Instead of removing active_bases use __ffs() on it to make loop more efficient.

I tried to use for_each_set_bit() first and then it looked overdone. And so used a simple form, __ffs() with some code to clear bits.

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

kernel/hrtimer.c +5 -6

index acfef5f..ea90228 100644
@@ -1265,6 +1265,7 @@ void hrtimer_interrupt(struct clock_event_device *dev)
 {
 	struct hrtimer_cpu_base *cpu_base = &__get_cpu_var(hrtimer_bases);
 	ktime_t expires_next, now, entry_time, delta;
+	unsigned long active_bases = cpu_base->active_bases;
 	int i, retries = 0;
 
 	BUG_ON(!cpu_base->hres_active);

@@ -1284,15 +1285,11 @@ retry:
 	 */
 	cpu_base->expires_next.tv64 = KTIME_MAX;
 
-	for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) {
-		struct hrtimer_clock_base *base;
+	while ((i = __ffs(active_bases))) {
+		struct hrtimer_clock_base *base = cpu_base->clock_base + i;
 		struct timerqueue_node *node;
 		ktime_t basenow;
 
-		if (!(cpu_base->active_bases & (1 << i)))
-			continue;
-
-		base = cpu_base->clock_base + i;
 		basenow = ktime_add(now, base->offset);
 
 		while ((node = timerqueue_getnext(&base->active))) {

@@ -1327,6 +1324,8 @@ retry:
 
 			__run_hrtimer(timer, &basenow);
 		}
+
+		active_bases &= ~(1 << i);
 	}
 
 	/*

Signature · 1 line

1.7.12.rc2.18.g61b472e

Thomas Gleixner

2014-03-27 05:40 UTC #27 lore raw

Re: [PATCH V2 2/2] hrtimer: use __ffs() to iterate over valid bits from active_bases

Quoted 30 lines · Viresh Kumar

Currently we are iterating over all possible (currently four) bits of active_bases to see if corresponding clock bases are active. This is good enough for cases where 3 or 4 bases are used but if only 1 or 2 are used then it makes more sense to use __ffs() to find the right bit directly.
Suggested-by Thomas Gleixner <[email protected]>
Signed-off-by Viresh Kumar <[email protected]>
---
V1->V2: Instead of removing active_bases use __ffs() on it to make loop more efficient.
I tried to use for_each_set_bit() first and then it looked overdone. And so used a simple form, __ffs() with some code to clear bits.
 kernel/hrtimer.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)
kernel/hrtimer.c +1 -0
index acfef5f..ea90228 100644
@@ -1265,6 +1265,7 @@ void hrtimer_interrupt(struct clock_event_device *dev)
 {
 	struct hrtimer_cpu_base *cpu_base = &__get_cpu_var(hrtimer_bases);
 	ktime_t expires_next, now, entry_time, delta;
+	unsigned long active_bases = cpu_base->active_bases;
 	int i, retries = 0;
 
 	BUG_ON(!cpu_base->hres_active);

kernel/hrtimer.c retry: · line 1288

Quoted 6 lines

kernel/hrtimer.c +1 -2

@@ -1284,15 +1285,11 @@ retry:
 	 */
 	cpu_base->expires_next.tv64 = KTIME_MAX;
 
-	for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) {
-		struct hrtimer_clock_base *base;
+	while ((i = __ffs(active_bases))) {

What if this is a spurious interrupt and active_bases is 0?

Thanks,

tglx

Viresh Kumar

author

2014-03-27 05:46 UTC #28 lore raw

Re: [PATCH V2 2/2] hrtimer: use __ffs() to iterate over valid bits from active_bases

Quoted 1 line · Thomas Gleixner

What if this is a spurious interrupt and active_bases is 0?

Hmm.. haven't thought about that actually.. I thought it would be guaranteed here that active_bases isn't zero.

Will fix it as the current code would end up in a infinite loop.

Viresh Kumar

author

2014-03-27 05:50 UTC #29 lore raw

PATCH v3 1/1 hrtimer: use __ffs() to iterate over valid bits from active_bases

In reply to Viresh Kumar

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

---
V2->V3: Use ffs() instead of __ffs() and decrement 'i' later.

V1->V2: Instead of removing active_bases use __ffs() on it to make loop more efficient.

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

kernel/hrtimer.c +5 -6

index acfef5f..2aad8a7 100644
@@ -1265,6 +1265,7 @@ void hrtimer_interrupt(struct clock_event_device *dev)
 {
 	struct hrtimer_cpu_base *cpu_base = &__get_cpu_var(hrtimer_bases);
 	ktime_t expires_next, now, entry_time, delta;
+	unsigned long active_bases = cpu_base->active_bases;
 	int i, retries = 0;
 
 	BUG_ON(!cpu_base->hres_active);

@@ -1284,15 +1285,11 @@ retry:
 	 */
 	cpu_base->expires_next.tv64 = KTIME_MAX;
 
-	for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) {
-		struct hrtimer_clock_base *base;
+	while ((i = ffs(active_bases))) {
+		struct hrtimer_clock_base *base = cpu_base->clock_base + --i;
 		struct timerqueue_node *node;
 		ktime_t basenow;
 
-		if (!(cpu_base->active_bases & (1 << i)))
-			continue;
-
-		base = cpu_base->clock_base + i;
 		basenow = ktime_add(now, base->offset);
 
 		while ((node = timerqueue_getnext(&base->active))) {

@@ -1327,6 +1324,8 @@ retry:
 
 			__run_hrtimer(timer, &basenow);
 		}
+
+		active_bases &= ~(1 << i);
 	}
 
 	/*

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author

2014-03-27 05:54 UTC #30 lore raw

PATCH RESEND v3 1/1 hrtimer: use ffs() to iterate over valid bits from active_bases

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

---
V3->Resend: s/__ffs/ffs in commit log :(

V2->V3: Use ffs() instead of __ffs() and decrement 'i' later.

V1->V2: Instead of removing active_bases use __ffs() on it to make loop more efficient.

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

kernel/hrtimer.c +5 -6

index acfef5f..2aad8a7 100644
@@ -1265,6 +1265,7 @@ void hrtimer_interrupt(struct clock_event_device *dev)
 {
 	struct hrtimer_cpu_base *cpu_base = &__get_cpu_var(hrtimer_bases);
 	ktime_t expires_next, now, entry_time, delta;
+	unsigned long active_bases = cpu_base->active_bases;
 	int i, retries = 0;
 
 	BUG_ON(!cpu_base->hres_active);

@@ -1284,15 +1285,11 @@ retry:
 	 */
 	cpu_base->expires_next.tv64 = KTIME_MAX;
 
-	for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) {
-		struct hrtimer_clock_base *base;
+	while ((i = ffs(active_bases))) {
+		struct hrtimer_clock_base *base = cpu_base->clock_base + --i;
 		struct timerqueue_node *node;
 		ktime_t basenow;
 
-		if (!(cpu_base->active_bases & (1 << i)))
-			continue;
-
-		base = cpu_base->clock_base + i;
 		basenow = ktime_add(now, base->offset);
 
 		while ((node = timerqueue_getnext(&base->active))) {

@@ -1327,6 +1324,8 @@ retry:
 
 			__run_hrtimer(timer, &basenow);
 		}
+
+		active_bases &= ~(1 << i);
 	}
 
 	/*

Signature · 1 line

1.7.12.rc2.18.g61b472e

Thomas Gleixner

2014-03-27 05:39 UTC #31 lore raw

Re: [PATCH 1/2] hrtimer: don't add clock-base to active_bases if already present

In reply to Viresh Kumar

Quoted 17 lines · Viresh Kumar

If active_bases already has entry for a particular clock type, then we don't need to rewrite it while queuing a hrtimer.
Signed-off-by Viresh Kumar <[email protected]>
---
Initially I thought of doing this but then thought better remove active_bases completely and so didn't sent this one. Now it might find some place for itself :).
 kernel/hrtimer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/hrtimer.c +0 -0
index da351ad..acfef5f 100644

kernel/hrtimer.c static int enqueue_hrtimer(struct hrtimer *timer, · line 868

Quoted 7 lines

kernel/hrtimer.c +2 -1

@@ -864,8 +864,9 @@ static int enqueue_hrtimer(struct hrtimer *timer,
 {
 	debug_activate(timer);
 
+	if (!timerqueue_getnext(&base->active))
+		base->cpu_base->active_bases |= 1 << base->index;
 	timerqueue_add(&base->active, &timer->node);
-	base->cpu_base->active_bases |= 1 << base->index;

The conditional is more expensive than actually doing the OR operation at least on x86 as it results in a branch.

Thanks,

tglx