generic kernel watchdog reset at pvclock read

In certain occasions it is possible for a hung task detector positive to be false: continuation from a paused VM, for example.

Add a method to reset detection, similar as is done with other kernel watchdogs.

Signed-off-by Marcelo Tosatti <[email protected]>

Index: kvm/kernel/hung_task.c
===================================================================

kernel/hung_task.c +11 -0

@@ -203,6 +203,14 @@ int proc_dohung_task_timeout_secs(struct
 	return ret;
 }
 
+static atomic_t reset_hung_task = ATOMIC_INIT(0);
+
+void reset_hung_task_detector(void)
+{
+	atomic_set(&reset_hung_task, 1);
+}
+EXPORT_SYMBOL_GPL(reset_hung_task_detector);
+
 /*
  * kthread which checks for tasks stuck in D state
  */

@@ -216,6 +224,9 @@ static int watchdog(void *dummy)
 		while (schedule_timeout_interruptible(timeout_jiffies(timeout)))
 			timeout = sysctl_hung_task_timeout_secs;
 
+		if (atomic_xchg(&reset_hung_task, 0))
+			continue;
+
 		check_hung_uninterruptible_tasks(timeout);
 	}
 

Index: kvm/include/linux/hung_task.h
===================================================================

include/linux/hung_task.h new file +15 -0

@@ -0,0 +1,15 @@
+/*
+ *  linux/include/linux/hung_task.h
+ */
+#ifndef LINUX_HUNG_TASK_H
+#define LINUX_HUNG_TASK_H
+
+#ifdef CONFIG_DETECT_HUNG_TASK
+void reset_hung_task_detector(void);
+#else
+static inline void reset_hung_task_detector(void)
+{
+}
+#endif
+
+#endif

Il 08/10/2013 03:05, Marcelo Tosatti ha scritto:

arch/x86/kernel/pvclock.c unsigned long pvclock_tsc_khz(struct pvc · line 54

Quoted 8 lines

arch/x86/kernel/pvclock.c +8 -0

+void pvclock_touch_watchdogs(void)
+{
+	touch_softlockup_watchdog_sync();
+	clocksource_touch_watchdog();
+	rcu_cpu_stall_reset();
+	reset_hung_task_detector();
+}
+

Should this function be in kernel/ instead? It's not really pvclock-specific.

Paolo

Quoted 13 lines

Il 08/10/2013 03:05, Marcelo Tosatti ha scritto:

arch/x86/kernel/pvclock.c unsigned long pvclock_tsc_khz(struct pvc · line 54

Quoted 8 lines

arch/x86/kernel/pvclock.c +8 -0

+void pvclock_touch_watchdogs(void)
+{
+	touch_softlockup_watchdog_sync();
+	clocksource_touch_watchdog();
+	rcu_cpu_stall_reset();
+	reset_hung_task_detector();
+}
+

Should this function be in kernel/ instead? It's not really pvclock-specific.

Quoted 1 line

Paolo

kernel/watchdog.c is configurable via CONFIG_LOCKUP_DETECTOR, so its not appropriate.

And, the choice of watchdogs to reset might be different for the caller.

Implement reset of kernel watchdogs at pvclock read time. This avoids adding special code to every watchdog.

This is possible for watchdogs which measure time based on sched_clock() or ktime_get() variants.

Suggested by Don Zickus.

Signed-off-by Marcelo Tosatti <[email protected]>

Index: kvm/arch/x86/kernel/kvmclock.c
===================================================================

arch/x86/kernel/kvmclock.c +1 -0

@@ -139,6 +139,7 @@ bool kvm_check_and_clear_guest_paused(vo
 	src = &hv_clock[cpu].pvti;
 	if ((src->flags & PVCLOCK_GUEST_STOPPED) != 0) {
 		src->flags &= ~PVCLOCK_GUEST_STOPPED;
+		pvclock_touch_watchdogs();
 		ret = true;
 	}
 

Index: kvm/arch/x86/kernel/pvclock.c
===================================================================

arch/x86/kernel/pvclock.c +14 -0

@@ -21,6 +21,7 @@
 #include <linux/sched.h>
 #include <linux/gfp.h>
 #include <linux/bootmem.h>
+#include <linux/hung_task.h>
 #include <asm/fixmap.h>
 #include <asm/pvclock.h>
 

@@ -43,6 +44,14 @@ unsigned long pvclock_tsc_khz(struct pvc
 	return pv_tsc_khz;
 }
 
+void pvclock_touch_watchdogs(void)
+{
+	touch_softlockup_watchdog_sync();
+	clocksource_touch_watchdog();
+	rcu_cpu_stall_reset();
+	reset_hung_task_detector();
+}
+
 static atomic64_t last_value = ATOMIC64_INIT(0);
 
 void pvclock_resume(void)

@@ -74,6 +83,11 @@ cycle_t pvclock_clocksource_read(struct
 		version = __pvclock_read_cycles(src, &ret, &flags);
 	} while ((src->version & 1) || version != src->version);
 
+	if (unlikely((flags & PVCLOCK_GUEST_STOPPED) != 0)) {
+		src->flags &= ~PVCLOCK_GUEST_STOPPED;
+		pvclock_touch_watchdogs();
+	}
+
 	if ((valid_flags & PVCLOCK_TSC_STABLE_BIT) &&
 		(flags & PVCLOCK_TSC_STABLE_BIT))
 		return ret;

Index: kvm/arch/x86/include/asm/pvclock.h
===================================================================

arch/x86/include/asm/pvclock.h +2 -0

@@ -14,6 +14,8 @@ void pvclock_read_wallclock(struct pvclo
 			    struct timespec *ts);
 void pvclock_resume(void);
 
+void pvclock_touch_watchdogs(void);
+
 /*
  * Scale a 64-bit delta by scaling and multiplying by a 32-bit fraction,
  * yielding a 64-bit result.

Quoted 6 lines · Marcelo Tosatti

Implement reset of kernel watchdogs at pvclock read time. This avoids adding special code to every watchdog.

This is possible for watchdogs which measure time based on sched_clock() or ktime_get() variants.

Quoted 3 lines

Suggested by Don Zickus.

Signed-off-by Marcelo Tosatti <[email protected]>

Awesome. Thanks for figuring this out Marcelo. Does that mean we can revert commit 5d1c0f4a now? :-)

This meets my expectations. I'll leave it to the virt folks to figure out if this covers all the corner cases or not.

Cheers,
Don

Quoted 9 lines · Don Zickus

Quoted 8 lines · Marcelo Tosatti

Implement reset of kernel watchdogs at pvclock read time. This avoids adding special code to every watchdog.

This is possible for watchdogs which measure time based on sched_clock() or ktime_get() variants.

Suggested by Don Zickus.

Quoted 4 lines

Quoted 1 line

Signed-off-by Marcelo Tosatti <[email protected]>

Awesome. Thanks for figuring this out Marcelo. Does that mean we can revert commit 5d1c0f4a now? :-)

Unfortunately no: soft lockup watchdog does not measure time based on sched_clock but on hrtimer interrupt count :-(

(see the the softlockup code in question, perhaps you can point to something that i'm missing).

BTW, are you OK with printing additional steal time information?
https://lkml.org/lkml/2013/6/27/755

From the host perspective, I didn't think you would have any problem

From the VM perspective, the whole overcommit/'wait a couple of minutes to

Quoted 10 lines · Marcelo Tosatti

Quoted 9 lines · Don Zickus

Quoted 8 lines · Marcelo Tosatti

Implement reset of kernel watchdogs at pvclock read time. This avoids adding special code to every watchdog.

This is possible for watchdogs which measure time based on sched_clock() or ktime_get() variants.

Suggested by Don Zickus.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Signed-off-by Marcelo Tosatti <[email protected]>

Awesome. Thanks for figuring this out Marcelo. Does that mean we can revert commit 5d1c0f4a now? :-)

Unfortunately no: soft lockup watchdog does not measure time based on sched_clock but on hrtimer interrupt count :-(

I believe it does. See __touch_watchdog() which calls get_timestamp() --> local_clock(). That is how it calculates the duration of the softlockup.

Now with your patch, it just sets the timestamp to zero with touch_softlockup_watchdog_sync(), which is fine. It will just sync up the clock, set a new timestamp, and check again in the next hrtimer interrupt.

So I guess I am confused what that commit does compared to this patch.

Quoted 3 lines

(see the the softlockup code in question, perhaps you can point to something that i'm missing).

Quoted 2 lines

BTW, are you OK with printing additional steal time information?
https://lkml.org/lkml/2013/6/27/755

Well, I thought this patch was supposed to replace that patch? Why do you still need that patch?

From https://lkml.org/lkml/2013/7/3/675:

From https://lkml.org/lkml/2013/7/3/675:

"Agree. However, can't see how there is a way around "having custom kvm/paravirt splat all over", for watchdogs that do:

1. check for watchdog resets
2. read time via sched_clock or xtime.
3. based on 2, decide whether there has been a longer delay than acceptable.

This is the case for the softlockup timer interrupt. So the splat there is necessary (otherwise any potential notification of vm-pause event noticed at 2 might be missed because its checked at 1).

For watchdogs that measure time based on interrupt event (such as hung task, rcu_cpu_stall, checking for the notification at sched_clock or lower is fine)."

See individual patches for details.

v2:
- do not create hung_task.h, move defines to sched.h (Don Zickus)
- switch patch order (Paolo)

v2:
- do not create hung_task.h, move defines to sched.h (Don Zickus)
- switch patch order (Paolo)

Quoted 3 lines · Marcelo Tosatti

v2:
- do not create hung_task.h, move defines to sched.h (Don Zickus)
- switch patch order (Paolo)

As long as it solves kvm's problems, I am ok with it.

Marcelo,

Is there still corner cases out there that get bit with softlockups or hung_task after this patchset?

Acked-by Don Zickus <[email protected]>

v2:
- do not create hung_task.h, move defines to sched.h (Don Zickus)
- switch patch order (Paolo)