* [patch 0/3] generic kernel watchdog reset at pvclock read
@ 2013-10-08 1:05 Marcelo Tosatti
2013-10-08 1:05 ` [patch 1/3] hung_task: add method to reset detector Marcelo Tosatti
` (4 more replies)
0 siblings, 5 replies; 22+ messages in thread
From: Marcelo Tosatti @ 2013-10-08 1:05 UTC (permalink / raw)
To: kvm; +Cc: dzickus, pbonzini, gleb, linux-kernel
See individual patches for details.
^ permalink raw reply [flat|nested] 22+ messages in thread* [patch 1/3] hung_task: add method to reset detector 2013-10-08 1:05 [patch 0/3] generic kernel watchdog reset at pvclock read Marcelo Tosatti @ 2013-10-08 1:05 ` Marcelo Tosatti 2013-10-08 13:35 ` Don Zickus 2013-10-08 1:05 ` [patch 2/3] pvclock: detect watchdog reset at pvclock read Marcelo Tosatti ` (3 subsequent siblings) 4 siblings, 1 reply; 22+ messages in thread From: Marcelo Tosatti @ 2013-10-08 1:05 UTC (permalink / raw) To: kvm; +Cc: dzickus, pbonzini, gleb, linux-kernel, Marcelo Tosatti [-- Attachment #1: 01-hung-task-watchdog-reset --] [-- Type: text/plain, Size: 1445 bytes --] 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 <mtosatti@redhat.com> Index: kvm/kernel/hung_task.c =================================================================== --- kvm.orig/kernel/hung_task.c +++ kvm/kernel/hung_task.c @@ -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 =================================================================== --- /dev/null +++ kvm/include/linux/hung_task.h @@ -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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch 1/3] hung_task: add method to reset detector 2013-10-08 1:05 ` [patch 1/3] hung_task: add method to reset detector Marcelo Tosatti @ 2013-10-08 13:35 ` Don Zickus 0 siblings, 0 replies; 22+ messages in thread From: Don Zickus @ 2013-10-08 13:35 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: kvm, pbonzini, gleb, linux-kernel On Mon, Oct 07, 2013 at 10:05:16PM -0300, Marcelo Tosatti wrote: > 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. This makes sense to me. Should we throw the 'reset_hung_task_detector' in include/linux/sched.h like some of the other watchdog functions? Or is having a small hung_task.h file like this fine? Cheers, Don > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > Index: kvm/kernel/hung_task.c > =================================================================== > --- kvm.orig/kernel/hung_task.c > +++ kvm/kernel/hung_task.c > @@ -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 > =================================================================== > --- /dev/null > +++ kvm/include/linux/hung_task.h > @@ -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 > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [patch 2/3] pvclock: detect watchdog reset at pvclock read 2013-10-08 1:05 [patch 0/3] generic kernel watchdog reset at pvclock read Marcelo Tosatti 2013-10-08 1:05 ` [patch 1/3] hung_task: add method to reset detector Marcelo Tosatti @ 2013-10-08 1:05 ` Marcelo Tosatti 2013-10-08 9:58 ` Paolo Bonzini 2013-10-08 13:37 ` Don Zickus 2013-10-08 1:05 ` [patch 3/3] 01-hung-task-watchdog-reset 02-kvmclock-touch-watchdog-on-kvmclock-read series Marcelo Tosatti ` (2 subsequent siblings) 4 siblings, 2 replies; 22+ messages in thread From: Marcelo Tosatti @ 2013-10-08 1:05 UTC (permalink / raw) To: kvm; +Cc: dzickus, pbonzini, gleb, linux-kernel, Marcelo Tosatti [-- Attachment #1: 02-kvmclock-touch-watchdog-on-kvmclock-read --] [-- Type: text/plain, Size: 2262 bytes --] 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 <mtosatti@redhat.com> Index: kvm/arch/x86/kernel/kvmclock.c =================================================================== --- kvm.orig/arch/x86/kernel/kvmclock.c +++ kvm/arch/x86/kernel/kvmclock.c @@ -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 =================================================================== --- kvm.orig/arch/x86/kernel/pvclock.c +++ kvm/arch/x86/kernel/pvclock.c @@ -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 =================================================================== --- kvm.orig/arch/x86/include/asm/pvclock.h +++ kvm/arch/x86/include/asm/pvclock.h @@ -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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch 2/3] pvclock: detect watchdog reset at pvclock read 2013-10-08 1:05 ` [patch 2/3] pvclock: detect watchdog reset at pvclock read Marcelo Tosatti @ 2013-10-08 9:58 ` Paolo Bonzini 2013-10-09 1:22 ` Marcelo Tosatti 2013-10-08 13:37 ` Don Zickus 1 sibling, 1 reply; 22+ messages in thread From: Paolo Bonzini @ 2013-10-08 9:58 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: kvm, dzickus, gleb, linux-kernel Il 08/10/2013 03:05, Marcelo Tosatti ha scritto: > +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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch 2/3] pvclock: detect watchdog reset at pvclock read 2013-10-08 9:58 ` Paolo Bonzini @ 2013-10-09 1:22 ` Marcelo Tosatti 2013-10-09 8:39 ` Paolo Bonzini 0 siblings, 1 reply; 22+ messages in thread From: Marcelo Tosatti @ 2013-10-09 1:22 UTC (permalink / raw) To: Paolo Bonzini; +Cc: kvm, dzickus, gleb, linux-kernel On Tue, Oct 08, 2013 at 11:58:10AM +0200, Paolo Bonzini wrote: > Il 08/10/2013 03:05, Marcelo Tosatti ha scritto: > > +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 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch 2/3] pvclock: detect watchdog reset at pvclock read 2013-10-09 1:22 ` Marcelo Tosatti @ 2013-10-09 8:39 ` Paolo Bonzini 0 siblings, 0 replies; 22+ messages in thread From: Paolo Bonzini @ 2013-10-09 8:39 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: kvm, dzickus, gleb, linux-kernel Il 09/10/2013 03:22, Marcelo Tosatti ha scritto: > On Tue, Oct 08, 2013 at 11:58:10AM +0200, Paolo Bonzini wrote: >> Il 08/10/2013 03:05, Marcelo Tosatti ha scritto: >>> +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 > > 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. > Thanks for clarifying. Paolo ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch 2/3] pvclock: detect watchdog reset at pvclock read 2013-10-08 1:05 ` [patch 2/3] pvclock: detect watchdog reset at pvclock read Marcelo Tosatti 2013-10-08 9:58 ` Paolo Bonzini @ 2013-10-08 13:37 ` Don Zickus 2013-10-08 22:08 ` Marcelo Tosatti 1 sibling, 1 reply; 22+ messages in thread From: Don Zickus @ 2013-10-08 13:37 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: kvm, pbonzini, gleb, linux-kernel On Mon, Oct 07, 2013 at 10:05:17PM -0300, Marcelo Tosatti wrote: > 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 <mtosatti@redhat.com> 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 > > Index: kvm/arch/x86/kernel/kvmclock.c > =================================================================== > --- kvm.orig/arch/x86/kernel/kvmclock.c > +++ kvm/arch/x86/kernel/kvmclock.c > @@ -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 > =================================================================== > --- kvm.orig/arch/x86/kernel/pvclock.c > +++ kvm/arch/x86/kernel/pvclock.c > @@ -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 > =================================================================== > --- kvm.orig/arch/x86/include/asm/pvclock.h > +++ kvm/arch/x86/include/asm/pvclock.h > @@ -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. > > ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch 2/3] pvclock: detect watchdog reset at pvclock read 2013-10-08 13:37 ` Don Zickus @ 2013-10-08 22:08 ` Marcelo Tosatti 2013-10-09 13:55 ` Don Zickus 0 siblings, 1 reply; 22+ messages in thread From: Marcelo Tosatti @ 2013-10-08 22:08 UTC (permalink / raw) To: Don Zickus; +Cc: kvm, pbonzini, gleb, linux-kernel On Tue, Oct 08, 2013 at 09:37:05AM -0400, Don Zickus wrote: > On Mon, Oct 07, 2013 at 10:05:17PM -0300, Marcelo Tosatti wrote: > > 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 <mtosatti@redhat.com> > > 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 > 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 Thanks ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch 2/3] pvclock: detect watchdog reset at pvclock read 2013-10-08 22:08 ` Marcelo Tosatti @ 2013-10-09 13:55 ` Don Zickus 2013-10-09 21:26 ` Marcelo Tosatti 0 siblings, 1 reply; 22+ messages in thread From: Don Zickus @ 2013-10-09 13:55 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: kvm, pbonzini, gleb, linux-kernel On Tue, Oct 08, 2013 at 07:08:11PM -0300, Marcelo Tosatti wrote: > On Tue, Oct 08, 2013 at 09:37:05AM -0400, Don Zickus wrote: > > On Mon, Oct 07, 2013 at 10:05:17PM -0300, Marcelo Tosatti wrote: > > > 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 <mtosatti@redhat.com> > > > > 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. > (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 Well, I thought this patch was supposed to replace that patch? Why do you still need that patch? Perhaps my confusion is centered around which softlockups are the problem the VM's or the host's. >From the host perspective, I didn't think you would have any problem because the VM is just another process that runs in its time slice. >From the VM perspective, the whole overcommit/'wait a couple of minutes to run again', could easily cause lockups. But I thought this patch set detected that and touched the watchdogs early enough that when the next iteration of the hrtimer came through, it would _not_ cause a softlockup (it would delay it an hrtimer cycle). So, if I am misunderstanding the problems (which I probably am :-) ), I could use a pointer or a quick explaination to remind what the issues are again and why you think the other patches are still necessary. :-) Cheers, Don ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch 2/3] pvclock: detect watchdog reset at pvclock read 2013-10-09 13:55 ` Don Zickus @ 2013-10-09 21:26 ` Marcelo Tosatti 2013-10-16 18:22 ` Don Zickus 0 siblings, 1 reply; 22+ messages in thread From: Marcelo Tosatti @ 2013-10-09 21:26 UTC (permalink / raw) To: Don Zickus; +Cc: kvm, pbonzini, gleb, linux-kernel On Wed, Oct 09, 2013 at 09:55:19AM -0400, Don Zickus wrote: > On Tue, Oct 08, 2013 at 07:08:11PM -0300, Marcelo Tosatti wrote: > > On Tue, Oct 08, 2013 at 09:37:05AM -0400, Don Zickus wrote: > > > On Mon, Oct 07, 2013 at 10:05:17PM -0300, Marcelo Tosatti wrote: > > > > 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 <mtosatti@redhat.com> > > > > > > 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. > > > (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 > > 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: "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)." ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch 2/3] pvclock: detect watchdog reset at pvclock read 2013-10-09 21:26 ` Marcelo Tosatti @ 2013-10-16 18:22 ` Don Zickus 0 siblings, 0 replies; 22+ messages in thread From: Don Zickus @ 2013-10-16 18:22 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: kvm, pbonzini, gleb, linux-kernel On Wed, Oct 09, 2013 at 06:26:33PM -0300, Marcelo Tosatti wrote: > 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)." > Sorry for the delay, I was trying to spend time understanding the problem again. Rik van Riel helped me (as I could just walk over to him). I was trying to figure out if there was a way to convert the softlockup to something more virt-friendly mechanism. I was toying with the idea of having the softlockup use schedule_timeout and then have the touch_softlockup routine keep rescheduling (delay) the timeout. The idea was that if it actually timed out, it was guaranteed to be a lockup. This removed the need for the duration calculation but more importantly, I believe the schedule_timeout routine was guest time aware. But I haven't had the chance to think through the whole thing to know if that was the right way to go or if there was pitfalls. Just busy with other stuff. Regardless, I think this patchset solves a particular problem and I am ok with it (even v2 I believe). Cheers, Don ^ permalink raw reply [flat|nested] 22+ messages in thread
* [patch 3/3] 01-hung-task-watchdog-reset 02-kvmclock-touch-watchdog-on-kvmclock-read series 2013-10-08 1:05 [patch 0/3] generic kernel watchdog reset at pvclock read Marcelo Tosatti 2013-10-08 1:05 ` [patch 1/3] hung_task: add method to reset detector Marcelo Tosatti 2013-10-08 1:05 ` [patch 2/3] pvclock: detect watchdog reset at pvclock read Marcelo Tosatti @ 2013-10-08 1:05 ` Marcelo Tosatti 2013-10-08 1:07 ` Marcelo Tosatti 2013-10-08 9:57 ` [patch 0/3] generic kernel watchdog reset at pvclock read Paolo Bonzini 2013-10-12 0:39 ` [patch 0/2] generic kernel watchdog reset at pvclock read (v2) Marcelo Tosatti 4 siblings, 1 reply; 22+ messages in thread From: Marcelo Tosatti @ 2013-10-08 1:05 UTC (permalink / raw) To: kvm; +Cc: dzickus, pbonzini, gleb, linux-kernel [-- Attachment #1: series --] [-- Type: text/plain, Size: 2 bytes --] ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch 3/3] 01-hung-task-watchdog-reset 02-kvmclock-touch-watchdog-on-kvmclock-read series 2013-10-08 1:05 ` [patch 3/3] 01-hung-task-watchdog-reset 02-kvmclock-touch-watchdog-on-kvmclock-read series Marcelo Tosatti @ 2013-10-08 1:07 ` Marcelo Tosatti 0 siblings, 0 replies; 22+ messages in thread From: Marcelo Tosatti @ 2013-10-08 1:07 UTC (permalink / raw) To: kvm; +Cc: dzickus, pbonzini, gleb, linux-kernel Please ignore patch 3/3 - there is none. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch 0/3] generic kernel watchdog reset at pvclock read 2013-10-08 1:05 [patch 0/3] generic kernel watchdog reset at pvclock read Marcelo Tosatti ` (2 preceding siblings ...) 2013-10-08 1:05 ` [patch 3/3] 01-hung-task-watchdog-reset 02-kvmclock-touch-watchdog-on-kvmclock-read series Marcelo Tosatti @ 2013-10-08 9:57 ` Paolo Bonzini 2013-10-12 0:39 ` [patch 0/2] generic kernel watchdog reset at pvclock read (v2) Marcelo Tosatti 4 siblings, 0 replies; 22+ messages in thread From: Paolo Bonzini @ 2013-10-08 9:57 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: kvm, dzickus, gleb, linux-kernel Il 08/10/2013 03:05, Marcelo Tosatti ha scritto: > See individual patches for details. Looks good. Perhaps you want to swap the two patches, so that patch 1 also includes the call to reset_hung_task_detector? If it goes through the KVM tree, I guess we need an Acked-by for the hung_task part. If it goes through someone else, you can add my Acked-by to the pvclock part. Paolo ^ permalink raw reply [flat|nested] 22+ messages in thread
* [patch 0/2] generic kernel watchdog reset at pvclock read (v2) 2013-10-08 1:05 [patch 0/3] generic kernel watchdog reset at pvclock read Marcelo Tosatti ` (3 preceding siblings ...) 2013-10-08 9:57 ` [patch 0/3] generic kernel watchdog reset at pvclock read Paolo Bonzini @ 2013-10-12 0:39 ` Marcelo Tosatti 2013-10-12 0:39 ` [patch 1/2] pvclock: detect watchdog reset at pvclock read Marcelo Tosatti ` (4 more replies) 4 siblings, 5 replies; 22+ messages in thread From: Marcelo Tosatti @ 2013-10-12 0:39 UTC (permalink / raw) To: kvm; +Cc: dzickus, pbonzini, gleb, linux-kernel v2: - do not create hung_task.h, move defines to sched.h (Don Zickus) - switch patch order (Paolo) ^ permalink raw reply [flat|nested] 22+ messages in thread
* [patch 1/2] pvclock: detect watchdog reset at pvclock read 2013-10-12 0:39 ` [patch 0/2] generic kernel watchdog reset at pvclock read (v2) Marcelo Tosatti @ 2013-10-12 0:39 ` Marcelo Tosatti 2013-10-12 0:39 ` [patch 2/2] hung_task: add method to reset detector Marcelo Tosatti ` (3 subsequent siblings) 4 siblings, 0 replies; 22+ messages in thread From: Marcelo Tosatti @ 2013-10-12 0:39 UTC (permalink / raw) To: kvm; +Cc: dzickus, pbonzini, gleb, linux-kernel, Marcelo Tosatti [-- Attachment #1: 02-kvmclock-touch-watchdog-on-kvmclock-read --] [-- Type: text/plain, Size: 2053 bytes --] 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 <mtosatti@redhat.com> Index: kvm/arch/x86/kernel/kvmclock.c =================================================================== --- kvm.orig/arch/x86/kernel/kvmclock.c +++ kvm/arch/x86/kernel/kvmclock.c @@ -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 =================================================================== --- kvm.orig/arch/x86/kernel/pvclock.c +++ kvm/arch/x86/kernel/pvclock.c @@ -43,6 +43,13 @@ 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(); +} + static atomic64_t last_value = ATOMIC64_INIT(0); void pvclock_resume(void) @@ -74,6 +81,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 =================================================================== --- kvm.orig/arch/x86/include/asm/pvclock.h +++ kvm/arch/x86/include/asm/pvclock.h @@ -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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [patch 2/2] hung_task: add method to reset detector 2013-10-12 0:39 ` [patch 0/2] generic kernel watchdog reset at pvclock read (v2) Marcelo Tosatti 2013-10-12 0:39 ` [patch 1/2] pvclock: detect watchdog reset at pvclock read Marcelo Tosatti @ 2013-10-12 0:39 ` Marcelo Tosatti 2013-10-14 11:30 ` [patch 0/2] generic kernel watchdog reset at pvclock read (v2) Paolo Bonzini ` (2 subsequent siblings) 4 siblings, 0 replies; 22+ messages in thread From: Marcelo Tosatti @ 2013-10-12 0:39 UTC (permalink / raw) To: kvm; +Cc: dzickus, pbonzini, gleb, linux-kernel, Marcelo Tosatti [-- Attachment #1: 01-hung-task-watchdog-reset --] [-- Type: text/plain, Size: 1951 bytes --] 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 <mtosatti@redhat.com> Index: kvm/kernel/hung_task.c =================================================================== --- kvm.orig/kernel/hung_task.c +++ kvm/kernel/hung_task.c @@ -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/sched.h =================================================================== --- kvm.orig/include/linux/sched.h +++ kvm/include/linux/sched.h @@ -285,6 +285,14 @@ static inline void lockup_detector_init( } #endif +#ifdef CONFIG_DETECT_HUNG_TASK +void reset_hung_task_detector(void); +#else +static inline void reset_hung_task_detector(void) +{ +} +#endif + /* Attach to any functions which should be ignored in wchan output. */ #define __sched __attribute__((__section__(".sched.text"))) Index: kvm/arch/x86/kernel/pvclock.c =================================================================== --- kvm.orig/arch/x86/kernel/pvclock.c +++ kvm/arch/x86/kernel/pvclock.c @@ -48,6 +48,7 @@ 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); ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch 0/2] generic kernel watchdog reset at pvclock read (v2) 2013-10-12 0:39 ` [patch 0/2] generic kernel watchdog reset at pvclock read (v2) Marcelo Tosatti 2013-10-12 0:39 ` [patch 1/2] pvclock: detect watchdog reset at pvclock read Marcelo Tosatti 2013-10-12 0:39 ` [patch 2/2] hung_task: add method to reset detector Marcelo Tosatti @ 2013-10-14 11:30 ` Paolo Bonzini 2013-10-16 18:25 ` Don Zickus 2013-11-06 7:49 ` Gleb Natapov 4 siblings, 0 replies; 22+ messages in thread From: Paolo Bonzini @ 2013-10-14 11:30 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: kvm, dzickus, gleb, linux-kernel Il 12/10/2013 02:39, Marcelo Tosatti ha scritto: > v2: > - do not create hung_task.h, move defines to sched.h (Don Zickus) > - switch patch order (Paolo) > Acked-by: Paolo Bonzini <pbonzini@redhat.com> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch 0/2] generic kernel watchdog reset at pvclock read (v2) 2013-10-12 0:39 ` [patch 0/2] generic kernel watchdog reset at pvclock read (v2) Marcelo Tosatti ` (2 preceding siblings ...) 2013-10-14 11:30 ` [patch 0/2] generic kernel watchdog reset at pvclock read (v2) Paolo Bonzini @ 2013-10-16 18:25 ` Don Zickus 2013-10-16 21:02 ` Marcelo Tosatti 2013-11-06 7:49 ` Gleb Natapov 4 siblings, 1 reply; 22+ messages in thread From: Don Zickus @ 2013-10-16 18:25 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: kvm, pbonzini, gleb, linux-kernel On Fri, Oct 11, 2013 at 09:39:24PM -0300, Marcelo Tosatti wrote: > 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 <dzickus@redhat.com> ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch 0/2] generic kernel watchdog reset at pvclock read (v2) 2013-10-16 18:25 ` Don Zickus @ 2013-10-16 21:02 ` Marcelo Tosatti 0 siblings, 0 replies; 22+ messages in thread From: Marcelo Tosatti @ 2013-10-16 21:02 UTC (permalink / raw) To: Don Zickus; +Cc: kvm, pbonzini, gleb, linux-kernel On Wed, Oct 16, 2013 at 02:25:00PM -0400, Don Zickus wrote: > On Fri, Oct 11, 2013 at 09:39:24PM -0300, Marcelo Tosatti wrote: > > 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? Not that i am aware of. > Acked-by: Don Zickus <dzickus@redhat.com> Thanks. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [patch 0/2] generic kernel watchdog reset at pvclock read (v2) 2013-10-12 0:39 ` [patch 0/2] generic kernel watchdog reset at pvclock read (v2) Marcelo Tosatti ` (3 preceding siblings ...) 2013-10-16 18:25 ` Don Zickus @ 2013-11-06 7:49 ` Gleb Natapov 4 siblings, 0 replies; 22+ messages in thread From: Gleb Natapov @ 2013-11-06 7:49 UTC (permalink / raw) To: Marcelo Tosatti; +Cc: kvm, dzickus, pbonzini, linux-kernel On Fri, Oct 11, 2013 at 09:39:24PM -0300, Marcelo Tosatti wrote: > v2: > - do not create hung_task.h, move defines to sched.h (Don Zickus) > - switch patch order (Paolo) Applied, thanks. -- Gleb. ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2013-11-06 7:49 UTC | newest] Thread overview: 22+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-10-08 1:05 [patch 0/3] generic kernel watchdog reset at pvclock read Marcelo Tosatti 2013-10-08 1:05 ` [patch 1/3] hung_task: add method to reset detector Marcelo Tosatti 2013-10-08 13:35 ` Don Zickus 2013-10-08 1:05 ` [patch 2/3] pvclock: detect watchdog reset at pvclock read Marcelo Tosatti 2013-10-08 9:58 ` Paolo Bonzini 2013-10-09 1:22 ` Marcelo Tosatti 2013-10-09 8:39 ` Paolo Bonzini 2013-10-08 13:37 ` Don Zickus 2013-10-08 22:08 ` Marcelo Tosatti 2013-10-09 13:55 ` Don Zickus 2013-10-09 21:26 ` Marcelo Tosatti 2013-10-16 18:22 ` Don Zickus 2013-10-08 1:05 ` [patch 3/3] 01-hung-task-watchdog-reset 02-kvmclock-touch-watchdog-on-kvmclock-read series Marcelo Tosatti 2013-10-08 1:07 ` Marcelo Tosatti 2013-10-08 9:57 ` [patch 0/3] generic kernel watchdog reset at pvclock read Paolo Bonzini 2013-10-12 0:39 ` [patch 0/2] generic kernel watchdog reset at pvclock read (v2) Marcelo Tosatti 2013-10-12 0:39 ` [patch 1/2] pvclock: detect watchdog reset at pvclock read Marcelo Tosatti 2013-10-12 0:39 ` [patch 2/2] hung_task: add method to reset detector Marcelo Tosatti 2013-10-14 11:30 ` [patch 0/2] generic kernel watchdog reset at pvclock read (v2) Paolo Bonzini 2013-10-16 18:25 ` Don Zickus 2013-10-16 21:02 ` Marcelo Tosatti 2013-11-06 7:49 ` Gleb Natapov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).