[PATCH 00/14] timers/hrtimers: Minor cleanups

[PATCH 00/14] timers/hrtimers: Minor cleanups

[Viresh Kumar]

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

-- 
1.7.12.rc2.18.g61b472e

[PATCH 01/14] hrtimer: replace 'tab' with 'space' after 'comma'

[Viresh Kumar]

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

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/hrtimer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

[PATCH 02/14] hrtimer: Coalesce format fragments in printk()

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

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

[PATCH 03/14] hrtimer: call hrtimer_set_expires_range() from hrtimer_set_expires_range_ns()

[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 <viresh.kumar@linaro.org>
---
 include/linux/hrtimer.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index e7a8d3f..17c08ca 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -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)
-- 
1.7.12.rc2.18.g61b472e

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

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

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 95243f2..b0bcc10 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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) {
 		/*
-- 
1.7.12.rc2.18.g61b472e

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

[Srivatsa S. Bhat]

On 03/26/2014 04:51 PM, Viresh Kumar wrote:
> 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.

> --- a/kernel/hrtimer.c
> +++ b/kernel/hrtimer.c
> @@ -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.

>  	if (base != new_base) {
>  		/*
> 

Regards,
Srivatsa S. Bhat

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

[Viresh Kumar]

On 26 March 2014 17:13, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> +     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.

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

[Thomas Gleixner]

On Wed, 26 Mar 2014, Srivatsa S. Bhat wrote:
> On 03/26/2014 04:51 PM, Viresh Kumar wrote:
> > 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.
> 
> > --- a/kernel/hrtimer.c
> > +++ b/kernel/hrtimer.c
> > @@ -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.

That's irrelevant. base is not changing.

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

[Srivatsa S. Bhat]

On 03/26/2014 11:01 PM, Thomas Gleixner wrote:
> On Wed, 26 Mar 2014, Srivatsa S. Bhat wrote:
>> On 03/26/2014 04:51 PM, Viresh Kumar wrote:
>>> 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.
>>
>>> --- a/kernel/hrtimer.c
>>> +++ b/kernel/hrtimer.c
>>> @@ -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.
> 
> That's irrelevant. base is not changing.
> 

Sorry, I missed that :-(

Regards,
Srivatsa S. Bhat

[PATCH 05/14] hrtimer: no need to rewrite '1' to hrtimer_hres_enabled

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

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

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

[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 <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 e5d81ee..32d1504 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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);
 }
-- 
1.7.12.rc2.18.g61b472e

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

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

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

[PATCH 08/14] hrtimer: remove dummy definition of hrtimer_force_reprogram()

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

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

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

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

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 5b3b212..e6e1255 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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()) {
-- 
1.7.12.rc2.18.g61b472e

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

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

diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index 17c08ca..6f524db 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -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);
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index e6e1255..1e4eedb 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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 */
 
-- 
1.7.12.rc2.18.g61b472e

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

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

diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index 6f524db..cb6a315 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -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;
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 1e4eedb..f14d861 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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))) {
-- 
1.7.12.rc2.18.g61b472e

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

[Thomas Gleixner]

On Wed, 26 Mar 2014, Viresh Kumar wrote:

> 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

 

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

[Viresh Kumar]

On 26 March 2014 22:58, Thomas Gleixner <tglx@linutronix.de> wrote:
> 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.

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

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

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

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

[Srivatsa S. Bhat]

On 03/26/2014 04:51 PM, Viresh Kumar wrote:
> 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
 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  kernel/hrtimer.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
> index f14d861..39dbdbd 100644
> --- a/kernel/hrtimer.c
> +++ b/kernel/hrtimer.c
> @@ -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);
> 

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

[Viresh Kumar]

On 26 March 2014 18:10, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> 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.

> 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.

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

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

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

diff --git a/kernel/timer.c b/kernel/timer.c
index 1d35dda..4360edc 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -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:
-- 
1.7.12.rc2.18.g61b472e

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

[Srivatsa S. Bhat]

On 03/26/2014 04:51 PM, Viresh Kumar wrote:
> 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 <viresh.kumar@linaro.org>

Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>

Regards,
Srivatsa S. Bhat

> ---
>  kernel/timer.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/kernel/timer.c b/kernel/timer.c
> index 1d35dda..4360edc 100644
> --- a/kernel/timer.c
> +++ b/kernel/timer.c
> @@ -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:
> 

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

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

diff --git a/kernel/timer.c b/kernel/timer.c
index 4360edc..d13eb56 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -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);
-- 
1.7.12.rc2.18.g61b472e

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

[Srivatsa S. Bhat]

On 03/26/2014 04:51 PM, Viresh Kumar wrote:
> 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
 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  kernel/timer.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/kernel/timer.c b/kernel/timer.c
> index 4360edc..d13eb56 100644
> --- a/kernel/timer.c
> +++ b/kernel/timer.c
> @@ -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);
> 

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

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

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

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

[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 <tglx@linutronix.de>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
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(-)

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

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

[Thomas Gleixner]

On Thu, 27 Mar 2014, Viresh Kumar wrote:

> 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 <tglx@linutronix.de>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> 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(-)
> 
> diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
> index acfef5f..ea90228 100644
> --- a/kernel/hrtimer.c
> +++ b/kernel/hrtimer.c
> @@ -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))) {

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

Thanks,

	tglx

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

[Viresh Kumar]

On 27 March 2014 11:10, Thomas Gleixner <tglx@linutronix.de> wrote:
> 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.

[PATCH V3] hrtimer: use __ffs() to iterate over valid bits from active_bases

[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 <tglx@linutronix.de>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
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(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index acfef5f..2aad8a7 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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);
 	}
 
 	/*
-- 
1.7.12.rc2.18.g61b472e

[PATCH V3 Resend] hrtimer: use ffs() to iterate over valid bits from active_bases

[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 <tglx@linutronix.de>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
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(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index acfef5f..2aad8a7 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -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);
 	}
 
 	/*
-- 
1.7.12.rc2.18.g61b472e

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

[Thomas Gleixner]

On Thu, 27 Mar 2014, Viresh Kumar wrote:

> 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 <viresh.kumar@linaro.org>
> ---
> 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(-)
> 
> diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
> index da351ad..acfef5f 100644
> --- a/kernel/hrtimer.c
> +++ b/kernel/hrtimer.c
> @@ -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