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(-)
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)); }
/*
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;
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)
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) { /*
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
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.
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.
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
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; }
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); }
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);
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) {
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()) {
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 */
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))) {
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
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.
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);
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);
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() :)
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:
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:
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);
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);
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
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); }
/*
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
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.
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); }
/*
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); }
/*
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;
- 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
linaro-kernel@lists.linaro.org
-
Srivatsa S. Bhat -
Thomas Gleixner -
Viresh Kumar