[PATCH 0/5] softlockup, sched: convert softlockup to real-time timestamping

[PATCH 0/5] softlockup, sched: convert softlockup to real-time timestamping

[Don Zickus]

Currently, the touch_softlockup_watchdog code just delays the setting of the
timestamp until the timer thread is run.  This causes the softlockup timer
to be unarmed for a brief period until the timer runs and re-arms the
watchdog.

It seems some workloads are affected by this, namely Anton's test code
that disables interrupts and idles the cpu on ppc.  By switching to a
model where we timestamp as soon as we are called in
touch_softlockup_watchdog(), we can trigger a little sooner and in
more cases.

The flip side, is there is some extra overhead to calculating the 
time at the touch.  Normally this shouldn't be an issue, because most
cases, any code touching the watchdog already believes it is in a
slow path.  This is not true for the scheduler.

The scheduler at any time can take large jumps of time (ie suspend/resume,
kgdb, virt guest pause/restart) and therefore needs to touch the watchdog
to prevent from falsely firing at almost every tick.

I noticed that the touch_softlockup_watchdog was being called very
frequently in the scheduler.  So the first two patches are an attempt
to minimize them from being called a million times/sec to zero under
normal circumstances.

The last patch removes a long standing hack that should be satisified
with the new changes (untested though).

I tested these patches through suspend/resume, a kvm guest and using
lkdtm (softlockup).  Everything seems to work as it did before.

Anton Blanchard (2):
  watchdog: Remove touch_all_softlockup_watchdogs
  watchdog: Softlockup has regular windows where it is not armed

Don Zickus (3):
  sched: Minimize the number of touch_softlockup_watchdogs
  time: Minimize number of touch_softlockup_watchdog
  kgdb, softlockup:  Remove touch_softlockup_watchdog_sync

 include/linux/sched.h     |    8 --------
 kernel/debug/debug_core.c |    2 +-
 kernel/sched/clock.c      |    3 ++-
 kernel/sched/core.c       |    2 +-
 kernel/time/tick-sched.c  |    7 ++-----
 kernel/watchdog.c         |   37 ++-----------------------------------
 6 files changed, 8 insertions(+), 51 deletions(-)

-- 
1.7.7.4

[PATCH 1/5] sched: Minimize the number of touch_softlockup_watchdogs

[Don Zickus]

I am looking to add a change to the softlockup code that
creates a timestamp every time it is touched.  However, there is
some overhead to touching cpu_clock all the time.

I noticed sched_clock_idle_wakeup_event runs
touch_softlockup_watchdog every microsecond or about a
million times a second.  That seemed excessive.

The only use I can see for touching the softlockup watchdog
is to reset the clock on long delays.  Considering most of the
time the delay is 0, it seems pointless to touch the watchdog.

This saves about a million calls a second and avoids any new
overhead by changes in the softlockup watchdog.

Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 kernel/sched/clock.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
index c685e31..802e03b 100644
--- a/kernel/sched/clock.c
+++ b/kernel/sched/clock.c
@@ -274,7 +274,8 @@ void sched_clock_idle_wakeup_event(u64 delta_ns)
 		return;
 
 	sched_clock_tick();
-	touch_softlockup_watchdog();
+	if (delta_ns > 0)
+		touch_softlockup_watchdog();
 }
 EXPORT_SYMBOL_GPL(sched_clock_idle_wakeup_event);
 
-- 
1.7.7.4

[PATCH 2/5] time: Minimize number of touch_softlockup_watchdog

[Don Zickus]

Another patch where I was worried about overhead on a change
in the softlockup code.  Currently on my x86 box it seems the
tick code was touch the softlockup every millisecond.

To minimize, I moved the touch_softlockup_watchdog to a
common function, tick_do_update_jiffies64.  Then the code
only ever touches the softlockup when there was a greater
than 1 second jump in the tick.

Considering the softlockup fires ever 20 seconds or so, this
should be safe for now.

This saves about a 1 thousand calls/per second.

Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 kernel/time/tick-sched.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 31cc061..00578c0 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -73,6 +73,8 @@ static void tick_do_update_jiffies64(ktime_t now)
 
 			last_jiffies_update = ktime_add_ns(last_jiffies_update,
 							   incr * ticks);
+			if (incr > NSEC_PER_SEC)
+				touch_softlockup_watchdog();
 		}
 		do_timer(++ticks);
 
@@ -144,8 +146,6 @@ static void tick_nohz_update_jiffies(ktime_t now)
 	local_irq_save(flags);
 	tick_do_update_jiffies64(now);
 	local_irq_restore(flags);
-
-	touch_softlockup_watchdog();
 }
 
 /*
@@ -572,7 +572,6 @@ void tick_nohz_restart_sched_tick(void)
 		account_idle_ticks(ticks);
 #endif
 
-	touch_softlockup_watchdog();
 	/*
 	 * Cancel the scheduled timer and restore the tick
 	 */
@@ -625,7 +624,6 @@ static void tick_nohz_handler(struct clock_event_device *dev)
 	 * do when we go busy again does not account too much ticks.
 	 */
 	if (ts->tick_stopped) {
-		touch_softlockup_watchdog();
 		ts->idle_jiffies++;
 	}
 
@@ -783,7 +781,6 @@ static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer)
 		 * when we go busy again does not account too much ticks.
 		 */
 		if (ts->tick_stopped) {
-			touch_softlockup_watchdog();
 			ts->idle_jiffies++;
 		}
 		update_process_times(user_mode(regs));
-- 
1.7.7.4

[PATCH 3/5] watchdog: Remove touch_all_softlockup_watchdogs

[Don Zickus]

From: Anton Blanchard <anton@samba.org>

commit 04c9167f91e3 (add touch_all_softlockup_watchdogs()) added a
way to touch the watchdog on all cpus because show_state_filter
could hold the tasklist_lock for extended periods of time.

commit 510f5acc4f4f (sched: Don't use tasklist_lock for debug prints)
has since removed the tasklist_lock in show_state_filter so we can
use touch_softlockup_watchdog instead.

Signed-off-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 include/linux/sched.h |    4 ----
 kernel/sched/core.c   |    2 +-
 kernel/watchdog.c     |   13 -------------
 3 files changed, 1 insertions(+), 18 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index cc8c620..b1e4364 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -312,7 +312,6 @@ extern void sched_show_task(struct task_struct *p);
 #ifdef CONFIG_LOCKUP_DETECTOR
 extern void touch_softlockup_watchdog(void);
 extern void touch_softlockup_watchdog_sync(void);
-extern void touch_all_softlockup_watchdogs(void);
 extern int proc_dowatchdog_thresh(struct ctl_table *table, int write,
 				  void __user *buffer,
 				  size_t *lenp, loff_t *ppos);
@@ -325,9 +324,6 @@ static inline void touch_softlockup_watchdog(void)
 static inline void touch_softlockup_watchdog_sync(void)
 {
 }
-static inline void touch_all_softlockup_watchdogs(void)
-{
-}
 static inline void lockup_detector_init(void)
 {
 }
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 3c5b21e..4b01286 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4812,7 +4812,7 @@ void show_state_filter(unsigned long state_filter)
 			sched_show_task(p);
 	} while_each_thread(g, p);
 
-	touch_all_softlockup_watchdogs();
+	touch_softlockup_watchdog();
 
 #ifdef CONFIG_SCHED_DEBUG
 	sysrq_sched_debug_show();
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 1d7bca7..d63975f 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -138,19 +138,6 @@ void touch_softlockup_watchdog(void)
 }
 EXPORT_SYMBOL(touch_softlockup_watchdog);
 
-void touch_all_softlockup_watchdogs(void)
-{
-	int cpu;
-
-	/*
-	 * this is done lockless
-	 * do we care if a 0 races with a timestamp?
-	 * all it means is the softlock check starts one cycle later
-	 */
-	for_each_online_cpu(cpu)
-		per_cpu(watchdog_touch_ts, cpu) = 0;
-}
-
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
 void touch_nmi_watchdog(void)
 {
-- 
1.7.7.4

[PATCH 4/5] watchdog: Softlockup has regular windows where it is not armed

[Don Zickus]

From: Anton Blanchard <anton@samba.org>

The softlockup watchdog has a two stage sync - touch_softlockup_watchdog
simply sets the timestamp to 0 and later on the timer routine notices
this and sets the timestamp.

The problem is this timer goes off every 4 seconds by default, so
each time we call touch_softlockup_watchdog there is a period
of up to 4 seconds where the softlockup watchdog is not armed.

We call touch_softlockup_watchdog very often in the NO_HZ code and
end up hitting this issue every time we go in and out of idle.

I wrote a simple test case:

http://ozlabs.org/~anton/junkcode/badguy.tar.gz

That disables interrupts on selected CPUs for a period of time. Don't
run it on a machine you care about. When I disable interrupts for 30
seconds on a previously idle CPU I get no warning:

insmod ./badguy.ko timeout=30 cpus=4

However if I keep the CPU busy so we don't switch in and out of NO_HZ
mode I get a warning as expected:

taskset -c 4 yes > /dev/null &
insmod ./badguy.ko timeout=30 cpus=4

With the following patch I get a warning even on a previously idle
CPU.

Signed-off-by: Anton Blanchard <anton@samba.org>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 kernel/watchdog.c |   22 ++++------------------
 1 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index d63975f..4d242f5 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -33,7 +33,6 @@ int __read_mostly watchdog_thresh = 10;
 static DEFINE_PER_CPU(unsigned long, watchdog_touch_ts);
 static DEFINE_PER_CPU(struct task_struct *, softlockup_watchdog);
 static DEFINE_PER_CPU(struct hrtimer, watchdog_hrtimer);
-static DEFINE_PER_CPU(bool, softlockup_touch_sync);
 static DEFINE_PER_CPU(bool, soft_watchdog_warn);
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
 static DEFINE_PER_CPU(bool, hard_watchdog_warn);
@@ -134,7 +133,7 @@ static void __touch_watchdog(void)
 
 void touch_softlockup_watchdog(void)
 {
-	__this_cpu_write(watchdog_touch_ts, 0);
+	__touch_watchdog();
 }
 EXPORT_SYMBOL(touch_softlockup_watchdog);
 
@@ -157,8 +156,8 @@ EXPORT_SYMBOL(touch_nmi_watchdog);
 
 void touch_softlockup_watchdog_sync(void)
 {
-	__raw_get_cpu_var(softlockup_touch_sync) = true;
-	__raw_get_cpu_var(watchdog_touch_ts) = 0;
+	sched_clock_tick();
+	__touch_watchdog();
 }
 
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
@@ -258,19 +257,6 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
 	/* .. and repeat */
 	hrtimer_forward_now(hrtimer, ns_to_ktime(get_sample_period()));
 
-	if (touch_ts == 0) {
-		if (unlikely(__this_cpu_read(softlockup_touch_sync))) {
-			/*
-			 * If the time stamp was touched atomically
-			 * make sure the scheduler tick is up to date.
-			 */
-			__this_cpu_write(softlockup_touch_sync, false);
-			sched_clock_tick();
-		}
-		__touch_watchdog();
-		return HRTIMER_RESTART;
-	}
-
 	/* check for a softlockup
 	 * This is done by making sure a high priority task is
 	 * being scheduled.  The task touches the watchdog to
@@ -438,7 +424,7 @@ static int watchdog_enable(int cpu)
 			goto out;
 		}
 		kthread_bind(p, cpu);
-		per_cpu(watchdog_touch_ts, cpu) = 0;
+		__touch_watchdog();
 		per_cpu(softlockup_watchdog, cpu) = p;
 		wake_up_process(p);
 	}
-- 
1.7.7.4

[PATCH 5/5] kgdb, softlockup: Remove touch_softlockup_watchdog_sync

[Don Zickus]

The previous softlockup patches remove the delayed timestamp
and instead just stamp the time at invocation of
touch_softlockup_watchdog.  As a result, I don't think
touch_softlockup_watchdog_sync is needed anymore because
the call to cpu_clock() mimics what the _sync command
was trying to do.

I haven't tested this to see if this is true, instead I did
this on code inspection.

Jason, do you think you can test this and let me know if it
works?

CC: Jason Wessel <jason.wessel@windriver.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 include/linux/sched.h     |    4 ----
 kernel/debug/debug_core.c |    2 +-
 kernel/watchdog.c         |    6 ------
 3 files changed, 1 insertions(+), 11 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index b1e4364..c896830 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -311,7 +311,6 @@ extern void sched_show_task(struct task_struct *p);
 
 #ifdef CONFIG_LOCKUP_DETECTOR
 extern void touch_softlockup_watchdog(void);
-extern void touch_softlockup_watchdog_sync(void);
 extern int proc_dowatchdog_thresh(struct ctl_table *table, int write,
 				  void __user *buffer,
 				  size_t *lenp, loff_t *ppos);
@@ -321,9 +320,6 @@ void lockup_detector_init(void);
 static inline void touch_softlockup_watchdog(void)
 {
 }
-static inline void touch_softlockup_watchdog_sync(void)
-{
-}
 static inline void lockup_detector_init(void)
 {
 }
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index 0d7c087..58f74e3 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -450,7 +450,7 @@ static int kgdb_reenter_check(struct kgdb_state *ks)
 
 static void dbg_touch_watchdogs(void)
 {
-	touch_softlockup_watchdog_sync();
+	touch_softlockup_watchdog();
 	clocksource_touch_watchdog();
 	rcu_cpu_stall_reset();
 }
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 4d242f5..a9fb54c 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -154,12 +154,6 @@ EXPORT_SYMBOL(touch_nmi_watchdog);
 
 #endif
 
-void touch_softlockup_watchdog_sync(void)
-{
-	sched_clock_tick();
-	__touch_watchdog();
-}
-
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
 /* watchdog detector functions */
 static int is_hardlockup(void)
-- 
1.7.7.4