* [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
* [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
* [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
* 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 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
* 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 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 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
* [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 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
* 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.