tick cleanups and bugfixes
cover tick cleanups and bugfixes
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(-)
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 1/38 tick: align to Coding Guidelines
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
--- 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(-)
include/linux/tick.h +2 -1
@@ -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 */
kernel/time/tick-broadcast.c +6 -10
@@ -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 */
kernel/time/tick-internal.h +4 -1
@@ -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) {
kernel/time/tick-oneshot.c +1 -2
@@ -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 {
kernel/time/tick-sched.c +16 -13
@@ -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);
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 2/38 tick: update doc comments for struct tick_sched
In reply to Viresh KumarSome fields of 'struct tick_sched' didn't had a description in the kernel doc comment present above its declaration. Add them.
--- include/linux/tick.h | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
include/linux/tick.h +9 -1
@@ -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;
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 3/38 tick: rearrange members of 'struct tick_sched'
In reply to Viresh KumarRearrange 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
--- include/linux/tick.h | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
include/linux/tick.h +17 -17
@@ -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; };
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 4/38 tick: move declaration of tick_cpu_device to tick.h
In reply to Viresh Kumartick_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.
--- include/linux/hrtimer.h | 3 --- include/linux/tick.h | 2 ++ kernel/time/tick-internal.h | 1 - 3 files changed, 2 insertions(+), 4 deletions(-)
include/linux/hrtimer.h +0 -3
@@ -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: */
include/linux/tick.h +2 -0
@@ -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
kernel/time/tick-internal.h +0 -1
@@ -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 ;
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 5/38 tick: move definition of tick_get_device() to tick.h
In reply to Viresh KumarThere 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.
--- 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(-)
include/linux/tick.h +4 -1
@@ -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 );
kernel/time/clockevents.c +3 -3
@@ -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
kernel/time/tick-broadcast.c +6 -6
@@ -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; }
kernel/time/tick-common.c +2 -7
@@ -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 ;
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 6/38 tick: create tick_get_cpu_device() to get tick_cpu_device on this cpu
In reply to Viresh KumarWe 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.
--- 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(-)
include/linux/tick.h +5 -0
@@ -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);
kernel/hrtimer.c +1 -1
@@ -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); }
kernel/time/tick-broadcast.c +2 -2
@@ -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
kernel/time/tick-common.c +4 -4
@@ -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 );
kernel/time/tick-oneshot.c +4 -4
@@ -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;
kernel/time/tick-sched.c +1 -1
@@ -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 ();
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 7/38 tick-oneshot: drop local_irq_save/restore from tick_switch_to_oneshot()
In reply to Viresh Kumartick_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.
--- kernel/time/tick-oneshot.c | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-)
kernel/time/tick-oneshot.c +1 -8
@@ -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
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 8/38 tick-oneshot: move tick_is_oneshot_available() to tick-oneshot.c
In reply to Viresh KumarThis 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.
--- 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(-)
include/linux/tick.h +0 -1
@@ -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)
kernel/time/tick-common.c +0 -14
@@ -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 */
kernel/time/tick-internal.h +2 -0
@@ -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 */
kernel/time/tick-oneshot.c +14 -0
@@ -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
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 9/38 tick-oneshot: remove tick_resume_oneshot()
In reply to Viresh Kumartick_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.
--- 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(-)
kernel/time/tick-common.c +5 -3
@@ -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 ()); } }
kernel/time/tick-internal.h +0 -5
@@ -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 ;
kernel/time/tick-oneshot.c +0 -11
@@ -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,
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 10/38 tick-common: remove extra checks from tick_check_new_device()
In reply to Viresh Kumartick_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.
--- kernel/time/tick-common.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)
kernel/time/tick-common.c +3 -10
@@ -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))
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 11/38 tick-common: fix wrong check in tick_check_replacement()
In reply to Viresh Kumartick_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().
--- kernel/time/tick-common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/time/tick-common.c +1 -1
@@ -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);
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 12/38 tick-common: call tick_check_percpu() from tick_check_preferred()
In reply to Viresh Kumartick_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().
--- kernel/time/tick-common.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
kernel/time/tick-common.c +3 -7
@@ -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;
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 13/38 tick-common: don't check tick_oneshot_mode_active() from tick_check_preferred()
In reply to Viresh KumarIf '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.
--- kernel/time/tick-common.c | 2 -- 1 file changed, 2 deletions(-)
kernel/time/tick-common.c +0 -2
@@ -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; }/*
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 14/38 tick-common: do additional checks in tick_check_preferred()
In reply to Viresh KumarWe 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.
--- kernel/time/tick-common.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
kernel/time/tick-common.c +18 -7
@@ -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); }
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 15/38 tick-common: remove tick_check_replacement()
In reply to Viresh Kumartick_check_replacement() is now nothing more than a wrapper over tick_check_preferred() and hence can be removed. Use tick_check_preferred() everywhere.
--- kernel/time/clockevents.c | 2 +- kernel/time/tick-common.c | 16 +++++----------- kernel/time/tick-internal.h | 2 +- 3 files changed, 7 insertions(+), 13 deletions(-)
kernel/time/clockevents.c +1 -1
@@ -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))
kernel/time/tick-common.c +5 -11
@@ -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. */
kernel/time/tick-internal.h +1 -1
@@ -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);
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 16/38 tick-common: don't pass cpumask to tick_setup_device()
In reply to Viresh Kumartick_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.
--- kernel/time/tick-common.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
kernel/time/tick-common.c +4 -4
@@ -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 ;
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 17/38 tick-common: call tick_install_replacement() from tick_check_new_device()
In reply to Viresh KumarPart 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.
--- kernel/time/tick-common.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
kernel/time/tick-common.c +3 -7
@@ -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:
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 18/38 tick-common: don't set mode to CLOCK_EVT_MODE_UNUSED in tick_shutdown()
In reply to Viresh Kumartick_shutdown() calls clockevents_exchange_device() which already sets mode to CLOCK_EVT_MODE_UNUSED and so tick_shutdown() doesn't need to do it.
--- kernel/time/tick-common.c | 1 - 1 file changed, 1 deletion(-)
kernel/time/tick-common.c +0 -1
@@ -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 ;
Signature · 1 line
1.7.12.rc2.18.g61b472e
Re: [PATCH 18/38] tick-common: don't set mode to CLOCK_EVT_MODE_UNUSED in tick_shutdown()
Quoted 2 lines · Viresh Kumar
tick_shutdown()callsclockevents_exchange_device()which already sets mode toCLOCK_EVT_MODE_UNUSED
Yes it does.
Quoted 12 lines
and so
tick_shutdown()doesn't need to do it.Signed-off-by Viresh Kumar <[email protected]>--- kernel/time/tick-common.c | 1 - 1 file changed, 1 deletion(-)kernel/time/tick-common.c +0
-0
kernel/time/tick-common.c void tick_shutdown(unsigned int *cpup) · line 337
Quoted 4 lines
kernel/time/tick-common.c +0
-1@@ -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.
Quoted 3 lines
kernel/time/tick-common.c +0
-0clockevents_exchange_device (dev,NULL ); dev->event_handler = clockevents_handle_noop; td->evtdev =NULL ;
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 18/38] tick-common: don't set mode to CLOCK_EVT_MODE_UNUSED in tick_shutdown()
Quoted 2 lines · Stephen Boyd
But see the comment above this assignment. The point is to avoid calling the ->set_mode function.
Okay, that was a stupid change :)
PATCH 19/38 tick-common: remove local variable 'broadcast' from tick_resume()
In reply to Viresh Kumar'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.
--- kernel/time/tick-common.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
kernel/time/tick-common.c +7 -8
@@ -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 ()); }/**
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 20/38 tick-sched: initialize 'cpu' while defining it in tick_nohz_full_setup()
In reply to Viresh KumarWe are doing this currently:
int cpu; cpu = smp_processor_id();
And we can rewrite it to make it shorter:
int cpu = smp_processor_id();
--- kernel/time/tick-sched.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
kernel/time/tick-sched.c +1 -2
@@ -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);
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 21/38 tick-sched: no need to rewrite '1' to tick_nohz_enabled
In reply to Viresh KumarNO_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.
--- kernel/time/tick-sched.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
kernel/time/tick-sched.c +1 -4
@@ -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);/**
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 22/38 tick-sched: no need to recheck cpu_online() in can_stop_idle_tick()
In reply to Viresh KumarWe have already checked if 'cpu' is online or not and so don't need to recheck it.
--- kernel/time/tick-sched.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
kernel/time/tick-sched.c +1 -1
@@ -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 &&
Signature · 1 line
1.7.12.rc2.18.g61b472e
Re: [PATCH 22/38] tick-sched: no need to recheck cpu_online() in can_stop_idle_tick()
Quoted 4 lines · Viresh Kumar
We have already checked if 'cpu' is online or not and so don't need to recheck it.
Signed-off-by Viresh Kumar <[email protected]>
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?
Quoted 17 lines
--- kernel/time/tick-sched.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)kernel/time/tick-sched.c +1
-1@@ -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 &&
Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH 22/38] tick-sched: no need to recheck cpu_online() in can_stop_idle_tick()
Quoted 10 lines · Stephen Boyd
Quoted 4 lines · Viresh Kumar
We have already checked if 'cpu' is online or not and so don't need to recheck it.
Signed-off-by Viresh Kumar <[email protected]>Hm... doing some git archeology shows
fa116ea35ec7(nohz: no softirq pending warnings for offline cpus, 2008-12-11), where thecpu_online()check was added. Before that commit we already checkedcpu_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:
kernel/time/tick-sched.c +0 -0
@@ -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.
PATCH 23/38 tick-sched: invert parameter of tick_check_oneshot_change()
In reply to Viresh KumarThere 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.
--- include/linux/tick.h | 6 +++--- kernel/hrtimer.c | 2 +- kernel/time/tick-sched.c | 10 +++++----- 3 files changed, 9 insertions(+), 9 deletions(-)
include/linux/tick.h +3 -3
@@ -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 */
kernel/hrtimer.c +1 -1
@@ -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 (); }
kernel/time/tick-sched.c +5 -5
@@ -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 ();
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 24/38 tick-sched: don't check tick_nohz_full_cpu() in __tick_nohz_task_switch()
In reply to Viresh Kumar__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.
--- kernel/time/tick-sched.c | 4 ---- 1 file changed, 4 deletions(-)
kernel/time/tick-sched.c +0 -4
@@ -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); }
Signature · 1 line
1.7.12.rc2.18.g61b472e
Re: [PATCH 24/38] tick-sched: don't check tick_nohz_full_cpu() in __tick_nohz_task_switch()
Quoted 6 lines · Viresh Kumar
__tick_nohz_task_switch()callstick_nohz_full_kick(), which is already checkingtick_nohz_full_cpu()and so we don't need to repeat the same check here.Remove it.
Signed-off-by Viresh Kumar <[email protected]>
Ack.
Quoted 25 lines
--- kernel/time/tick-sched.c | 4 ---- 1 file changed, 4 deletions(-)kernel/time/tick-sched.c +0
-4@@ -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); }Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 25/38 tick-sched: don't call local_softirq_pending() thrice in can_stop_idle_tick()
In reply to Viresh Kumarcan_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.
--- kernel/time/tick-sched.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
kernel/time/tick-sched.c +5 -5
@@ -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;
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 26/38 tick-sched: don't call update_wall_time() when delta is lesser than tick_period
In reply to Viresh KumarIn 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 <[email protected]> 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.
--- kernel/time/tick-sched.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)
kernel/time/tick-sched.c +17 -15
@@ -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 (); }
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 27/38 tick-sched: remove 'regs' parameter of tick_sched_handle()
In reply to Viresh Kumartick_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.
--- kernel/time/tick-sched.c | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-)
kernel/time/tick-sched.c +12 -11
@@ -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);
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 28/38 tick-sched: remove parameters to {__}tick_nohz_task_switch() routines
In reply to Viresh Kumartick_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.
--- include/linux/tick.h | 8 ++++---- kernel/sched/core.c | 2 +- kernel/time/tick-sched.c | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-)
include/linux/tick.h +4 -4
@@ -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 (); }
kernel/sched/core.c +1 -1
@@ -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
kernel/time/tick-sched.c +1 -1
@@ -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;
Signature · 1 line
1.7.12.rc2.18.g61b472e
Re: [PATCH 28/38] tick-sched: remove parameters to {__}tick_nohz_task_switch() routines
Quoted 5 lines · Viresh Kumar
tick_nohz_task_switch()and__tick_nohz_task_switch()routines gettask_structpassed to them (always for the 'current' task), but they never use it. Remove it.Signed-off-by Viresh Kumar <[email protected]>
Ack.
PATCH 29/38 tick-sched: remove wrapper around __tick_nohz_task_switch()
In reply to Viresh Kumar__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.
--- include/linux/tick.h | 11 ++--------- kernel/time/tick-sched.c | 7 +++++-- 2 files changed, 7 insertions(+), 11 deletions(-)
include/linux/tick.h +2 -9
@@ -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
kernel/time/tick-sched.c +5 -2
@@ -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);
Signature · 1 line
1.7.12.rc2.18.g61b472e
Re: [PATCH 29/38] tick-sched: remove wrapper around __tick_nohz_task_switch()
Quoted 8 lines · Viresh Kumar
__tick_nohz_task_switch()was called only fromtick_nohz_task_switch()and there is nothing much intick_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()intotick_nohz_task_switch()and move it totick-sched.c.This also moves check for
tick_nohz_tick_stopped()outside ofirq_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().
Re: [PATCH 29/38] tick-sched: remove wrapper around __tick_nohz_task_switch()
Quoted 7 lines · Frederic Weisbecker
Quoted 6 lines · Viresh Kumar
__tick_nohz_task_switch()was called only fromtick_nohz_task_switch()and there is nothing much intick_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()intotick_nohz_task_switch()and move it totick-sched.c.
Quoted 5 lines
Quoted 2 lines
This also moves check for
tick_nohz_tick_stopped()outside ofirq_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 ?
Re: [PATCH 29/38] tick-sched: remove wrapper around __tick_nohz_task_switch()
Quoted 11 lines · Viresh Kumar
Quoted 10 lines · Frederic Weisbecker
Quoted 8 lines · Viresh Kumar
__tick_nohz_task_switch()was called only fromtick_nohz_task_switch()and there is nothing much intick_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()intotick_nohz_task_switch()and move it totick-sched.c.This also moves check for
tick_nohz_tick_stopped()outside ofirq_save()context.
Quoted 7 lines
Quoted 2 lines
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, astick_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.
Quoted 2 lines
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.
Re: [PATCH 29/38] tick-sched: remove wrapper around __tick_nohz_task_switch()
Quoted 24 lines · Frederic Weisbecker
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).
Quoted 6 lines
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 infinish_task_switch()to turn the goto offcase into a nop. Then the condition is actually verified on every call tofinish_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.
Quoted 7 lines
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
Re: [PATCH 29/38] tick-sched: remove wrapper around __tick_nohz_task_switch()
Quoted 30 lines · Viresh Kumar
Quoted 29 lines · Frederic Weisbecker
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 infinish_task_switch()to turn the goto offcase into a nop. Then the condition is actually verified on every call tofinish_task_switch().
Quoted 4 lines
Quoted 1 line
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 :)
Quoted 6 lines
So lets clarify things one by one :)
- What do you mean by offcase?
CONFIG_NO_HZ_FULLnot 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.
Quoted 11 lines
- 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.
Quoted 7 lines
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.
Quoted 9 lines
__tick_nohz_task_switch()may or maynot be available at all depending onCONFIG_NO_HZ_FULLis enabled into the kernel or not. But that was the case withtick_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.
Quoted 3 lines
--
viresh
Re: [PATCH 29/38] tick-sched: remove wrapper around __tick_nohz_task_switch()
Quoted 8 lines · Frederic Weisbecker
Sure but check out the
static_key_false()in the implementation oftick_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:
Quoted 5 lines
Quoted 1 line
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()totick_nohz_task_switch()? I much prefer we don't do that. This is going to makecan_stop_full_tick()a publicly visible nohz internal. And it may uglifytick.has 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:
include/linux/tick.h +1 -1
@@ -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 (); }
kernel/time/tick-sched.c +1 -1
@@ -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);
PATCH 30/38 tick-sched: move nohz_full_buf[] inside tick_nohz_init()
In reply to Viresh Kumarnohz_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.
--- kernel/time/tick-sched.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
kernel/time/tick-sched.c +6 -7
@@ -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) {
Signature · 1 line
1.7.12.rc2.18.g61b472e
Re: [PATCH 30/38] tick-sched: move nohz_full_buf[] inside tick_nohz_init()
Quoted 3 lines · Viresh Kumar
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.
Quoted 40 lines
Signed-off-by Viresh Kumar <[email protected]>--- kernel/time/tick-sched.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)kernel/time/tick-sched.c +6
-7@@ -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) {Signature · 1 line
1.7.12.rc2.18.g61b472e
Re: [PATCH 30/38] tick-sched: move nohz_full_buf[] inside tick_nohz_init()
Quoted 7 lines · Frederic Weisbecker
OTOH
nohz_full_bufcan 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
__initdataso it's released after bootup. No issue of concurrent use either, so keeping it as is is fine.
Okay, this patch is dropped.
PATCH 31/38 tick-sched: initialize 'ts' during its definition __tick_nohz_idle_enter()
In reply to Viresh Kumar--- kernel/time/tick-sched.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
kernel/time/tick-sched.c +1 -2
@@ -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);
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 32/38 tick-sched: add comment about 'idle_active' in tick_nohz_idle_exit()
In reply to Viresh KumarThe 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
--- kernel/time/tick-sched.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)
kernel/time/tick-sched.c +16 -0
@@ -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 ();
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 34/38 tick-sched: remove local variable 'now' from tick_setup_sched_timer()
In reply to Viresh KumarLocal variable 'now' is used at only one place and so can be easily replaced by ktime_get() instead.
--- kernel/time/tick-sched.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
kernel/time/tick-sched.c +1 -3
@@ -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
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 35/38 tick-broadcast: do checks before taking locks in tick_do_broadcast_on_off()
In reply to Viresh KumarSome 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.
--- kernel/time/tick-broadcast.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
kernel/time/tick-broadcast.c +7 -13
@@ -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); }
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 36/38 tick-broadcast: get rid of extra comparison in tick_do_broadcast_on_off()
In reply to Viresh KumarWe 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.
--- kernel/time/tick-broadcast.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
kernel/time/tick-broadcast.c +2 -3
@@ -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)
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 37/38 tick-broadcast: merge tick_do_broadcast_on_off() into tick_broadcast_on_off()
In reply to Viresh KumarWe 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("**");
--- kernel/time/tick-broadcast.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-)
kernel/time/tick-broadcast.c +8 -15
@@ -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)
Signature · 1 line
1.7.12.rc2.18.g61b472e
PATCH 38/38 clockevents: set event_handler to clockevents_handle_noop() in clockevents_exchange_device()
In reply to Viresh KumarAll 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.
--- 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(-)
include/linux/clockchips.h +0 -2
@@ -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) {
kernel/time/clockevents.c +2 -1
@@ -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) {
kernel/time/tick-broadcast.c +0 -2
@@ -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);
kernel/time/tick-common.c +5 -6
@@ -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 ; } }
Signature · 1 line
1.7.12.rc2.18.g61b472e
Quoted 8 lines · Viresh Kumar
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
Quoted 4 lines · Thomas Gleixner
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 :(
Quoted 11 lines
Now looking at the thing some more, it contains gems like this:
(quoted diff) +2
-2- 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)topr_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 :)
Quoted 3 lines
Please send me the next series in the following way:
- send a bug fix series, which does nothing else than fixing bugs.
Right.
Quoted 4 lines
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.