[PATCH 0/3] watchdog: kvm: disable hard lockup detection by default

[PATCH 0/3] watchdog: kvm: disable hard lockup detection by default

[Andrew Jones]

It's not recommended for KVM guests to enable hard lockup detection, as
false positives may be easily triggered by, for example, vcpu overcommit.
However any kernel compiled with HARDLOCKUP_DETECTOR that detects a PMU
on boot will by default enable hard lockup detection. This series gives
a kernel a mechanism to opt out of this default. Users can still force
hard lockup detection on using the kernel command line option
'nmi_watchdog=1'.

The first patch is a watchdog fix, and can be taken separately. The next
patch provides the default opt out mechanism, and the final patch applies
it to kvm guests.

Thanks in advance for reviews,
drew


Ulrich Obergfell (3):
  watchdog: fix print-once on enable
  watchdog: control hard lockup detection default
  kvm: ensure hard lockup detection is disabled by default

 arch/x86/kernel/kvm.c |  8 ++++++++
 include/linux/nmi.h   |  9 +++++++++
 kernel/watchdog.c     | 48 ++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 63 insertions(+), 2 deletions(-)

-- 
1.9.3

[PATCH 1/3] watchdog: fix print-once on enable

[Andrew Jones]

From: Ulrich Obergfell <uobergfe@redhat.com>

This patch avoids printing the message 'enabled on all CPUs, ...'
multiple times. For example, the issue can occur in the following
scenario:

1) watchdog_nmi_enable() fails to enable PMU counters and sets
   cpu0_err.

2) 'echo [0|1] > /proc/sys/kernel/nmi_watchdog' is executed to
   disable and re-enable the watchdog mechanism 'on the fly'.

3) If watchdog_nmi_enable() succeeds to enable PMU counters, each
   CPU will print the message because step1 left behind a non-zero
   cpu0_err.

   if (!IS_ERR(event)) {
       if (cpu == 0 || cpu0_err)
       pr_info("enabled on all CPUs, ...")

The patch avoids this by clearing cpu0_err in watchdog_nmi_disable().

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 kernel/watchdog.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index c3319bd1b0408..c985a21926545 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -511,6 +511,9 @@ static void watchdog_nmi_disable(unsigned int cpu)
 		/* should be in cleanup, but blocks oprofile */
 		perf_event_release_kernel(event);
 	}
+	if (cpu == 0)
+		/* watchdog_nmi_enable() expects this to be zero initially. */
+		cpu0_err = 0;
 	return;
 }
 #else
-- 
1.9.3

[PATCH 2/3] watchdog: control hard lockup detection default

[Andrew Jones]

From: Ulrich Obergfell <uobergfe@redhat.com>

In some cases we don't want hard lockup detection enabled by default.
An example is when running as a guest. Introduce

  watchdog_enable_hardlockup_detector(bool)

allowing those cases to disable hard lockup detection. This must be
executed early by the boot processor from e.g. smp_prepare_boot_cpu,
in order to allow kernel command line arguments to override it, as
well as to avoid hard lockup detection being enabled before we've
had a chance to indicate that it's unwanted. In summary,

  initial boot:					default=enabled
  smp_prepare_boot_cpu
    watchdog_enable_hardlockup_detector(false):	default=disabled
  cmdline has 'nmi_watchdog=1':			default=enabled

The running kernel still has the ability to enable/disable at any
time with /proc/sys/kernel/nmi_watchdog us usual. However even
when the default has been overridden /proc/sys/kernel/nmi_watchdog
will initially show '1'. To truly turn it on one must disable/enable
it, i.e.
  echo 0 > /proc/sys/kernel/nmi_watchdog
  echo 1 > /proc/sys/kernel/nmi_watchdog

This patch will be immediately useful for KVM with the next patch
of this series. Other hypervisor guest types may find it useful as
well.

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 include/linux/nmi.h |  9 +++++++++
 kernel/watchdog.c   | 45 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/include/linux/nmi.h b/include/linux/nmi.h
index 447775ee2c4b0..72aacf4e3d539 100644
--- a/include/linux/nmi.h
+++ b/include/linux/nmi.h
@@ -17,11 +17,20 @@
 #if defined(CONFIG_HAVE_NMI_WATCHDOG) || defined(CONFIG_HARDLOCKUP_DETECTOR)
 #include <asm/nmi.h>
 extern void touch_nmi_watchdog(void);
+extern void watchdog_enable_hardlockup_detector(bool val);
+extern bool watchdog_hardlockup_detector_is_enabled(void);
 #else
 static inline void touch_nmi_watchdog(void)
 {
 	touch_softlockup_watchdog();
 }
+static inline void watchdog_enable_hardlockup_detector(bool)
+{
+}
+static inline bool watchdog_hardlockup_detector_is_enabled(void)
+{
+	return true;
+}
 #endif
 
 /*
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index c985a21926545..34eca29e28a4c 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -63,6 +63,25 @@ static unsigned long soft_lockup_nmi_warn;
 static int hardlockup_panic =
 			CONFIG_BOOTPARAM_HARDLOCKUP_PANIC_VALUE;
 
+static bool hardlockup_detector_enabled = true;
+/*
+ * We may not want to enable hard lockup detection by default in all cases,
+ * for example when running the kernel as a guest on a hypervisor. In these
+ * cases this function can be called to disable hard lockup detection. This
+ * function should only be executed once by the boot processor before the
+ * kernel command line parameters are parsed, because otherwise it is not
+ * possible to override this in hardlockup_panic_setup().
+ */
+void watchdog_enable_hardlockup_detector(bool val)
+{
+	hardlockup_detector_enabled = val;
+}
+
+bool watchdog_hardlockup_detector_is_enabled(void)
+{
+	return hardlockup_detector_enabled;
+}
+
 static int __init hardlockup_panic_setup(char *str)
 {
 	if (!strncmp(str, "panic", 5))
@@ -71,6 +90,14 @@ static int __init hardlockup_panic_setup(char *str)
 		hardlockup_panic = 0;
 	else if (!strncmp(str, "0", 1))
 		watchdog_user_enabled = 0;
+	else if (!strncmp(str, "1", 1) || !strncmp(str, "2", 1)) {
+		/*
+		 * Setting 'nmi_watchdog=1' or 'nmi_watchdog=2' (legacy option)
+		 * has the same effect.
+		 */
+		watchdog_user_enabled = 1;
+		watchdog_enable_hardlockup_detector(true);
+	}
 	return 1;
 }
 __setup("nmi_watchdog=", hardlockup_panic_setup);
@@ -451,6 +478,15 @@ static int watchdog_nmi_enable(unsigned int cpu)
 	struct perf_event_attr *wd_attr;
 	struct perf_event *event = per_cpu(watchdog_ev, cpu);
 
+	/*
+	 * Some kernels need to default hard lockup detection to
+	 * 'disabled', for example a guest on a hypervisor.
+	 */
+	if (!watchdog_hardlockup_detector_is_enabled()) {
+		event = ERR_PTR(-ENOENT);
+		goto handle_err;
+	}
+
 	/* is it already setup and enabled? */
 	if (event && event->state > PERF_EVENT_STATE_OFF)
 		goto out;
@@ -465,6 +501,7 @@ static int watchdog_nmi_enable(unsigned int cpu)
 	/* Try to register using hardware perf events */
 	event = perf_event_create_kernel_counter(wd_attr, cpu, NULL, watchdog_overflow_callback, NULL);
 
+handle_err:
 	/* save cpu0 error for future comparision */
 	if (cpu == 0 && IS_ERR(event))
 		cpu0_err = PTR_ERR(event);
@@ -610,11 +647,13 @@ int proc_dowatchdog(struct ctl_table *table, int write,
 		    void __user *buffer, size_t *lenp, loff_t *ppos)
 {
 	int err, old_thresh, old_enabled;
+	bool old_hardlockup;
 	static DEFINE_MUTEX(watchdog_proc_mutex);
 
 	mutex_lock(&watchdog_proc_mutex);
 	old_thresh = ACCESS_ONCE(watchdog_thresh);
 	old_enabled = ACCESS_ONCE(watchdog_user_enabled);
+	old_hardlockup = watchdog_hardlockup_detector_is_enabled();
 
 	err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 	if (err || !write)
@@ -626,15 +665,17 @@ int proc_dowatchdog(struct ctl_table *table, int write,
 	 * disabled. The 'watchdog_running' variable check in
 	 * watchdog_*_all_cpus() function takes care of this.
 	 */
-	if (watchdog_user_enabled && watchdog_thresh)
+	if (watchdog_user_enabled && watchdog_thresh) {
+		watchdog_enable_hardlockup_detector(true);
 		err = watchdog_enable_all_cpus(old_thresh != watchdog_thresh);
-	else
+	} else
 		watchdog_disable_all_cpus();
 
 	/* Restore old values on failure */
 	if (err) {
 		watchdog_thresh = old_thresh;
 		watchdog_user_enabled = old_enabled;
+		watchdog_enable_hardlockup_detector(old_hardlockup);
 	}
 out:
 	mutex_unlock(&watchdog_proc_mutex);
-- 
1.9.3

Re: [PATCH 2/3] watchdog: control hard lockup detection default

[Paolo Bonzini]

Il 24/07/2014 12:13, Andrew Jones ha scritto:
> 
> The running kernel still has the ability to enable/disable at any
> time with /proc/sys/kernel/nmi_watchdog us usual. However even
> when the default has been overridden /proc/sys/kernel/nmi_watchdog
> will initially show '1'. To truly turn it on one must disable/enable
> it, i.e.
>   echo 0 > /proc/sys/kernel/nmi_watchdog
>   echo 1 > /proc/sys/kernel/nmi_watchdog

Why is it hard to make this show the right value? :)

Paolo

Re: [PATCH 2/3] watchdog: control hard lockup detection default

[Ulrich Obergfell]

> ----- Original Message -----
> From: "Paolo Bonzini" <pbonzini@redhat.com>
> To: "Andrew Jones" <drjones@redhat.com>, linux-kernel@vger.kernel.org, kvm@vger.kernel.org
> Cc: uobergfe@redhat.com, dzickus@redhat.com, akpm@linux-foundation.org, mingo@redhat.com
> Sent: Thursday, July 24, 2014 12:46:11 PM
> Subject: Re: [PATCH 2/3] watchdog: control hard lockup detection default
>
>Il 24/07/2014 12:13, Andrew Jones ha scritto:
>> 
>> The running kernel still has the ability to enable/disable at any
>> time with /proc/sys/kernel/nmi_watchdog us usual. However even
>> when the default has been overridden /proc/sys/kernel/nmi_watchdog
>> will initially show '1'. To truly turn it on one must disable/enable
>> it, i.e.
>>   echo 0 > /proc/sys/kernel/nmi_watchdog
>>   echo 1 > /proc/sys/kernel/nmi_watchdog
>
> Why is it hard to make this show the right value? :)
>
> Paolo

'echo 1 > /proc/sys/kernel/nmi_watchdog' enables both - hard lockup and
soft lockup detection. watchdog_enable_all_cpus() starts a 'watchdog/N'
thread for each CPU. If the kernel runs on a bare metal system where the
processor does not have a PMU, or when perf_event_create_kernel_counter()
returns failure to watchdog_nmi_enable(), or when the kernel runs as a
guest on a hypervisor that does not emulate a PMU, then the 'watchdog/N'
threads are still active for soft lockup detection. Patch 2/3 essentially
makes watchdog_nmi_enable() behave in the same way as if -ENOENT would
have been returned by perf_event_create_kernel_counter(). This is then
reported via a console message.

  NMI watchdog: disabled (cpu0): hardware events not enabled

It's hard say what _is_ 'the right value' (because lockup detection is
then enabled 'partially'), regardless of whether patch 2/3 is applied
or not.

Regards,

Uli

Re: [PATCH 2/3] watchdog: control hard lockup detection default

[Paolo Bonzini]

Il 24/07/2014 13:18, Ulrich Obergfell ha scritto:
>>> >> The running kernel still has the ability to enable/disable at any
>>> >> time with /proc/sys/kernel/nmi_watchdog us usual. However even
>>> >> when the default has been overridden /proc/sys/kernel/nmi_watchdog
>>> >> will initially show '1'. To truly turn it on one must disable/enable
>>> >> it, i.e.
>>> >>   echo 0 > /proc/sys/kernel/nmi_watchdog
>>> >>   echo 1 > /proc/sys/kernel/nmi_watchdog
>> >
>> > Why is it hard to make this show the right value? :)
>> >
>> > Paolo
> 'echo 1 > /proc/sys/kernel/nmi_watchdog' enables both - hard lockup and
> soft lockup detection. watchdog_enable_all_cpus() starts a 'watchdog/N'
> thread for each CPU. If the kernel runs on a bare metal system where the
> processor does not have a PMU, or when perf_event_create_kernel_counter()
> returns failure to watchdog_nmi_enable(), or when the kernel runs as a
> guest on a hypervisor that does not emulate a PMU, then the 'watchdog/N'
> threads are still active for soft lockup detection. Patch 2/3 essentially
> makes watchdog_nmi_enable() behave in the same way as if -ENOENT would
> have been returned by perf_event_create_kernel_counter(). This is then
> reported via a console message.
> 
>   NMI watchdog: disabled (cpu0): hardware events not enabled
> 
> It's hard say what _is_ 'the right value' (because lockup detection is
> then enabled 'partially'), regardless of whether patch 2/3 is applied
> or not.

But this means that it is not possible to re-enable softlockup detection
only.  I think that should be the effect of echo 0 + echo 1, if
hardlockup detection was disabled by either the command line or patch 3.

Paolo

Re: [PATCH 2/3] watchdog: control hard lockup detection default

[Ulrich Obergfell]

>----- Original Message -----
>From: "Paolo Bonzini" <pbonzini@redhat.com>
>To: "Ulrich Obergfell" <uobergfe@redhat.com>
>Cc: "Andrew Jones" <drjones@redhat.com>, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, dzickus@redhat.com, akpm@linux-foundation.org, >mingo@redhat.com
>Sent: Thursday, July 24, 2014 1:26:40 PM
>Subject: Re: [PATCH 2/3] watchdog: control hard lockup detection default
>
>Il 24/07/2014 13:18, Ulrich Obergfell ha scritto:
>>>> >> The running kernel still has the ability to enable/disable at any
>>>> >> time with /proc/sys/kernel/nmi_watchdog us usual. However even
>>>> >> when the default has been overridden /proc/sys/kernel/nmi_watchdog
>>>> >> will initially show '1'. To truly turn it on one must disable/enable
>>>> >> it, i.e.
>>>> >>   echo 0 > /proc/sys/kernel/nmi_watchdog
>>>> >>   echo 1 > /proc/sys/kernel/nmi_watchdog
>>> >
>>> > Why is it hard to make this show the right value? :)
>>> >
>>> > Paolo
>> 'echo 1 > /proc/sys/kernel/nmi_watchdog' enables both - hard lockup and
>> soft lockup detection. watchdog_enable_all_cpus() starts a 'watchdog/N'
>> thread for each CPU. If the kernel runs on a bare metal system where the
>> processor does not have a PMU, or when perf_event_create_kernel_counter()
>> returns failure to watchdog_nmi_enable(), or when the kernel runs as a
>> guest on a hypervisor that does not emulate a PMU, then the 'watchdog/N'
>> threads are still active for soft lockup detection. Patch 2/3 essentially
>> makes watchdog_nmi_enable() behave in the same way as if -ENOENT would
>> have been returned by perf_event_create_kernel_counter(). This is then
>> reported via a console message.
>> 
>>   NMI watchdog: disabled (cpu0): hardware events not enabled
>> 
>> It's hard say what _is_ 'the right value' (because lockup detection is
>> then enabled 'partially'), regardless of whether patch 2/3 is applied
>> or not.
>
> But this means that it is not possible to re-enable softlockup detection
> only.  I think that should be the effect of echo 0 + echo 1, if
> hardlockup detection was disabled by either the command line or patch 3.
>
> Paolo

The idea was to give the user two options to override the effect of patch 3/3.
Either via the kernel command line ('nmi_watchdog=') at boot time or via /proc
('echo 0' + 'echo 1') when the system is up and running.

Regards,

Uli

Re: [PATCH 2/3] watchdog: control hard lockup detection default

[Paolo Bonzini]

Il 24/07/2014 13:44, Ulrich Obergfell ha scritto:
> > But this means that it is not possible to re-enable softlockup detection
> > only.  I think that should be the effect of echo 0 + echo 1, if
> > hardlockup detection was disabled by either the command line or patch 3.
>
> The idea was to give the user two options to override the effect of patch 3/3.
> Either via the kernel command line ('nmi_watchdog=') at boot time or via /proc
> ('echo 0' + 'echo 1') when the system is up and running.

I think the kernel command line is enough; another alternative is to
split the nmi_watchdog /proc entry in two.

Paolo

Re: [PATCH 2/3] watchdog: control hard lockup detection default

[Ulrich Obergfell]

>----- Original Message -----
>From: "Paolo Bonzini" <pbonzini@redhat.com>
>To: "Ulrich Obergfell" <uobergfe@redhat.com>
>Cc: "Andrew Jones" <drjones@redhat.com>, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, dzickus@redhat.com, akpm@linux-foundation.org, >mingo@redhat.com
>Sent: Thursday, July 24, 2014 1:45:47 PM
>Subject: Re: [PATCH 2/3] watchdog: control hard lockup detection default
>
>Il 24/07/2014 13:44, Ulrich Obergfell ha scritto:
>> > But this means that it is not possible to re-enable softlockup detection
>> > only.  I think that should be the effect of echo 0 + echo 1, if
>> > hardlockup detection was disabled by either the command line or patch 3.
>>
>> The idea was to give the user two options to override the effect of patch 3/3.
>> Either via the kernel command line ('nmi_watchdog=') at boot time or via /proc
>> ('echo 0' + 'echo 1') when the system is up and running.
>
> I think the kernel command line is enough; another alternative is to
> split the nmi_watchdog /proc entry in two.
>
> Paolo

The current behaviour (without the patch) already allows a user to disable
NMI watchdog at boot time ('nmi_watchdog=0') and enable it explicitly when
the system is up and running ('echo 0' + 'echo 1'). I think it would be
more consistent with this behaviour and more intuitive if we would give
the user the option to override the effect of patch 3/3 via /proc. By
'intuitive' I mean that the user says: 'I _want_ this to be enabled'.

Regards,

Uli

Re: [PATCH 2/3] watchdog: control hard lockup detection default

[Ulrich Obergfell]

> ----- Original Message -----
> From: "Andrew Jones" <drjones@redhat.com>
> To: linux-kernel@vger.kernel.org, kvm@vger.kernel.org
> Cc: uobergfe@redhat.com, dzickus@redhat.com, pbonzini@redhat.com, akpm@linux-foundation.org, mingo@redhat.com
> Sent: Thursday, July 24, 2014 12:13:30 PM
> Subject: [PATCH 2/3] watchdog: control hard lockup detection default

[...]

> The running kernel still has the ability to enable/disable at any
> time with /proc/sys/kernel/nmi_watchdog us usual. However even
> when the default has been overridden /proc/sys/kernel/nmi_watchdog
> will initially show '1'. To truly turn it on one must disable/enable
> it, i.e.
>   echo 0 > /proc/sys/kernel/nmi_watchdog
>   echo 1 > /proc/sys/kernel/nmi_watchdog

[...]

> @@ -626,15 +665,17 @@ int proc_dowatchdog(struct ctl_table *table, int write,
>          * disabled. The 'watchdog_running' variable check in
>          * watchdog_*_all_cpus() function takes care of this.
>          */
> -        if (watchdog_user_enabled && watchdog_thresh)
> +        if (watchdog_user_enabled && watchdog_thresh) {
> +                watchdog_enable_hardlockup_detector(true);
>                  err = watchdog_enable_all_cpus(old_thresh != watchdog_thresh);
> -        else
> +        } else

[...]


I just realized a possible issue in the above part of the patch:

If we would want to give the user the option to override the effect of patch 3/3
via /proc, I think proc_dowatchdog() should enable hard lockup detection _only_
in case of a state transition from 'NOT watchdog_running' to 'watchdog_running'.
                                                          |
   if (watchdog_user_enabled && watchdog_thresh) {        | need to add this
       if (!watchdog_running) <---------------------------'
           watchdog_enable_hardlockup_detector(true);
       err = watchdog_enable_all_cpus(old_thresh != watchdog_thresh);
   } else
       ...

The additional 'if (!watchdog_running)' would _require_ the user to perform the
sequence of commands

   echo 0 > /proc/sys/kernel/nmi_watchdog
   echo 1 > /proc/sys/kernel/nmi_watchdog

to enable hard lockup detection explicitly.

I think changing the 'watchdog_thresh' while 'watchdog_running' is true should
_not_ enable hard lockup detection as a side-effect, because a user may have a
'sysctl.conf' entry such as

   kernel.watchdog_thresh = ...

or may only want to change the 'watchdog_thresh' on the fly.

I think the following flow of execution could cause such undesired side-effect.

   proc_dowatchdog
     if (watchdog_user_enabled && watchdog_thresh) {

         watchdog_enable_hardlockup_detector
           hardlockup_detector_enabled = true

         watchdog_enable_all_cpus
           if (!watchdog_running) {
               ...
           } else if (sample_period_changed)
                      update_timers_all_cpus
                        for_each_online_cpu
                            update_timers
                              watchdog_nmi_disable
                              ...
                              watchdog_nmi_enable

                                watchdog_hardlockup_detector_is_enabled
                                  return true

                                enable perf counter for hard lockup detection

Regards,

Uli

Re: [PATCH 2/3] watchdog: control hard lockup detection default

[Andrew Jones]

On Fri, Jul 25, 2014 at 04:32:55AM -0400, Ulrich Obergfell wrote:
> > ----- Original Message -----
> > From: "Andrew Jones" <drjones@redhat.com>
> > To: linux-kernel@vger.kernel.org, kvm@vger.kernel.org
> > Cc: uobergfe@redhat.com, dzickus@redhat.com, pbonzini@redhat.com, akpm@linux-foundation.org, mingo@redhat.com
> > Sent: Thursday, July 24, 2014 12:13:30 PM
> > Subject: [PATCH 2/3] watchdog: control hard lockup detection default
> 
> [...]
> 
> > The running kernel still has the ability to enable/disable at any
> > time with /proc/sys/kernel/nmi_watchdog us usual. However even
> > when the default has been overridden /proc/sys/kernel/nmi_watchdog
> > will initially show '1'. To truly turn it on one must disable/enable
> > it, i.e.
> >   echo 0 > /proc/sys/kernel/nmi_watchdog
> >   echo 1 > /proc/sys/kernel/nmi_watchdog
> 
> [...]
> 
> > @@ -626,15 +665,17 @@ int proc_dowatchdog(struct ctl_table *table, int write,
> >          * disabled. The 'watchdog_running' variable check in
> >          * watchdog_*_all_cpus() function takes care of this.
> >          */
> > -        if (watchdog_user_enabled && watchdog_thresh)
> > +        if (watchdog_user_enabled && watchdog_thresh) {
> > +                watchdog_enable_hardlockup_detector(true);
> >                  err = watchdog_enable_all_cpus(old_thresh != watchdog_thresh);
> > -        else
> > +        } else
> 
> [...]
> 
> 
> I just realized a possible issue in the above part of the patch:
> 
> If we would want to give the user the option to override the effect of patch 3/3
> via /proc, I think proc_dowatchdog() should enable hard lockup detection _only_
> in case of a state transition from 'NOT watchdog_running' to 'watchdog_running'.
>                                                           |
>    if (watchdog_user_enabled && watchdog_thresh) {        | need to add this
>        if (!watchdog_running) <---------------------------'
>            watchdog_enable_hardlockup_detector(true);
>        err = watchdog_enable_all_cpus(old_thresh != watchdog_thresh);
>    } else
>        ...
> 
> The additional 'if (!watchdog_running)' would _require_ the user to perform the
> sequence of commands
> 
>    echo 0 > /proc/sys/kernel/nmi_watchdog
>    echo 1 > /proc/sys/kernel/nmi_watchdog
> 
> to enable hard lockup detection explicitly.
> 
> I think changing the 'watchdog_thresh' while 'watchdog_running' is true should
> _not_ enable hard lockup detection as a side-effect, because a user may have a
> 'sysctl.conf' entry such as
> 
>    kernel.watchdog_thresh = ...
> 
> or may only want to change the 'watchdog_thresh' on the fly.
> 
> I think the following flow of execution could cause such undesired side-effect.
> 
>    proc_dowatchdog
>      if (watchdog_user_enabled && watchdog_thresh) {
> 
>          watchdog_enable_hardlockup_detector
>            hardlockup_detector_enabled = true
> 
>          watchdog_enable_all_cpus
>            if (!watchdog_running) {
>                ...
>            } else if (sample_period_changed)
>                       update_timers_all_cpus
>                         for_each_online_cpu
>                             update_timers
>                               watchdog_nmi_disable
>                               ...
>                               watchdog_nmi_enable
> 
>                                 watchdog_hardlockup_detector_is_enabled
>                                   return true
> 
>                                 enable perf counter for hard lockup detection
> 
> Regards,
> 
> Uli

Nice catch. Looks like this will need a v2. Paolo, do we have a
consensus on the proc echoing? Or should that be revisited in the v2 as
well?

drew

Re: [PATCH 2/3] watchdog: control hard lockup detection default

[Don Zickus]

On Fri, Jul 25, 2014 at 01:25:11PM +0200, Andrew Jones wrote:
> > to enable hard lockup detection explicitly.
> > 
> > I think changing the 'watchdog_thresh' while 'watchdog_running' is true should
> > _not_ enable hard lockup detection as a side-effect, because a user may have a
> > 'sysctl.conf' entry such as
> > 
> >    kernel.watchdog_thresh = ...
> > 
> > or may only want to change the 'watchdog_thresh' on the fly.
> > 
> > I think the following flow of execution could cause such undesired side-effect.
> > 
> >    proc_dowatchdog
> >      if (watchdog_user_enabled && watchdog_thresh) {
> > 
> >          watchdog_enable_hardlockup_detector
> >            hardlockup_detector_enabled = true
> > 
> >          watchdog_enable_all_cpus
> >            if (!watchdog_running) {
> >                ...
> >            } else if (sample_period_changed)
> >                       update_timers_all_cpus
> >                         for_each_online_cpu
> >                             update_timers
> >                               watchdog_nmi_disable
> >                               ...
> >                               watchdog_nmi_enable
> > 
> >                                 watchdog_hardlockup_detector_is_enabled
> >                                   return true
> > 
> >                                 enable perf counter for hard lockup detection
> > 
> > Regards,
> > 
> > Uli
> 
> Nice catch. Looks like this will need a v2. Paolo, do we have a
> consensus on the proc echoing? Or should that be revisited in the v2 as
> well?

As discussed privately, how about something like this to handle that case:
(applied on top of these patches)

Cheers,
Don

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 34eca29..027fb6c 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -666,7 +666,12 @@ int proc_dowatchdog(struct ctl_table *table, int write,
 	 * watchdog_*_all_cpus() function takes care of this.
 	 */
 	if (watchdog_user_enabled && watchdog_thresh) {
-		watchdog_enable_hardlockup_detector(true);
+		/*
+		 * Prevent a change in watchdog_thresh accidentally overriding
+		 * the enablement of the hardlockup detector.
+		 */
+		if (watchdog_user_enabled != old_enabled)
+			watchdog_enable_hardlockup_detector(true);
 		err = watchdog_enable_all_cpus(old_thresh != watchdog_thresh);
 	} else
 		watchdog_disable_all_cpus();

Re: [PATCH 2/3] watchdog: control hard lockup detection default

[Paolo Bonzini]

Il 30/07/2014 15:43, Don Zickus ha scritto:
>> > Nice catch. Looks like this will need a v2. Paolo, do we have a
>> > consensus on the proc echoing? Or should that be revisited in the v2 as
>> > well?
> As discussed privately, how about something like this to handle that case:
> (applied on top of these patches)

Don, what do you think about proc?

My opinion is still what I mentioned earlier in the thread, i.e. that if
the file says "1", writing "0" and then "1" should not constitute a
change WRT to the initial state.

Paolo

Re: [PATCH 2/3] watchdog: control hard lockup detection default

[Don Zickus]

On Wed, Jul 30, 2014 at 04:16:38PM +0200, Paolo Bonzini wrote:
> Il 30/07/2014 15:43, Don Zickus ha scritto:
> >> > Nice catch. Looks like this will need a v2. Paolo, do we have a
> >> > consensus on the proc echoing? Or should that be revisited in the v2 as
> >> > well?
> > As discussed privately, how about something like this to handle that case:
> > (applied on top of these patches)
> 
> Don, what do you think about proc?
> 
> My opinion is still what I mentioned earlier in the thread, i.e. that if
> the file says "1", writing "0" and then "1" should not constitute a
> change WRT to the initial state.
> 

I can agree.  The problem is there are two things this proc value
controls, softlockup and hardlockup.  I have always tried to keep the both
disabled or enabled together.

This patchset tries to separate them for an edge case.  Hence the proc
value becomes slightly confusing.

I don't know the right way to solve this without introducing more proc
values.

We have /proc/sys/kernel/nmi_watchdog and /proc/sys/kernel/watchdog which
point to the same internal variable.  Do I separate them and have
'nmi_watchdog' just mean hardlockup and 'watchdog' mean softlockup?  Then
we can be clear on what the output is.  Or does 'watchdog' represent a
superset of 'nmi_watchdog' && softlockup?

That is where the confusion lies.

Cheers,
Don

[PATCH v2 2/3] watchdog: control hard lockup detection default

[Andrew Jones]

From: Ulrich Obergfell <uobergfe@redhat.com>

In some cases we don't want hard lockup detection enabled by default.
An example is when running as a guest. Introduce

  watchdog_enable_hardlockup_detector(bool)

allowing those cases to disable hard lockup detection. This must be
executed early by the boot processor from e.g. smp_prepare_boot_cpu,
in order to allow kernel command line arguments to override it, as
well as to avoid hard lockup detection being enabled before we've
had a chance to indicate that it's unwanted. In summary,

  initial boot:					default=enabled
  smp_prepare_boot_cpu
    watchdog_enable_hardlockup_detector(false):	default=disabled
  cmdline has 'nmi_watchdog=1':			default=enabled

The running kernel still has the ability to enable/disable at any
time with /proc/sys/kernel/nmi_watchdog us usual. However even
when the default has been overridden /proc/sys/kernel/nmi_watchdog
will initially show '1'. To truly turn it on one must disable/enable
it, i.e.
  echo 0 > /proc/sys/kernel/nmi_watchdog
  echo 1 > /proc/sys/kernel/nmi_watchdog

This patch will be immediately useful for KVM with the next patch
of this series. Other hypervisor guest types may find it useful as
well.

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>

---
v2: don't override hardlockup detector setting when just changing
    the threshold [Uli and Don]
---
 include/linux/nmi.h |  9 +++++++++
 kernel/watchdog.c   | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/include/linux/nmi.h b/include/linux/nmi.h
index 447775ee2c4b0..72aacf4e3d539 100644
--- a/include/linux/nmi.h
+++ b/include/linux/nmi.h
@@ -17,11 +17,20 @@
 #if defined(CONFIG_HAVE_NMI_WATCHDOG) || defined(CONFIG_HARDLOCKUP_DETECTOR)
 #include <asm/nmi.h>
 extern void touch_nmi_watchdog(void);
+extern void watchdog_enable_hardlockup_detector(bool val);
+extern bool watchdog_hardlockup_detector_is_enabled(void);
 #else
 static inline void touch_nmi_watchdog(void)
 {
 	touch_softlockup_watchdog();
 }
+static inline void watchdog_enable_hardlockup_detector(bool)
+{
+}
+static inline bool watchdog_hardlockup_detector_is_enabled(void)
+{
+	return true;
+}
 #endif
 
 /*
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index c985a21926545..027fb6cfa4638 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -63,6 +63,25 @@ static unsigned long soft_lockup_nmi_warn;
 static int hardlockup_panic =
 			CONFIG_BOOTPARAM_HARDLOCKUP_PANIC_VALUE;
 
+static bool hardlockup_detector_enabled = true;
+/*
+ * We may not want to enable hard lockup detection by default in all cases,
+ * for example when running the kernel as a guest on a hypervisor. In these
+ * cases this function can be called to disable hard lockup detection. This
+ * function should only be executed once by the boot processor before the
+ * kernel command line parameters are parsed, because otherwise it is not
+ * possible to override this in hardlockup_panic_setup().
+ */
+void watchdog_enable_hardlockup_detector(bool val)
+{
+	hardlockup_detector_enabled = val;
+}
+
+bool watchdog_hardlockup_detector_is_enabled(void)
+{
+	return hardlockup_detector_enabled;
+}
+
 static int __init hardlockup_panic_setup(char *str)
 {
 	if (!strncmp(str, "panic", 5))
@@ -71,6 +90,14 @@ static int __init hardlockup_panic_setup(char *str)
 		hardlockup_panic = 0;
 	else if (!strncmp(str, "0", 1))
 		watchdog_user_enabled = 0;
+	else if (!strncmp(str, "1", 1) || !strncmp(str, "2", 1)) {
+		/*
+		 * Setting 'nmi_watchdog=1' or 'nmi_watchdog=2' (legacy option)
+		 * has the same effect.
+		 */
+		watchdog_user_enabled = 1;
+		watchdog_enable_hardlockup_detector(true);
+	}
 	return 1;
 }
 __setup("nmi_watchdog=", hardlockup_panic_setup);
@@ -451,6 +478,15 @@ static int watchdog_nmi_enable(unsigned int cpu)
 	struct perf_event_attr *wd_attr;
 	struct perf_event *event = per_cpu(watchdog_ev, cpu);
 
+	/*
+	 * Some kernels need to default hard lockup detection to
+	 * 'disabled', for example a guest on a hypervisor.
+	 */
+	if (!watchdog_hardlockup_detector_is_enabled()) {
+		event = ERR_PTR(-ENOENT);
+		goto handle_err;
+	}
+
 	/* is it already setup and enabled? */
 	if (event && event->state > PERF_EVENT_STATE_OFF)
 		goto out;
@@ -465,6 +501,7 @@ static int watchdog_nmi_enable(unsigned int cpu)
 	/* Try to register using hardware perf events */
 	event = perf_event_create_kernel_counter(wd_attr, cpu, NULL, watchdog_overflow_callback, NULL);
 
+handle_err:
 	/* save cpu0 error for future comparision */
 	if (cpu == 0 && IS_ERR(event))
 		cpu0_err = PTR_ERR(event);
@@ -610,11 +647,13 @@ int proc_dowatchdog(struct ctl_table *table, int write,
 		    void __user *buffer, size_t *lenp, loff_t *ppos)
 {
 	int err, old_thresh, old_enabled;
+	bool old_hardlockup;
 	static DEFINE_MUTEX(watchdog_proc_mutex);
 
 	mutex_lock(&watchdog_proc_mutex);
 	old_thresh = ACCESS_ONCE(watchdog_thresh);
 	old_enabled = ACCESS_ONCE(watchdog_user_enabled);
+	old_hardlockup = watchdog_hardlockup_detector_is_enabled();
 
 	err = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
 	if (err || !write)
@@ -626,15 +665,22 @@ int proc_dowatchdog(struct ctl_table *table, int write,
 	 * disabled. The 'watchdog_running' variable check in
 	 * watchdog_*_all_cpus() function takes care of this.
 	 */
-	if (watchdog_user_enabled && watchdog_thresh)
+	if (watchdog_user_enabled && watchdog_thresh) {
+		/*
+		 * Prevent a change in watchdog_thresh accidentally overriding
+		 * the enablement of the hardlockup detector.
+		 */
+		if (watchdog_user_enabled != old_enabled)
+			watchdog_enable_hardlockup_detector(true);
 		err = watchdog_enable_all_cpus(old_thresh != watchdog_thresh);
-	else
+	} else
 		watchdog_disable_all_cpus();
 
 	/* Restore old values on failure */
 	if (err) {
 		watchdog_thresh = old_thresh;
 		watchdog_user_enabled = old_enabled;
+		watchdog_enable_hardlockup_detector(old_hardlockup);
 	}
 out:
 	mutex_unlock(&watchdog_proc_mutex);
-- 
1.9.3

[PATCH 3/3] kvm: ensure hard lockup detection is disabled by default

[Andrew Jones]

From: Ulrich Obergfell <uobergfe@redhat.com>

Use watchdog_enable_hardlockup_detector() to set hard lockup detection's
default value to false. It's risky to run this detection in a guest, as
false positives are easy to trigger, especially if the host is
overcommitted.

Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arch/x86/kernel/kvm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 3dd8e2c4d74a9..95c3cb16af3e5 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -35,6 +35,7 @@
 #include <linux/slab.h>
 #include <linux/kprobes.h>
 #include <linux/debugfs.h>
+#include <linux/nmi.h>
 #include <asm/timer.h>
 #include <asm/cpu.h>
 #include <asm/traps.h>
@@ -499,6 +500,13 @@ void __init kvm_guest_init(void)
 #else
 	kvm_guest_cpu_init();
 #endif
+
+	/*
+	 * Hard lockup detection is enabled by default. Disable it, as guests
+	 * can get false positives too easily, for example if the host is
+	 * overcommitted.
+	 */
+	watchdog_enable_hardlockup_detector(false);
 }
 
 static noinline uint32_t __kvm_cpuid_base(void)
-- 
1.9.3