[PATCH v2 0/2] Quieten softlockup detector on virtualised kernels

[PATCH v2 0/2] Quieten softlockup detector on virtualised kernels

[Cyril Bur]

When the hypervisor pauses a virtualised kernel the kernel will observe a jump
in timebase, this can cause spurious messages from the softlockup detector.

Whilst these messages are harmless, they are accompanied with a stack trace
which causes undue concern and more problematically the stack trace in the
guest has nothing to do with the observed problem and can only be misleading.

Futhermore, on POWER8 this is completely avoidable with the introduction of
the Virtual Time Base (VTB) register.

V2:
	Remove the export of running_clock
	Added #ifdef CONFIG_PPC_PSERIES and optimised the non lpar + vtb cases.
	Replaced the use of sched_clock_with local_clock it was used originally in
the softlockup detector.

Cyril Bur (2):
  Add another clock for use with the soft lockup watchdog.
  powerpc: add running_clock for powerpc to prevent spurious softlockup
    warnings

 arch/powerpc/kernel/time.c | 32 ++++++++++++++++++++++++++++++++
 include/linux/sched.h      |  1 +
 kernel/sched/clock.c       | 13 +++++++++++++
 kernel/watchdog.c          |  2 +-
 4 files changed, 47 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH v2 1/2] Add another clock for use with the soft lockup watchdog.

[Cyril Bur]

This permits the use of arch specific clocks for which virtualised kernels can
use their notion of 'running' time, not the elpased wall time which will
include host execution time.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
V2:
   Remove the export of running_clock
   Use local_clock instead of sched_clock as was initally used in the
softlockup detector

---
 include/linux/sched.h |  1 +
 kernel/sched/clock.c  | 13 +++++++++++++
 kernel/watchdog.c     |  2 +-
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 8db31ef..e400162 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2145,6 +2145,7 @@ extern unsigned long long notrace sched_clock(void);
  */
 extern u64 cpu_clock(int cpu);
 extern u64 local_clock(void);
+extern u64 running_clock(void);
 extern u64 sched_clock_cpu(int cpu);
 
 
diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
index c27e4f8..c0a2051 100644
--- a/kernel/sched/clock.c
+++ b/kernel/sched/clock.c
@@ -420,3 +420,16 @@ u64 local_clock(void)
 
 EXPORT_SYMBOL_GPL(cpu_clock);
 EXPORT_SYMBOL_GPL(local_clock);
+
+/*
+ * Running clock - returns the time that has elapsed while a guest has been
+ * running.
+ * On a guest this value should be local_clock minus the time the guest was
+ * suspended by the hypervisor (for any reason).
+ * On bare metal this function should return the same as local_clock.
+ * Architectures and sub-architectures can override this.
+ */
+u64 __weak running_clock(void)
+{
+	return local_clock();
+}
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 70bf118..3174bf8 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -154,7 +154,7 @@ static int get_softlockup_thresh(void)
  */
 static unsigned long get_timestamp(void)
 {
-	return local_clock() >> 30LL;  /* 2^30 ~= 10^9 */
+	return running_clock() >> 30LL;  /* 2^30 ~= 10^9 */
 }
 
 static void set_sample_period(void)
-- 
1.9.1

[PATCH v2 2/2] powerpc: add running_clock for powerpc to prevent spurious softlockup warnings

[Cyril Bur]

On POWER8 virtualised kernels the VTB register can be read to have a view of
time that only increases while the guest is running. This will prevent guests
from seeing time jump if a guest is paused for significant amounts of time.

On POWER7 and below virtualised kernels stolen time is subtracted from
local_clock as a best effort approximation. This will not eliminate spurious
warnings in the case of a suspended guest but may reduce the occurance in the
case of softlockups due to host over commit.

Bare metal kernels should avoid reading the VTB as KVM does not restore sane
values when not executing, the approxmation is fine as host kernels won't
observe any stolen time.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
V2:
   Replaced the use of sched_clock_with local_clock it was used originally in
the softlockup detector.
   Added #ifdef CONFIG_PPC_PSERIES and optimised the non lpar + vtb cases.

---
 arch/powerpc/kernel/time.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index fa7c4f1..fd35e5b 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -621,6 +621,38 @@ unsigned long long sched_clock(void)
 	return mulhdu(get_tb() - boot_tb, tb_to_ns_scale) << tb_to_ns_shift;
 }
 
+
+#ifdef CONFIG_PPC_PSERIES
+
+/*
+ * Running clock - attempts to give a view of time passing for a virtualised
+ * kernels.
+ * Uses the VTB register if available otherwise a next best guess.
+ */
+unsigned long long running_clock(void)
+{
+	/*
+	 * Don't read the VTB as a host since KVM does not switch in host timebase
+	 * into the VTB when it takes a guest off the CPU, reading the VTB would
+	 * result in reading 'last switched out' guest VTB.
+	 *
+	 * Host kernels are often compiled with CONFIG_PSERIES checked, it would be
+	 * unsafe to rely only on the #ifdef above.
+	 */
+	if (firmware_has_feature(FW_FEATURE_LPAR) &&
+	    cpu_has_feature(CPU_FTR_ARCH_207S))
+		return mulhdu(get_vtb() - boot_tb, tb_to_ns_scale) << tb_to_ns_shift;
+
+	/*
+	 * This is a next best approximation without a VTB.
+	 * On a host which is running bare metal there should never be any stolen
+	 * time and on a host which doesn't do any virtualisation TB *should* equal
+	 * VTB so it makes no difference anyway.
+	 */
+	return local_clock() - cputime_to_nsecs(kcpustat_this_cpu->cpustat[CPUTIME_STEAL]);
+}
+#endif
+
 static int __init get_freq(char *name, int cells, unsigned long *val)
 {
 	struct device_node *cpu;
-- 
1.9.1

Re: [PATCH v2 0/2] Quieten softlockup detector on virtualised kernels

[Cyril Bur]

Hi Andrew,

Could you please pick these patches up through your tree?

Thanks,

Cyril

On Fri, 2015-01-09 at 14:34 +1100, Cyril Bur wrote:
> When the hypervisor pauses a virtualised kernel the kernel will observe a jump
> in timebase, this can cause spurious messages from the softlockup detector.
> 
> Whilst these messages are harmless, they are accompanied with a stack trace
> which causes undue concern and more problematically the stack trace in the
> guest has nothing to do with the observed problem and can only be misleading.
> 
> Futhermore, on POWER8 this is completely avoidable with the introduction of
> the Virtual Time Base (VTB) register.
> 
> V2:
> 	Remove the export of running_clock
> 	Added #ifdef CONFIG_PPC_PSERIES and optimised the non lpar + vtb cases.
> 	Replaced the use of sched_clock_with local_clock it was used originally in
> the softlockup detector.
> 
> Cyril Bur (2):
>   Add another clock for use with the soft lockup watchdog.
>   powerpc: add running_clock for powerpc to prevent spurious softlockup
>     warnings
> 
>  arch/powerpc/kernel/time.c | 32 ++++++++++++++++++++++++++++++++
>  include/linux/sched.h      |  1 +
>  kernel/sched/clock.c       | 13 +++++++++++++
>  kernel/watchdog.c          |  2 +-
>  4 files changed, 47 insertions(+), 1 deletion(-)
> 

Re: [PATCH v2 0/2] Quieten softlockup detector on virtualised kernels

[Andrew Morton]

On Mon, 02 Feb 2015 15:58:38 +1100 Cyril Bur <cyrilbur@gmail.com> wrote:

> On Fri, 2015-01-09 at 14:34 +1100, Cyril Bur wrote:
> > When the hypervisor pauses a virtualised kernel the kernel will observe a jump
> > in timebase, this can cause spurious messages from the softlockup detector.
> > 
> > Whilst these messages are harmless, they are accompanied with a stack trace
> > which causes undue concern and more problematically the stack trace in the
> > guest has nothing to do with the observed problem and can only be misleading.
> > 
> > Futhermore, on POWER8 this is completely avoidable with the introduction of
> > the Virtual Time Base (VTB) register.
> > 

(Top-posting repaired.  Please don't).

> Could you please pick these patches up through your tree?

I shall.  There were quite a few review comments last time and I *think*
they've been addressed(?).  I'll cc the involved parties on the commits
- please check?

Re: [PATCH v2 2/2] powerpc: add running_clock for powerpc to prevent spurious softlockup warnings

[Paul Bolle]

On Fri, 2015-01-09 at 14:34 +1100, Cyril Bur wrote:
> On POWER8 virtualised kernels the VTB register can be read to have a view of
> time that only increases while the guest is running. This will prevent guests
> from seeing time jump if a guest is paused for significant amounts of time.
> 
> On POWER7 and below virtualised kernels stolen time is subtracted from
> local_clock as a best effort approximation. This will not eliminate spurious
> warnings in the case of a suspended guest but may reduce the occurance in the
> case of softlockups due to host over commit.
> 
> Bare metal kernels should avoid reading the VTB as KVM does not restore sane
> values when not executing, the approxmation is fine as host kernels won't
> observe any stolen time.
> 
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
> V2:
>    Replaced the use of sched_clock_with local_clock it was used originally in
> the softlockup detector.
>    Added #ifdef CONFIG_PPC_PSERIES and optimised the non lpar + vtb cases.

This became commit 3e5aba51e929 ("powerpc: add running_clock for powerpc
to prevent spurious softlockup warnings") in today's linux-next (ie,
next-20150204). I noticed because a script I use to check linux-next
spotted a trivial issues with it.

> ---
>  arch/powerpc/kernel/time.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index fa7c4f1..fd35e5b 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -621,6 +621,38 @@ unsigned long long sched_clock(void)
>  	return mulhdu(get_tb() - boot_tb, tb_to_ns_scale) << tb_to_ns_shift;
>  }
>  
> +
> +#ifdef CONFIG_PPC_PSERIES
> +
> +/*
> + * Running clock - attempts to give a view of time passing for a virtualised
> + * kernels.
> + * Uses the VTB register if available otherwise a next best guess.
> + */
> +unsigned long long running_clock(void)
> +{
> +	/*
> +	 * Don't read the VTB as a host since KVM does not switch in host timebase
> +	 * into the VTB when it takes a guest off the CPU, reading the VTB would
> +	 * result in reading 'last switched out' guest VTB.
> +	 *
> +	 * Host kernels are often compiled with CONFIG_PSERIES checked, it would be

You obviously wanted to use CONFIG_PPC_PSERIES here.

Should I submit a trivial patch to fix that typo or do you prefer to do
that yourself?

Thanks,


Paul Bolle

Re: [PATCH v2 2/2] powerpc: add running_clock for powerpc to prevent spurious softlockup warnings

[Cyril Bur]

On Wed, 2015-02-04 at 11:42 +0100, Paul Bolle wrote:
> On Fri, 2015-01-09 at 14:34 +1100, Cyril Bur wrote:
> > On POWER8 virtualised kernels the VTB register can be read to have a view of
> > time that only increases while the guest is running. This will prevent guests
> > from seeing time jump if a guest is paused for significant amounts of time.
> > 
> > On POWER7 and below virtualised kernels stolen time is subtracted from
> > local_clock as a best effort approximation. This will not eliminate spurious
> > warnings in the case of a suspended guest but may reduce the occurance in the
> > case of softlockups due to host over commit.
> > 
> > Bare metal kernels should avoid reading the VTB as KVM does not restore sane
> > values when not executing, the approxmation is fine as host kernels won't
> > observe any stolen time.
> > 
> > Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> > ---
> > V2:
> >    Replaced the use of sched_clock_with local_clock it was used originally in
> > the softlockup detector.
> >    Added #ifdef CONFIG_PPC_PSERIES and optimised the non lpar + vtb cases.
> 
> This became commit 3e5aba51e929 ("powerpc: add running_clock for powerpc
> to prevent spurious softlockup warnings") in today's linux-next (ie,
> next-20150204). I noticed because a script I use to check linux-next
> spotted a trivial issues with it.
> 
> > ---
> >  arch/powerpc/kernel/time.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> > index fa7c4f1..fd35e5b 100644
> > --- a/arch/powerpc/kernel/time.c
> > +++ b/arch/powerpc/kernel/time.c
> > @@ -621,6 +621,38 @@ unsigned long long sched_clock(void)
> >  	return mulhdu(get_tb() - boot_tb, tb_to_ns_scale) << tb_to_ns_shift;
> >  }
> >  
> > +
> > +#ifdef CONFIG_PPC_PSERIES
> > +
> > +/*
> > + * Running clock - attempts to give a view of time passing for a virtualised
> > + * kernels.
> > + * Uses the VTB register if available otherwise a next best guess.
> > + */
> > +unsigned long long running_clock(void)
> > +{
> > +	/*
> > +	 * Don't read the VTB as a host since KVM does not switch in host timebase
> > +	 * into the VTB when it takes a guest off the CPU, reading the VTB would
> > +	 * result in reading 'last switched out' guest VTB.
> > +	 *
> > +	 * Host kernels are often compiled with CONFIG_PSERIES checked, it would be
> 
> You obviously wanted to use CONFIG_PPC_PSERIES here.
> 
Yep, sorry.

> Should I submit a trivial patch to fix that typo or do you prefer to do
> that yourself?
> 
Indeed a trivial fixup, I've pasted the hunk below. Andrew are you ok to
fold it in?

Thanks,

Cyril


diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index fd35e5b..770dc16 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -636,8 +636,8 @@ unsigned long long running_clock(void)
         * into the VTB when it takes a guest off the CPU, reading the VTB would
         * result in reading 'last switched out' guest VTB.
         *
-        * Host kernels are often compiled with CONFIG_PSERIES checked, it would be
-        * unsafe to rely only on the #ifdef above.
+        * Host kernels are often compiled with CONFIG_PPC_PSERIES checked, it
+        * would be unsafe to rely only on the #ifdef above.
         */
        if (firmware_has_feature(FW_FEATURE_LPAR) &&
            cpu_has_feature(CPU_FTR_ARCH_207S))
> Thanks,
> 
> 
> Paul Bolle
> 

Re: [PATCH v2 0/2] Quieten softlockup detector on virtualised kernels

[Don Zickus]

On Fri, Jan 09, 2015 at 02:34:36PM +1100, Cyril Bur wrote:
> When the hypervisor pauses a virtualised kernel the kernel will observe a jump
> in timebase, this can cause spurious messages from the softlockup detector.
> 
> Whilst these messages are harmless, they are accompanied with a stack trace
> which causes undue concern and more problematically the stack trace in the
> guest has nothing to do with the observed problem and can only be misleading.

Originally I was suggesting to talk with the x86/kvm folks about coming up
with a common solution.  But I didn't hear a response from those cc'd at
the time.  This solution looks a lot cleaner than what x86 is doing.

I am willing to see how this works out on ppc64's kvm and see if they still
see any issues after awhile.  If not, maybe I can poke the x86 folks to
migrate to something similar.

Acked-by: Don Zickus <dzickus@redhat.com>

> 
> Futhermore, on POWER8 this is completely avoidable with the introduction of
> the Virtual Time Base (VTB) register.
> 
> V2:
> 	Remove the export of running_clock
> 	Added #ifdef CONFIG_PPC_PSERIES and optimised the non lpar + vtb cases.
> 	Replaced the use of sched_clock_with local_clock it was used originally in
> the softlockup detector.
> 
> Cyril Bur (2):
>   Add another clock for use with the soft lockup watchdog.
>   powerpc: add running_clock for powerpc to prevent spurious softlockup
>     warnings
> 
>  arch/powerpc/kernel/time.c | 32 ++++++++++++++++++++++++++++++++
>  include/linux/sched.h      |  1 +
>  kernel/sched/clock.c       | 13 +++++++++++++
>  kernel/watchdog.c          |  2 +-
>  4 files changed, 47 insertions(+), 1 deletion(-)
> 
> -- 
> 1.9.1
> 

Re: [PATCH v2 1/2] Add another clock for use with the soft lockup watchdog.

[Chai Wen]

On 01/09/2015 11:34 AM, Cyril Bur wrote:

> This permits the use of arch specific clocks for which virtualised kernels can
> use their notion of 'running' time, not the elpased wall time which will


s/elpased/elapsed/


thanks
chai wen

> include host execution time.
> 
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
> ---
> V2:
>    Remove the export of running_clock
>    Use local_clock instead of sched_clock as was initally used in the
> softlockup detector
> 
> ---
>  include/linux/sched.h |  1 +
>  kernel/sched/clock.c  | 13 +++++++++++++
>  kernel/watchdog.c     |  2 +-
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 8db31ef..e400162 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2145,6 +2145,7 @@ extern unsigned long long notrace sched_clock(void);
>   */
>  extern u64 cpu_clock(int cpu);
>  extern u64 local_clock(void);
> +extern u64 running_clock(void);
>  extern u64 sched_clock_cpu(int cpu);
>  
>  
> diff --git a/kernel/sched/clock.c b/kernel/sched/clock.c
> index c27e4f8..c0a2051 100644
> --- a/kernel/sched/clock.c
> +++ b/kernel/sched/clock.c
> @@ -420,3 +420,16 @@ u64 local_clock(void)
>  
>  EXPORT_SYMBOL_GPL(cpu_clock);
>  EXPORT_SYMBOL_GPL(local_clock);
> +
> +/*
> + * Running clock - returns the time that has elapsed while a guest has been
> + * running.
> + * On a guest this value should be local_clock minus the time the guest was
> + * suspended by the hypervisor (for any reason).
> + * On bare metal this function should return the same as local_clock.
> + * Architectures and sub-architectures can override this.
> + */
> +u64 __weak running_clock(void)
> +{
> +	return local_clock();
> +}
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 70bf118..3174bf8 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -154,7 +154,7 @@ static int get_softlockup_thresh(void)
>   */
>  static unsigned long get_timestamp(void)
>  {
> -	return local_clock() >> 30LL;  /* 2^30 ~= 10^9 */
> +	return running_clock() >> 30LL;  /* 2^30 ~= 10^9 */
>  }
>  
>  static void set_sample_period(void)