[PATCH 00/38] tick cleanups and bugfixes

[PATCH 00/38] tick cleanups and bugfixes

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

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

-- 
1.7.12.rc2.18.g61b472e

[PATCH 01/38] tick: align to Coding Guidelines

[Viresh Kumar]

This does some trivial fixups:
- breaking lines longer than 80 columns
- merging few lines together
- don't break print messages even if they cross 80 columns
- remove few whitespaces
- remove few extra blank lines

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/tick.h         |  3 ++-
 kernel/time/tick-broadcast.c | 16 ++++++----------
 kernel/time/tick-internal.h  |  5 ++++-
 kernel/time/tick-oneshot.c   |  3 +--
 kernel/time/tick-sched.c     | 29 ++++++++++++++++-------------
 5 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index b84773c..8c865fb 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -47,7 +47,8 @@ enum tick_nohz_mode {
  * @idle_waketime:	Time when the idle was interrupted
  * @idle_exittime:	Time when the idle state was left
  * @idle_sleeptime:	Sum of the time slept in idle with sched tick stopped
- * @iowait_sleeptime:	Sum of the time slept in idle with sched tick stopped, with IO outstanding
+ * @iowait_sleeptime:	Sum of the time slept in idle with sched tick stopped,
+ *			with IO outstanding
  * @sleep_length:	Duration of the current idle sleep
  * @do_timer_lst:	CPU was the last one doing do_timer before going idle
  */
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 64c5990..c4c50ba 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -132,7 +132,6 @@ int tick_broadcast_update_freq(struct clock_event_device *dev, u32 freq)
 	return ret;
 }
 
-
 static void err_broadcast(const struct cpumask *mask)
 {
 	pr_crit_once("Failed to broadcast timer tick. Some CPUs may be unresponsive.\n");
@@ -358,8 +357,7 @@ static void tick_do_broadcast_on_off(unsigned long *reason)
 	case CLOCK_EVT_NOTIFY_BROADCAST_FORCE:
 		cpumask_set_cpu(cpu, tick_broadcast_on);
 		if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_mask)) {
-			if (tick_broadcast_device.mode ==
-			    TICKDEV_MODE_PERIODIC)
+			if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
 				clockevents_shutdown(dev);
 		}
 		if (*reason == CLOCK_EVT_NOTIFY_BROADCAST_FORCE)
@@ -372,8 +370,7 @@ static void tick_do_broadcast_on_off(unsigned long *reason)
 		if (!tick_device_is_functional(dev))
 			break;
 		if (cpumask_test_and_clear_cpu(cpu, tick_broadcast_mask)) {
-			if (tick_broadcast_device.mode ==
-			    TICKDEV_MODE_PERIODIC)
+			if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
 				tick_setup_periodic(dev, 0);
 		}
 		break;
@@ -399,8 +396,8 @@ out:
 void tick_broadcast_on_off(unsigned long reason, int *oncpu)
 {
 	if (!cpumask_test_cpu(*oncpu, cpu_online_mask))
-		printk(KERN_ERR "tick-broadcast: ignoring broadcast for "
-		       "offline CPU #%d\n", *oncpu);
+		printk(KERN_ERR "tick-broadcast: ignoring broadcast for offline CPU #%d\n",
+		       *oncpu);
 	else
 		tick_do_broadcast_on_off(&reason);
 }
@@ -484,7 +481,6 @@ int tick_resume_broadcast(void)
 	return broadcast;
 }
 
-
 #ifdef CONFIG_TICK_ONESHOT
 
 static cpumask_var_t tick_broadcast_oneshot_mask;
@@ -727,7 +723,8 @@ int tick_broadcast_oneshot_control(unsigned long reason)
 			 */
 			if (!cpumask_test_cpu(cpu, tick_broadcast_force_mask) &&
 			    dev->next_event.tv64 < bc->next_event.tv64)
-				tick_broadcast_set_event(bc, cpu, dev->next_event, 1);
+				tick_broadcast_set_event(bc, cpu,
+							 dev->next_event, 1);
 		}
 		/*
 		 * If the current CPU owns the hrtimer broadcast
@@ -894,7 +891,6 @@ void tick_broadcast_switch_to_oneshot(void)
 	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
 }
 
-
 /*
  * Remove a dead CPU from broadcasting
  */
diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
index 7ab92b1..855c513 100644
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -87,7 +87,10 @@ static inline void tick_broadcast_setup_oneshot(struct clock_event_device *bc)
 {
 	BUG();
 }
-static inline int tick_broadcast_oneshot_control(unsigned long reason) { return 0; }
+static inline int tick_broadcast_oneshot_control(unsigned long reason)
+{
+	return 0;
+}
 static inline void tick_shutdown_broadcast_oneshot(unsigned int *cpup) { }
 static inline int tick_resume_broadcast_oneshot(struct clock_event_device *bc)
 {
diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c
index 8241090..e04d5a0 100644
--- a/kernel/time/tick-oneshot.c
+++ b/kernel/time/tick-oneshot.c
@@ -65,8 +65,7 @@ int tick_switch_to_oneshot(void (*handler)(struct clock_event_device *))
 	if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT) ||
 		    !tick_device_is_functional(dev)) {
 
-		printk(KERN_INFO "Clockevents: "
-		       "could not switch to one-shot mode:");
+		printk(KERN_INFO "Clockevents: could not switch to one-shot mode:");
 		if (!dev) {
 			printk(" no tick device\n");
 		} else {
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 9f8af69..d48d648 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -105,7 +105,6 @@ static ktime_t tick_init_jiffy_update(void)
 	return period;
 }
 
-
 static void tick_sched_do_timer(ktime_t now)
 {
 	int cpu = smp_processor_id();
@@ -245,8 +244,8 @@ void tick_nohz_full_kick_all(void)
 		return;
 
 	preempt_disable();
-	smp_call_function_many(tick_nohz_full_mask,
-			       nohz_full_kick_ipi, NULL, false);
+	smp_call_function_many(tick_nohz_full_mask, nohz_full_kick_ipi, NULL,
+			       false);
 	tick_nohz_full_kick();
 	preempt_enable();
 }
@@ -285,7 +284,8 @@ static int __init tick_nohz_full_setup(char *str)
 
 	cpu = smp_processor_id();
 	if (cpumask_test_cpu(cpu, tick_nohz_full_mask)) {
-		pr_warning("NO_HZ: Clearing %d from nohz_full range for timekeeping\n", cpu);
+		pr_warning("NO_HZ: Clearing %d from nohz_full range for timekeeping\n",
+			   cpu);
 		cpumask_clear_cpu(cpu, tick_nohz_full_mask);
 	}
 	tick_nohz_full_running = true;
@@ -295,8 +295,7 @@ static int __init tick_nohz_full_setup(char *str)
 __setup("nohz_full=", tick_nohz_full_setup);
 
 static int tick_nohz_cpu_down_callback(struct notifier_block *nfb,
-						 unsigned long action,
-						 void *hcpu)
+				       unsigned long action, void *hcpu)
 {
 	unsigned int cpu = (unsigned long)hcpu;
 
@@ -350,7 +349,8 @@ void __init tick_nohz_init(void)
 		context_tracking_cpu_set(cpu);
 
 	cpu_notifier(tick_nohz_cpu_down_callback, 0);
-	cpulist_scnprintf(nohz_full_buf, sizeof(nohz_full_buf), tick_nohz_full_mask);
+	cpulist_scnprintf(nohz_full_buf, sizeof(nohz_full_buf),
+			  tick_nohz_full_mask);
 	pr_info("NO_HZ: Full dynticks CPUs: %s.\n", nohz_full_buf);
 }
 #endif
@@ -362,8 +362,8 @@ void __init tick_nohz_init(void)
 /*
  * NO HZ enabled ?
  */
-static int tick_nohz_enabled __read_mostly  = 1;
-int tick_nohz_active  __read_mostly;
+static int tick_nohz_enabled __read_mostly = 1;
+int tick_nohz_active __read_mostly;
 /*
  * Enable / Disable tickless mode
  */
@@ -407,16 +407,19 @@ static void tick_nohz_update_jiffies(ktime_t now)
  * Updates the per cpu time idle statistics counters
  */
 static void
-update_ts_time_stats(int cpu, struct tick_sched *ts, ktime_t now, u64 *last_update_time)
+update_ts_time_stats(int cpu, struct tick_sched *ts, ktime_t now,
+		     u64 *last_update_time)
 {
 	ktime_t delta;
 
 	if (ts->idle_active) {
 		delta = ktime_sub(now, ts->idle_entrytime);
 		if (nr_iowait_cpu(cpu) > 0)
-			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
+			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime,
+							 delta);
 		else
-			ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
+			ts->idle_sleeptime = ktime_add(ts->idle_sleeptime,
+						       delta);
 		ts->idle_entrytime = now;
 	}
 
@@ -873,7 +876,7 @@ static void tick_nohz_restart_sched_tick(struct tick_sched *ts, ktime_t now)
 	/*
 	 * Cancel the scheduled timer and restore the tick
 	 */
-	ts->tick_stopped  = 0;
+	ts->tick_stopped = 0;
 	ts->idle_exittime = now;
 
 	tick_nohz_restart(ts, now);
-- 
1.7.12.rc2.18.g61b472e

[PATCH 02/38] tick: update doc comments for struct tick_sched

[Viresh Kumar]

Some fields of 'struct tick_sched' didn't had a description in the kernel doc
comment present above its declaration. Add them.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/tick.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index 8c865fb..8cc804c 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -35,14 +35,19 @@ enum tick_nohz_mode {
  * struct tick_sched - sched tick emulation and no idle tick control/stats
  * @sched_timer:	hrtimer to schedule the periodic tick in high
  *			resolution mode
+ * @check_clocks:	tracks if clockevent device is recently changed
+ * @nohz_mode:		Current NOHZ mode
  * @last_tick:		Store the last tick expiry time when the tick
  *			timer is modified for nohz sleeps. This is necessary
  *			to resume the tick timer operation in the timeline
  *			when the CPU returns from nohz sleep.
+ * @inidle:		CPU is currently executing from within the idle loop
  * @tick_stopped:	Indicator that the idle tick has been stopped
  * @idle_jiffies:	jiffies at the entry to idle for idle time accounting
  * @idle_calls:		Total number of idle calls
  * @idle_sleeps:	Number of idle calls, where the sched tick was stopped
+ * @idle_active:	similar to inidle, but is reset when we get an interrupt
+ *			while being in idle
  * @idle_entrytime:	Time when the idle call was entered
  * @idle_waketime:	Time when the idle was interrupted
  * @idle_exittime:	Time when the idle state was left
@@ -50,7 +55,10 @@ enum tick_nohz_mode {
  * @iowait_sleeptime:	Sum of the time slept in idle with sched tick stopped,
  *			with IO outstanding
  * @sleep_length:	Duration of the current idle sleep
- * @do_timer_lst:	CPU was the last one doing do_timer before going idle
+ * @last_jiffies:	Last updated value of jiffies
+ * @next_jiffies:	Next jiffie for which timer is requested
+ * @idle_expires:	Time when we should come out of idle or next timer event
+ * @do_timer_last:	CPU was the last one doing do_timer before going idle
  */
 struct tick_sched {
 	struct hrtimer			sched_timer;
-- 
1.7.12.rc2.18.g61b472e

[PATCH 03/38] tick: rearrange members of 'struct tick_sched'

[Viresh Kumar]

Rearrange members of 'struct tick_sched' to make it more readable. i.e. By
keeping all ktime_t members together, similarly 'unsigned long' and 'int' as
well. I couldn't figure out breaking any logical blocks here which we wanted to
keep together.

This is the change in footprints after this change.
For x86: size of this structure changed from 240 to 232 bytes
For ARM: didn't see a difference, was 160 bytes

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/tick.h | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index 8cc804c..edabc7d 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -35,19 +35,17 @@ enum tick_nohz_mode {
  * struct tick_sched - sched tick emulation and no idle tick control/stats
  * @sched_timer:	hrtimer to schedule the periodic tick in high
  *			resolution mode
- * @check_clocks:	tracks if clockevent device is recently changed
  * @nohz_mode:		Current NOHZ mode
+ * @check_clocks:	tracks if clockevent device is recently changed
+ * @idle_jiffies:	jiffies at the entry to idle for idle time accounting
+ * @idle_calls:		Total number of idle calls
+ * @idle_sleeps:	Number of idle calls, where the sched tick was stopped
+ * @last_jiffies:	Last updated value of jiffies
+ * @next_jiffies:	Next jiffie for which timer is requested
  * @last_tick:		Store the last tick expiry time when the tick
  *			timer is modified for nohz sleeps. This is necessary
  *			to resume the tick timer operation in the timeline
  *			when the CPU returns from nohz sleep.
- * @inidle:		CPU is currently executing from within the idle loop
- * @tick_stopped:	Indicator that the idle tick has been stopped
- * @idle_jiffies:	jiffies at the entry to idle for idle time accounting
- * @idle_calls:		Total number of idle calls
- * @idle_sleeps:	Number of idle calls, where the sched tick was stopped
- * @idle_active:	similar to inidle, but is reset when we get an interrupt
- *			while being in idle
  * @idle_entrytime:	Time when the idle call was entered
  * @idle_waketime:	Time when the idle was interrupted
  * @idle_exittime:	Time when the idle state was left
@@ -55,31 +53,33 @@ enum tick_nohz_mode {
  * @iowait_sleeptime:	Sum of the time slept in idle with sched tick stopped,
  *			with IO outstanding
  * @sleep_length:	Duration of the current idle sleep
- * @last_jiffies:	Last updated value of jiffies
- * @next_jiffies:	Next jiffie for which timer is requested
  * @idle_expires:	Time when we should come out of idle or next timer event
+ * @inidle:		CPU is currently executing from within the idle loop
+ * @idle_active:	similar to inidle, but is reset when we get an interrupt
+ *			while being in idle
+ * @tick_stopped:	Indicator that the idle tick has been stopped
  * @do_timer_last:	CPU was the last one doing do_timer before going idle
  */
 struct tick_sched {
 	struct hrtimer			sched_timer;
-	unsigned long			check_clocks;
 	enum tick_nohz_mode		nohz_mode;
-	ktime_t				last_tick;
-	int				inidle;
-	int				tick_stopped;
+	unsigned long			check_clocks;
 	unsigned long			idle_jiffies;
 	unsigned long			idle_calls;
 	unsigned long			idle_sleeps;
-	int				idle_active;
+	unsigned long			last_jiffies;
+	unsigned long			next_jiffies;
+	ktime_t				last_tick;
 	ktime_t				idle_entrytime;
 	ktime_t				idle_waketime;
 	ktime_t				idle_exittime;
 	ktime_t				idle_sleeptime;
 	ktime_t				iowait_sleeptime;
 	ktime_t				sleep_length;
-	unsigned long			last_jiffies;
-	unsigned long			next_jiffies;
 	ktime_t				idle_expires;
+	int				inidle;
+	int				idle_active;
+	int				tick_stopped;
 	int				do_timer_last;
 };
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH 04/38] tick: move declaration of tick_cpu_device to tick.h

[Viresh Kumar]

tick_cpu_device isn't local only to kernel/time/ directory and its declaration
is present in hrtimer.h and tick-internal.h. Rather move it to a common place,
i.e. tick.h.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/hrtimer.h     | 3 ---
 include/linux/tick.h        | 2 ++
 kernel/time/tick-internal.h | 1 -
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index e7a8d3f..a31f83e 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -332,9 +332,6 @@ extern ktime_t ktime_get_clocktai(void);
 extern ktime_t ktime_get_update_offsets(ktime_t *offs_real, ktime_t *offs_boot,
 					 ktime_t *offs_tai);
 
-DECLARE_PER_CPU(struct tick_device, tick_cpu_device);
-
-
 /* Exported timer functions: */
 
 /* Initialize timers: */
diff --git a/include/linux/tick.h b/include/linux/tick.h
index edabc7d..1a9908a 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -85,6 +85,8 @@ struct tick_sched {
 
 extern void __init tick_init(void);
 extern int tick_is_oneshot_available(void);
+
+DECLARE_PER_CPU(struct tick_device, tick_cpu_device);
 extern struct tick_device *tick_get_device(int cpu);
 
 # ifdef CONFIG_HIGH_RES_TIMERS
diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
index 855c513..57c1a76 100644
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -13,7 +13,6 @@ extern seqlock_t jiffies_lock;
 #define TICK_DO_TIMER_NONE	-1
 #define TICK_DO_TIMER_BOOT	-2
 
-DECLARE_PER_CPU(struct tick_device, tick_cpu_device);
 extern ktime_t tick_next_period;
 extern ktime_t tick_period;
 extern int tick_do_timer_cpu __read_mostly;
-- 
1.7.12.rc2.18.g61b472e

[PATCH 05/38] tick: move definition of tick_get_device() to tick.h

[Viresh Kumar]

There are multiple users of tick_get_device() which are currently using
&per_cpu(tick_cpu_device, cpu) directly. Would be better to move definition of
tick_get_device() to tick.h and move others to use this routine instead.

This change reduced size of bzImage for x86 by 96 bytes.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/tick.h         |  5 ++++-
 kernel/time/clockevents.c    |  6 +++---
 kernel/time/tick-broadcast.c | 12 ++++++------
 kernel/time/tick-common.c    |  9 ++-------
 4 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index 1a9908a..45e1331 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -87,7 +87,10 @@ extern void __init tick_init(void);
 extern int tick_is_oneshot_available(void);
 
 DECLARE_PER_CPU(struct tick_device, tick_cpu_device);
-extern struct tick_device *tick_get_device(int cpu);
+static inline struct tick_device *tick_get_device(int cpu)
+{
+	return &per_cpu(tick_cpu_device, cpu);
+}
 
 # ifdef CONFIG_HIGH_RES_TIMERS
 extern int tick_init_highres(void);
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index ad362c2..6680a0d 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -326,7 +326,7 @@ static int __clockevents_try_unbind(struct clock_event_device *ced, int cpu)
 		return 0;
 	}
 
-	return ced == per_cpu(tick_cpu_device, cpu).evtdev ? -EAGAIN : -EBUSY;
+	return ced == tick_get_device(cpu)->evtdev ? -EAGAIN : -EBUSY;
 }
 
 /*
@@ -675,7 +675,7 @@ static struct device tick_bc_dev = {
 static struct tick_device *tick_get_tick_dev(struct device *dev)
 {
 	return dev == &tick_bc_dev ? tick_get_broadcast_device() :
-		&per_cpu(tick_cpu_device, dev->id);
+		tick_get_device(dev->id);
 }
 
 static __init int tick_broadcast_init_sysfs(void)
@@ -689,7 +689,7 @@ static __init int tick_broadcast_init_sysfs(void)
 #else
 static struct tick_device *tick_get_tick_dev(struct device *dev)
 {
-	return &per_cpu(tick_cpu_device, dev->id);
+	return tick_get_device(dev->id);
 }
 static inline int tick_broadcast_init_sysfs(void) { return 0; }
 #endif
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index c4c50ba..eb0479a 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -262,7 +262,7 @@ static void tick_do_broadcast(struct cpumask *mask)
 	 */
 	if (cpumask_test_cpu(cpu, mask)) {
 		cpumask_clear_cpu(cpu, mask);
-		td = &per_cpu(tick_cpu_device, cpu);
+		td = tick_get_device(cpu);
 		td->evtdev->event_handler(td->evtdev);
 	}
 
@@ -273,7 +273,7 @@ static void tick_do_broadcast(struct cpumask *mask)
 		 * one of the first device. This works as long as we have this
 		 * misfeature only on x86 (lapic)
 		 */
-		td = &per_cpu(tick_cpu_device, cpumask_first(mask));
+		td = tick_get_device(cpumask_first(mask));
 		td->evtdev->broadcast(mask);
 	}
 }
@@ -337,7 +337,7 @@ static void tick_do_broadcast_on_off(unsigned long *reason)
 	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
 
 	cpu = smp_processor_id();
-	td = &per_cpu(tick_cpu_device, cpu);
+	td = tick_get_device(cpu);
 	dev = td->evtdev;
 	bc = tick_broadcast_device.evtdev;
 
@@ -581,7 +581,7 @@ again:
 	now = ktime_get();
 	/* Find all expired events */
 	for_each_cpu(cpu, tick_broadcast_oneshot_mask) {
-		td = &per_cpu(tick_cpu_device, cpu);
+		td = tick_get_device(cpu);
 		if (td->evtdev->next_event.tv64 <= now.tv64) {
 			cpumask_set_cpu(cpu, tmpmask);
 			/*
@@ -700,7 +700,7 @@ int tick_broadcast_oneshot_control(unsigned long reason)
 	 * idle code, so we can't be moved away.
 	 */
 	cpu = smp_processor_id();
-	td = &per_cpu(tick_cpu_device, cpu);
+	td = tick_get_device(cpu);
 	dev = td->evtdev;
 
 	if (!(dev->features & CLOCK_EVT_FEAT_C3STOP))
@@ -824,7 +824,7 @@ static void tick_broadcast_init_next_event(struct cpumask *mask,
 	int cpu;
 
 	for_each_cpu(cpu, mask) {
-		td = &per_cpu(tick_cpu_device, cpu);
+		td = tick_get_device(cpu);
 		if (td->evtdev)
 			td->evtdev->next_event = expires;
 	}
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index 0156612..c7859b7 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -53,11 +53,6 @@ int tick_do_timer_cpu __read_mostly = TICK_DO_TIMER_BOOT;
 /*
  * Debugging: see timer_list.c
  */
-struct tick_device *tick_get_device(int cpu)
-{
-	return &per_cpu(tick_cpu_device, cpu);
-}
-
 /**
  * tick_is_oneshot_available - check for a oneshot capable event device
  */
@@ -296,7 +291,7 @@ void tick_check_new_device(struct clock_event_device *newdev)
 	if (!cpumask_test_cpu(cpu, newdev->cpumask))
 		goto out_bc;
 
-	td = &per_cpu(tick_cpu_device, cpu);
+	td = tick_get_device(cpu);
 	curdev = td->evtdev;
 
 	/* cpu local device ? */
@@ -356,7 +351,7 @@ void tick_handover_do_timer(int *cpup)
  */
 void tick_shutdown(unsigned int *cpup)
 {
-	struct tick_device *td = &per_cpu(tick_cpu_device, *cpup);
+	struct tick_device *td = tick_get_device(*cpup);
 	struct clock_event_device *dev = td->evtdev;
 
 	td->mode = TICKDEV_MODE_PERIODIC;
-- 
1.7.12.rc2.18.g61b472e

[PATCH 06/38] tick: create tick_get_cpu_device() to get tick_cpu_device on this cpu

[Viresh Kumar]

We are accessing &__get_cpu_var(tick_cpu_device) at several places in kernel.
Lets create another routine tick_get_cpu_device() which would return
tick_cpu_device for this-cpu.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/tick.h         | 5 +++++
 kernel/hrtimer.c             | 2 +-
 kernel/time/tick-broadcast.c | 4 ++--
 kernel/time/tick-common.c    | 8 ++++----
 kernel/time/tick-oneshot.c   | 8 ++++----
 kernel/time/tick-sched.c     | 2 +-
 6 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index 45e1331..98065e5 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -92,6 +92,11 @@ static inline struct tick_device *tick_get_device(int cpu)
 	return &per_cpu(tick_cpu_device, cpu);
 }
 
+static inline struct tick_device *tick_get_cpu_device(void)
+{
+	return &__get_cpu_var(tick_cpu_device);
+}
+
 # ifdef CONFIG_HIGH_RES_TIMERS
 extern int tick_init_highres(void);
 extern int tick_program_event(ktime_t expires, int force);
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index d55092c..393f422 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -1403,7 +1403,7 @@ static void __hrtimer_peek_ahead_timers(void)
 	if (!hrtimer_hres_active())
 		return;
 
-	td = &__get_cpu_var(tick_cpu_device);
+	td = tick_get_cpu_device();
 	if (td && td->evtdev)
 		hrtimer_interrupt(td->evtdev);
 }
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index eb0479a..1597d03 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -235,7 +235,7 @@ int tick_device_uses_broadcast(struct clock_event_device *dev, int cpu)
 #ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 int tick_receive_broadcast(void)
 {
-	struct tick_device *td = this_cpu_ptr(&tick_cpu_device);
+	struct tick_device *td = tick_get_cpu_device();
 	struct clock_event_device *evt = td->evtdev;
 
 	if (!evt)
@@ -550,7 +550,7 @@ int tick_resume_broadcast_oneshot(struct clock_event_device *bc)
 void tick_check_oneshot_broadcast_this_cpu(void)
 {
 	if (cpumask_test_cpu(smp_processor_id(), tick_broadcast_oneshot_mask)) {
-		struct tick_device *td = &__get_cpu_var(tick_cpu_device);
+		struct tick_device *td = tick_get_cpu_device();
 
 		/*
 		 * We might be in the middle of switching over from
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index c7859b7..b4fdb28 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -58,7 +58,7 @@ int tick_do_timer_cpu __read_mostly = TICK_DO_TIMER_BOOT;
  */
 int tick_is_oneshot_available(void)
 {
-	struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev);
+	struct clock_event_device *dev = tick_get_cpu_device()->evtdev;
 
 	if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT))
 		return 0;
@@ -219,7 +219,7 @@ static void tick_setup_device(struct tick_device *td,
 
 void tick_install_replacement(struct clock_event_device *newdev)
 {
-	struct tick_device *td = &__get_cpu_var(tick_cpu_device);
+	struct tick_device *td = tick_get_cpu_device();
 	int cpu = smp_processor_id();
 
 	clockevents_exchange_device(td->evtdev, newdev);
@@ -369,14 +369,14 @@ void tick_shutdown(unsigned int *cpup)
 
 void tick_suspend(void)
 {
-	struct tick_device *td = &__get_cpu_var(tick_cpu_device);
+	struct tick_device *td = tick_get_cpu_device();
 
 	clockevents_shutdown(td->evtdev);
 }
 
 void tick_resume(void)
 {
-	struct tick_device *td = &__get_cpu_var(tick_cpu_device);
+	struct tick_device *td = tick_get_cpu_device();
 	int broadcast = tick_resume_broadcast();
 
 	clockevents_set_mode(td->evtdev, CLOCK_EVT_MODE_RESUME);
diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c
index e04d5a0..6a531ec 100644
--- a/kernel/time/tick-oneshot.c
+++ b/kernel/time/tick-oneshot.c
@@ -26,7 +26,7 @@
  */
 int tick_program_event(ktime_t expires, int force)
 {
-	struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev);
+	struct clock_event_device *dev = tick_get_cpu_device()->evtdev;
 
 	return clockevents_program_event(dev, expires, force);
 }
@@ -36,7 +36,7 @@ int tick_program_event(ktime_t expires, int force)
  */
 void tick_resume_oneshot(void)
 {
-	struct clock_event_device *dev = __this_cpu_read(tick_cpu_device.evtdev);
+	struct clock_event_device *dev = tick_get_cpu_device()->evtdev;
 
 	clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT);
 	clockevents_program_event(dev, ktime_get(), true);
@@ -59,7 +59,7 @@ void tick_setup_oneshot(struct clock_event_device *newdev,
  */
 int tick_switch_to_oneshot(void (*handler)(struct clock_event_device *))
 {
-	struct tick_device *td = &__get_cpu_var(tick_cpu_device);
+	struct tick_device *td = tick_get_cpu_device();
 	struct clock_event_device *dev = td->evtdev;
 
 	if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT) ||
@@ -96,7 +96,7 @@ int tick_oneshot_mode_active(void)
 	int ret;
 
 	local_irq_save(flags);
-	ret = __this_cpu_read(tick_cpu_device.mode) == TICKDEV_MODE_ONESHOT;
+	ret = tick_get_cpu_device()->mode == TICKDEV_MODE_ONESHOT;
 	local_irq_restore(flags);
 
 	return ret;
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index d48d648..649b920 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -533,7 +533,7 @@ static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
 	unsigned long seq, last_jiffies, next_jiffies, delta_jiffies;
 	ktime_t last_update, expires, ret = { .tv64 = 0 };
 	unsigned long rcu_delta_jiffies;
-	struct clock_event_device *dev = __get_cpu_var(tick_cpu_device).evtdev;
+	struct clock_event_device *dev = tick_get_cpu_device()->evtdev;
 	u64 time_delta;
 
 	time_delta = timekeeping_max_deferment();
-- 
1.7.12.rc2.18.g61b472e

[PATCH 07/38] tick-oneshot: drop local_irq_save/restore from tick_switch_to_oneshot()

[Viresh Kumar]

tick_switch_to_oneshot() is just reading value of
__this_cpu_read(tick_cpu_device.mode) and doesn't need to disable local
interrupts for that. Drop local_irq_save/restore calls from this routine.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-oneshot.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c
index 6a531ec..39492f1 100644
--- a/kernel/time/tick-oneshot.c
+++ b/kernel/time/tick-oneshot.c
@@ -92,14 +92,7 @@ int tick_switch_to_oneshot(void (*handler)(struct clock_event_device *))
  */
 int tick_oneshot_mode_active(void)
 {
-	unsigned long flags;
-	int ret;
-
-	local_irq_save(flags);
-	ret = tick_get_cpu_device()->mode == TICKDEV_MODE_ONESHOT;
-	local_irq_restore(flags);
-
-	return ret;
+	return tick_get_cpu_device()->mode == TICKDEV_MODE_ONESHOT;
 }
 
 #ifdef CONFIG_HIGH_RES_TIMERS
-- 
1.7.12.rc2.18.g61b472e

[PATCH 08/38] tick-oneshot: move tick_is_oneshot_available() to tick-oneshot.c

[Viresh Kumar]

This is tick-oneshot specific routine and hence must be there in this file. Also
it isn't used outside of kernel/time/ and so moving its declaration to
tick-internal.h.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/tick.h        |  1 -
 kernel/time/tick-common.c   | 14 --------------
 kernel/time/tick-internal.h |  2 ++
 kernel/time/tick-oneshot.c  | 14 ++++++++++++++
 4 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index 98065e5..b8ee6f4 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -84,7 +84,6 @@ struct tick_sched {
 };
 
 extern void __init tick_init(void);
-extern int tick_is_oneshot_available(void);
 
 DECLARE_PER_CPU(struct tick_device, tick_cpu_device);
 static inline struct tick_device *tick_get_device(int cpu)
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index b4fdb28..298467e 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -53,20 +53,6 @@ int tick_do_timer_cpu __read_mostly = TICK_DO_TIMER_BOOT;
 /*
  * Debugging: see timer_list.c
  */
-/**
- * tick_is_oneshot_available - check for a oneshot capable event device
- */
-int tick_is_oneshot_available(void)
-{
-	struct clock_event_device *dev = tick_get_cpu_device()->evtdev;
-
-	if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT))
-		return 0;
-	if (!(dev->features & CLOCK_EVT_FEAT_C3STOP))
-		return 1;
-	return tick_broadcast_oneshot_available();
-}
-
 /*
  * Periodic tick
  */
diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
index 57c1a76..09f4307 100644
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -42,6 +42,7 @@ extern void tick_setup_oneshot(struct clock_event_device *newdev,
 extern int tick_program_event(ktime_t expires, int force);
 extern void tick_oneshot_notify(void);
 extern int tick_switch_to_oneshot(void (*handler)(struct clock_event_device *));
+extern int tick_is_oneshot_available(void);
 extern void tick_resume_oneshot(void);
 # ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 extern void tick_broadcast_setup_oneshot(struct clock_event_device *bc);
@@ -96,6 +97,7 @@ static inline int tick_resume_broadcast_oneshot(struct clock_event_device *bc)
 	return 0;
 }
 static inline int tick_broadcast_oneshot_active(void) { return 0; }
+static inline int tick_is_oneshot_available(void) { return 0; };
 static inline bool tick_broadcast_oneshot_available(void) { return false; }
 #endif /* !TICK_ONESHOT */
 
diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c
index 39492f1..cb158f7 100644
--- a/kernel/time/tick-oneshot.c
+++ b/kernel/time/tick-oneshot.c
@@ -95,6 +95,20 @@ int tick_oneshot_mode_active(void)
 	return tick_get_cpu_device()->mode == TICKDEV_MODE_ONESHOT;
 }
 
+/**
+ * tick_is_oneshot_available - check for a oneshot capable event device
+ */
+int tick_is_oneshot_available(void)
+{
+	struct clock_event_device *dev = tick_get_cpu_device()->evtdev;
+
+	if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT))
+		return 0;
+	if (!(dev->features & CLOCK_EVT_FEAT_C3STOP))
+		return 1;
+	return tick_broadcast_oneshot_available();
+}
+
 #ifdef CONFIG_HIGH_RES_TIMERS
 /**
  * tick_init_highres - switch to high resolution mode
-- 
1.7.12.rc2.18.g61b472e

[PATCH 09/38] tick-oneshot: remove tick_resume_oneshot()

[Viresh Kumar]

tick_resume_oneshot() and tick_setup_oneshot() have almost the same
implementation and tick_setup_oneshot() can be used easily instead of
tick_resume_oneshot(). So, remove tick_resume_oneshot() and use
tick_setup_oneshot() in its place.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-common.c   |  8 +++++---
 kernel/time/tick-internal.h |  5 -----
 kernel/time/tick-oneshot.c  | 11 -----------
 3 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index 298467e..949f3d3 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -363,15 +363,17 @@ void tick_suspend(void)
 void tick_resume(void)
 {
 	struct tick_device *td = tick_get_cpu_device();
+	struct clock_event_device *dev = td->evtdev;
 	int broadcast = tick_resume_broadcast();
 
-	clockevents_set_mode(td->evtdev, CLOCK_EVT_MODE_RESUME);
+	clockevents_set_mode(dev, CLOCK_EVT_MODE_RESUME);
 
 	if (!broadcast) {
 		if (td->mode == TICKDEV_MODE_PERIODIC)
-			tick_setup_periodic(td->evtdev, 0);
+			tick_setup_periodic(dev, 0);
 		else
-			tick_resume_oneshot();
+			tick_setup_oneshot(dev, dev->event_handler,
+					   ktime_get());
 	}
 }
 
diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
index 09f4307..65f080a 100644
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -43,7 +43,6 @@ extern int tick_program_event(ktime_t expires, int force);
 extern void tick_oneshot_notify(void);
 extern int tick_switch_to_oneshot(void (*handler)(struct clock_event_device *));
 extern int tick_is_oneshot_available(void);
-extern void tick_resume_oneshot(void);
 # ifdef CONFIG_GENERIC_CLOCKEVENTS_BROADCAST
 extern void tick_broadcast_setup_oneshot(struct clock_event_device *bc);
 extern int tick_broadcast_oneshot_control(unsigned long reason);
@@ -74,10 +73,6 @@ void tick_setup_oneshot(struct clock_event_device *newdev,
 {
 	BUG();
 }
-static inline void tick_resume_oneshot(void)
-{
-	BUG();
-}
 static inline int tick_program_event(ktime_t expires, int force)
 {
 	return 0;
diff --git a/kernel/time/tick-oneshot.c b/kernel/time/tick-oneshot.c
index cb158f7..82fc3f5 100644
--- a/kernel/time/tick-oneshot.c
+++ b/kernel/time/tick-oneshot.c
@@ -32,17 +32,6 @@ int tick_program_event(ktime_t expires, int force)
 }
 
 /**
- * tick_resume_onshot - resume oneshot mode
- */
-void tick_resume_oneshot(void)
-{
-	struct clock_event_device *dev = tick_get_cpu_device()->evtdev;
-
-	clockevents_set_mode(dev, CLOCK_EVT_MODE_ONESHOT);
-	clockevents_program_event(dev, ktime_get(), true);
-}
-
-/**
  * tick_setup_oneshot - setup the event device for oneshot mode (hres or nohz)
  */
 void tick_setup_oneshot(struct clock_event_device *newdev,
-- 
1.7.12.rc2.18.g61b472e

[PATCH 10/38] tick-common: remove extra checks from tick_check_new_device()

[Viresh Kumar]

tick_check_new_device() is calling tick_check_percpu() which has this as the
first test:

	if (!cpumask_test_cpu(cpu, newdev->cpumask))
		return false;

And so the same isn't required to be repeated by tick_check_new_device(). Remove
it.

This also initializes several variables during their definition only.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-common.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index 949f3d3..395cbbd 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -269,16 +269,9 @@ bool tick_check_replacement(struct clock_event_device *curdev,
  */
 void tick_check_new_device(struct clock_event_device *newdev)
 {
-	struct clock_event_device *curdev;
-	struct tick_device *td;
-	int cpu;
-
-	cpu = smp_processor_id();
-	if (!cpumask_test_cpu(cpu, newdev->cpumask))
-		goto out_bc;
-
-	td = tick_get_device(cpu);
-	curdev = td->evtdev;
+	int cpu = smp_processor_id();
+	struct tick_device *td = tick_get_device(cpu);
+	struct clock_event_device *curdev = td->evtdev;
 
 	/* cpu local device ? */
 	if (!tick_check_percpu(curdev, newdev, cpu))
-- 
1.7.12.rc2.18.g61b472e

[PATCH 11/38] tick-common: fix wrong check in tick_check_replacement()

[Viresh Kumar]

tick_check_replacement() returns if a replacement of clock_event_device is
possible or not. It does this as the first check:

	if (tick_check_percpu(curdev, newdev, smp_processor_id()))
		return false;

This looks wrong as we are returning false when tick_check_percpu() cpu returned
true. Probably Thomas forgot '!' here in his commit: 03e13cf5e ?

Fix it by placing a '!' before tick_check_percpu().

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

diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index 395cbbd..8650026 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -257,7 +257,7 @@ static bool tick_check_preferred(struct clock_event_device *curdev,
 bool tick_check_replacement(struct clock_event_device *curdev,
 			    struct clock_event_device *newdev)
 {
-	if (tick_check_percpu(curdev, newdev, smp_processor_id()))
+	if (!tick_check_percpu(curdev, newdev, smp_processor_id()))
 		return false;
 
 	return tick_check_preferred(curdev, newdev);
-- 
1.7.12.rc2.18.g61b472e

[PATCH 12/38] tick-common: call tick_check_percpu() from tick_check_preferred()

[Viresh Kumar]

tick_check_percpu() and tick_check_preferred() are called from two places and in
exactly same order. So, would make sense to call tick_check_percpu() from
tick_check_preferred() instead, so that their common caller can just call
tick_check_preferred().

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-common.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index 8650026..6868d6a 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -233,6 +233,9 @@ static bool tick_check_percpu(struct clock_event_device *curdev,
 static bool tick_check_preferred(struct clock_event_device *curdev,
 				 struct clock_event_device *newdev)
 {
+	if (!tick_check_percpu(curdev, newdev, smp_processor_id()))
+		return false;
+
 	/* Prefer oneshot capable device */
 	if (!(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) {
 		if (curdev && (curdev->features & CLOCK_EVT_FEAT_ONESHOT))
@@ -257,9 +260,6 @@ static bool tick_check_preferred(struct clock_event_device *curdev,
 bool tick_check_replacement(struct clock_event_device *curdev,
 			    struct clock_event_device *newdev)
 {
-	if (!tick_check_percpu(curdev, newdev, smp_processor_id()))
-		return false;
-
 	return tick_check_preferred(curdev, newdev);
 }
 
@@ -273,10 +273,6 @@ void tick_check_new_device(struct clock_event_device *newdev)
 	struct tick_device *td = tick_get_device(cpu);
 	struct clock_event_device *curdev = td->evtdev;
 
-	/* cpu local device ? */
-	if (!tick_check_percpu(curdev, newdev, cpu))
-		goto out_bc;
-
 	/* Preference decision */
 	if (!tick_check_preferred(curdev, newdev))
 		goto out_bc;
-- 
1.7.12.rc2.18.g61b472e

[PATCH 13/38] tick-common: don't check tick_oneshot_mode_active() from tick_check_preferred()

[Viresh Kumar]

If 'curdev' passed to tick_check_preferred() is the current clock_event_device
then these two checks look exactly same, because td->mode = TICKDEV_MODE_ONESHOT
is only called when the event device had ONESHOT feature.

	if (curdev && (curdev->features & CLOCK_EVT_FEAT_ONESHOT))
		return false;

	if (tick_oneshot_mode_active())
		return false;

Now left the case where 'curdev' is not the current clock_event_device. This can
happen from the sequence started from clockevents_replace(). Here we are trying
to find the best possible device that we should choose. And so even in this case
we don't need the above check as we aren't really worried about the current
device.

So, the second check can be removed.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-common.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index 6868d6a..3dc31f5 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -240,8 +240,6 @@ static bool tick_check_preferred(struct clock_event_device *curdev,
 	if (!(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) {
 		if (curdev && (curdev->features & CLOCK_EVT_FEAT_ONESHOT))
 			return false;
-		if (tick_oneshot_mode_active())
-			return false;
 	}
 
 	/*
-- 
1.7.12.rc2.18.g61b472e

[PATCH 14/38] tick-common: do additional checks in tick_check_preferred()

[Viresh Kumar]

We return false from tick_check_preferred() if newdev doesn't have ONESHOT
feature but curdev has, but we don't return true when newdev has ONESHOT and
curdev doesn't. Instead we go on, check ratings and other things in that case.

This patch tries to fix this by rewriting some portion of this code and adds
sufficient comments to make logic clear.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-common.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index 3dc31f5..3bd9f9c 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -236,18 +236,29 @@ static bool tick_check_preferred(struct clock_event_device *curdev,
 	if (!tick_check_percpu(curdev, newdev, smp_processor_id()))
 		return false;
 
-	/* Prefer oneshot capable device */
-	if (!(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) {
-		if (curdev && (curdev->features & CLOCK_EVT_FEAT_ONESHOT))
-			return false;
-	}
+	if (!curdev)
+		return true;
+
+	/*
+	 * Prefer oneshot capable device.
+	 * return values based on if ONESHOT is available or not:
+	 *
+	 * curdev	newdev		operation
+	 * 0		0		check priority
+	 * 0		1		return true
+	 * 1		0		return false
+	 * 1		1		check priority
+	 */
+
+	if ((newdev->features & CLOCK_EVT_FEAT_ONESHOT) ^
+		(curdev->features & CLOCK_EVT_FEAT_ONESHOT))
+		return newdev->features & CLOCK_EVT_FEAT_ONESHOT;
 
 	/*
 	 * Use the higher rated one, but prefer a CPU local device with a lower
 	 * rating than a non-CPU local device
 	 */
-	return !curdev ||
-		newdev->rating > curdev->rating ||
+	return newdev->rating > curdev->rating ||
 	       !cpumask_equal(curdev->cpumask, newdev->cpumask);
 }
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH 15/38] tick-common: remove tick_check_replacement()

[Viresh Kumar]

tick_check_replacement() is now nothing more than a wrapper over
tick_check_preferred() and hence can be removed. Use tick_check_preferred()
everywhere.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/clockevents.c   |  2 +-
 kernel/time/tick-common.c   | 16 +++++-----------
 kernel/time/tick-internal.h |  2 +-
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 6680a0d..d80e891 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -298,7 +298,7 @@ static int clockevents_replace(struct clock_event_device *ced)
 		if (dev == ced || dev->mode != CLOCK_EVT_MODE_UNUSED)
 			continue;
 
-		if (!tick_check_replacement(newdev, dev))
+		if (!tick_check_preferred(newdev, dev))
 			continue;
 
 		if (!try_module_get(dev->owner))
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index 3bd9f9c..c1b3619 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -230,7 +230,11 @@ static bool tick_check_percpu(struct clock_event_device *curdev,
 	return true;
 }
 
-static bool tick_check_preferred(struct clock_event_device *curdev,
+/*
+ * Check whether the new device is a better fit than curdev. curdev
+ * can be NULL !
+ */
+bool tick_check_preferred(struct clock_event_device *curdev,
 				 struct clock_event_device *newdev)
 {
 	if (!tick_check_percpu(curdev, newdev, smp_processor_id()))
@@ -263,16 +267,6 @@ static bool tick_check_preferred(struct clock_event_device *curdev,
 }
 
 /*
- * Check whether the new device is a better fit than curdev. curdev
- * can be NULL !
- */
-bool tick_check_replacement(struct clock_event_device *curdev,
-			    struct clock_event_device *newdev)
-{
-	return tick_check_preferred(curdev, newdev);
-}
-
-/*
  * Check, if the new registered device should be used. Called with
  * clockevents_lock held and interrupts disabled.
  */
diff --git a/kernel/time/tick-internal.h b/kernel/time/tick-internal.h
index 65f080a..8c6e85d 100644
--- a/kernel/time/tick-internal.h
+++ b/kernel/time/tick-internal.h
@@ -24,7 +24,7 @@ extern void tick_handover_do_timer(int *cpup);
 extern void tick_shutdown(unsigned int *cpup);
 extern void tick_suspend(void);
 extern void tick_resume(void);
-extern bool tick_check_replacement(struct clock_event_device *curdev,
+extern bool tick_check_preferred(struct clock_event_device *curdev,
 				   struct clock_event_device *newdev);
 extern void tick_install_replacement(struct clock_event_device *dev);
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH 16/38] tick-common: don't pass cpumask to tick_setup_device()

[Viresh Kumar]

tick_setup_device() is always passed cpumask of 'cpu', which is also passed as
parameter to this routine. So, just calculate the cpumask(cpu) in
tick_setup_device() instead of passing from every caller.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-common.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index c1b3619..c2aa441 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -145,9 +145,9 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast)
  * Setup the tick device
  */
 static void tick_setup_device(struct tick_device *td,
-			      struct clock_event_device *newdev, int cpu,
-			      const struct cpumask *cpumask)
+			      struct clock_event_device *newdev, int cpu)
 {
+	const struct cpumask *cpumask = cpumask_of(cpu);
 	ktime_t next_event;
 	void (*handler)(struct clock_event_device *) = NULL;
 
@@ -209,7 +209,7 @@ void tick_install_replacement(struct clock_event_device *newdev)
 	int cpu = smp_processor_id();
 
 	clockevents_exchange_device(td->evtdev, newdev);
-	tick_setup_device(td, newdev, cpu, cpumask_of(cpu));
+	tick_setup_device(td, newdev, cpu);
 	if (newdev->features & CLOCK_EVT_FEAT_ONESHOT)
 		tick_oneshot_notify();
 }
@@ -293,7 +293,7 @@ void tick_check_new_device(struct clock_event_device *newdev)
 		curdev = NULL;
 	}
 	clockevents_exchange_device(curdev, newdev);
-	tick_setup_device(td, newdev, cpu, cpumask_of(cpu));
+	tick_setup_device(td, newdev, cpu);
 	if (newdev->features & CLOCK_EVT_FEAT_ONESHOT)
 		tick_oneshot_notify();
 	return;
-- 
1.7.12.rc2.18.g61b472e

[PATCH 17/38] tick-common: call tick_install_replacement() from tick_check_new_device()

[Viresh Kumar]

Part of tick_check_new_device() is exactly same as what's being done by
tick_install_replacement(). And so we can actually call
tick_install_replacement() directly from tick_check_new_device() instead of
replicating code.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-common.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index c2aa441..bb9a218 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -272,9 +272,7 @@ bool tick_check_preferred(struct clock_event_device *curdev,
  */
 void tick_check_new_device(struct clock_event_device *newdev)
 {
-	int cpu = smp_processor_id();
-	struct tick_device *td = tick_get_device(cpu);
-	struct clock_event_device *curdev = td->evtdev;
+	struct clock_event_device *curdev = tick_get_cpu_device()->evtdev;
 
 	/* Preference decision */
 	if (!tick_check_preferred(curdev, newdev))
@@ -292,10 +290,8 @@ void tick_check_new_device(struct clock_event_device *newdev)
 		clockevents_shutdown(curdev);
 		curdev = NULL;
 	}
-	clockevents_exchange_device(curdev, newdev);
-	tick_setup_device(td, newdev, cpu);
-	if (newdev->features & CLOCK_EVT_FEAT_ONESHOT)
-		tick_oneshot_notify();
+
+	tick_install_replacement(newdev);
 	return;
 
 out_bc:
-- 
1.7.12.rc2.18.g61b472e

[PATCH 18/38] tick-common: don't set mode to CLOCK_EVT_MODE_UNUSED in tick_shutdown()

[Viresh Kumar]

tick_shutdown() calls clockevents_exchange_device() which already sets mode to
CLOCK_EVT_MODE_UNUSED and so tick_shutdown() doesn't need to do it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-common.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index bb9a218..3954f4d 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -334,7 +334,6 @@ void tick_shutdown(unsigned int *cpup)
 		 * Prevent that the clock events layer tries to call
 		 * the set mode function!
 		 */
-		dev->mode = CLOCK_EVT_MODE_UNUSED;
 		clockevents_exchange_device(dev, NULL);
 		dev->event_handler = clockevents_handle_noop;
 		td->evtdev = NULL;
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 18/38] tick-common: don't set mode to CLOCK_EVT_MODE_UNUSED in tick_shutdown()

[Stephen Boyd]

On 04/14/14 09:23, Viresh Kumar wrote:
> tick_shutdown() calls clockevents_exchange_device() which already sets mode to
> CLOCK_EVT_MODE_UNUSED 

Yes it does.

> and so tick_shutdown() doesn't need to do it.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  kernel/time/tick-common.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
> index bb9a218..3954f4d 100644
> --- a/kernel/time/tick-common.c
> +++ b/kernel/time/tick-common.c
> @@ -334,7 +334,6 @@ void tick_shutdown(unsigned int *cpup)
>  		 * Prevent that the clock events layer tries to call
>  		 * the set mode function!
>  		 */
> -		dev->mode = CLOCK_EVT_MODE_UNUSED;

But see the comment above this assignment. The point is to avoid calling
the ->set_mode function.

>  		clockevents_exchange_device(dev, NULL);
>  		dev->event_handler = clockevents_handle_noop;
>  		td->evtdev = NULL;

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

[Viresh Kumar]

On 15 April 2014 01:08, Stephen Boyd <sboyd@codeaurora.org> wrote:
> But see the comment above this assignment. The point is to avoid calling
> the ->set_mode function.

Okay, that was a stupid change :)

[PATCH 19/38] tick-common: remove local variable 'broadcast' from tick_resume()

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

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-common.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index 3954f4d..e1b2947 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -351,17 +351,16 @@ void tick_resume(void)
 {
 	struct tick_device *td = tick_get_cpu_device();
 	struct clock_event_device *dev = td->evtdev;
-	int broadcast = tick_resume_broadcast();
 
 	clockevents_set_mode(dev, CLOCK_EVT_MODE_RESUME);
 
-	if (!broadcast) {
-		if (td->mode == TICKDEV_MODE_PERIODIC)
-			tick_setup_periodic(dev, 0);
-		else
-			tick_setup_oneshot(dev, dev->event_handler,
-					   ktime_get());
-	}
+	if (tick_resume_broadcast())
+		return;
+
+	if (td->mode == TICKDEV_MODE_PERIODIC)
+		tick_setup_periodic(dev, 0);
+	else
+		tick_setup_oneshot(dev, dev->event_handler, ktime_get());
 }
 
 /**
-- 
1.7.12.rc2.18.g61b472e

[PATCH 20/38] tick-sched: initialize 'cpu' while defining it in tick_nohz_full_setup()

[Viresh Kumar]

We are doing this currently:

	int cpu;

	cpu = smp_processor_id();

And we can rewrite it to make it shorter:
	int cpu = smp_processor_id();

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

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 649b920..e9f9f87 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -274,7 +274,7 @@ out:
 /* Parse the boot-time nohz CPU list from the kernel parameters. */
 static int __init tick_nohz_full_setup(char *str)
 {
-	int cpu;
+	int cpu = smp_processor_id();
 
 	alloc_bootmem_cpumask_var(&tick_nohz_full_mask);
 	if (cpulist_parse(str, tick_nohz_full_mask) < 0) {
@@ -282,7 +282,6 @@ static int __init tick_nohz_full_setup(char *str)
 		return 1;
 	}
 
-	cpu = smp_processor_id();
 	if (cpumask_test_cpu(cpu, tick_nohz_full_mask)) {
 		pr_warning("NO_HZ: Clearing %d from nohz_full range for timekeeping\n",
 			   cpu);
-- 
1.7.12.rc2.18.g61b472e

[PATCH 21/38] tick-sched: no need to rewrite '1' to tick_nohz_enabled

[Viresh Kumar]

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

Also, an extra blank line is removed.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-sched.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index e9f9f87..9cbba513 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -370,13 +370,10 @@ static int __init setup_tick_nohz(char *str)
 {
 	if (!strcmp(str, "off"))
 		tick_nohz_enabled = 0;
-	else if (!strcmp(str, "on"))
-		tick_nohz_enabled = 1;
-	else
+	else if (strcmp(str, "on"))
 		return 0;
 	return 1;
 }
-
 __setup("nohz=", setup_tick_nohz);
 
 /**
-- 
1.7.12.rc2.18.g61b472e

[PATCH 22/38] tick-sched: no need to recheck cpu_online() in can_stop_idle_tick()

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

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 9cbba513..c81b6cf 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -716,7 +716,7 @@ static bool can_stop_idle_tick(int cpu, struct tick_sched *ts)
 	if (need_resched())
 		return false;
 
-	if (unlikely(local_softirq_pending() && cpu_online(cpu))) {
+	if (unlikely(local_softirq_pending())) {
 		static int ratelimit;
 
 		if (ratelimit < 10 &&
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 22/38] tick-sched: no need to recheck cpu_online() in can_stop_idle_tick()

[Stephen Boyd]

On 04/14/14 09:23, Viresh Kumar wrote:
> We have already checked if 'cpu' is online or not and so don't need to recheck
> it.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Hm... doing some git archeology shows fa116ea35ec7 (nohz: no softirq
pending warnings for offline cpus, 2008-12-11), where the cpu_online()
check was added. Before that commit we already checked cpu_online()
similar to how the code is today. Perhaps we need to add a comment here?

> ---
>  kernel/time/tick-sched.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 9cbba513..c81b6cf 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -716,7 +716,7 @@ static bool can_stop_idle_tick(int cpu, struct tick_sched *ts)
>  	if (need_resched())
>  		return false;
>  
> -	if (unlikely(local_softirq_pending() && cpu_online(cpu))) {
> +	if (unlikely(local_softirq_pending())) {
>  		static int ratelimit;
>  
>  		if (ratelimit < 10 &&


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

[Viresh Kumar]

On 15 April 2014 01:22, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 04/14/14 09:23, Viresh Kumar wrote:
>> We have already checked if 'cpu' is online or not and so don't need to recheck
>> it.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> Hm... doing some git archeology shows fa116ea35ec7 (nohz: no softirq
> pending warnings for offline cpus, 2008-12-11), where the cpu_online()
> check was added. Before that commit we already checked cpu_online()
> similar to how the code is today. Perhaps we need to add a comment here?

Okay, I didn't understood what you want completely. You want me to update
commit logs or drop this patch and update code?

But yes there is one more thing worth mentioning:

commit f7ea0fd6 (tick: Don't invoke tick_nohz_stop_sched_tick() if the cpu is
offline, May 13 2013), which did this:

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 0eed1db..0121421 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -469,6 +469,7 @@ static bool can_stop_idle_tick(int cpu, struct
tick_sched *ts)
        if (unlikely(!cpu_online(cpu))) {
                if (cpu == tick_do_timer_cpu)
                        tick_do_timer_cpu = TICK_DO_TIMER_NONE;
+               return false;
        }

        if (unlikely(ts->nohz_mode == NOHZ_MODE_INACTIVE))


So, when the patch you mentioned was added, we didn't had this and a
recheck of cpu_online() was perhaps required. But it is surely not required
anymore.

[PATCH 23/38] tick-sched: invert parameter of tick_check_oneshot_change()

[Viresh Kumar]

There is only one caller of tick_check_oneshot_change(), i.e.
hrtimer_run_pending(). Firstly hrtimer_run_pending() is calling this routine
after doing a '!' of its parameter and then tick_check_oneshot_change() is also
using it after doing a '!' of its parameter.

It would be more efficient and readable if we can just invert the meaning of
this parameter. So, it is hres_enabled instead of allow_nohz now.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/tick.h     |  6 +++---
 kernel/hrtimer.c         |  2 +-
 kernel/time/tick-sched.c | 10 +++++-----
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index b8ee6f4..801e844 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -120,7 +120,7 @@ extern struct cpumask *tick_get_broadcast_oneshot_mask(void);
 
 # ifdef CONFIG_TICK_ONESHOT
 extern void tick_clock_notify(void);
-extern int tick_check_oneshot_change(int allow_nohz);
+extern int tick_check_oneshot_change(int hres_enabled);
 extern struct tick_sched *tick_get_tick_sched(int cpu);
 extern void tick_irq_enter(void);
 extern int tick_oneshot_mode_active(void);
@@ -129,7 +129,7 @@ extern int tick_oneshot_mode_active(void);
 #  endif
 # else
 static inline void tick_clock_notify(void) { }
-static inline int tick_check_oneshot_change(int allow_nohz) { return 0; }
+static inline int tick_check_oneshot_change(int hres_enabled) { return 0; }
 static inline void tick_irq_enter(void) { }
 static inline int tick_oneshot_mode_active(void) { return 0; }
 # endif
@@ -138,7 +138,7 @@ static inline int tick_oneshot_mode_active(void) { return 0; }
 static inline void tick_init(void) { }
 static inline void tick_cancel_sched_timer(int cpu) { }
 static inline void tick_clock_notify(void) { }
-static inline int tick_check_oneshot_change(int allow_nohz) { return 0; }
+static inline int tick_check_oneshot_change(int hres_enabled) { return 0; }
 static inline void tick_irq_enter(void) { }
 static inline int tick_oneshot_mode_active(void) { return 0; }
 #endif /* !CONFIG_GENERIC_CLOCKEVENTS */
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 393f422..bfa7811 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -1457,7 +1457,7 @@ void hrtimer_run_pending(void)
 	 * check bit in the tick_oneshot code, otherwise we might
 	 * deadlock vs. xtime_lock.
 	 */
-	if (tick_check_oneshot_change(!hrtimer_is_hres_enabled()))
+	if (tick_check_oneshot_change(hrtimer_is_hres_enabled()))
 		hrtimer_switch_to_hres();
 }
 
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index c81b6cf..256f4a3 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -1177,11 +1177,11 @@ void tick_oneshot_notify(void)
  * Check, if a change happened, which makes oneshot possible.
  *
  * Called cyclic from the hrtimer softirq (driven by the timer
- * softirq) allow_nohz signals, that we can switch into low-res nohz
- * mode, because high resolution timers are disabled (either compile
- * or runtime).
+ * softirq). If hres_enabled is non zero, it means we can't switch into low-res
+ * nohz mode, because high resolution timers are enabled(either compile or
+ * runtime).
  */
-int tick_check_oneshot_change(int allow_nohz)
+int tick_check_oneshot_change(int hres_enabled)
 {
 	struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched);
 
@@ -1194,7 +1194,7 @@ int tick_check_oneshot_change(int allow_nohz)
 	if (!timekeeping_valid_for_hres() || !tick_is_oneshot_available())
 		return 0;
 
-	if (!allow_nohz)
+	if (hres_enabled)
 		return 1;
 
 	tick_nohz_switch_to_nohz();
-- 
1.7.12.rc2.18.g61b472e

[PATCH 24/38] tick-sched: don't check tick_nohz_full_cpu() in __tick_nohz_task_switch()

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

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-sched.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 256f4a3..5a99859 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -261,13 +261,9 @@ void __tick_nohz_task_switch(struct task_struct *tsk)
 
 	local_irq_save(flags);
 
-	if (!tick_nohz_full_cpu(smp_processor_id()))
-		goto out;
-
 	if (tick_nohz_tick_stopped() && !can_stop_full_tick())
 		tick_nohz_full_kick();
 
-out:
 	local_irq_restore(flags);
 }
 
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 24/38] tick-sched: don't check tick_nohz_full_cpu() in __tick_nohz_task_switch()

[Frederic Weisbecker]

On Mon, Apr 14, 2014 at 09:53:46PM +0530, Viresh Kumar wrote:
> __tick_nohz_task_switch() calls tick_nohz_full_kick(), which is already checking
> tick_nohz_full_cpu() and so we don't need to repeat the same check here.
> 
> Remove it.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Ack.

> ---
>  kernel/time/tick-sched.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index 256f4a3..5a99859 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -261,13 +261,9 @@ void __tick_nohz_task_switch(struct task_struct *tsk)
>  
>  	local_irq_save(flags);
>  
> -	if (!tick_nohz_full_cpu(smp_processor_id()))
> -		goto out;
> -
>  	if (tick_nohz_tick_stopped() && !can_stop_full_tick())
>  		tick_nohz_full_kick();
>  
> -out:
>  	local_irq_restore(flags);
>  }
>  
> -- 
> 1.7.12.rc2.18.g61b472e
> 

[PATCH 25/38] tick-sched: don't call local_softirq_pending() thrice in can_stop_idle_tick()

[Viresh Kumar]

can_stop_idle_tick() checks if a local softirq is pending or not, and in case
it is pending we call the same routine two more times.

Rather store the value in a local variable and reuse it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-sched.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 5a99859..b7b09b9 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -691,6 +691,8 @@ static void tick_nohz_full_stop_tick(struct tick_sched *ts)
 
 static bool can_stop_idle_tick(int cpu, struct tick_sched *ts)
 {
+	unsigned int softirq;
+
 	/*
 	 * If this cpu is offline and it is the one which updates
 	 * jiffies, then give up the assignment and let it be taken by
@@ -712,13 +714,11 @@ static bool can_stop_idle_tick(int cpu, struct tick_sched *ts)
 	if (need_resched())
 		return false;
 
-	if (unlikely(local_softirq_pending())) {
+	if (unlikely((softirq = local_softirq_pending()))) {
 		static int ratelimit;
 
-		if (ratelimit < 10 &&
-		    (local_softirq_pending() & SOFTIRQ_STOP_IDLE_MASK)) {
-			pr_warn("NOHZ: local_softirq_pending %02x\n",
-				(unsigned int) local_softirq_pending());
+		if (ratelimit < 10 && (softirq & SOFTIRQ_STOP_IDLE_MASK)) {
+			pr_warn("NOHZ: local_softirq_pending %02x\n", softirq);
 			ratelimit++;
 		}
 		return false;
-- 
1.7.12.rc2.18.g61b472e

[PATCH 26/38] tick-sched: don't call update_wall_time() when delta is lesser than tick_period

[Viresh Kumar]

In tick_do_update_jiffies64() we are processing ticks only if delta is greater
than tick_period. This is what we are supposed to do here and it broke a bit
with this patch:

commit 47a1b796306356f358e515149d86baf0cc6bf007
Author: John Stultz <john.stultz@linaro.org>
Date:   Thu Dec 12 13:10:55 2013 -0800

    tick/timekeeping: Call update_wall_time outside the jiffies lock

With above patch, we might end up calling update_wall_time() even if delta is
found to be smaller that tick_period. Fix this by reversing the check and
returning early.

Cc: John Stultz <john.stultz@linaro.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-sched.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index b7b09b9..bff7f97 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -65,26 +65,28 @@ static void tick_do_update_jiffies64(ktime_t now)
 	write_seqlock(&jiffies_lock);
 
 	delta = ktime_sub(now, last_jiffies_update);
-	if (delta.tv64 >= tick_period.tv64) {
-
-		delta = ktime_sub(delta, tick_period);
-		last_jiffies_update = ktime_add(last_jiffies_update,
-						tick_period);
+	if (delta.tv64 < tick_period.tv64) {
+		write_sequnlock(&jiffies_lock);
+		return;
+	}
 
-		/* Slow path for long timeouts */
-		if (unlikely(delta.tv64 >= tick_period.tv64)) {
-			s64 incr = ktime_to_ns(tick_period);
+	delta = ktime_sub(delta, tick_period);
+	last_jiffies_update = ktime_add(last_jiffies_update, tick_period);
 
-			ticks = ktime_divns(delta, incr);
+	/* Slow path for long timeouts */
+	if (unlikely(delta.tv64 >= tick_period.tv64)) {
+		s64 incr = ktime_to_ns(tick_period);
 
-			last_jiffies_update = ktime_add_ns(last_jiffies_update,
-							   incr * ticks);
-		}
-		do_timer(++ticks);
+		ticks = ktime_divns(delta, incr);
 
-		/* Keep the tick_next_period variable up to date */
-		tick_next_period = ktime_add(last_jiffies_update, tick_period);
+		last_jiffies_update = ktime_add_ns(last_jiffies_update,
+						   incr * ticks);
 	}
+	do_timer(++ticks);
+
+	/* Keep the tick_next_period variable up to date */
+	tick_next_period = ktime_add(last_jiffies_update, tick_period);
+
 	write_sequnlock(&jiffies_lock);
 	update_wall_time();
 }
-- 
1.7.12.rc2.18.g61b472e

[PATCH 27/38] tick-sched: remove 'regs' parameter of tick_sched_handle()

[Viresh Kumar]

tick_sched_handle() is called from two places and both pass 'regs' to it almost
same way. This patch removes this parameter to tick_sched_handle() and updates
tick_sched_handle() to get that by itself.

The only point of difference in the way this routine was called from its callers
was, don't call it from process context. To make sure the nohz path doesn't
suffer from an additional branch instruction, add unlikely to the if()
statement.

Though I still don't understand when will we run tick_sched_timer() from process
context. I couldn't see a single instance of that happening on my test machine
for normal boot followed by some userspace operations.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-sched.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index bff7f97..ee0768b 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -129,8 +129,17 @@ static void tick_sched_do_timer(ktime_t now)
 		tick_do_update_jiffies64(now);
 }
 
-static void tick_sched_handle(struct tick_sched *ts, struct pt_regs *regs)
+static void tick_sched_handle(struct tick_sched *ts)
 {
+	struct pt_regs *regs = get_irq_regs();
+
+	/*
+	 * Do not call, when we are not in irq context and have
+	 * no valid regs pointer
+	 */
+	if (unlikely(!regs))
+		return;
+
 #ifdef CONFIG_NO_HZ_COMMON
 	/*
 	 * When we are idle and the tick is stopped, we have to touch
@@ -942,13 +951,12 @@ static int tick_nohz_reprogram(struct tick_sched *ts, ktime_t now)
 static void tick_nohz_handler(struct clock_event_device *dev)
 {
 	struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched);
-	struct pt_regs *regs = get_irq_regs();
 	ktime_t now = ktime_get();
 
 	dev->next_event.tv64 = KTIME_MAX;
 
 	tick_sched_do_timer(now);
-	tick_sched_handle(ts, regs);
+	tick_sched_handle(ts);
 
 	while (tick_nohz_reprogram(ts, now)) {
 		now = ktime_get();
@@ -1065,17 +1073,10 @@ static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer)
 {
 	struct tick_sched *ts =
 		container_of(timer, struct tick_sched, sched_timer);
-	struct pt_regs *regs = get_irq_regs();
 	ktime_t now = ktime_get();
 
 	tick_sched_do_timer(now);
-
-	/*
-	 * Do not call, when we are not in irq context and have
-	 * no valid regs pointer
-	 */
-	if (regs)
-		tick_sched_handle(ts, regs);
+	tick_sched_handle(ts);
 
 	hrtimer_forward(timer, now, tick_period);
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH 28/38] tick-sched: remove parameters to {__}tick_nohz_task_switch() routines

[Viresh Kumar]

tick_nohz_task_switch() and __tick_nohz_task_switch() routines get task_struct
passed to them (always for the 'current' task), but they never use it. Remove
it.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/tick.h     | 8 ++++----
 kernel/sched/core.c      | 2 +-
 kernel/time/tick-sched.c | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index 801e844..1065a51 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -201,7 +201,7 @@ extern void tick_nohz_init(void);
 extern void __tick_nohz_full_check(void);
 extern void tick_nohz_full_kick(void);
 extern void tick_nohz_full_kick_all(void);
-extern void __tick_nohz_task_switch(struct task_struct *tsk);
+extern void __tick_nohz_task_switch(void);
 #else
 static inline void tick_nohz_init(void) { }
 static inline bool tick_nohz_full_enabled(void) { return false; }
@@ -209,7 +209,7 @@ static inline bool tick_nohz_full_cpu(int cpu) { return false; }
 static inline void __tick_nohz_full_check(void) { }
 static inline void tick_nohz_full_kick(void) { }
 static inline void tick_nohz_full_kick_all(void) { }
-static inline void __tick_nohz_task_switch(struct task_struct *tsk) { }
+static inline void __tick_nohz_task_switch(void) { }
 #endif
 
 static inline void tick_nohz_full_check(void)
@@ -218,10 +218,10 @@ static inline void tick_nohz_full_check(void)
 		__tick_nohz_full_check();
 }
 
-static inline void tick_nohz_task_switch(struct task_struct *tsk)
+static inline void tick_nohz_task_switch(void)
 {
 	if (tick_nohz_full_enabled())
-		__tick_nohz_task_switch(tsk);
+		__tick_nohz_task_switch();
 }
 
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 13299c5..84dc349 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -2156,7 +2156,7 @@ static void finish_task_switch(struct rq *rq, struct task_struct *prev)
 		put_task_struct(prev);
 	}
 
-	tick_nohz_task_switch(current);
+	tick_nohz_task_switch();
 }
 
 #ifdef CONFIG_SMP
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index ee0768b..5f7796d 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -266,7 +266,7 @@ void tick_nohz_full_kick_all(void)
  * It might need the tick due to per task/process properties:
  * perf events, posix cpu timers, ...
  */
-void __tick_nohz_task_switch(struct task_struct *tsk)
+void __tick_nohz_task_switch(void)
 {
 	unsigned long flags;
 
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 28/38] tick-sched: remove parameters to {__}tick_nohz_task_switch() routines

[Frederic Weisbecker]

On Mon, Apr 14, 2014 at 09:53:50PM +0530, Viresh Kumar wrote:
> tick_nohz_task_switch() and __tick_nohz_task_switch() routines get task_struct
> passed to them (always for the 'current' task), but they never use it. Remove
> it.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Ack.

[PATCH 29/38] tick-sched: remove wrapper around __tick_nohz_task_switch()

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

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/tick.h     | 11 ++---------
 kernel/time/tick-sched.c |  7 +++++--
 2 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/include/linux/tick.h b/include/linux/tick.h
index 1065a51..585be84 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -201,7 +201,7 @@ extern void tick_nohz_init(void);
 extern void __tick_nohz_full_check(void);
 extern void tick_nohz_full_kick(void);
 extern void tick_nohz_full_kick_all(void);
-extern void __tick_nohz_task_switch(void);
+extern void tick_nohz_task_switch(void);
 #else
 static inline void tick_nohz_init(void) { }
 static inline bool tick_nohz_full_enabled(void) { return false; }
@@ -209,7 +209,7 @@ static inline bool tick_nohz_full_cpu(int cpu) { return false; }
 static inline void __tick_nohz_full_check(void) { }
 static inline void tick_nohz_full_kick(void) { }
 static inline void tick_nohz_full_kick_all(void) { }
-static inline void __tick_nohz_task_switch(void) { }
+static inline void tick_nohz_task_switch(void) { }
 #endif
 
 static inline void tick_nohz_full_check(void)
@@ -218,11 +218,4 @@ static inline void tick_nohz_full_check(void)
 		__tick_nohz_full_check();
 }
 
-static inline void tick_nohz_task_switch(void)
-{
-	if (tick_nohz_full_enabled())
-		__tick_nohz_task_switch();
-}
-
-
 #endif
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 5f7796d..d8b9a69 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -266,13 +266,16 @@ void tick_nohz_full_kick_all(void)
  * It might need the tick due to per task/process properties:
  * perf events, posix cpu timers, ...
  */
-void __tick_nohz_task_switch(void)
+void tick_nohz_task_switch(void)
 {
 	unsigned long flags;
 
+	if (!tick_nohz_full_enabled() || !tick_nohz_tick_stopped())
+		return;
+
 	local_irq_save(flags);
 
-	if (tick_nohz_tick_stopped() && !can_stop_full_tick())
+	if (!can_stop_full_tick())
 		tick_nohz_full_kick();
 
 	local_irq_restore(flags);
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 29/38] tick-sched: remove wrapper around __tick_nohz_task_switch()

[Frederic Weisbecker]

On Mon, Apr 14, 2014 at 09:53:51PM +0530, Viresh Kumar wrote:
> __tick_nohz_task_switch() was called only from tick_nohz_task_switch() and there
> is nothing much in tick_nohz_task_switch() as well. IOW, we don't need
> unnecessary wrapper over __tick_nohz_task_switch() to be there. Merge all code
> from __tick_nohz_task_switch() into tick_nohz_task_switch() and move it to
> tick-sched.c.
> 
> This also moves check for tick_nohz_tick_stopped() outside of irq_save()
> context.

No, the wrapper is there on purpose in order to optimize the full dynticks off case in
the context switch path with the jump label'ed check on tick_nohz_full_enabled().

Re: [PATCH 29/38] tick-sched: remove wrapper around __tick_nohz_task_switch()

[Viresh Kumar]

On 15 April 2014 04:52, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Mon, Apr 14, 2014 at 09:53:51PM +0530, Viresh Kumar wrote:
>> __tick_nohz_task_switch() was called only from tick_nohz_task_switch() and there
>> is nothing much in tick_nohz_task_switch() as well. IOW, we don't need
>> unnecessary wrapper over __tick_nohz_task_switch() to be there. Merge all code
>> from __tick_nohz_task_switch() into tick_nohz_task_switch() and move it to
>> tick-sched.c.
>>
>> This also moves check for tick_nohz_tick_stopped() outside of irq_save()
>> context.
>
> No, the wrapper is there on purpose in order to optimize the full dynticks off case in
> the context switch path with the jump label'ed check on tick_nohz_full_enabled().

Just to clarify, you are saying that:

Wrapper was there to save an extra function call when tick_nohz_full_enabled()
returns false, as tick_nohz_task_switch() will be inlined ?

In this case probably we can move !can_stop_full_tick() as well to the wrapper ?

Re: [PATCH 29/38] tick-sched: remove wrapper around __tick_nohz_task_switch()

[Frederic Weisbecker]

On Tue, Apr 15, 2014 at 10:15:24AM +0530, Viresh Kumar wrote:
> On 15 April 2014 04:52, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > On Mon, Apr 14, 2014 at 09:53:51PM +0530, Viresh Kumar wrote:
> >> __tick_nohz_task_switch() was called only from tick_nohz_task_switch() and there
> >> is nothing much in tick_nohz_task_switch() as well. IOW, we don't need
> >> unnecessary wrapper over __tick_nohz_task_switch() to be there. Merge all code
> >> from __tick_nohz_task_switch() into tick_nohz_task_switch() and move it to
> >> tick-sched.c.
> >>
> >> This also moves check for tick_nohz_tick_stopped() outside of irq_save()
> >> context.
> >
> > No, the wrapper is there on purpose in order to optimize the full dynticks off case in
> > the context switch path with the jump label'ed check on tick_nohz_full_enabled().
> 
> Just to clarify, you are saying that:
> 
> Wrapper was there to save an extra function call when tick_nohz_full_enabled()
> returns false, as tick_nohz_task_switch() will be inlined ?

Yeah. But not just that.

Using an inline saves a function call and reduce the offline case to a simple
condition check. But there is also the jump label that reduce the condition check
to an unconditional jump in the off case.

To summarize, here's how calling tick_nohz_task_switch() maps to final C code:

finish_task_switch()
{
       //do things before calling tick_nohz_task_switch()...
       // call tick_nohz_task_switch
       goto offcase;
       if (tick_nohz_full_enabled())
           __tick_nohz_task_switch(tsk);
offcase:
      //end of call to tick_nohz_task_switch
      //do things before calling tick_nohz_task_switch()...
}

In the offcase, the code is like above. We don't even do the check, thanks to
the jump label code we unconditionally jump to what's next in finish_task_switch()
(there is actually nothing afterward but that's for the picture).

Now if there is at least a CPU that is full dynticks on boot, it is enabled
with context_tracking_cpu_set(). Then the jump label code patches the code in
finish_task_switch() to turn the goto offcase into a nop. Then the condition is
actually verified on every call to finish_task_switch().

So it goes beyond than just saving a function call.

> 
> In this case probably we can move !can_stop_full_tick() as well to the wrapper ?

Do you mean moving all the code of __tick_nohz_task_switch() to tick_nohz_task_switch()?
I much prefer we don't do that. This is going to make can_stop_full_tick() a publicly
visible nohz internal. And it may uglify tick.h as well.

Re: [PATCH 29/38] tick-sched: remove wrapper around __tick_nohz_task_switch()

[Viresh Kumar]

On 15 April 2014 14:43, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> Yeah. But not just that.
>
> Using an inline saves a function call and reduce the offline case to a simple
> condition check. But there is also the jump label that reduce the condition check
> to an unconditional jump in the off case.
>
> To summarize, here's how calling tick_nohz_task_switch() maps to final C code:
>
> finish_task_switch()
> {
>        //do things before calling tick_nohz_task_switch()...
>        // call tick_nohz_task_switch
>        goto offcase;
>        if (tick_nohz_full_enabled())
>            __tick_nohz_task_switch(tsk);
> offcase:
>       //end of call to tick_nohz_task_switch
>       //do things before calling tick_nohz_task_switch()...
> }
>
> In the offcase, the code is like above. We don't even do the check, thanks to
> the jump label code we unconditionally jump to what's next in finish_task_switch()
> (there is actually nothing afterward but that's for the picture).
>
> Now if there is at least a CPU that is full dynticks on boot, it is enabled
> with context_tracking_cpu_set(). Then the jump label code patches the code in
> finish_task_switch() to turn the goto offcase into a nop. Then the condition is
> actually verified on every call to finish_task_switch().
>
> So it goes beyond than just saving a function call.

Sorry, but my poor mind still couldn't understand what you are trying to
tell me :(

So lets clarify things one by one :)

- What do you mean by offcase? CONFIG_NO_HZ_FULL not configured
into the kernel or it is configured but none of the CPUs is running in that
mode?

- Also what does it correspond to in code: goto offcase; ? There is no labels
or goto statements in code that I can see.. This is how the code looks to me.

> finish_task_switch()
> {
>        //do things before calling tick_nohz_task_switch()...
>        // call tick_nohz_task_switch
>        if (tick_nohz_full_enabled())
>            __tick_nohz_task_switch(tsk);
> }

__tick_nohz_task_switch() may or maynot be available at all depending
on CONFIG_NO_HZ_FULL is enabled into the kernel or not. But that
was the case with tick_nohz_task_switch() as well in my patch. So
shouldn't make a difference..

Again, sorry for not understanding what you are trying to explain here.
I want to understand this once and for all and probably add a comment
here as well :)

--
viresh

Re: [PATCH 29/38] tick-sched: remove wrapper around __tick_nohz_task_switch()

[Frederic Weisbecker]

On Tue, Apr 15, 2014 at 03:23:37PM +0530, Viresh Kumar wrote:
> On 15 April 2014 14:43, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > Yeah. But not just that.
> >
> > Using an inline saves a function call and reduce the offline case to a simple
> > condition check. But there is also the jump label that reduce the condition check
> > to an unconditional jump in the off case.
> >
> > To summarize, here's how calling tick_nohz_task_switch() maps to final C code:
> >
> > finish_task_switch()
> > {
> >        //do things before calling tick_nohz_task_switch()...
> >        // call tick_nohz_task_switch
> >        goto offcase;
> >        if (tick_nohz_full_enabled())
> >            __tick_nohz_task_switch(tsk);
> > offcase:
> >       //end of call to tick_nohz_task_switch
> >       //do things before calling tick_nohz_task_switch()...
> > }
> >
> > In the offcase, the code is like above. We don't even do the check, thanks to
> > the jump label code we unconditionally jump to what's next in finish_task_switch()
> > (there is actually nothing afterward but that's for the picture).
> >
> > Now if there is at least a CPU that is full dynticks on boot, it is enabled
> > with context_tracking_cpu_set(). Then the jump label code patches the code in
> > finish_task_switch() to turn the goto offcase into a nop. Then the condition is
> > actually verified on every call to finish_task_switch().
> >
> > So it goes beyond than just saving a function call.
> 
> Sorry, but my poor mind still couldn't understand what you are trying to
> tell me :(

Welcome to the club of the daily confused people.
I'm happy to hear I'm not alone :)

> 
> So lets clarify things one by one :)
> 
> - What do you mean by offcase? CONFIG_NO_HZ_FULL not configured
> into the kernel or it is configured but none of the CPUs is running in that
> mode?

So by offcase I mean CONFIG_NO_HZ_FULL=y but the nohz_full boot parameter
is empty, or simply not passed at all. And of course CONFIG_NO_HZ_FULL_ALL=n

This config is now likely on some distros because we want to make full
dynticks available for users who want it. But if it's not used (which is 99.999%
of the usecases), we want to minimize as much as possible its overhead.

Lets call that dynamic off-case.

> 
> - Also what does it correspond to in code: goto offcase; ? There is no labels
> or goto statements in code that I can see.. This is how the code looks to me.
> 
> > finish_task_switch()
> > {
> >        //do things before calling tick_nohz_task_switch()...
> >        // call tick_nohz_task_switch
> >        if (tick_nohz_full_enabled())
> >            __tick_nohz_task_switch(tsk);
> > }

Sure but check out the static_key_false() in the implementation of tick_nohz_full_enabled().
That's where the magic hides.

> 
> __tick_nohz_task_switch() may or maynot be available at all depending
> on CONFIG_NO_HZ_FULL is enabled into the kernel or not. But that
> was the case with tick_nohz_task_switch() as well in my patch. So
> shouldn't make a difference..
> 
> Again, sorry for not understanding what you are trying to explain here.
> I want to understand this once and for all and probably add a comment
> here as well :)

No problem, the jump label/static key code is quite tricky. And its use
can be easily missed, as in here.

Also its unfamous API naming (static_key_true/static_key_true) that is
anything but intuitive.

> 
> --
> viresh

Re: [PATCH 29/38] tick-sched: remove wrapper around __tick_nohz_task_switch()

[Viresh Kumar]

On 15 April 2014 18:14, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> Sure but check out the static_key_false() in the implementation of tick_nohz_full_enabled().
> That's where the magic hides.

> No problem, the jump label/static key code is quite tricky. And its use
> can be easily missed, as in here.
>
> Also its unfamous API naming (static_key_true/static_key_true) that is
> anything but intuitive.

That was tricky enough to be missed on first look :)

Okay here is the answer to what you asked earlier:

>> In this case probably we can move !can_stop_full_tick() as well to the wrapper ?
>
> Do you mean moving all the code of __tick_nohz_task_switch() to tick_nohz_task_switch()?
> I much prefer we don't do that. This is going to make can_stop_full_tick() a publicly
> visible nohz internal. And it may uglify tick.h as well.

I probably asked the wrong question, I meant tick_nohz_tick_stopped()
instead of !can_stop_full_tick().

Or, can we make the code more efficient by avoiding a branch by doing this:

diff --git a/include/linux/tick.h b/include/linux/tick.h
index 1065a51..12632cc 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -220,7 +220,7 @@ static inline void tick_nohz_full_check(void)

 static inline void tick_nohz_task_switch(void)
 {
-       if (tick_nohz_full_enabled())
+       if (tick_nohz_full_enabled() && tick_nohz_tick_stopped())
                __tick_nohz_task_switch();
 }

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 45037c4..904e09b 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -273,7 +273,7 @@ void __tick_nohz_task_switch(void)

        local_irq_save(flags);

-       if (tick_nohz_tick_stopped() && !can_stop_full_tick())
+       if (!can_stop_full_tick())
                tick_nohz_full_kick();

        local_irq_restore(flags);

[PATCH 30/38] tick-sched: move nohz_full_buf[] inside tick_nohz_init()

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

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-sched.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index d8b9a69..22b9505 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -321,13 +321,6 @@ static int tick_nohz_cpu_down_callback(struct notifier_block *nfb,
 	return NOTIFY_OK;
 }
 
-/*
- * Worst case string length in chunks of CPU range seems 2 steps
- * separations: 0,2,4,6,...
- * This is NR_CPUS + sizeof('\0')
- */
-static char __initdata nohz_full_buf[NR_CPUS + 1];
-
 static int tick_nohz_init_all(void)
 {
 	int err = -1;
@@ -347,6 +340,12 @@ static int tick_nohz_init_all(void)
 
 void __init tick_nohz_init(void)
 {
+	/*
+	 * Worst case string length in chunks of CPU range seems 2 steps
+	 * separations: 0,2,4,6,...
+	 * This is NR_CPUS + sizeof('\0')
+	 */
+	char nohz_full_buf[NR_CPUS + 1];
 	int cpu;
 
 	if (!tick_nohz_full_running) {
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 30/38] tick-sched: move nohz_full_buf[] inside tick_nohz_init()

[Frederic Weisbecker]

On Mon, Apr 14, 2014 at 09:53:52PM +0530, Viresh Kumar wrote:
> nohz_full_buf[] is used at only one place, i.e. inside tick_nohz_init(). Make it
> a local variable. Can move it out in case it is used in some other routines in
> future.

OTOH nohz_full_buf can have a big size and moving it to a local variable
will result in big increase in kernel stack consumption.

Keeping it static is safer.

Also it's already __initdata so it's released after bootup. No issue of
concurrent use either, so keeping it as is is fine.

> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  kernel/time/tick-sched.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index d8b9a69..22b9505 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -321,13 +321,6 @@ static int tick_nohz_cpu_down_callback(struct notifier_block *nfb,
>  	return NOTIFY_OK;
>  }
>  
> -/*
> - * Worst case string length in chunks of CPU range seems 2 steps
> - * separations: 0,2,4,6,...
> - * This is NR_CPUS + sizeof('\0')
> - */
> -static char __initdata nohz_full_buf[NR_CPUS + 1];
> -
>  static int tick_nohz_init_all(void)
>  {
>  	int err = -1;
> @@ -347,6 +340,12 @@ static int tick_nohz_init_all(void)
>  
>  void __init tick_nohz_init(void)
>  {
> +	/*
> +	 * Worst case string length in chunks of CPU range seems 2 steps
> +	 * separations: 0,2,4,6,...
> +	 * This is NR_CPUS + sizeof('\0')
> +	 */
> +	char nohz_full_buf[NR_CPUS + 1];
>  	int cpu;
>  
>  	if (!tick_nohz_full_running) {
> -- 
> 1.7.12.rc2.18.g61b472e
> 

Re: [PATCH 30/38] tick-sched: move nohz_full_buf[] inside tick_nohz_init()

[Viresh Kumar]

On 15 April 2014 04:58, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> OTOH nohz_full_buf can have a big size and moving it to a local variable
> will result in big increase in kernel stack consumption.
>
> Keeping it static is safer.
>
> Also it's already __initdata so it's released after bootup. No issue of
> concurrent use either, so keeping it as is is fine.

Okay, this patch is dropped.

[PATCH 31/38] tick-sched: initialize 'ts' during its definition __tick_nohz_idle_enter()

[Viresh Kumar]

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

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 22b9505..f1bc258 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -792,7 +792,7 @@ static void __tick_nohz_idle_enter(struct tick_sched *ts)
  */
 void tick_nohz_idle_enter(void)
 {
-	struct tick_sched *ts;
+	struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched);
 
 	WARN_ON_ONCE(irqs_disabled());
 
@@ -806,7 +806,6 @@ void tick_nohz_idle_enter(void)
 
 	local_irq_disable();
 
-	ts = &__get_cpu_var(tick_cpu_sched);
 	ts->inidle = 1;
 	__tick_nohz_idle_enter(ts);
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH 32/38] tick-sched: add comment about 'idle_active' in tick_nohz_idle_exit()

[Viresh Kumar]

The sequence of calls for dynticks CPUs was a bit confusing and so adding a
comment in tick_nohz_idle_exit() routine to mention it clearly. All information
required is in commit and this conversation with Frederic.

https://lkml.org/lkml/2014/4/10/355

Suggested-by: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-sched.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index f1bc258..85a4e90 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -925,6 +925,22 @@ void tick_nohz_idle_exit(void)
 
 	ts->inidle = 0;
 
+	/*
+	 * Can idle_active be false here?
+	 * Ideally this would be the sequence of calls:
+	 * - tick_nohz_idle_enter(), i.e. idle_active = true;
+	 * - local_irq_disable()
+	 * - IDLE
+	 * - wake up due to IPI or other interrupt
+	 * - local_irq_enable()
+	 * - tick_nohz_irq_enter(), i.e. idle_active = false;
+	 * - tick_nohz_irq_exit(), i.e. idle_active = true; This is not called
+	 *   in case of IPI's as need_resched() will prevent that in
+	 *   tick_irq_exit(), as we don't need to account any more for idle time
+	 *   or try to enter dyntics mode (We are going to exit idle state).
+	 *
+	 * - tick_nohz_idle_exit()
+	 */
 	if (ts->idle_active || ts->tick_stopped)
 		now = ktime_get();
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH 34/38] tick-sched: remove local variable 'now' from tick_setup_sched_timer()

[Viresh Kumar]

Local variable 'now' is used at only one place and so can be easily replaced by
ktime_get() instead.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-sched.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index a0a6705..78751b3 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -1116,7 +1116,6 @@ early_param("skew_tick", skew_tick);
 void tick_setup_sched_timer(void)
 {
 	struct tick_sched *ts = &__get_cpu_var(tick_cpu_sched);
-	ktime_t now = ktime_get();
 
 	/*
 	 * Emulate tick processing via per-CPU hrtimers:
@@ -1136,13 +1135,12 @@ void tick_setup_sched_timer(void)
 	}
 
 	for (;;) {
-		hrtimer_forward(&ts->sched_timer, now, tick_period);
+		hrtimer_forward(&ts->sched_timer, ktime_get(), tick_period);
 		hrtimer_start_expires(&ts->sched_timer,
 				      HRTIMER_MODE_ABS_PINNED);
 		/* Check, if the timer was already in the past */
 		if (hrtimer_active(&ts->sched_timer))
 			break;
-		now = ktime_get();
 	}
 
 #ifdef CONFIG_NO_HZ_COMMON
-- 
1.7.12.rc2.18.g61b472e

[PATCH 35/38] tick-broadcast: do checks before taking locks in tick_do_broadcast_on_off()

[Viresh Kumar]

Some of the checks which may force tick_do_broadcast_on_off() to return early
can be done before taking locks. This would make these codepaths faster.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-broadcast.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 1597d03..3b07569 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -329,27 +329,22 @@ unlock:
  */
 static void tick_do_broadcast_on_off(unsigned long *reason)
 {
-	struct clock_event_device *bc, *dev;
-	struct tick_device *td;
+	struct tick_device *td = tick_get_cpu_device();
+	struct clock_event_device *bc, *dev = td->evtdev;
+	int cpu = smp_processor_id(), bc_stopped;
 	unsigned long flags;
-	int cpu, bc_stopped;
-
-	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
-
-	cpu = smp_processor_id();
-	td = tick_get_device(cpu);
-	dev = td->evtdev;
-	bc = tick_broadcast_device.evtdev;
 
 	/*
 	 * Is the device not affected by the powerstate ?
 	 */
 	if (!dev || !(dev->features & CLOCK_EVT_FEAT_C3STOP))
-		goto out;
+		return;
 
 	if (!tick_device_is_functional(dev))
-		goto out;
+		return;
 
+	raw_spin_lock_irqsave(&tick_broadcast_lock, flags);
+	bc = tick_broadcast_device.evtdev;
 	bc_stopped = cpumask_empty(tick_broadcast_mask);
 
 	switch (*reason) {
@@ -385,7 +380,6 @@ static void tick_do_broadcast_on_off(unsigned long *reason)
 		else
 			tick_broadcast_setup_oneshot(bc);
 	}
-out:
 	raw_spin_unlock_irqrestore(&tick_broadcast_lock, flags);
 }
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH 36/38] tick-broadcast: get rid of extra comparison in tick_do_broadcast_on_off()

[Viresh Kumar]

We are doing an extra comparison in tick_do_broadcast_on_off():

               if (*reason == CLOCK_EVT_NOTIFY_BROADCAST_FORCE)
                       tick_broadcast_force = 1;

Whereas it can be handled easily in the switch block only.

It doesn't look like there is any strict ordering of instructions required here
and so move setting 'tick_broadcast_force' at the beginning.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-broadcast.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 3b07569..ec86227 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -348,15 +348,14 @@ static void tick_do_broadcast_on_off(unsigned long *reason)
 	bc_stopped = cpumask_empty(tick_broadcast_mask);
 
 	switch (*reason) {
-	case CLOCK_EVT_NOTIFY_BROADCAST_ON:
 	case CLOCK_EVT_NOTIFY_BROADCAST_FORCE:
+		tick_broadcast_force = 1;
+	case CLOCK_EVT_NOTIFY_BROADCAST_ON:
 		cpumask_set_cpu(cpu, tick_broadcast_on);
 		if (!cpumask_test_and_set_cpu(cpu, tick_broadcast_mask)) {
 			if (tick_broadcast_device.mode == TICKDEV_MODE_PERIODIC)
 				clockevents_shutdown(dev);
 		}
-		if (*reason == CLOCK_EVT_NOTIFY_BROADCAST_FORCE)
-			tick_broadcast_force = 1;
 		break;
 	case CLOCK_EVT_NOTIFY_BROADCAST_OFF:
 		if (tick_broadcast_force)
-- 
1.7.12.rc2.18.g61b472e

[PATCH 37/38] tick-broadcast: merge tick_do_broadcast_on_off() into tick_broadcast_on_off()

[Viresh Kumar]

We have added a wrapper around tick_do_broadcast_on_off() which doesn't have
anything so special that we should keep it. It just had a check if cpu is online
or not. Just merge these two routines and keep one: tick_broadcast_on_off().
This would get rid of an extra function call that we are doing currently.

This also replaces printk(KERN_ERR "**"); with pr_err("**");

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/time/tick-broadcast.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index ec86227..9d81a81 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -327,13 +327,19 @@ unlock:
  * Powerstate information: The system enters/leaves a state, where
  * affected devices might stop
  */
-static void tick_do_broadcast_on_off(unsigned long *reason)
+void tick_broadcast_on_off(unsigned long reason, int *oncpu)
 {
 	struct tick_device *td = tick_get_cpu_device();
 	struct clock_event_device *bc, *dev = td->evtdev;
 	int cpu = smp_processor_id(), bc_stopped;
 	unsigned long flags;
 
+	if (!cpumask_test_cpu(*oncpu, cpu_online_mask)) {
+		pr_err("tick-broadcast: ignoring broadcast for offline CPU #%d\n",
+		       *oncpu);
+		return;
+	}
+
 	/*
 	 * Is the device not affected by the powerstate ?
 	 */
@@ -347,7 +353,7 @@ static void tick_do_broadcast_on_off(unsigned long *reason)
 	bc = tick_broadcast_device.evtdev;
 	bc_stopped = cpumask_empty(tick_broadcast_mask);
 
-	switch (*reason) {
+	switch (reason) {
 	case CLOCK_EVT_NOTIFY_BROADCAST_FORCE:
 		tick_broadcast_force = 1;
 	case CLOCK_EVT_NOTIFY_BROADCAST_ON:
@@ -383,19 +389,6 @@ static void tick_do_broadcast_on_off(unsigned long *reason)
 }
 
 /*
- * Powerstate information: The system enters/leaves a state, where
- * affected devices might stop.
- */
-void tick_broadcast_on_off(unsigned long reason, int *oncpu)
-{
-	if (!cpumask_test_cpu(*oncpu, cpu_online_mask))
-		printk(KERN_ERR "tick-broadcast: ignoring broadcast for offline CPU #%d\n",
-		       *oncpu);
-	else
-		tick_do_broadcast_on_off(&reason);
-}
-
-/*
  * Set the periodic handler depending on broadcast on/off
  */
 void tick_set_periodic_handler(struct clock_event_device *dev, int broadcast)
-- 
1.7.12.rc2.18.g61b472e

[PATCH 38/38] clockevents: set event_handler to clockevents_handle_noop() in clockevents_exchange_device()

[Viresh Kumar]

All users of clockevents_handle_noop() are setting old->event_handler to it
after calling clockevents_exchange_device() and so it would be better if we can
do this at a single place. The only thing we need to make sure is, we preserve
it before calling clockevents_exchange_device() in case we need it.

As there are no external users of it, make it static.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 include/linux/clockchips.h   |  2 --
 kernel/time/clockevents.c    |  3 ++-
 kernel/time/tick-broadcast.c |  2 --
 kernel/time/tick-common.c    | 11 +++++------
 4 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 2e4cb67..7e492ad 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -165,8 +165,6 @@ extern void clockevents_set_mode(struct clock_event_device *dev,
 extern int clockevents_program_event(struct clock_event_device *dev,
 				     ktime_t expires, bool force);
 
-extern void clockevents_handle_noop(struct clock_event_device *dev);
-
 static inline void
 clockevents_calc_mult_shift(struct clock_event_device *ce, u32 freq, u32 minsec)
 {
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index d80e891..757dae8 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -480,7 +480,7 @@ int clockevents_update_freq(struct clock_event_device *dev, u32 freq)
 /*
  * Noop handler when we shut down an event device
  */
-void clockevents_handle_noop(struct clock_event_device *dev)
+static void clockevents_handle_noop(struct clock_event_device *dev)
 {
 }
 
@@ -506,6 +506,7 @@ void clockevents_exchange_device(struct clock_event_device *old,
 		clockevents_set_mode(old, CLOCK_EVT_MODE_UNUSED);
 		list_del(&old->list);
 		list_add(&old->list, &clockevents_released);
+		old->event_handler = clockevents_handle_noop;
 	}
 
 	if (new) {
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 9d81a81..bb96da7 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -95,8 +95,6 @@ void tick_install_broadcast_device(struct clock_event_device *dev)
 		return;
 
 	clockevents_exchange_device(cur, dev);
-	if (cur)
-		cur->event_handler = clockevents_handle_noop;
 	tick_broadcast_device.evtdev = dev;
 	if (!cpumask_empty(tick_broadcast_mask))
 		tick_broadcast_start_periodic(dev);
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index e1b2947..b0d14e3 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -145,11 +145,11 @@ void tick_setup_periodic(struct clock_event_device *dev, int broadcast)
  * Setup the tick device
  */
 static void tick_setup_device(struct tick_device *td,
-			      struct clock_event_device *newdev, int cpu)
+			      struct clock_event_device *newdev, int cpu,
+			      void (*handler)(struct clock_event_device *))
 {
 	const struct cpumask *cpumask = cpumask_of(cpu);
 	ktime_t next_event;
-	void (*handler)(struct clock_event_device *) = NULL;
 
 	/*
 	 * First device setup ?
@@ -173,9 +173,7 @@ static void tick_setup_device(struct tick_device *td,
 		 */
 		td->mode = TICKDEV_MODE_PERIODIC;
 	} else {
-		handler = td->evtdev->event_handler;
 		next_event = td->evtdev->next_event;
-		td->evtdev->event_handler = clockevents_handle_noop;
 	}
 
 	td->evtdev = newdev;
@@ -206,10 +204,12 @@ static void tick_setup_device(struct tick_device *td,
 void tick_install_replacement(struct clock_event_device *newdev)
 {
 	struct tick_device *td = tick_get_cpu_device();
+	void (*handler)(struct clock_event_device *) =
+		td->evtdev->event_handler;
 	int cpu = smp_processor_id();
 
 	clockevents_exchange_device(td->evtdev, newdev);
-	tick_setup_device(td, newdev, cpu);
+	tick_setup_device(td, newdev, cpu, handler);
 	if (newdev->features & CLOCK_EVT_FEAT_ONESHOT)
 		tick_oneshot_notify();
 }
@@ -335,7 +335,6 @@ void tick_shutdown(unsigned int *cpup)
 		 * the set mode function!
 		 */
 		clockevents_exchange_device(dev, NULL);
-		dev->event_handler = clockevents_handle_noop;
 		td->evtdev = NULL;
 	}
 }
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 00/38] tick cleanups and bugfixes

[Thomas Gleixner]

On Mon, 14 Apr 2014, Viresh Kumar wrote:
> Hi Thomas,
> 
> These are separate cleanups from the timers/hrtimers ones I did. I was waiting
> for the merge window to close in order to send these and by the time it
> happened, I got a long pending list.
> 
> These are mostly cleanups, reorders for better readability or efficiency, and
> few bugfixes.

And that's wrong to begin with.

Bugfixes first and then all other stuff. We dont want dependencies of
bugfixes on cleanups, reordering of code ...

I'm not applying a wholesale checkpatch.pl patch which creates noise
for no value.

I don't mind if you cleanup stuff while doing other changes, but
definitely not as a stand alone starter of a large patch queue with
bugfixes which depend on that.

Now looking at the thing some more, it contains gems like this:

-               printk(KERN_ERR "tick-broadcast: ignoring broadcast for "
-                      "offline CPU #%d\n", *oncpu);
+               printk(KERN_ERR "tick-broadcast: ignoring broadcast for offline CPU #%d\n",
+                      *oncpu);

If you fix that checkpatch.pl line wrap issue, why do you not change
printk(KERN_ERR) to pr_err() as well?

checkpatch.pl is happy, right?

I'm tired of this, really.

Please send me the next series in the following way:

- send a bug fix series, which does nothing else than fixing bugs.

  when that is applied, then

- send a small batch of improvements for a particular issue and not a
  mixed bag of random patches.

Thanks,

	tglx

Re: [PATCH 00/38] tick cleanups and bugfixes

[Viresh Kumar]

On 15 April 2014 05:08, Thomas Gleixner <tglx@linutronix.de> wrote:
> And that's wrong to begin with.
>
> Bugfixes first and then all other stuff. We dont want dependencies of
> bugfixes on cleanups, reordering of code ...

Correct, should have taken care of this initially :(

> Now looking at the thing some more, it contains gems like this:
>
> -               printk(KERN_ERR "tick-broadcast: ignoring broadcast for "
> -                      "offline CPU #%d\n", *oncpu);
> +               printk(KERN_ERR "tick-broadcast: ignoring broadcast for offline CPU #%d\n",
> +                      *oncpu);
>
> If you fix that checkpatch.pl line wrap issue, why do you not change
> printk(KERN_ERR) to pr_err() as well?
>
> checkpatch.pl is happy, right?

I did this for one patch and missed it for other.. I will fix all printk's in
kernel/time/ now :)

> Please send me the next series in the following way:
>
> - send a bug fix series, which does nothing else than fixing bugs.

Right.

>   when that is applied, then
>
> - send a small batch of improvements for a particular issue and not a
>   mixed bag of random patches.

Okay.. Thanks.