Hi Thomas,
These are separate cleanups from the timers/hrtimers ones I did. I was waiting for the merge window to close in order to send these and by the time it happened, I got a long pending list.
These are mostly cleanups, reorders for better readability or efficiency, and few bugfixes.
I have pushed these here as well:
git://git.linaro.org/people/viresh.kumar/linux.git tick-cleanups
They will be tested by kbuild bot also starting from tonight.
Viresh Kumar (38): tick: align to Coding Guidelines tick: update doc comments for struct tick_sched tick: rearrange members of 'struct tick_sched' tick: move declaration of tick_cpu_device to tick.h tick: move definition of tick_get_device() to tick.h tick: create tick_get_cpu_device() to get tick_cpu_device on this cpu tick-oneshot: drop local_irq_save/restore from tick_switch_to_oneshot() tick-oneshot: move tick_is_oneshot_available() to tick-oneshot.c tick-oneshot: remove tick_resume_oneshot() tick-common: remove extra checks from tick_check_new_device() tick-common: fix wrong check in tick_check_replacement() tick-common: call tick_check_percpu() from tick_check_preferred() tick-common: don't check tick_oneshot_mode_active() from tick_check_preferred() tick-common: do additional checks in tick_check_preferred() tick-common: remove tick_check_replacement() tick-common: don't pass cpumask to tick_setup_device() tick-common: call tick_install_replacement() from tick_check_new_device() tick-common: don't set mode to CLOCK_EVT_MODE_UNUSED in tick_shutdown() tick-common: remove local variable 'broadcast' from tick_resume() tick-sched: initialize 'cpu' while defining it in tick_nohz_full_setup() tick-sched: no need to rewrite '1' to tick_nohz_enabled tick-sched: no need to recheck cpu_online() in can_stop_idle_tick() tick-sched: invert parameter of tick_check_oneshot_change() tick-sched: don't check tick_nohz_full_cpu() in __tick_nohz_task_switch() tick-sched: don't call local_softirq_pending() thrice in can_stop_idle_tick() tick-sched: don't call update_wall_time() when delta is lesser than tick_period tick-sched: remove 'regs' parameter of tick_sched_handle() tick-sched: remove parameters to {__}tick_nohz_task_switch() routines tick-sched: remove wrapper around __tick_nohz_task_switch() tick-sched: move nohz_full_buf[] inside tick_nohz_init() tick-sched: initialize 'ts' during its definition __tick_nohz_idle_enter() tick-sched: add comment about 'idle_active' in tick_nohz_idle_exit() tick-sched: replace tick_nohz_active with tick_nohz_enabled in tick_nohz_switch_to_nohz() tick-sched: remove local variable 'now' from tick_setup_sched_timer() tick-broadcast: do checks before taking locks in tick_do_broadcast_on_off() tick-broadcast: get rid of extra comparison in tick_do_broadcast_on_off() tick-broadcast: merge tick_do_broadcast_on_off() into tick_broadcast_on_off() clockevents: set event_handler to clockevents_handle_noop() in clockevents_exchange_device()
include/linux/clockchips.h | 2 - include/linux/hrtimer.h | 3 - include/linux/tick.h | 65 ++++++++++------- kernel/hrtimer.c | 4 +- kernel/sched/core.c | 2 +- kernel/time/clockevents.c | 11 +-- kernel/time/tick-broadcast.c | 74 +++++++------------- kernel/time/tick-common.c | 126 +++++++++++++-------------------- kernel/time/tick-internal.h | 15 ++-- kernel/time/tick-oneshot.c | 37 +++++----- kernel/time/tick-sched.c | 163 +++++++++++++++++++++++-------------------- 11 files changed, 232 insertions(+), 270 deletions(-)
This does some trivial fixups: - breaking lines longer than 80 columns - merging few lines together - don't break print messages even if they cross 80 columns - remove few whitespaces - remove few extra blank lines
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/tick.h | 3 ++- kernel/time/tick-broadcast.c | 16 ++++++---------- kernel/time/tick-internal.h | 5 ++++- kernel/time/tick-oneshot.c | 3 +-- kernel/time/tick-sched.c | 29 ++++++++++++++++------------- 5 files changed, 29 insertions(+), 27 deletions(-)
diff --git a/include/linux/tick.h b/include/linux/tick.h index b84773c..8c865fb 100644 --- a/include/linux/tick.h +++ b/include/linux/tick.h @@ -47,7 +47,8 @@ enum tick_nohz_mode { * @idle_waketime: Time when the idle was interrupted * @idle_exittime: Time when the idle state was left * @idle_sleeptime: Sum of the time slept in idle with sched tick stopped - * @iowait_sleeptime: Sum of the time slept in idle with sched tick stopped, with IO outstanding + * @iowait_sleeptime: Sum of the time slept in idle with sched tick stopped, + * with IO outstanding * @sleep_length: Duration of the current idle sleep * @do_timer_lst: CPU was the last one doing do_timer before going idle */ diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index 64c5990..c4c50ba 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -132,7 +132,6 @@ int tick_broadcast_update_freq(struct clock_event_device *dev, u32 freq) return ret; }
- static void err_broadcast(const struct cpumask *mask) { pr_crit_once("Failed to broadcast timer tick. Some CPUs may be unresponsive.\n"); @@ -358,8 +357,7 @@ static void tick_do_broadcast_on_off(unsigned long *reason) case CLOCK_EVT_NOTIFY_BROADCAST_FORCE: cpumask_set_cpu(cpu, tick_broadcast_on); if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_mask)) { - if (tick_broadcast_device.mode == - TICKDEV_MODE_PERIODIC) + if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC) clockevents_shutdown(dev); } if (*reason == CLOCK_EVT_NOTIFY_BROADCAST_FORCE) @@ -372,8 +370,7 @@ static void tick_do_broadcast_on_off(unsigned long *reason) if (!tick_device_is_functional(dev)) break; if (cpumask_test_and_clear_cpu(cpu, tick_broadcast_mask)) { - if (tick_broadcast_device.mode == - TICKDEV_MODE_PERIODIC) + if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC) tick_setup_periodic(dev, 0); } break; @@ -399,8 +396,8 @@ out: void tick_broadcast_on_off(unsigned long reason, int *oncpu) { if (!cpumask_test_cpu(*oncpu, cpu_online_mask)) - printk(KERN_ERR "tick-broadcast: ignoring broadcast for " - "offline CPU #%d\n", *oncpu); + printk(KERN_ERR "tick-broadcast: ignoring broadcast for offline CPU #%d\n", + *oncpu); else tick_do_broadcast_on_off(&reason); } @@ -484,7 +481,6 @@ int tick_resume_broadcast(void) return broadcast; }
- #ifdef CONFIG_TICK_ONESHOT
static cpumask_var_t tick_broadcast_oneshot_mask; @@ -727,7 +723,8 @@ int tick_broadcast_oneshot_control(unsigned long reason) */ if (!cpumask_test_cpu(cpu, tick_broadcast_force_mask) && dev->next_event.tv64 < bc->next_event.tv64) - tick_broadcast_set_event(bc, cpu, dev->next_event, 1); + tick_broadcast_set_event(bc, cpu, + dev->next_event, 1); } /* * If the current CPU owns the hrtimer broadcast @@ -894,7 +891,6 @@ void tick_broadcast_switch_to_oneshot(void) raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags); }
- /* * Remove a dead CPU from broadcasting */ diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h index 7ab92b1..855c513 100644 --- a/kernel/time/tick-internal.h +++ b/kernel/time/tick-internal.h @@ -87,7 +87,10 @@ static inline void tick_broadcast_setup_oneshot(struct clock_event_device *bc) { BUG(); } -static inline int tick_broadcast_oneshot_control(unsigned long reason) { return 0; } +static inline int tick_broadcast_oneshot_control(unsigned long reason) +{ + return 0; +} static inline void tick_shutdown_broadcast_oneshot(unsigned int *cpup) { } static inline int tick_resume_broadcast_oneshot(struct clock_event_device *bc) { diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c index 8241090..e04d5a0 100644 --- a/kernel/time/tick-oneshot.c +++ b/kernel/time/tick-oneshot.c @@ -65,8 +65,7 @@ int tick_switch_to_oneshot(void (*handler)(struct clock_event_device *)) if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT) || !tick_device_is_functional(dev)) {
- printk(KERN_INFO "Clockevents: " - "could not switch to one-shot mode:"); + printk(KERN_INFO "Clockevents: could not switch to one-shot mode:"); if (!dev) { printk(" no tick device\n"); } else { diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 9f8af69..d48d648 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -105,7 +105,6 @@ static ktime_t tick_init_jiffy_update(void) return period; }
- static void tick_sched_do_timer(ktime_t now) { int cpu = smp_processor_id(); @@ -245,8 +244,8 @@ void tick_nohz_full_kick_all(void) return;
preempt_disable(); - smp_call_function_many(tick_nohz_full_mask, - nohz_full_kick_ipi, NULL, false); + smp_call_function_many(tick_nohz_full_mask, nohz_full_kick_ipi, NULL, + false); tick_nohz_full_kick(); preempt_enable(); } @@ -285,7 +284,8 @@ static int __init tick_nohz_full_setup(char *str)
cpu = smp_processor_id(); if (cpumask_test_cpu(cpu, tick_nohz_full_mask)) { - pr_warning("NO_HZ: Clearing %d from nohz_full range for timekeeping\n", cpu); + pr_warning("NO_HZ: Clearing %d from nohz_full range for timekeeping\n", + cpu); cpumask_clear_cpu(cpu, tick_nohz_full_mask); } tick_nohz_full_running = true; @@ -295,8 +295,7 @@ static int __init tick_nohz_full_setup(char *str) __setup("nohz_full=", tick_nohz_full_setup);
static int tick_nohz_cpu_down_callback(struct notifier_block *nfb, - unsigned long action, - void *hcpu) + unsigned long action, void *hcpu) { unsigned int cpu = (unsigned long)hcpu;
@@ -350,7 +349,8 @@ void __init tick_nohz_init(void) context_tracking_cpu_set(cpu);
cpu_notifier(tick_nohz_cpu_down_callback, 0); - cpulist_scnprintf(nohz_full_buf, sizeof(nohz_full_buf), tick_nohz_full_mask); + cpulist_scnprintf(nohz_full_buf, sizeof(nohz_full_buf), + tick_nohz_full_mask); pr_info("NO_HZ: Full dynticks CPUs: %s.\n", nohz_full_buf); } #endif @@ -362,8 +362,8 @@ void __init tick_nohz_init(void) /* * NO HZ enabled ? */ -static int tick_nohz_enabled __read_mostly = 1; -int tick_nohz_active __read_mostly; +static int tick_nohz_enabled __read_mostly = 1; +int tick_nohz_active __read_mostly; /* * Enable / Disable tickless mode */ @@ -407,16 +407,19 @@ static void tick_nohz_update_jiffies(ktime_t now) * Updates the per cpu time idle statistics counters */ static void -update_ts_time_stats(int cpu, struct tick_sched *ts, ktime_t now, u64 *last_update_time) +update_ts_time_stats(int cpu, struct tick_sched *ts, ktime_t now, + u64 *last_update_time) { ktime_t delta;
if (ts->idle_active) { delta = ktime_sub(now, ts->idle_entrytime); if (nr_iowait_cpu(cpu) > 0) - ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta); + ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, + delta); else - ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta); + ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, + delta); ts->idle_entrytime = now; }
@@ -873,7 +876,7 @@ static void tick_nohz_restart_sched_tick(struct tick_sched *ts, ktime_t now) /* * Cancel the scheduled timer and restore the tick */ - ts->tick_stopped = 0; + ts->tick_stopped = 0; ts->idle_exittime = now;
tick_nohz_restart(ts, now);
Some fields of 'struct tick_sched' didn't had a description in the kernel doc comment present above its declaration. Add them.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/tick.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/include/linux/tick.h b/include/linux/tick.h index 8c865fb..8cc804c 100644 --- a/include/linux/tick.h +++ b/include/linux/tick.h @@ -35,14 +35,19 @@ enum tick_nohz_mode { * struct tick_sched - sched tick emulation and no idle tick control/stats * @sched_timer: hrtimer to schedule the periodic tick in high * resolution mode + * @check_clocks: tracks if clockevent device is recently changed + * @nohz_mode: Current NOHZ mode * @last_tick: Store the last tick expiry time when the tick * timer is modified for nohz sleeps. This is necessary * to resume the tick timer operation in the timeline * when the CPU returns from nohz sleep. + * @inidle: CPU is currently executing from within the idle loop * @tick_stopped: Indicator that the idle tick has been stopped * @idle_jiffies: jiffies at the entry to idle for idle time accounting * @idle_calls: Total number of idle calls * @idle_sleeps: Number of idle calls, where the sched tick was stopped + * @idle_active: similar to inidle, but is reset when we get an interrupt + * while being in idle * @idle_entrytime: Time when the idle call was entered * @idle_waketime: Time when the idle was interrupted * @idle_exittime: Time when the idle state was left @@ -50,7 +55,10 @@ enum tick_nohz_mode { * @iowait_sleeptime: Sum of the time slept in idle with sched tick stopped, * with IO outstanding * @sleep_length: Duration of the current idle sleep - * @do_timer_lst: CPU was the last one doing do_timer before going idle + * @last_jiffies: Last updated value of jiffies + * @next_jiffies: Next jiffie for which timer is requested + * @idle_expires: Time when we should come out of idle or next timer event + * @do_timer_last: CPU was the last one doing do_timer before going idle */ struct tick_sched { struct hrtimer sched_timer;
Rearrange members of 'struct tick_sched' to make it more readable. i.e. By keeping all ktime_t members together, similarly 'unsigned long' and 'int' as well. I couldn't figure out breaking any logical blocks here which we wanted to keep together.
This is the change in footprints after this change. For x86: size of this structure changed from 240 to 232 bytes For ARM: didn't see a difference, was 160 bytes
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/tick.h | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/include/linux/tick.h b/include/linux/tick.h index 8cc804c..edabc7d 100644 --- a/include/linux/tick.h +++ b/include/linux/tick.h @@ -35,19 +35,17 @@ enum tick_nohz_mode { * struct tick_sched - sched tick emulation and no idle tick control/stats * @sched_timer: hrtimer to schedule the periodic tick in high * resolution mode - * @check_clocks: tracks if clockevent device is recently changed * @nohz_mode: Current NOHZ mode + * @check_clocks: tracks if clockevent device is recently changed + * @idle_jiffies: jiffies at the entry to idle for idle time accounting + * @idle_calls: Total number of idle calls + * @idle_sleeps: Number of idle calls, where the sched tick was stopped + * @last_jiffies: Last updated value of jiffies + * @next_jiffies: Next jiffie for which timer is requested * @last_tick: Store the last tick expiry time when the tick * timer is modified for nohz sleeps. This is necessary * to resume the tick timer operation in the timeline * when the CPU returns from nohz sleep. - * @inidle: CPU is currently executing from within the idle loop - * @tick_stopped: Indicator that the idle tick has been stopped - * @idle_jiffies: jiffies at the entry to idle for idle time accounting - * @idle_calls: Total number of idle calls - * @idle_sleeps: Number of idle calls, where the sched tick was stopped - * @idle_active: similar to inidle, but is reset when we get an interrupt - * while being in idle * @idle_entrytime: Time when the idle call was entered * @idle_waketime: Time when the idle was interrupted * @idle_exittime: Time when the idle state was left @@ -55,31 +53,33 @@ enum tick_nohz_mode { * @iowait_sleeptime: Sum of the time slept in idle with sched tick stopped, * with IO outstanding * @sleep_length: Duration of the current idle sleep - * @last_jiffies: Last updated value of jiffies - * @next_jiffies: Next jiffie for which timer is requested * @idle_expires: Time when we should come out of idle or next timer event + * @inidle: CPU is currently executing from within the idle loop + * @idle_active: similar to inidle, but is reset when we get an interrupt + * while being in idle + * @tick_stopped: Indicator that the idle tick has been stopped * @do_timer_last: CPU was the last one doing do_timer before going idle */ struct tick_sched { struct hrtimer sched_timer; - unsigned long check_clocks; enum tick_nohz_mode nohz_mode; - ktime_t last_tick; - int inidle; - int tick_stopped; + unsigned long check_clocks; unsigned long idle_jiffies; unsigned long idle_calls; unsigned long idle_sleeps; - int idle_active; + unsigned long last_jiffies; + unsigned long next_jiffies; + ktime_t last_tick; ktime_t idle_entrytime; ktime_t idle_waketime; ktime_t idle_exittime; ktime_t idle_sleeptime; ktime_t iowait_sleeptime; ktime_t sleep_length; - unsigned long last_jiffies; - unsigned long next_jiffies; ktime_t idle_expires; + int inidle; + int idle_active; + int tick_stopped; int do_timer_last; };
tick_cpu_device isn't local only to kernel/time/ directory and its declaration is present in hrtimer.h and tick-internal.h. Rather move it to a common place, i.e. tick.h.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/hrtimer.h | 3 --- include/linux/tick.h | 2 ++ kernel/time/tick-internal.h | 1 - 3 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h index e7a8d3f..a31f83e 100644 --- a/include/linux/hrtimer.h +++ b/include/linux/hrtimer.h @@ -332,9 +332,6 @@ extern ktime_t ktime_get_clocktai(void); extern ktime_t ktime_get_update_offsets(ktime_t *offs_real, ktime_t *offs_boot, ktime_t *offs_tai);
-DECLARE_PER_CPU(struct tick_device, tick_cpu_device); - - /* Exported timer functions: */
/* Initialize timers: */ diff --git a/include/linux/tick.h b/include/linux/tick.h index edabc7d..1a9908a 100644 --- a/include/linux/tick.h +++ b/include/linux/tick.h @@ -85,6 +85,8 @@ struct tick_sched {
extern void __init tick_init(void); extern int tick_is_oneshot_available(void); + +DECLARE_PER_CPU(struct tick_device, tick_cpu_device); extern struct tick_device *tick_get_device(int cpu);
# ifdef CONFIG_HIGH_RES_TIMERS diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h index 855c513..57c1a76 100644 --- a/kernel/time/tick-internal.h +++ b/kernel/time/tick-internal.h @@ -13,7 +13,6 @@ extern seqlock_t jiffies_lock; #define TICK_DO_TIMER_NONE -1 #define TICK_DO_TIMER_BOOT -2
-DECLARE_PER_CPU(struct tick_device, tick_cpu_device); extern ktime_t tick_next_period; extern ktime_t tick_period; extern int tick_do_timer_cpu __read_mostly;
There are multiple users of tick_get_device() which are currently using &per_cpu(tick_cpu_device, cpu) directly. Would be better to move definition of tick_get_device() to tick.h and move others to use this routine instead.
This change reduced size of bzImage for x86 by 96 bytes.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/tick.h | 5 ++++- kernel/time/clockevents.c | 6 +++--- kernel/time/tick-broadcast.c | 12 ++++++------ kernel/time/tick-common.c | 9 ++------- 4 files changed, 15 insertions(+), 17 deletions(-)
diff --git a/include/linux/tick.h b/include/linux/tick.h index 1a9908a..45e1331 100644 --- a/include/linux/tick.h +++ b/include/linux/tick.h @@ -87,7 +87,10 @@ extern void __init tick_init(void); extern int tick_is_oneshot_available(void);
DECLARE_PER_CPU(struct tick_device, tick_cpu_device); -extern struct tick_device *tick_get_device(int cpu); +static inline struct tick_device *tick_get_device(int cpu) +{ + return &per_cpu(tick_cpu_device, cpu); +}
# ifdef CONFIG_HIGH_RES_TIMERS extern int tick_init_highres(void); diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index ad362c2..6680a0d 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -326,7 +326,7 @@ static int __clockevents_try_unbind(struct clock_event_device *ced, int cpu) return 0; }
- return ced == per_cpu(tick_cpu_device, cpu).evtdev ? -EAGAIN : -EBUSY; + return ced == tick_get_device(cpu)->evtdev ? -EAGAIN : -EBUSY; }
/* @@ -675,7 +675,7 @@ static struct device tick_bc_dev = { static struct tick_device *tick_get_tick_dev(struct device *dev) { return dev == &tick_bc_dev ? tick_get_broadcast_device() : - &per_cpu(tick_cpu_device, dev->id); + tick_get_device(dev->id); }
static __init int tick_broadcast_init_sysfs(void) @@ -689,7 +689,7 @@ static __init int tick_broadcast_init_sysfs(void) #else static struct tick_device *tick_get_tick_dev(struct device *dev) { - return &per_cpu(tick_cpu_device, dev->id); + return tick_get_device(dev->id); } static inline int tick_broadcast_init_sysfs(void) { return 0; } #endif diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index c4c50ba..eb0479a 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -262,7 +262,7 @@ static void tick_do_broadcast(struct cpumask *mask) */ if (cpumask_test_cpu(cpu, mask)) { cpumask_clear_cpu(cpu, mask); - td = &per_cpu(tick_cpu_device, cpu); + td = tick_get_device(cpu); td->evtdev->event_handler(td->evtdev); }
@@ -273,7 +273,7 @@ static void tick_do_broadcast(struct cpumask *mask) * one of the first device. This works as long as we have this * misfeature only on x86 (lapic) */ - td = &per_cpu(tick_cpu_device, cpumask_first(mask)); + td = tick_get_device(cpumask_first(mask)); td->evtdev->broadcast(mask); } } @@ -337,7 +337,7 @@ static void tick_do_broadcast_on_off(unsigned long *reason) raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
cpu = smp_processor_id(); - td = &per_cpu(tick_cpu_device, cpu); + td = tick_get_device(cpu); dev = td->evtdev; bc = tick_broadcast_device.evtdev;
@@ -581,7 +581,7 @@ again: now = ktime_get(); /* Find all expired events */ for_each_cpu(cpu, tick_broadcast_oneshot_mask) { - td = &per_cpu(tick_cpu_device, cpu); + td = tick_get_device(cpu); if (td->evtdev->next_event.tv64 <= now.tv64) { cpumask_set_cpu(cpu, tmpmask); /* @@ -700,7 +700,7 @@ int tick_broadcast_oneshot_control(unsigned long reason) * idle code, so we can't be moved away. */ cpu = smp_processor_id(); - td = &per_cpu(tick_cpu_device, cpu); + td = tick_get_device(cpu); dev = td->evtdev;
if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) @@ -824,7 +824,7 @@ static void tick_broadcast_init_next_event(struct cpumask *mask, int cpu;
for_each_cpu(cpu, mask) { - td = &per_cpu(tick_cpu_device, cpu); + td = tick_get_device(cpu); if (td->evtdev) td->evtdev->next_event = expires; } diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index 0156612..c7859b7 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -53,11 +53,6 @@ int tick_do_timer_cpu __read_mostly = TICK_DO_TIMER_BOOT; /* * Debugging: see timer_list.c */ -struct tick_device *tick_get_device(int cpu) -{ - return &per_cpu(tick_cpu_device, cpu); -} - /** * tick_is_oneshot_available - check for a oneshot capable event device */ @@ -296,7 +291,7 @@ void tick_check_new_device(struct clock_event_device *newdev) if (!cpumask_test_cpu(cpu, newdev->cpumask)) goto out_bc;
- td = &per_cpu(tick_cpu_device, cpu); + td = tick_get_device(cpu); curdev = td->evtdev;
/* cpu local device ? */ @@ -356,7 +351,7 @@ void tick_handover_do_timer(int *cpup) */ void tick_shutdown(unsigned int *cpup) { - struct tick_device *td = &per_cpu(tick_cpu_device, *cpup); + struct tick_device *td = tick_get_device(*cpup); struct clock_event_device *dev = td->evtdev;
td->mode = TICKDEV_MODE_PERIODIC;
We are accessing &__get_cpu_var(tick_cpu_device) at several places in kernel. Lets create another routine tick_get_cpu_device() which would return tick_cpu_device for this-cpu.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/tick.h | 5 +++++ kernel/hrtimer.c | 2 +- kernel/time/tick-broadcast.c | 4 ++-- kernel/time/tick-common.c | 8 ++++---- kernel/time/tick-oneshot.c | 8 ++++---- kernel/time/tick-sched.c | 2 +- 6 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/include/linux/tick.h b/include/linux/tick.h index 45e1331..98065e5 100644 --- a/include/linux/tick.h +++ b/include/linux/tick.h @@ -92,6 +92,11 @@ static inline struct tick_device *tick_get_device(int cpu) return &per_cpu(tick_cpu_device, cpu); }
+static inline struct tick_device *tick_get_cpu_device(void) +{ + return &__get_cpu_var(tick_cpu_device); +} + # ifdef CONFIG_HIGH_RES_TIMERS extern int tick_init_highres(void); extern int tick_program_event(ktime_t expires, int force); diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index d55092c..393f422 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -1403,7 +1403,7 @@ static void __hrtimer_peek_ahead_timers(void) if (!hrtimer_hres_active()) return;
- td = &__get_cpu_var(tick_cpu_device); + td = tick_get_cpu_device(); if (td && td->evtdev) hrtimer_interrupt(td->evtdev); } diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index eb0479a..1597d03 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -235,7 +235,7 @@ int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu) #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST int tick_receive_broadcast(void) { - struct tick_device *td = this_cpu_ptr(&tick_cpu_device); + struct tick_device *td = tick_get_cpu_device(); struct clock_event_device *evt = td->evtdev;
if (!evt) @@ -550,7 +550,7 @@ int tick_resume_broadcast_oneshot(struct clock_event_device *bc) void tick_check_oneshot_broadcast_this_cpu(void) { if (cpumask_test_cpu(smp_processor_id(), tick_broadcast_oneshot_mask)) { - struct tick_device *td = &__get_cpu_var(tick_cpu_device); + struct tick_device *td = tick_get_cpu_device();
/* * We might be in the middle of switching over from diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index c7859b7..b4fdb28 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -58,7 +58,7 @@ int tick_do_timer_cpu __read_mostly = TICK_DO_TIMER_BOOT; */ int tick_is_oneshot_available(void) { - struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev); + struct clock_event_device *dev = tick_get_cpu_device()->evtdev;
if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT)) return 0; @@ -219,7 +219,7 @@ static void tick_setup_device(struct tick_device *td,
void tick_install_replacement(struct clock_event_device *newdev) { - struct tick_device *td = &__get_cpu_var(tick_cpu_device); + struct tick_device *td = tick_get_cpu_device(); int cpu = smp_processor_id();
clockevents_exchange_device(td->evtdev, newdev); @@ -369,14 +369,14 @@ void tick_shutdown(unsigned int *cpup)
void tick_suspend(void) { - struct tick_device *td = &__get_cpu_var(tick_cpu_device); + struct tick_device *td = tick_get_cpu_device();
clockevents_shutdown(td->evtdev); }
void tick_resume(void) { - struct tick_device *td = &__get_cpu_var(tick_cpu_device); + struct tick_device *td = tick_get_cpu_device(); int broadcast = tick_resume_broadcast();
clockevents_set_mode(td->evtdev, CLOCK_EVT_MODE_RESUME); diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c index e04d5a0..6a531ec 100644 --- a/kernel/time/tick-oneshot.c +++ b/kernel/time/tick-oneshot.c @@ -26,7 +26,7 @@ */ int tick_program_event(ktime_t expires, int force) { - struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev); + struct clock_event_device *dev = tick_get_cpu_device()->evtdev;
return clockevents_program_event(dev, expires, force); } @@ -36,7 +36,7 @@ int tick_program_event(ktime_t expires, int force) */ void tick_resume_oneshot(void) { - struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev); + struct clock_event_device *dev = tick_get_cpu_device()->evtdev;
clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT); clockevents_program_event(dev, ktime_get(), true); @@ -59,7 +59,7 @@ void tick_setup_oneshot(struct clock_event_device *newdev, */ int tick_switch_to_oneshot(void (*handler)(struct clock_event_device *)) { - struct tick_device *td = &__get_cpu_var(tick_cpu_device); + struct tick_device *td = tick_get_cpu_device(); struct clock_event_device *dev = td->evtdev;
if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT) || @@ -96,7 +96,7 @@ int tick_oneshot_mode_active(void) int ret;
local_irq_save(flags); - ret = __this_cpu_read(tick_cpu_device.mode) == TICKDEV_MODE_ONESHOT; + ret = tick_get_cpu_device()->mode == TICKDEV_MODE_ONESHOT; local_irq_restore(flags);
return ret; diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index d48d648..649b920 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -533,7 +533,7 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts, unsigned long seq, last_jiffies, next_jiffies, delta_jiffies; ktime_t last_update, expires, ret = { .tv64 = 0 }; unsigned long rcu_delta_jiffies; - struct clock_event_device *dev = __get_cpu_var(tick_cpu_device).evtdev; + struct clock_event_device *dev = tick_get_cpu_device()->evtdev; u64 time_delta;
time_delta = timekeeping_max_deferment();
tick_switch_to_oneshot() is just reading value of __this_cpu_read(tick_cpu_device.mode) and doesn't need to disable local interrupts for that. Drop local_irq_save/restore calls from this routine.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-oneshot.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c index 6a531ec..39492f1 100644 --- a/kernel/time/tick-oneshot.c +++ b/kernel/time/tick-oneshot.c @@ -92,14 +92,7 @@ int tick_switch_to_oneshot(void (*handler)(struct clock_event_device *)) */ int tick_oneshot_mode_active(void) { - unsigned long flags; - int ret; - - local_irq_save(flags); - ret = tick_get_cpu_device()->mode == TICKDEV_MODE_ONESHOT; - local_irq_restore(flags); - - return ret; + return tick_get_cpu_device()->mode == TICKDEV_MODE_ONESHOT; }
#ifdef CONFIG_HIGH_RES_TIMERS
This is tick-oneshot specific routine and hence must be there in this file. Also it isn't used outside of kernel/time/ and so moving its declaration to tick-internal.h.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/tick.h | 1 - kernel/time/tick-common.c | 14 -------------- kernel/time/tick-internal.h | 2 ++ kernel/time/tick-oneshot.c | 14 ++++++++++++++ 4 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/include/linux/tick.h b/include/linux/tick.h index 98065e5..b8ee6f4 100644 --- a/include/linux/tick.h +++ b/include/linux/tick.h @@ -84,7 +84,6 @@ struct tick_sched { };
extern void __init tick_init(void); -extern int tick_is_oneshot_available(void);
DECLARE_PER_CPU(struct tick_device, tick_cpu_device); static inline struct tick_device *tick_get_device(int cpu) diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index b4fdb28..298467e 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -53,20 +53,6 @@ int tick_do_timer_cpu __read_mostly = TICK_DO_TIMER_BOOT; /* * Debugging: see timer_list.c */ -/** - * tick_is_oneshot_available - check for a oneshot capable event device - */ -int tick_is_oneshot_available(void) -{ - struct clock_event_device *dev = tick_get_cpu_device()->evtdev; - - if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT)) - return 0; - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) - return 1; - return tick_broadcast_oneshot_available(); -} - /* * Periodic tick */ diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h index 57c1a76..09f4307 100644 --- a/kernel/time/tick-internal.h +++ b/kernel/time/tick-internal.h @@ -42,6 +42,7 @@ extern void tick_setup_oneshot(struct clock_event_device *newdev, extern int tick_program_event(ktime_t expires, int force); extern void tick_oneshot_notify(void); extern int tick_switch_to_oneshot(void (*handler)(struct clock_event_device *)); +extern int tick_is_oneshot_available(void); extern void tick_resume_oneshot(void); # ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST extern void tick_broadcast_setup_oneshot(struct clock_event_device *bc); @@ -96,6 +97,7 @@ static inline int tick_resume_broadcast_oneshot(struct clock_event_device *bc) return 0; } static inline int tick_broadcast_oneshot_active(void) { return 0; } +static inline int tick_is_oneshot_available(void) { return 0; }; static inline bool tick_broadcast_oneshot_available(void) { return false; } #endif /* !TICK_ONESHOT */
diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c index 39492f1..cb158f7 100644 --- a/kernel/time/tick-oneshot.c +++ b/kernel/time/tick-oneshot.c @@ -95,6 +95,20 @@ int tick_oneshot_mode_active(void) return tick_get_cpu_device()->mode == TICKDEV_MODE_ONESHOT; }
+/** + * tick_is_oneshot_available - check for a oneshot capable event device + */ +int tick_is_oneshot_available(void) +{ + struct clock_event_device *dev = tick_get_cpu_device()->evtdev; + + if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT)) + return 0; + if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) + return 1; + return tick_broadcast_oneshot_available(); +} + #ifdef CONFIG_HIGH_RES_TIMERS /** * tick_init_highres - switch to high resolution mode
tick_resume_oneshot() and tick_setup_oneshot() have almost the same implementation and tick_setup_oneshot() can be used easily instead of tick_resume_oneshot(). So, remove tick_resume_oneshot() and use tick_setup_oneshot() in its place.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-common.c | 8 +++++--- kernel/time/tick-internal.h | 5 ----- kernel/time/tick-oneshot.c | 11 ----------- 3 files changed, 5 insertions(+), 19 deletions(-)
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index 298467e..949f3d3 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -363,15 +363,17 @@ void tick_suspend(void) void tick_resume(void) { struct tick_device *td = tick_get_cpu_device(); + struct clock_event_device *dev = td->evtdev; int broadcast = tick_resume_broadcast();
- clockevents_set_mode(td->evtdev, CLOCK_EVT_MODE_RESUME); + clockevents_set_mode(dev, CLOCK_EVT_MODE_RESUME);
if (!broadcast) { if (td->mode == TICKDEV_MODE_PERIODIC) - tick_setup_periodic(td->evtdev, 0); + tick_setup_periodic(dev, 0); else - tick_resume_oneshot(); + tick_setup_oneshot(dev, dev->event_handler, + ktime_get()); } }
diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h index 09f4307..65f080a 100644 --- a/kernel/time/tick-internal.h +++ b/kernel/time/tick-internal.h @@ -43,7 +43,6 @@ extern int tick_program_event(ktime_t expires, int force); extern void tick_oneshot_notify(void); extern int tick_switch_to_oneshot(void (*handler)(struct clock_event_device *)); extern int tick_is_oneshot_available(void); -extern void tick_resume_oneshot(void); # ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST extern void tick_broadcast_setup_oneshot(struct clock_event_device *bc); extern int tick_broadcast_oneshot_control(unsigned long reason); @@ -74,10 +73,6 @@ void tick_setup_oneshot(struct clock_event_device *newdev, { BUG(); } -static inline void tick_resume_oneshot(void) -{ - BUG(); -} static inline int tick_program_event(ktime_t expires, int force) { return 0; diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c index cb158f7..82fc3f5 100644 --- a/kernel/time/tick-oneshot.c +++ b/kernel/time/tick-oneshot.c @@ -32,17 +32,6 @@ int tick_program_event(ktime_t expires, int force) }
/** - * tick_resume_onshot - resume oneshot mode - */ -void tick_resume_oneshot(void) -{ - struct clock_event_device *dev = tick_get_cpu_device()->evtdev; - - clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT); - clockevents_program_event(dev, ktime_get(), true); -} - -/** * tick_setup_oneshot - setup the event device for oneshot mode (hres or nohz) */ void tick_setup_oneshot(struct clock_event_device *newdev,
tick_check_new_device() is calling tick_check_percpu() which has this as the first test:
if (!cpumask_test_cpu(cpu, newdev->cpumask)) return false;
And so the same isn't required to be repeated by tick_check_new_device(). Remove it.
This also initializes several variables during their definition only.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-common.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index 949f3d3..395cbbd 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -269,16 +269,9 @@ bool tick_check_replacement(struct clock_event_device *curdev, */ void tick_check_new_device(struct clock_event_device *newdev) { - struct clock_event_device *curdev; - struct tick_device *td; - int cpu; - - cpu = smp_processor_id(); - if (!cpumask_test_cpu(cpu, newdev->cpumask)) - goto out_bc; - - td = tick_get_device(cpu); - curdev = td->evtdev; + int cpu = smp_processor_id(); + struct tick_device *td = tick_get_device(cpu); + struct clock_event_device *curdev = td->evtdev;
/* cpu local device ? */ if (!tick_check_percpu(curdev, newdev, cpu))
tick_check_replacement() returns if a replacement of clock_event_device is possible or not. It does this as the first check:
if (tick_check_percpu(curdev, newdev, smp_processor_id())) return false;
This looks wrong as we are returning false when tick_check_percpu() cpu returned true. Probably Thomas forgot '!' here in his commit: 03e13cf5e ?
Fix it by placing a '!' before tick_check_percpu().
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index 395cbbd..8650026 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -257,7 +257,7 @@ static bool tick_check_preferred(struct clock_event_device *curdev, bool tick_check_replacement(struct clock_event_device *curdev, struct clock_event_device *newdev) { - if (tick_check_percpu(curdev, newdev, smp_processor_id())) + if (!tick_check_percpu(curdev, newdev, smp_processor_id())) return false;
return tick_check_preferred(curdev, newdev);
tick_check_percpu() and tick_check_preferred() are called from two places and in exactly same order. So, would make sense to call tick_check_percpu() from tick_check_preferred() instead, so that their common caller can just call tick_check_preferred().
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-common.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index 8650026..6868d6a 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -233,6 +233,9 @@ static bool tick_check_percpu(struct clock_event_device *curdev, static bool tick_check_preferred(struct clock_event_device *curdev, struct clock_event_device *newdev) { + if (!tick_check_percpu(curdev, newdev, smp_processor_id())) + return false; + /* Prefer oneshot capable device */ if (!(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) { if (curdev && (curdev->features & CLOCK_EVT_FEAT_ONESHOT)) @@ -257,9 +260,6 @@ static bool tick_check_preferred(struct clock_event_device *curdev, bool tick_check_replacement(struct clock_event_device *curdev, struct clock_event_device *newdev) { - if (!tick_check_percpu(curdev, newdev, smp_processor_id())) - return false; - return tick_check_preferred(curdev, newdev); }
@@ -273,10 +273,6 @@ void tick_check_new_device(struct clock_event_device *newdev) struct tick_device *td = tick_get_device(cpu); struct clock_event_device *curdev = td->evtdev;
- /* cpu local device ? */ - if (!tick_check_percpu(curdev, newdev, cpu)) - goto out_bc; - /* Preference decision */ if (!tick_check_preferred(curdev, newdev)) goto out_bc;
If 'curdev' passed to tick_check_preferred() is the current clock_event_device then these two checks look exactly same, because td->mode = TICKDEV_MODE_ONESHOT is only called when the event device had ONESHOT feature.
if (curdev && (curdev->features & CLOCK_EVT_FEAT_ONESHOT)) return false;
if (tick_oneshot_mode_active()) return false;
Now left the case where 'curdev' is not the current clock_event_device. This can happen from the sequence started from clockevents_replace(). Here we are trying to find the best possible device that we should choose. And so even in this case we don't need the above check as we aren't really worried about the current device.
So, the second check can be removed.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-common.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index 6868d6a..3dc31f5 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -240,8 +240,6 @@ static bool tick_check_preferred(struct clock_event_device *curdev, if (!(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) { if (curdev && (curdev->features & CLOCK_EVT_FEAT_ONESHOT)) return false; - if (tick_oneshot_mode_active()) - return false; }
/*
We return false from tick_check_preferred() if newdev doesn't have ONESHOT feature but curdev has, but we don't return true when newdev has ONESHOT and curdev doesn't. Instead we go on, check ratings and other things in that case.
This patch tries to fix this by rewriting some portion of this code and adds sufficient comments to make logic clear.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-common.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index 3dc31f5..3bd9f9c 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -236,18 +236,29 @@ static bool tick_check_preferred(struct clock_event_device *curdev, if (!tick_check_percpu(curdev, newdev, smp_processor_id())) return false;
- /* Prefer oneshot capable device */ - if (!(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) { - if (curdev && (curdev->features & CLOCK_EVT_FEAT_ONESHOT)) - return false; - } + if (!curdev) + return true; + + /* + * Prefer oneshot capable device. + * return values based on if ONESHOT is available or not: + * + * curdev newdev operation + * 0 0 check priority + * 0 1 return true + * 1 0 return false + * 1 1 check priority + */ + + if ((newdev->features & CLOCK_EVT_FEAT_ONESHOT) ^ + (curdev->features & CLOCK_EVT_FEAT_ONESHOT)) + return newdev->features & CLOCK_EVT_FEAT_ONESHOT;
/* * Use the higher rated one, but prefer a CPU local device with a lower * rating than a non-CPU local device */ - return !curdev || - newdev->rating > curdev->rating || + return newdev->rating > curdev->rating || !cpumask_equal(curdev->cpumask, newdev->cpumask); }
tick_check_replacement() is now nothing more than a wrapper over tick_check_preferred() and hence can be removed. Use tick_check_preferred() everywhere.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/clockevents.c | 2 +- kernel/time/tick-common.c | 16 +++++----------- kernel/time/tick-internal.h | 2 +- 3 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index 6680a0d..d80e891 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -298,7 +298,7 @@ static int clockevents_replace(struct clock_event_device *ced) if (dev == ced || dev->mode != CLOCK_EVT_MODE_UNUSED) continue;
- if (!tick_check_replacement(newdev, dev)) + if (!tick_check_preferred(newdev, dev)) continue;
if (!try_module_get(dev->owner)) diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index 3bd9f9c..c1b3619 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -230,7 +230,11 @@ static bool tick_check_percpu(struct clock_event_device *curdev, return true; }
-static bool tick_check_preferred(struct clock_event_device *curdev, +/* + * Check whether the new device is a better fit than curdev. curdev + * can be NULL ! + */ +bool tick_check_preferred(struct clock_event_device *curdev, struct clock_event_device *newdev) { if (!tick_check_percpu(curdev, newdev, smp_processor_id())) @@ -263,16 +267,6 @@ static bool tick_check_preferred(struct clock_event_device *curdev, }
/* - * Check whether the new device is a better fit than curdev. curdev - * can be NULL ! - */ -bool tick_check_replacement(struct clock_event_device *curdev, - struct clock_event_device *newdev) -{ - return tick_check_preferred(curdev, newdev); -} - -/* * Check, if the new registered device should be used. Called with * clockevents_lock held and interrupts disabled. */ diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h index 65f080a..8c6e85d 100644 --- a/kernel/time/tick-internal.h +++ b/kernel/time/tick-internal.h @@ -24,7 +24,7 @@ extern void tick_handover_do_timer(int *cpup); extern void tick_shutdown(unsigned int *cpup); extern void tick_suspend(void); extern void tick_resume(void); -extern bool tick_check_replacement(struct clock_event_device *curdev, +extern bool tick_check_preferred(struct clock_event_device *curdev, struct clock_event_device *newdev); extern void tick_install_replacement(struct clock_event_device *dev);
tick_setup_device() is always passed cpumask of 'cpu', which is also passed as parameter to this routine. So, just calculate the cpumask(cpu) in tick_setup_device() instead of passing from every caller.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-common.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index c1b3619..c2aa441 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -145,9 +145,9 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast) * Setup the tick device */ static void tick_setup_device(struct tick_device *td, - struct clock_event_device *newdev, int cpu, - const struct cpumask *cpumask) + struct clock_event_device *newdev, int cpu) { + const struct cpumask *cpumask = cpumask_of(cpu); ktime_t next_event; void (*handler)(struct clock_event_device *) = NULL;
@@ -209,7 +209,7 @@ void tick_install_replacement(struct clock_event_device *newdev) int cpu = smp_processor_id();
clockevents_exchange_device(td->evtdev, newdev); - tick_setup_device(td, newdev, cpu, cpumask_of(cpu)); + tick_setup_device(td, newdev, cpu); if (newdev->features & CLOCK_EVT_FEAT_ONESHOT) tick_oneshot_notify(); } @@ -293,7 +293,7 @@ void tick_check_new_device(struct clock_event_device *newdev) curdev = NULL; } clockevents_exchange_device(curdev, newdev); - tick_setup_device(td, newdev, cpu, cpumask_of(cpu)); + tick_setup_device(td, newdev, cpu); if (newdev->features & CLOCK_EVT_FEAT_ONESHOT) tick_oneshot_notify(); return;
Part of tick_check_new_device() is exactly same as what's being done by tick_install_replacement(). And so we can actually call tick_install_replacement() directly from tick_check_new_device() instead of replicating code.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-common.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index c2aa441..bb9a218 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -272,9 +272,7 @@ bool tick_check_preferred(struct clock_event_device *curdev, */ void tick_check_new_device(struct clock_event_device *newdev) { - int cpu = smp_processor_id(); - struct tick_device *td = tick_get_device(cpu); - struct clock_event_device *curdev = td->evtdev; + struct clock_event_device *curdev = tick_get_cpu_device()->evtdev;
/* Preference decision */ if (!tick_check_preferred(curdev, newdev)) @@ -292,10 +290,8 @@ void tick_check_new_device(struct clock_event_device *newdev) clockevents_shutdown(curdev); curdev = NULL; } - clockevents_exchange_device(curdev, newdev); - tick_setup_device(td, newdev, cpu); - if (newdev->features & CLOCK_EVT_FEAT_ONESHOT) - tick_oneshot_notify(); + + tick_install_replacement(newdev); return;
out_bc:
tick_shutdown() calls clockevents_exchange_device() which already sets mode to CLOCK_EVT_MODE_UNUSED and so tick_shutdown() doesn't need to do it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-common.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index bb9a218..3954f4d 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -334,7 +334,6 @@ void tick_shutdown(unsigned int *cpup) * Prevent that the clock events layer tries to call * the set mode function! */ - dev->mode = CLOCK_EVT_MODE_UNUSED; clockevents_exchange_device(dev, NULL); dev->event_handler = clockevents_handle_noop; td->evtdev = NULL;
On 04/14/14 09:23, Viresh Kumar wrote:
tick_shutdown() calls clockevents_exchange_device() which already sets mode to CLOCK_EVT_MODE_UNUSED
Yes it does.
and so tick_shutdown() doesn't need to do it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
kernel/time/tick-common.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index bb9a218..3954f4d 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -334,7 +334,6 @@ void tick_shutdown(unsigned int *cpup) * Prevent that the clock events layer tries to call * the set mode function! */
dev->mode = CLOCK_EVT_MODE_UNUSED;
But see the comment above this assignment. The point is to avoid calling the ->set_mode function.
clockevents_exchange_device(dev, NULL); dev->event_handler = clockevents_handle_noop; td->evtdev = NULL;
On 15 April 2014 01:08, Stephen Boyd sboyd@codeaurora.org wrote:
But see the comment above this assignment. The point is to avoid calling the ->set_mode function.
Okay, that was a stupid change :)
'broadcast' is used just once and we can easily use tick_resume_broadcast() directly instead. Also it changes the code a bit to get rid of extra indentation level which was forcing us to break function calls into two lines.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-common.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index 3954f4d..e1b2947 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -351,17 +351,16 @@ void tick_resume(void) { struct tick_device *td = tick_get_cpu_device(); struct clock_event_device *dev = td->evtdev; - int broadcast = tick_resume_broadcast();
clockevents_set_mode(dev, CLOCK_EVT_MODE_RESUME);
- if (!broadcast) { - if (td->mode == TICKDEV_MODE_PERIODIC) - tick_setup_periodic(dev, 0); - else - tick_setup_oneshot(dev, dev->event_handler, - ktime_get()); - } + if (tick_resume_broadcast()) + return; + + if (td->mode == TICKDEV_MODE_PERIODIC) + tick_setup_periodic(dev, 0); + else + tick_setup_oneshot(dev, dev->event_handler, ktime_get()); }
/**
We are doing this currently:
int cpu;
cpu = smp_processor_id();
And we can rewrite it to make it shorter: int cpu = smp_processor_id();
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-sched.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 649b920..e9f9f87 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -274,7 +274,7 @@ out: /* Parse the boot-time nohz CPU list from the kernel parameters. */ static int __init tick_nohz_full_setup(char *str) { - int cpu; + int cpu = smp_processor_id();
alloc_bootmem_cpumask_var(&tick_nohz_full_mask); if (cpulist_parse(str, tick_nohz_full_mask) < 0) { @@ -282,7 +282,6 @@ static int __init tick_nohz_full_setup(char *str) return 1; }
- cpu = smp_processor_id(); if (cpumask_test_cpu(cpu, tick_nohz_full_mask)) { pr_warning("NO_HZ: Clearing %d from nohz_full range for timekeeping\n", cpu);
NO_HZ feature can be enabled/disabled from bootargs if we have a string 'nohz=' 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'.
Also, an extra blank line is removed.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-sched.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index e9f9f87..9cbba513 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -370,13 +370,10 @@ static int __init setup_tick_nohz(char *str) { if (!strcmp(str, "off")) tick_nohz_enabled = 0; - else if (!strcmp(str, "on")) - tick_nohz_enabled = 1; - else + else if (strcmp(str, "on")) return 0; return 1; } - __setup("nohz=", setup_tick_nohz);
/**
We have already checked if 'cpu' is online or not and so don't need to recheck it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-sched.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 9cbba513..c81b6cf 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -716,7 +716,7 @@ static bool can_stop_idle_tick(int cpu, struct tick_sched *ts) if (need_resched()) return false;
- if (unlikely(local_softirq_pending() && cpu_online(cpu))) { + if (unlikely(local_softirq_pending())) { static int ratelimit;
if (ratelimit < 10 &&
On 04/14/14 09:23, Viresh Kumar wrote:
We have already checked if 'cpu' is online or not and so don't need to recheck it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Hm... doing some git archeology shows fa116ea35ec7 (nohz: no softirq pending warnings for offline cpus, 2008-12-11), where the cpu_online() check was added. Before that commit we already checked cpu_online() similar to how the code is today. Perhaps we need to add a comment here?
kernel/time/tick-sched.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 9cbba513..c81b6cf 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -716,7 +716,7 @@ static bool can_stop_idle_tick(int cpu, struct tick_sched *ts) if (need_resched()) return false;
- if (unlikely(local_softirq_pending() && cpu_online(cpu))) {
- if (unlikely(local_softirq_pending())) { static int ratelimit;
if (ratelimit < 10 &&
On 15 April 2014 01:22, Stephen Boyd sboyd@codeaurora.org wrote:
On 04/14/14 09:23, Viresh Kumar wrote:
We have already checked if 'cpu' is online or not and so don't need to recheck it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Hm... doing some git archeology shows fa116ea35ec7 (nohz: no softirq pending warnings for offline cpus, 2008-12-11), where the cpu_online() check was added. Before that commit we already checked cpu_online() similar to how the code is today. Perhaps we need to add a comment here?
Okay, I didn't understood what you want completely. You want me to update commit logs or drop this patch and update code?
But yes there is one more thing worth mentioning:
commit f7ea0fd6 (tick: Don't invoke tick_nohz_stop_sched_tick() if the cpu is offline, May 13 2013), which did this:
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 0eed1db..0121421 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -469,6 +469,7 @@ static bool can_stop_idle_tick(int cpu, struct tick_sched *ts) if (unlikely(!cpu_online(cpu))) { if (cpu == tick_do_timer_cpu) tick_do_timer_cpu = TICK_DO_TIMER_NONE; + return false; }
if (unlikely(ts->nohz_mode == NOHZ_MODE_INACTIVE))
So, when the patch you mentioned was added, we didn't had this and a recheck of cpu_online() was perhaps required. But it is surely not required anymore.
There is only one caller of tick_check_oneshot_change(), i.e. hrtimer_run_pending(). Firstly hrtimer_run_pending() is calling this routine after doing a '!' of its parameter and then tick_check_oneshot_change() is also using it after doing a '!' of its parameter.
It would be more efficient and readable if we can just invert the meaning of this parameter. So, it is hres_enabled instead of allow_nohz now.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/tick.h | 6 +++--- kernel/hrtimer.c | 2 +- kernel/time/tick-sched.c | 10 +++++----- 3 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/include/linux/tick.h b/include/linux/tick.h index b8ee6f4..801e844 100644 --- a/include/linux/tick.h +++ b/include/linux/tick.h @@ -120,7 +120,7 @@ extern struct cpumask *tick_get_broadcast_oneshot_mask(void);
# ifdef CONFIG_TICK_ONESHOT extern void tick_clock_notify(void); -extern int tick_check_oneshot_change(int allow_nohz); +extern int tick_check_oneshot_change(int hres_enabled); extern struct tick_sched *tick_get_tick_sched(int cpu); extern void tick_irq_enter(void); extern int tick_oneshot_mode_active(void); @@ -129,7 +129,7 @@ extern int tick_oneshot_mode_active(void); # endif # else static inline void tick_clock_notify(void) { } -static inline int tick_check_oneshot_change(int allow_nohz) { return 0; } +static inline int tick_check_oneshot_change(int hres_enabled) { return 0; } static inline void tick_irq_enter(void) { } static inline int tick_oneshot_mode_active(void) { return 0; } # endif @@ -138,7 +138,7 @@ static inline int tick_oneshot_mode_active(void) { return 0; } static inline void tick_init(void) { } static inline void tick_cancel_sched_timer(int cpu) { } static inline void tick_clock_notify(void) { } -static inline int tick_check_oneshot_change(int allow_nohz) { return 0; } +static inline int tick_check_oneshot_change(int hres_enabled) { return 0; } static inline void tick_irq_enter(void) { } static inline int tick_oneshot_mode_active(void) { return 0; } #endif /* !CONFIG_GENERIC_CLOCKEVENTS */ diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index 393f422..bfa7811 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -1457,7 +1457,7 @@ void hrtimer_run_pending(void) * check bit in the tick_oneshot code, otherwise we might * deadlock vs. xtime_lock. */ - if (tick_check_oneshot_change(!hrtimer_is_hres_enabled())) + if (tick_check_oneshot_change(hrtimer_is_hres_enabled())) hrtimer_switch_to_hres(); }
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index c81b6cf..256f4a3 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -1177,11 +1177,11 @@ void tick_oneshot_notify(void) * Check, if a change happened, which makes oneshot possible. * * Called cyclic from the hrtimer softirq (driven by the timer - * softirq) allow_nohz signals, that we can switch into low-res nohz - * mode, because high resolution timers are disabled (either compile - * or runtime). + * softirq). If hres_enabled is non zero, it means we can't switch into low-res + * nohz mode, because high resolution timers are enabled(either compile or + * runtime). */ -int tick_check_oneshot_change(int allow_nohz) +int tick_check_oneshot_change(int hres_enabled) { struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched);
@@ -1194,7 +1194,7 @@ int tick_check_oneshot_change(int allow_nohz) if (!timekeeping_valid_for_hres() || !tick_is_oneshot_available()) return 0;
- if (!allow_nohz) + if (hres_enabled) return 1;
tick_nohz_switch_to_nohz();
__tick_nohz_task_switch() calls tick_nohz_full_kick(), which is already checking tick_nohz_full_cpu() and so we don't need to repeat the same check here.
Remove it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-sched.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 256f4a3..5a99859 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -261,13 +261,9 @@ void __tick_nohz_task_switch(struct task_struct *tsk)
local_irq_save(flags);
- if (!tick_nohz_full_cpu(smp_processor_id())) - goto out; - if (tick_nohz_tick_stopped() && !can_stop_full_tick()) tick_nohz_full_kick();
-out: local_irq_restore(flags); }
On Mon, Apr 14, 2014 at 09:53:46PM +0530, Viresh Kumar wrote:
__tick_nohz_task_switch() calls tick_nohz_full_kick(), which is already checking tick_nohz_full_cpu() and so we don't need to repeat the same check here.
Remove it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Ack.
kernel/time/tick-sched.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 256f4a3..5a99859 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -261,13 +261,9 @@ void __tick_nohz_task_switch(struct task_struct *tsk) local_irq_save(flags);
- if (!tick_nohz_full_cpu(smp_processor_id()))
goto out;- if (tick_nohz_tick_stopped() && !can_stop_full_tick()) tick_nohz_full_kick();
-out: local_irq_restore(flags); } -- 1.7.12.rc2.18.g61b472e
can_stop_idle_tick() checks if a local softirq is pending or not, and in case it is pending we call the same routine two more times.
Rather store the value in a local variable and reuse it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-sched.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 5a99859..b7b09b9 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -691,6 +691,8 @@ static void tick_nohz_full_stop_tick(struct tick_sched *ts)
static bool can_stop_idle_tick(int cpu, struct tick_sched *ts) { + unsigned int softirq; + /* * If this cpu is offline and it is the one which updates * jiffies, then give up the assignment and let it be taken by @@ -712,13 +714,11 @@ static bool can_stop_idle_tick(int cpu, struct tick_sched *ts) if (need_resched()) return false;
- if (unlikely(local_softirq_pending())) { + if (unlikely((softirq = local_softirq_pending()))) { static int ratelimit;
- if (ratelimit < 10 && - (local_softirq_pending() & SOFTIRQ_STOP_IDLE_MASK)) { - pr_warn("NOHZ: local_softirq_pending %02x\n", - (unsigned int) local_softirq_pending()); + if (ratelimit < 10 && (softirq & SOFTIRQ_STOP_IDLE_MASK)) { + pr_warn("NOHZ: local_softirq_pending %02x\n", softirq); ratelimit++; } return false;
In tick_do_update_jiffies64() we are processing ticks only if delta is greater than tick_period. This is what we are supposed to do here and it broke a bit with this patch:
commit 47a1b796306356f358e515149d86baf0cc6bf007 Author: John Stultz john.stultz@linaro.org Date: Thu Dec 12 13:10:55 2013 -0800
tick/timekeeping: Call update_wall_time outside the jiffies lock
With above patch, we might end up calling update_wall_time() even if delta is found to be smaller that tick_period. Fix this by reversing the check and returning early.
Cc: John Stultz john.stultz@linaro.org Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-sched.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index b7b09b9..bff7f97 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -65,26 +65,28 @@ static void tick_do_update_jiffies64(ktime_t now) write_seqlock(&jiffies_lock);
delta = ktime_sub(now, last_jiffies_update); - if (delta.tv64 >= tick_period.tv64) { - - delta = ktime_sub(delta, tick_period); - last_jiffies_update = ktime_add(last_jiffies_update, - tick_period); + if (delta.tv64 < tick_period.tv64) { + write_sequnlock(&jiffies_lock); + return; + }
- /* Slow path for long timeouts */ - if (unlikely(delta.tv64 >= tick_period.tv64)) { - s64 incr = ktime_to_ns(tick_period); + delta = ktime_sub(delta, tick_period); + last_jiffies_update = ktime_add(last_jiffies_update, tick_period);
- ticks = ktime_divns(delta, incr); + /* Slow path for long timeouts */ + if (unlikely(delta.tv64 >= tick_period.tv64)) { + s64 incr = ktime_to_ns(tick_period);
- last_jiffies_update = ktime_add_ns(last_jiffies_update, - incr * ticks); - } - do_timer(++ticks); + ticks = ktime_divns(delta, incr);
- /* Keep the tick_next_period variable up to date */ - tick_next_period = ktime_add(last_jiffies_update, tick_period); + last_jiffies_update = ktime_add_ns(last_jiffies_update, + incr * ticks); } + do_timer(++ticks); + + /* Keep the tick_next_period variable up to date */ + tick_next_period = ktime_add(last_jiffies_update, tick_period); + write_sequnlock(&jiffies_lock); update_wall_time(); }
tick_sched_handle() is called from two places and both pass 'regs' to it almost same way. This patch removes this parameter to tick_sched_handle() and updates tick_sched_handle() to get that by itself.
The only point of difference in the way this routine was called from its callers was, don't call it from process context. To make sure the nohz path doesn't suffer from an additional branch instruction, add unlikely to the if() statement.
Though I still don't understand when will we run tick_sched_timer() from process context. I couldn't see a single instance of that happening on my test machine for normal boot followed by some userspace operations.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-sched.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index bff7f97..ee0768b 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -129,8 +129,17 @@ static void tick_sched_do_timer(ktime_t now) tick_do_update_jiffies64(now); }
-static void tick_sched_handle(struct tick_sched *ts, struct pt_regs *regs) +static void tick_sched_handle(struct tick_sched *ts) { + struct pt_regs *regs = get_irq_regs(); + + /* + * Do not call, when we are not in irq context and have + * no valid regs pointer + */ + if (unlikely(!regs)) + return; + #ifdef CONFIG_NO_HZ_COMMON /* * When we are idle and the tick is stopped, we have to touch @@ -942,13 +951,12 @@ static int tick_nohz_reprogram(struct tick_sched *ts, ktime_t now) static void tick_nohz_handler(struct clock_event_device *dev) { struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched); - struct pt_regs *regs = get_irq_regs(); ktime_t now = ktime_get();
dev->next_event.tv64 = KTIME_MAX;
tick_sched_do_timer(now); - tick_sched_handle(ts, regs); + tick_sched_handle(ts);
while (tick_nohz_reprogram(ts, now)) { now = ktime_get(); @@ -1065,17 +1073,10 @@ static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer) { struct tick_sched *ts = container_of(timer, struct tick_sched, sched_timer); - struct pt_regs *regs = get_irq_regs(); ktime_t now = ktime_get();
tick_sched_do_timer(now); - - /* - * Do not call, when we are not in irq context and have - * no valid regs pointer - */ - if (regs) - tick_sched_handle(ts, regs); + tick_sched_handle(ts);
hrtimer_forward(timer, now, tick_period);
tick_nohz_task_switch() and __tick_nohz_task_switch() routines get task_struct passed to them (always for the 'current' task), but they never use it. Remove it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/tick.h | 8 ++++---- kernel/sched/core.c | 2 +- kernel/time/tick-sched.c | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/linux/tick.h b/include/linux/tick.h index 801e844..1065a51 100644 --- a/include/linux/tick.h +++ b/include/linux/tick.h @@ -201,7 +201,7 @@ extern void tick_nohz_init(void); extern void __tick_nohz_full_check(void); extern void tick_nohz_full_kick(void); extern void tick_nohz_full_kick_all(void); -extern void __tick_nohz_task_switch(struct task_struct *tsk); +extern void __tick_nohz_task_switch(void); #else static inline void tick_nohz_init(void) { } static inline bool tick_nohz_full_enabled(void) { return false; } @@ -209,7 +209,7 @@ static inline bool tick_nohz_full_cpu(int cpu) { return false; } static inline void __tick_nohz_full_check(void) { } static inline void tick_nohz_full_kick(void) { } static inline void tick_nohz_full_kick_all(void) { } -static inline void __tick_nohz_task_switch(struct task_struct *tsk) { } +static inline void __tick_nohz_task_switch(void) { } #endif
static inline void tick_nohz_full_check(void) @@ -218,10 +218,10 @@ static inline void tick_nohz_full_check(void) __tick_nohz_full_check(); }
-static inline void tick_nohz_task_switch(struct task_struct *tsk) +static inline void tick_nohz_task_switch(void) { if (tick_nohz_full_enabled()) - __tick_nohz_task_switch(tsk); + __tick_nohz_task_switch(); }
diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 13299c5..84dc349 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -2156,7 +2156,7 @@ static void finish_task_switch(struct rq *rq, struct task_struct *prev) put_task_struct(prev); }
- tick_nohz_task_switch(current); + tick_nohz_task_switch(); }
#ifdef CONFIG_SMP diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index ee0768b..5f7796d 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -266,7 +266,7 @@ void tick_nohz_full_kick_all(void) * It might need the tick due to per task/process properties: * perf events, posix cpu timers, ... */ -void __tick_nohz_task_switch(struct task_struct *tsk) +void __tick_nohz_task_switch(void) { unsigned long flags;
On Mon, Apr 14, 2014 at 09:53:50PM +0530, Viresh Kumar wrote:
tick_nohz_task_switch() and __tick_nohz_task_switch() routines get task_struct passed to them (always for the 'current' task), but they never use it. Remove it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Ack.
__tick_nohz_task_switch() was called only from tick_nohz_task_switch() and there is nothing much in tick_nohz_task_switch() as well. IOW, we don't need unnecessary wrapper over __tick_nohz_task_switch() to be there. Merge all code from __tick_nohz_task_switch() into tick_nohz_task_switch() and move it to tick-sched.c.
This also moves check for tick_nohz_tick_stopped() outside of irq_save() context.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/tick.h | 11 ++--------- kernel/time/tick-sched.c | 7 +++++-- 2 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/include/linux/tick.h b/include/linux/tick.h index 1065a51..585be84 100644 --- a/include/linux/tick.h +++ b/include/linux/tick.h @@ -201,7 +201,7 @@ extern void tick_nohz_init(void); extern void __tick_nohz_full_check(void); extern void tick_nohz_full_kick(void); extern void tick_nohz_full_kick_all(void); -extern void __tick_nohz_task_switch(void); +extern void tick_nohz_task_switch(void); #else static inline void tick_nohz_init(void) { } static inline bool tick_nohz_full_enabled(void) { return false; } @@ -209,7 +209,7 @@ static inline bool tick_nohz_full_cpu(int cpu) { return false; } static inline void __tick_nohz_full_check(void) { } static inline void tick_nohz_full_kick(void) { } static inline void tick_nohz_full_kick_all(void) { } -static inline void __tick_nohz_task_switch(void) { } +static inline void tick_nohz_task_switch(void) { } #endif
static inline void tick_nohz_full_check(void) @@ -218,11 +218,4 @@ static inline void tick_nohz_full_check(void) __tick_nohz_full_check(); }
-static inline void tick_nohz_task_switch(void) -{ - if (tick_nohz_full_enabled()) - __tick_nohz_task_switch(); -} - - #endif diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 5f7796d..d8b9a69 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -266,13 +266,16 @@ void tick_nohz_full_kick_all(void) * It might need the tick due to per task/process properties: * perf events, posix cpu timers, ... */ -void __tick_nohz_task_switch(void) +void tick_nohz_task_switch(void) { unsigned long flags;
+ if (!tick_nohz_full_enabled() || !tick_nohz_tick_stopped()) + return; + local_irq_save(flags);
- if (tick_nohz_tick_stopped() && !can_stop_full_tick()) + if (!can_stop_full_tick()) tick_nohz_full_kick();
local_irq_restore(flags);
On Mon, Apr 14, 2014 at 09:53:51PM +0530, Viresh Kumar wrote:
__tick_nohz_task_switch() was called only from tick_nohz_task_switch() and there is nothing much in tick_nohz_task_switch() as well. IOW, we don't need unnecessary wrapper over __tick_nohz_task_switch() to be there. Merge all code from __tick_nohz_task_switch() into tick_nohz_task_switch() and move it to tick-sched.c.
This also moves check for tick_nohz_tick_stopped() outside of irq_save() context.
No, the wrapper is there on purpose in order to optimize the full dynticks off case in the context switch path with the jump label'ed check on tick_nohz_full_enabled().
On 15 April 2014 04:52, Frederic Weisbecker fweisbec@gmail.com wrote:
On Mon, Apr 14, 2014 at 09:53:51PM +0530, Viresh Kumar wrote:
__tick_nohz_task_switch() was called only from tick_nohz_task_switch() and there is nothing much in tick_nohz_task_switch() as well. IOW, we don't need unnecessary wrapper over __tick_nohz_task_switch() to be there. Merge all code from __tick_nohz_task_switch() into tick_nohz_task_switch() and move it to tick-sched.c.
This also moves check for tick_nohz_tick_stopped() outside of irq_save() context.
No, the wrapper is there on purpose in order to optimize the full dynticks off case in the context switch path with the jump label'ed check on tick_nohz_full_enabled().
Just to clarify, you are saying that:
Wrapper was there to save an extra function call when tick_nohz_full_enabled() returns false, as tick_nohz_task_switch() will be inlined ?
In this case probably we can move !can_stop_full_tick() as well to the wrapper ?
On Tue, Apr 15, 2014 at 10:15:24AM +0530, Viresh Kumar wrote:
On 15 April 2014 04:52, Frederic Weisbecker fweisbec@gmail.com wrote:
On Mon, Apr 14, 2014 at 09:53:51PM +0530, Viresh Kumar wrote:
__tick_nohz_task_switch() was called only from tick_nohz_task_switch() and there is nothing much in tick_nohz_task_switch() as well. IOW, we don't need unnecessary wrapper over __tick_nohz_task_switch() to be there. Merge all code from __tick_nohz_task_switch() into tick_nohz_task_switch() and move it to tick-sched.c.
This also moves check for tick_nohz_tick_stopped() outside of irq_save() context.
No, the wrapper is there on purpose in order to optimize the full dynticks off case in the context switch path with the jump label'ed check on tick_nohz_full_enabled().
Just to clarify, you are saying that:
Wrapper was there to save an extra function call when tick_nohz_full_enabled() returns false, as tick_nohz_task_switch() will be inlined ?
Yeah. But not just that.
Using an inline saves a function call and reduce the offline case to a simple condition check. But there is also the jump label that reduce the condition check to an unconditional jump in the off case.
To summarize, here's how calling tick_nohz_task_switch() maps to final C code:
finish_task_switch() { //do things before calling tick_nohz_task_switch()... // call tick_nohz_task_switch goto offcase; if (tick_nohz_full_enabled()) __tick_nohz_task_switch(tsk); offcase: //end of call to tick_nohz_task_switch //do things before calling tick_nohz_task_switch()... }
In the offcase, the code is like above. We don't even do the check, thanks to the jump label code we unconditionally jump to what's next in finish_task_switch() (there is actually nothing afterward but that's for the picture).
Now if there is at least a CPU that is full dynticks on boot, it is enabled with context_tracking_cpu_set(). Then the jump label code patches the code in finish_task_switch() to turn the goto offcase into a nop. Then the condition is actually verified on every call to finish_task_switch().
So it goes beyond than just saving a function call.
In this case probably we can move !can_stop_full_tick() as well to the wrapper ?
Do you mean moving all the code of __tick_nohz_task_switch() to tick_nohz_task_switch()? I much prefer we don't do that. This is going to make can_stop_full_tick() a publicly visible nohz internal. And it may uglify tick.h as well.
On 15 April 2014 14:43, Frederic Weisbecker fweisbec@gmail.com wrote:
Yeah. But not just that.
Using an inline saves a function call and reduce the offline case to a simple condition check. But there is also the jump label that reduce the condition check to an unconditional jump in the off case.
To summarize, here's how calling tick_nohz_task_switch() maps to final C code:
finish_task_switch() { //do things before calling tick_nohz_task_switch()... // call tick_nohz_task_switch goto offcase; if (tick_nohz_full_enabled()) __tick_nohz_task_switch(tsk); offcase: //end of call to tick_nohz_task_switch //do things before calling tick_nohz_task_switch()... }
In the offcase, the code is like above. We don't even do the check, thanks to the jump label code we unconditionally jump to what's next in finish_task_switch() (there is actually nothing afterward but that's for the picture).
Now if there is at least a CPU that is full dynticks on boot, it is enabled with context_tracking_cpu_set(). Then the jump label code patches the code in finish_task_switch() to turn the goto offcase into a nop. Then the condition is actually verified on every call to finish_task_switch().
So it goes beyond than just saving a function call.
Sorry, but my poor mind still couldn't understand what you are trying to tell me :(
So lets clarify things one by one :)
- What do you mean by offcase? CONFIG_NO_HZ_FULL not configured into the kernel or it is configured but none of the CPUs is running in that mode?
- Also what does it correspond to in code: goto offcase; ? There is no labels or goto statements in code that I can see.. This is how the code looks to me.
finish_task_switch() { //do things before calling tick_nohz_task_switch()... // call tick_nohz_task_switch if (tick_nohz_full_enabled()) __tick_nohz_task_switch(tsk); }
__tick_nohz_task_switch() may or maynot be available at all depending on CONFIG_NO_HZ_FULL is enabled into the kernel or not. But that was the case with tick_nohz_task_switch() as well in my patch. So shouldn't make a difference..
Again, sorry for not understanding what you are trying to explain here. I want to understand this once and for all and probably add a comment here as well :)
-- viresh
On Tue, Apr 15, 2014 at 03:23:37PM +0530, Viresh Kumar wrote:
On 15 April 2014 14:43, Frederic Weisbecker fweisbec@gmail.com wrote:
Yeah. But not just that.
Using an inline saves a function call and reduce the offline case to a simple condition check. But there is also the jump label that reduce the condition check to an unconditional jump in the off case.
To summarize, here's how calling tick_nohz_task_switch() maps to final C code:
finish_task_switch() { //do things before calling tick_nohz_task_switch()... // call tick_nohz_task_switch goto offcase; if (tick_nohz_full_enabled()) __tick_nohz_task_switch(tsk); offcase: //end of call to tick_nohz_task_switch //do things before calling tick_nohz_task_switch()... }
In the offcase, the code is like above. We don't even do the check, thanks to the jump label code we unconditionally jump to what's next in finish_task_switch() (there is actually nothing afterward but that's for the picture).
Now if there is at least a CPU that is full dynticks on boot, it is enabled with context_tracking_cpu_set(). Then the jump label code patches the code in finish_task_switch() to turn the goto offcase into a nop. Then the condition is actually verified on every call to finish_task_switch().
So it goes beyond than just saving a function call.
Sorry, but my poor mind still couldn't understand what you are trying to tell me :(
Welcome to the club of the daily confused people. I'm happy to hear I'm not alone :)
So lets clarify things one by one :)
- What do you mean by offcase? CONFIG_NO_HZ_FULL not configured
into the kernel or it is configured but none of the CPUs is running in that mode?
So by offcase I mean CONFIG_NO_HZ_FULL=y but the nohz_full boot parameter is empty, or simply not passed at all. And of course CONFIG_NO_HZ_FULL_ALL=n
This config is now likely on some distros because we want to make full dynticks available for users who want it. But if it's not used (which is 99.999% of the usecases), we want to minimize as much as possible its overhead.
Lets call that dynamic off-case.
- Also what does it correspond to in code: goto offcase; ? There is no labels
or goto statements in code that I can see.. This is how the code looks to me.
finish_task_switch() { //do things before calling tick_nohz_task_switch()... // call tick_nohz_task_switch if (tick_nohz_full_enabled()) __tick_nohz_task_switch(tsk); }
Sure but check out the static_key_false() in the implementation of tick_nohz_full_enabled(). That's where the magic hides.
__tick_nohz_task_switch() may or maynot be available at all depending on CONFIG_NO_HZ_FULL is enabled into the kernel or not. But that was the case with tick_nohz_task_switch() as well in my patch. So shouldn't make a difference..
Again, sorry for not understanding what you are trying to explain here. I want to understand this once and for all and probably add a comment here as well :)
No problem, the jump label/static key code is quite tricky. And its use can be easily missed, as in here.
Also its unfamous API naming (static_key_true/static_key_true) that is anything but intuitive.
-- viresh
On 15 April 2014 18:14, Frederic Weisbecker fweisbec@gmail.com wrote:
Sure but check out the static_key_false() in the implementation of tick_nohz_full_enabled(). That's where the magic hides.
No problem, the jump label/static key code is quite tricky. And its use can be easily missed, as in here.
Also its unfamous API naming (static_key_true/static_key_true) that is anything but intuitive.
That was tricky enough to be missed on first look :)
Okay here is the answer to what you asked earlier:
In this case probably we can move !can_stop_full_tick() as well to the wrapper ?
Do you mean moving all the code of __tick_nohz_task_switch() to tick_nohz_task_switch()? I much prefer we don't do that. This is going to make can_stop_full_tick() a publicly visible nohz internal. And it may uglify tick.h as well.
I probably asked the wrong question, I meant tick_nohz_tick_stopped() instead of !can_stop_full_tick().
Or, can we make the code more efficient by avoiding a branch by doing this:
diff --git a/include/linux/tick.h b/include/linux/tick.h index 1065a51..12632cc 100644 --- a/include/linux/tick.h +++ b/include/linux/tick.h @@ -220,7 +220,7 @@ static inline void tick_nohz_full_check(void)
static inline void tick_nohz_task_switch(void) { - if (tick_nohz_full_enabled()) + if (tick_nohz_full_enabled() && tick_nohz_tick_stopped()) __tick_nohz_task_switch(); }
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 45037c4..904e09b 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -273,7 +273,7 @@ void __tick_nohz_task_switch(void)
local_irq_save(flags);
- if (tick_nohz_tick_stopped() && !can_stop_full_tick()) + if (!can_stop_full_tick()) tick_nohz_full_kick();
local_irq_restore(flags);
nohz_full_buf[] is used at only one place, i.e. inside tick_nohz_init(). Make it a local variable. Can move it out in case it is used in some other routines in future.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-sched.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index d8b9a69..22b9505 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -321,13 +321,6 @@ static int tick_nohz_cpu_down_callback(struct notifier_block *nfb, return NOTIFY_OK; }
-/* - * Worst case string length in chunks of CPU range seems 2 steps - * separations: 0,2,4,6,... - * This is NR_CPUS + sizeof('\0') - */ -static char __initdata nohz_full_buf[NR_CPUS + 1]; - static int tick_nohz_init_all(void) { int err = -1; @@ -347,6 +340,12 @@ static int tick_nohz_init_all(void)
void __init tick_nohz_init(void) { + /* + * Worst case string length in chunks of CPU range seems 2 steps + * separations: 0,2,4,6,... + * This is NR_CPUS + sizeof('\0') + */ + char nohz_full_buf[NR_CPUS + 1]; int cpu;
if (!tick_nohz_full_running) {
On Mon, Apr 14, 2014 at 09:53:52PM +0530, Viresh Kumar wrote:
nohz_full_buf[] is used at only one place, i.e. inside tick_nohz_init(). Make it a local variable. Can move it out in case it is used in some other routines in future.
OTOH nohz_full_buf can have a big size and moving it to a local variable will result in big increase in kernel stack consumption.
Keeping it static is safer.
Also it's already __initdata so it's released after bootup. No issue of concurrent use either, so keeping it as is is fine.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
kernel/time/tick-sched.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index d8b9a69..22b9505 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -321,13 +321,6 @@ static int tick_nohz_cpu_down_callback(struct notifier_block *nfb, return NOTIFY_OK; } -/*
- Worst case string length in chunks of CPU range seems 2 steps
- separations: 0,2,4,6,...
- This is NR_CPUS + sizeof('\0')
- */
-static char __initdata nohz_full_buf[NR_CPUS + 1];
static int tick_nohz_init_all(void) { int err = -1; @@ -347,6 +340,12 @@ static int tick_nohz_init_all(void) void __init tick_nohz_init(void) {
- /*
* Worst case string length in chunks of CPU range seems 2 steps* separations: 0,2,4,6,...* This is NR_CPUS + sizeof('\0')*/- char nohz_full_buf[NR_CPUS + 1]; int cpu;
if (!tick_nohz_full_running) { -- 1.7.12.rc2.18.g61b472e
On 15 April 2014 04:58, Frederic Weisbecker fweisbec@gmail.com wrote:
OTOH nohz_full_buf can have a big size and moving it to a local variable will result in big increase in kernel stack consumption.
Keeping it static is safer.
Also it's already __initdata so it's released after bootup. No issue of concurrent use either, so keeping it as is is fine.
Okay, this patch is dropped.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-sched.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 22b9505..f1bc258 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -792,7 +792,7 @@ static void __tick_nohz_idle_enter(struct tick_sched *ts) */ void tick_nohz_idle_enter(void) { - struct tick_sched *ts; + struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched);
WARN_ON_ONCE(irqs_disabled());
@@ -806,7 +806,6 @@ void tick_nohz_idle_enter(void)
local_irq_disable();
- ts = &__get_cpu_var(tick_cpu_sched); ts->inidle = 1; __tick_nohz_idle_enter(ts);
The sequence of calls for dynticks CPUs was a bit confusing and so adding a comment in tick_nohz_idle_exit() routine to mention it clearly. All information required is in commit and this conversation with Frederic.
https://lkml.org/lkml/2014/4/10/355
Suggested-by: Frederic Weisbecker fweisbec@gmail.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-sched.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index f1bc258..85a4e90 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -925,6 +925,22 @@ void tick_nohz_idle_exit(void)
ts->inidle = 0;
+ /* + * Can idle_active be false here? + * Ideally this would be the sequence of calls: + * - tick_nohz_idle_enter(), i.e. idle_active = true; + * - local_irq_disable() + * - IDLE + * - wake up due to IPI or other interrupt + * - local_irq_enable() + * - tick_nohz_irq_enter(), i.e. idle_active = false; + * - tick_nohz_irq_exit(), i.e. idle_active = true; This is not called + * in case of IPI's as need_resched() will prevent that in + * tick_irq_exit(), as we don't need to account any more for idle time + * or try to enter dyntics mode (We are going to exit idle state). + * + * - tick_nohz_idle_exit() + */ if (ts->idle_active || ts->tick_stopped) now = ktime_get();
Recently this patch came in:
commit d689fe222a858c767cb8594faf280048e532b53f Author: Thomas Gleixner tglx@linutronix.de Date: Wed Nov 13 21:01:57 2013 +0100
NOHZ: Check for nohz active instead of nohz enabled
and introduced a bug here. tick_nohz_active is initialized to zero and we are returning from this routine if it is zero. And hence we will never be able to cross this check. Actually this should not have been changed by above patch and should have stayed to tick_nohz_enabled.
Fix it.
Cc: stable@vger.kernel.org # 3.13.x Cc: stable@vger.kernel.org # 3.14.x Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-sched.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index 85a4e90..a0a6705 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -989,7 +989,7 @@ static void tick_nohz_switch_to_nohz(void) struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched); ktime_t next;
- if (!tick_nohz_active) + if (!tick_nohz_enabled) return;
local_irq_disable();
Local variable 'now' is used at only one place and so can be easily replaced by ktime_get() instead.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-sched.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c index a0a6705..78751b3 100644 --- a/kernel/time/tick-sched.c +++ b/kernel/time/tick-sched.c @@ -1116,7 +1116,6 @@ early_param("skew_tick", skew_tick); void tick_setup_sched_timer(void) { struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched); - ktime_t now = ktime_get();
/* * Emulate tick processing via per-CPU hrtimers: @@ -1136,13 +1135,12 @@ void tick_setup_sched_timer(void) }
for (;;) { - hrtimer_forward(&ts->sched_timer, now, tick_period); + hrtimer_forward(&ts->sched_timer, ktime_get(), tick_period); hrtimer_start_expires(&ts->sched_timer, HRTIMER_MODE_ABS_PINNED); /* Check, if the timer was already in the past */ if (hrtimer_active(&ts->sched_timer)) break; - now = ktime_get(); }
#ifdef CONFIG_NO_HZ_COMMON
Some of the checks which may force tick_do_broadcast_on_off() to return early can be done before taking locks. This would make these codepaths faster.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-broadcast.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index 1597d03..3b07569 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -329,27 +329,22 @@ unlock: */ static void tick_do_broadcast_on_off(unsigned long *reason) { - struct clock_event_device *bc, *dev; - struct tick_device *td; + struct tick_device *td = tick_get_cpu_device(); + struct clock_event_device *bc, *dev = td->evtdev; + int cpu = smp_processor_id(), bc_stopped; unsigned long flags; - int cpu, bc_stopped; - - raw_spin_lock_irqsave(&tick_broadcast_lock, flags); - - cpu = smp_processor_id(); - td = tick_get_device(cpu); - dev = td->evtdev; - bc = tick_broadcast_device.evtdev;
/* * Is the device not affected by the powerstate ? */ if (!dev || !(dev->features & CLOCK_EVT_FEAT_C3STOP)) - goto out; + return;
if (!tick_device_is_functional(dev)) - goto out; + return;
+ raw_spin_lock_irqsave(&tick_broadcast_lock, flags); + bc = tick_broadcast_device.evtdev; bc_stopped = cpumask_empty(tick_broadcast_mask);
switch (*reason) { @@ -385,7 +380,6 @@ static void tick_do_broadcast_on_off(unsigned long *reason) else tick_broadcast_setup_oneshot(bc); } -out: raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags); }
We are doing an extra comparison in tick_do_broadcast_on_off():
if (*reason == CLOCK_EVT_NOTIFY_BROADCAST_FORCE) tick_broadcast_force = 1;
Whereas it can be handled easily in the switch block only.
It doesn't look like there is any strict ordering of instructions required here and so move setting 'tick_broadcast_force' at the beginning.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-broadcast.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index 3b07569..ec86227 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -348,15 +348,14 @@ static void tick_do_broadcast_on_off(unsigned long *reason) bc_stopped = cpumask_empty(tick_broadcast_mask);
switch (*reason) { - case CLOCK_EVT_NOTIFY_BROADCAST_ON: case CLOCK_EVT_NOTIFY_BROADCAST_FORCE: + tick_broadcast_force = 1; + case CLOCK_EVT_NOTIFY_BROADCAST_ON: cpumask_set_cpu(cpu, tick_broadcast_on); if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_mask)) { if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC) clockevents_shutdown(dev); } - if (*reason == CLOCK_EVT_NOTIFY_BROADCAST_FORCE) - tick_broadcast_force = 1; break; case CLOCK_EVT_NOTIFY_BROADCAST_OFF: if (tick_broadcast_force)
We have added a wrapper around tick_do_broadcast_on_off() which doesn't have anything so special that we should keep it. It just had a check if cpu is online or not. Just merge these two routines and keep one: tick_broadcast_on_off(). This would get rid of an extra function call that we are doing currently.
This also replaces printk(KERN_ERR "**"); with pr_err("**");
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- kernel/time/tick-broadcast.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-)
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index ec86227..9d81a81 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -327,13 +327,19 @@ unlock: * Powerstate information: The system enters/leaves a state, where * affected devices might stop */ -static void tick_do_broadcast_on_off(unsigned long *reason) +void tick_broadcast_on_off(unsigned long reason, int *oncpu) { struct tick_device *td = tick_get_cpu_device(); struct clock_event_device *bc, *dev = td->evtdev; int cpu = smp_processor_id(), bc_stopped; unsigned long flags;
+ if (!cpumask_test_cpu(*oncpu, cpu_online_mask)) { + pr_err("tick-broadcast: ignoring broadcast for offline CPU #%d\n", + *oncpu); + return; + } + /* * Is the device not affected by the powerstate ? */ @@ -347,7 +353,7 @@ static void tick_do_broadcast_on_off(unsigned long *reason) bc = tick_broadcast_device.evtdev; bc_stopped = cpumask_empty(tick_broadcast_mask);
- switch (*reason) { + switch (reason) { case CLOCK_EVT_NOTIFY_BROADCAST_FORCE: tick_broadcast_force = 1; case CLOCK_EVT_NOTIFY_BROADCAST_ON: @@ -383,19 +389,6 @@ static void tick_do_broadcast_on_off(unsigned long *reason) }
/* - * Powerstate information: The system enters/leaves a state, where - * affected devices might stop. - */ -void tick_broadcast_on_off(unsigned long reason, int *oncpu) -{ - if (!cpumask_test_cpu(*oncpu, cpu_online_mask)) - printk(KERN_ERR "tick-broadcast: ignoring broadcast for offline CPU #%d\n", - *oncpu); - else - tick_do_broadcast_on_off(&reason); -} - -/* * Set the periodic handler depending on broadcast on/off */ void tick_set_periodic_handler(struct clock_event_device *dev, int broadcast)
All users of clockevents_handle_noop() are setting old->event_handler to it after calling clockevents_exchange_device() and so it would be better if we can do this at a single place. The only thing we need to make sure is, we preserve it before calling clockevents_exchange_device() in case we need it.
As there are no external users of it, make it static.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- include/linux/clockchips.h | 2 -- kernel/time/clockevents.c | 3 ++- kernel/time/tick-broadcast.c | 2 -- kernel/time/tick-common.c | 11 +++++------ 4 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 2e4cb67..7e492ad 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -165,8 +165,6 @@ extern void clockevents_set_mode(struct clock_event_device *dev, extern int clockevents_program_event(struct clock_event_device *dev, ktime_t expires, bool force);
-extern void clockevents_handle_noop(struct clock_event_device *dev); - static inline void clockevents_calc_mult_shift(struct clock_event_device *ce, u32 freq, u32 minsec) { diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c index d80e891..757dae8 100644 --- a/kernel/time/clockevents.c +++ b/kernel/time/clockevents.c @@ -480,7 +480,7 @@ int clockevents_update_freq(struct clock_event_device *dev, u32 freq) /* * Noop handler when we shut down an event device */ -void clockevents_handle_noop(struct clock_event_device *dev) +static void clockevents_handle_noop(struct clock_event_device *dev) { }
@@ -506,6 +506,7 @@ void clockevents_exchange_device(struct clock_event_device *old, clockevents_set_mode(old, CLOCK_EVT_MODE_UNUSED); list_del(&old->list); list_add(&old->list, &clockevents_released); + old->event_handler = clockevents_handle_noop; }
if (new) { diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index 9d81a81..bb96da7 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -95,8 +95,6 @@ void tick_install_broadcast_device(struct clock_event_device *dev) return;
clockevents_exchange_device(cur, dev); - if (cur) - cur->event_handler = clockevents_handle_noop; tick_broadcast_device.evtdev = dev; if (!cpumask_empty(tick_broadcast_mask)) tick_broadcast_start_periodic(dev); diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c index e1b2947..b0d14e3 100644 --- a/kernel/time/tick-common.c +++ b/kernel/time/tick-common.c @@ -145,11 +145,11 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast) * Setup the tick device */ static void tick_setup_device(struct tick_device *td, - struct clock_event_device *newdev, int cpu) + struct clock_event_device *newdev, int cpu, + void (*handler)(struct clock_event_device *)) { const struct cpumask *cpumask = cpumask_of(cpu); ktime_t next_event; - void (*handler)(struct clock_event_device *) = NULL;
/* * First device setup ? @@ -173,9 +173,7 @@ static void tick_setup_device(struct tick_device *td, */ td->mode = TICKDEV_MODE_PERIODIC; } else { - handler = td->evtdev->event_handler; next_event = td->evtdev->next_event; - td->evtdev->event_handler = clockevents_handle_noop; }
td->evtdev = newdev; @@ -206,10 +204,12 @@ static void tick_setup_device(struct tick_device *td, void tick_install_replacement(struct clock_event_device *newdev) { struct tick_device *td = tick_get_cpu_device(); + void (*handler)(struct clock_event_device *) = + td->evtdev->event_handler; int cpu = smp_processor_id();
clockevents_exchange_device(td->evtdev, newdev); - tick_setup_device(td, newdev, cpu); + tick_setup_device(td, newdev, cpu, handler); if (newdev->features & CLOCK_EVT_FEAT_ONESHOT) tick_oneshot_notify(); } @@ -335,7 +335,6 @@ void tick_shutdown(unsigned int *cpup) * the set mode function! */ clockevents_exchange_device(dev, NULL); - dev->event_handler = clockevents_handle_noop; td->evtdev = NULL; } }
On Mon, 14 Apr 2014, Viresh Kumar wrote:
Hi Thomas,
These are separate cleanups from the timers/hrtimers ones I did. I was waiting for the merge window to close in order to send these and by the time it happened, I got a long pending list.
These are mostly cleanups, reorders for better readability or efficiency, and few bugfixes.
And that's wrong to begin with.
Bugfixes first and then all other stuff. We dont want dependencies of bugfixes on cleanups, reordering of code ...
I'm not applying a wholesale checkpatch.pl patch which creates noise for no value.
I don't mind if you cleanup stuff while doing other changes, but definitely not as a stand alone starter of a large patch queue with bugfixes which depend on that.
Now looking at the thing some more, it contains gems like this:
- printk(KERN_ERR "tick-broadcast: ignoring broadcast for " - "offline CPU #%d\n", *oncpu); + printk(KERN_ERR "tick-broadcast: ignoring broadcast for offline CPU #%d\n", + *oncpu);
If you fix that checkpatch.pl line wrap issue, why do you not change printk(KERN_ERR) to pr_err() as well?
checkpatch.pl is happy, right?
I'm tired of this, really.
Please send me the next series in the following way:
- send a bug fix series, which does nothing else than fixing bugs.
when that is applied, then
- send a small batch of improvements for a particular issue and not a mixed bag of random patches.
Thanks,
tglx
On 15 April 2014 05:08, Thomas Gleixner tglx@linutronix.de wrote:
And that's wrong to begin with.
Bugfixes first and then all other stuff. We dont want dependencies of bugfixes on cleanups, reordering of code ...
Correct, should have taken care of this initially :(
Now looking at the thing some more, it contains gems like this:
printk(KERN_ERR "tick-broadcast: ignoring broadcast for ""offline CPU #%d\n", *oncpu);
printk(KERN_ERR "tick-broadcast: ignoring broadcast for offline CPU #%d\n",*oncpu);If you fix that checkpatch.pl line wrap issue, why do you not change printk(KERN_ERR) to pr_err() as well?
checkpatch.pl is happy, right?
I did this for one patch and missed it for other.. I will fix all printk's in kernel/time/ now :)
Please send me the next series in the following way:
- send a bug fix series, which does nothing else than fixing bugs.
Right.
when that is applied, then
- send a small batch of improvements for a particular issue and not a mixed bag of random patches.
Okay.. Thanks.
linaro-kernel@lists.linaro.org
-
Frederic Weisbecker -
Stephen Boyd -
Thomas Gleixner -
Viresh Kumar