Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1671736 > unrolled thread

[PATCH V2] kernel/watchdog: fix spurious hard lockups

Back to article view | Back to linux.kernel

Contents

  [PATCH V2] kernel/watchdog: fix spurious hard lockups kan.liang@intel.com - 2017-06-21 16:50 +0200
    Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Thomas Gleixner <tglx@linutronix.de> - 2017-06-21 17:20 +0200
      RE: [PATCH V2] kernel/watchdog: fix spurious hard lockups "Liang, Kan" <kan.liang@intel.com> - 2017-06-21 19:10 +0200
        Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Prarit Bhargava <prarit@redhat.com> - 2017-06-21 19:50 +0200
      Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Andi Kleen <ak@linux.intel.com> - 2017-06-21 19:10 +0200
        Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Thomas Gleixner <tglx@linutronix.de> - 2017-06-21 22:10 +0200
    Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Thomas Gleixner <tglx@linutronix.de> - 2017-06-22 00:00 +0200
      Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Thomas Gleixner <tglx@linutronix.de> - 2017-06-22 17:40 +0200
      Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Don Zickus <dzickus@redhat.com> - 2017-06-22 17:50 +0200
        RE: [PATCH V2] kernel/watchdog: fix spurious hard lockups "Liang, Kan" <kan.liang@intel.com> - 2017-06-22 18:00 +0200
        Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Thomas Gleixner <tglx@linutronix.de> - 2017-06-23 10:10 +0200
          Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Don Zickus <dzickus@redhat.com> - 2017-06-23 18:30 +0200
            Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Thomas Gleixner <tglx@linutronix.de> - 2017-06-24 00:00 +0200
              Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Don Zickus <dzickus@redhat.com> - 2017-06-26 22:30 +0200
                Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Thomas Gleixner <tglx@linutronix.de> - 2017-06-26 22:40 +0200
                Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Don Zickus <dzickus@redhat.com> - 2017-06-27 22:20 +0200
                  RE: [PATCH V2] kernel/watchdog: fix spurious hard lockups "Liang, Kan" <kan.liang@intel.com> - 2017-06-27 22:50 +0200
                    Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Don Zickus <dzickus@redhat.com> - 2017-06-27 23:10 +0200
                    Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Andi Kleen <ak@linux.intel.com> - 2017-06-28 01:50 +0200
                      Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Don Zickus <dzickus@redhat.com> - 2017-06-28 21:10 +0200
                        Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Andi Kleen <ak@linux.intel.com> - 2017-06-28 22:20 +0200
                          Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Don Zickus <dzickus@redhat.com> - 2017-06-29 17:50 +0200
                            Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Andi Kleen <ak@linux.intel.com> - 2017-06-29 18:20 +0200
                              Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Don Zickus <dzickus@redhat.com> - 2017-06-29 18:30 +0200
                                Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups Andi Kleen <ak@linux.intel.com> - 2017-06-29 18:40 +0200

Page 1 of 2  [1] 2  Next page →

#1671736 — [PATCH V2] kernel/watchdog: fix spurious hard lockups

From: Kan Liang <Kan.liang@intel.com>

Some users reported spurious NMI watchdog timeouts.

We now have more and more systems where the Turbo range is wide enough
that the NMI watchdog expires faster than the soft watchdog timer that
updates the interrupt tick the NMI watchdog relies on.

This problem was originally added by commit 58687acba592
("lockup_detector: Combine nmi_watchdog and softlockup detector").
Previously the NMI watchdog would always check jiffies, which were
ticking fast enough. But now the backing is quite slow so the expire
time becomes more sensitive.

For mainline the right fix is to switch the NMI watchdog to reference
cycles, which tick always at the same rate independent of turbo mode.
But this is requires some complicated changes in perf, which are too
difficult to backport. Since we need a stable fix to just increase the
NMI watchdog rate here to avoid the spurious timeouts. This is not an
ideal fix because a 3x as large Turbo range could still fail, but for
now that's not likely.

Signed-off-by: Kan Liang <Kan.liang@intel.com>
Cc: stable@vger.kernel.org
Fixes: 58687acba592 ("lockup_detector: Combine nmi_watchdog and
softlockup detector")
---

The right fix for mainline can be found here.
perf/x86/intel: enable CPU ref_cycles for GP counter
perf/x86/intel, watchdog: Switch NMI watchdog to ref cycles on x86
https://patchwork.kernel.org/patch/9779087/
https://patchwork.kernel.org/patch/9779089/

Change since V1:
 - Restrict the period in hw_nmi.c for Intel platform. (Don Zickus)

 arch/x86/kernel/apic/hw_nmi.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
index c73c9fb281e1..716d44e986f9 100644
--- a/arch/x86/kernel/apic/hw_nmi.c
+++ b/arch/x86/kernel/apic/hw_nmi.c
@@ -20,9 +20,15 @@
 #include <linux/delay.h>
 
 #ifdef CONFIG_HARDLOCKUP_DETECTOR
+/*
+ * The NMI watchdog relies on PERF_COUNT_HW_CPU_CYCLES event, which
+ * can tick faster than the measured CPU Frequency due to Turbo mode.
+ * That can lead to spurious timeouts.
+ * To workaround the issue, extending the period by 3 times.
+ */
 u64 hw_nmi_get_sample_period(int watchdog_thresh)
 {
-	return (u64)(cpu_khz) * 1000 * watchdog_thresh;
+	return (u64)(cpu_khz) * 1000 * watchdog_thresh * 3;
 }
 #endif
 
-- 
2.11.0

[toc] | [next] | [standalone]

#1671748

On Wed, 21 Jun 2017, kan.liang@intel.com wrote:
>  
>  #ifdef CONFIG_HARDLOCKUP_DETECTOR
> +/*
> + * The NMI watchdog relies on PERF_COUNT_HW_CPU_CYCLES event, which
> + * can tick faster than the measured CPU Frequency due to Turbo mode.
> + * That can lead to spurious timeouts.
> + * To workaround the issue, extending the period by 3 times.
> + */
>  u64 hw_nmi_get_sample_period(int watchdog_thresh)
>  {
> -	return (u64)(cpu_khz) * 1000 * watchdog_thresh;
> +	return (u64)(cpu_khz) * 1000 * watchdog_thresh * 3;

The maximum turbo frequency of any given machine can be retrieved.

So why don't you simply take that ratio into account and apply it for the
machines which have those insane turbo loaders? That's not a huge effort,
can be easily backported and does not inflict this unconditially.

So what you want is:

	return get_max_turbo_khz() * 1000 * watchdog_thresh;

Where get_max_turbo_khz() by default returns cpu_khz for non turbo
motors.

And instead of silently doing this it should emit a info into dmesg:

   	u64 period, max_khz = get_max_turbo_khz();
	static int once;

	period = max_khz * 1000 * watchdog_thresh;

	if (max_khz != cpu_khz && !once) {
		unsigned int msec = period / cpu_khz;

		once = 1;
		pr_info("Adjusted watchdog threshold to %u.%04u sec\n",
			msec / 1000, msec % 1000);
	}

	return period;

Hmm?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1671824

> On Wed, 21 Jun 2017, kan.liang@intel.com wrote:
> >
> >  #ifdef CONFIG_HARDLOCKUP_DETECTOR
> > +/*
> > + * The NMI watchdog relies on PERF_COUNT_HW_CPU_CYCLES event,
> which
> > + * can tick faster than the measured CPU Frequency due to Turbo mode.
> > + * That can lead to spurious timeouts.
> > + * To workaround the issue, extending the period by 3 times.
> > + */
> >  u64 hw_nmi_get_sample_period(int watchdog_thresh)  {
> > -	return (u64)(cpu_khz) * 1000 * watchdog_thresh;
> > +	return (u64)(cpu_khz) * 1000 * watchdog_thresh * 3;
> 
> The maximum turbo frequency of any given machine can be retrieved.

The maximum turbo frequency is determined by the model of processor.
I'm not sure if there is a generic way to get the maximum turbo frequency.
Is there?

Thanks,
Kan

> 
> So why don't you simply take that ratio into account and apply it for the
> machines which have those insane turbo loaders? That's not a huge effort,
> can be easily backported and does not inflict this unconditially.
> 
> So what you want is:
> 
> 	return get_max_turbo_khz() * 1000 * watchdog_thresh;
> 
> Where get_max_turbo_khz() by default returns cpu_khz for non turbo
> motors.
> 
> And instead of silently doing this it should emit a info into dmesg:
> 
>    	u64 period, max_khz = get_max_turbo_khz();
> 	static int once;
> 
> 	period = max_khz * 1000 * watchdog_thresh;
> 
> 	if (max_khz != cpu_khz && !once) {
> 		unsigned int msec = period / cpu_khz;
> 
> 		once = 1;
> 		pr_info("Adjusted watchdog threshold to %u.%04u sec\n",
> 			msec / 1000, msec % 1000);
> 	}
> 
> 	return period;
> 
> Hmm?
> 
> Thanks,
> 
> 	tglx

[toc] | [prev] | [next] | [standalone]

#1671854

On 06/21/2017 11:47 AM, Liang, Kan wrote:
> 
> 
>> On Wed, 21 Jun 2017, kan.liang@intel.com wrote:
>>>
>>>  #ifdef CONFIG_HARDLOCKUP_DETECTOR
>>> +/*
>>> + * The NMI watchdog relies on PERF_COUNT_HW_CPU_CYCLES event,
>> which
>>> + * can tick faster than the measured CPU Frequency due to Turbo mode.
>>> + * That can lead to spurious timeouts.
>>> + * To workaround the issue, extending the period by 3 times.
>>> + */
>>>  u64 hw_nmi_get_sample_period(int watchdog_thresh)  {
>>> -	return (u64)(cpu_khz) * 1000 * watchdog_thresh;
>>> +	return (u64)(cpu_khz) * 1000 * watchdog_thresh * 3;
>>
>> The maximum turbo frequency of any given machine can be retrieved.
> 
> The maximum turbo frequency is determined by the model of processor.
> I'm not sure if there is a generic way to get the maximum turbo frequency.
> Is there?
> 

cpufreq_quick_get_max()

but iff cpufreq subsystem is initialized.  O/w 0 is returned for the freq.

Quick test shows the correct turbo max of 3700000 on my 2000000 (2.00GHz)
system.

P.

> Thanks,
> Kan
> 
>>
>> So why don't you simply take that ratio into account and apply it for the
>> machines which have those insane turbo loaders? That's not a huge effort,
>> can be easily backported and does not inflict this unconditially.
>>
>> So what you want is:
>>
>> 	return get_max_turbo_khz() * 1000 * watchdog_thresh;
>>
>> Where get_max_turbo_khz() by default returns cpu_khz for non turbo
>> motors.
>>
>> And instead of silently doing this it should emit a info into dmesg:
>>
>>    	u64 period, max_khz = get_max_turbo_khz();
>> 	static int once;
>>
>> 	period = max_khz * 1000 * watchdog_thresh;
>>
>> 	if (max_khz != cpu_khz && !once) {
>> 		unsigned int msec = period / cpu_khz;
>>
>> 		once = 1;
>> 		pr_info("Adjusted watchdog threshold to %u.%04u sec\n",
>> 			msec / 1000, msec % 1000);
>> 	}
>>
>> 	return period;
>>

>> Hmm?
>>
>> Thanks,
>>
>> 	tglx
> 

[toc] | [prev] | [next] | [standalone]

#1671827

On Wed, Jun 21, 2017 at 05:12:06PM +0200, Thomas Gleixner wrote:
> On Wed, 21 Jun 2017, kan.liang@intel.com wrote:
> >  
> >  #ifdef CONFIG_HARDLOCKUP_DETECTOR
> > +/*
> > + * The NMI watchdog relies on PERF_COUNT_HW_CPU_CYCLES event, which
> > + * can tick faster than the measured CPU Frequency due to Turbo mode.
> > + * That can lead to spurious timeouts.
> > + * To workaround the issue, extending the period by 3 times.
> > + */
> >  u64 hw_nmi_get_sample_period(int watchdog_thresh)
> >  {
> > -	return (u64)(cpu_khz) * 1000 * watchdog_thresh;
> > +	return (u64)(cpu_khz) * 1000 * watchdog_thresh * 3;
> 
> The maximum turbo frequency of any given machine can be retrieved.

Not reliably, e.g. not in virtualization. Also it would require
model specific checks, so as soon as you have a new model and an
old kernel it could still randomly fail.

-Andi

[toc] | [prev] | [next] | [standalone]

#1671996

On Wed, 21 Jun 2017, Andi Kleen wrote:
> On Wed, Jun 21, 2017 at 05:12:06PM +0200, Thomas Gleixner wrote:
> > On Wed, 21 Jun 2017, kan.liang@intel.com wrote:
> > >  
> > >  #ifdef CONFIG_HARDLOCKUP_DETECTOR
> > > +/*
> > > + * The NMI watchdog relies on PERF_COUNT_HW_CPU_CYCLES event, which
> > > + * can tick faster than the measured CPU Frequency due to Turbo mode.
> > > + * That can lead to spurious timeouts.
> > > + * To workaround the issue, extending the period by 3 times.
> > > + */
> > >  u64 hw_nmi_get_sample_period(int watchdog_thresh)
> > >  {
> > > -	return (u64)(cpu_khz) * 1000 * watchdog_thresh;
> > > +	return (u64)(cpu_khz) * 1000 * watchdog_thresh * 3;
> > 
> > The maximum turbo frequency of any given machine can be retrieved.
> 
> Not reliably, e.g. not in virtualization. Also it would require
> model specific checks, so as soon as you have a new model and an
> old kernel it could still randomly fail.

And that's in no way an argument for breaking every existing setup which
relies on the way stuff works now. Lots of crap on new models does not work
with older kernels.

Fact is, this is a user visible change and people have pointed out, that it
will break their setups and expectations. So, no this is not going to
happen with just slapping a randomly chosen factor on it.

Fact is, that the watchdog works this way since it got implemented and it
really can sensibly argued that the way it works is correct.

The treshold is based on the non-turbo max frequency of the CPU. That's how
the period is calculated. And that makes sense in terms of frequency
scaling in either direction.

If your CPU is stuck for 1 second @2GHZ, then it wastes exactly the same
amount of cycles when it is stuck for 0.5 seconds @4Ghz. So the watchdog
can rightfully kick in after that and tell the world that crap is stuck.

If your newfangled machine triggers the watchdog after 0.2 seconds, then
you already have a mechanism to fix that. It's a user space interface after
all. There are lot of things which need to be adjusted with new machines
and if the boot default of 10 seconds is not sufficient, then something is
really wrong with these systems.

TBH, I rather whish the hard lockup watchdog treshhold would be
configurable in milliseconds rather than seconds to catch crap faster.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1672105

On Wed, 21 Jun 2017, kan.liang@intel.com wrote:
> We now have more and more systems where the Turbo range is wide enough
> that the NMI watchdog expires faster than the soft watchdog timer that
> updates the interrupt tick the NMI watchdog relies on.
> 
> This problem was originally added by commit 58687acba592
> ("lockup_detector: Combine nmi_watchdog and softlockup detector").
> Previously the NMI watchdog would always check jiffies, which were
> ticking fast enough. But now the backing is quite slow so the expire
> time becomes more sensitive.

And slapping a factor 3 on the NMI period is the wrong answer to the
problem. The simple solution would be to increase the hrtimer frequency,
but that's not really desired either.

Find an untested patch below, which should cure the issue.

Thanks,

	tglx
	
8<---------------
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -96,6 +96,7 @@ config X86
 	select GENERIC_STRNCPY_FROM_USER
 	select GENERIC_STRNLEN_USER
 	select GENERIC_TIME_VSYSCALL
+	select HARDLOCKUP_CHECK_TIMESTAMP	if X86_64
 	select HAVE_ACPI_APEI			if ACPI
 	select HAVE_ACPI_APEI_NMI		if ACPI
 	select HAVE_ALIGNED_STRUCT_PAGE		if SLUB
--- a/include/linux/nmi.h
+++ b/include/linux/nmi.h
@@ -155,6 +155,14 @@ extern int sysctl_hardlockup_all_cpu_bac
 #define sysctl_softlockup_all_cpu_backtrace 0
 #define sysctl_hardlockup_all_cpu_backtrace 0
 #endif
+
+#if defined(CONFIG_HARDLOCKUP_CHECK_TIMESTAMP) && \
+    defined(CONFIG_HARDLOCKUP_DETECTOR)
+void watchdog_update_hrtimer_threshold(u64 period);
+#else
+static inline void watchdog_update_hrtimer_threshold(u64 period) { }
+#endif
+
 extern bool is_hardlockup(void);
 struct ctl_table;
 extern int proc_watchdog(struct ctl_table *, int ,
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -161,6 +161,7 @@ static void set_sample_period(void)
 	 * hardlockup detector generates a warning
 	 */
 	sample_period = get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 5);
+	watchdog_update_hrtimer_threshold(sample_period);
 }
 
 /* Commands for resetting the watchdog */
--- a/kernel/watchdog_hld.c
+++ b/kernel/watchdog_hld.c
@@ -70,6 +70,54 @@ void touch_nmi_watchdog(void)
 }
 EXPORT_SYMBOL(touch_nmi_watchdog);
 
+#ifdef CONFIG_HARDLOCKUP_CHECK_TIMESTAMP
+static DEFINE_PER_CPU(ktime_t, last_timestamp);
+static ktime_t watchdog_hrtimer_sample_threshold __read_mostly;
+
+void watchdog_update_hrtimer_threshold(u64 period)
+{
+	/*
+	 * The hrtimer runs with a period of (watchdog_threshold * 2) / 5
+	 *
+	 * So it runs effectively with 2.5 times the rate of the NMI
+	 * watchdog. That means the hrtimer should fire 2-3 times before
+	 * the NMI watchdog expires. The NMI watchdog on x86 is based on
+	 * unhalted CPU cycles, so if Turbo-Mode is enabled the CPU cycles
+	 * might run way faster than expected and the NMI fires in a
+	 * smaller period than the one deduced from the nominal CPU
+	 * frequency. Depending on the Turbo-Mode factor this might be fast
+	 * enough to get the NMI period smaller than the hrtimer watchdog
+	 * period and trigger false positives.
+	 *
+	 * The sample threshold is used to check in the NMI handler whether
+	 * the minimum time between two NMI samples has elapsed. That
+	 * prevents false positives.
+	 *
+	 * Set this to 4/5 of the actual watchdog threshold period so the
+	 * hrtimer is guaranteed to fire at least once within the real
+	 * watchdog threshold.
+	 */
+	watchdog_hrtimer_sample_threshold = period * 2;
+}
+
+static bool watchdog_check_timestamp(void)
+{
+	ktime_t delta, now = ktime_get_mono_fast_ns();
+
+	delta = now - __this_cpu_read(last_timestamp);
+	if (delta < watchdog_hrtimer_sample_threshold)
+		return false;
+	__this_cpu_write(last_timestamp, now);
+	return true;
+}
+#else
+static inline bool watchdog_check_timestamp(void)
+{
+	return true;
+}
+#endif
+
+
 static struct perf_event_attr wd_hw_attr = {
 	.type		= PERF_TYPE_HARDWARE,
 	.config		= PERF_COUNT_HW_CPU_CYCLES,
@@ -94,6 +142,9 @@ static void watchdog_overflow_callback(s
 		return;
 	}
 
+	if (!watchdog_check_timestamp())
+		return;
+
 	/* check for a hardlockup
 	 * This is done by making sure our timer interrupt
 	 * is incrementing.  The timer interrupt should have
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -806,6 +806,9 @@ config HARDLOCKUP_DETECTOR
 	depends on LOCKUP_DETECTOR && !HAVE_NMI_WATCHDOG
 	depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI
 
+config HARDLOCKUP_CHECK_TIMESTAMP
+	bool
+
 config BOOTPARAM_HARDLOCKUP_PANIC
 	bool "Panic (Reboot) On Hard Lockups"
 	depends on HARDLOCKUP_DETECTOR

[toc] | [prev] | [next] | [standalone]

#1672755

On Wed, 21 Jun 2017, Thomas Gleixner wrote:

> On Wed, 21 Jun 2017, kan.liang@intel.com wrote:
> > We now have more and more systems where the Turbo range is wide enough
> > that the NMI watchdog expires faster than the soft watchdog timer that
> > updates the interrupt tick the NMI watchdog relies on.
> > 
> > This problem was originally added by commit 58687acba592
> > ("lockup_detector: Combine nmi_watchdog and softlockup detector").
> > Previously the NMI watchdog would always check jiffies, which were
> > ticking fast enough. But now the backing is quite slow so the expire
> > time becomes more sensitive.
> 
> And slapping a factor 3 on the NMI period is the wrong answer to the
> problem. The simple solution would be to increase the hrtimer frequency,
> but that's not really desired either.

Thinking a bit more about it. Increasing the hrtimer frequency and
maintaining the current frequency of softlockup_watchdog wakeups, would be
probably the most trivial workaround for now.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1672764

On Wed, Jun 21, 2017 at 11:53:57PM +0200, Thomas Gleixner wrote:
> On Wed, 21 Jun 2017, kan.liang@intel.com wrote:
> > We now have more and more systems where the Turbo range is wide enough
> > that the NMI watchdog expires faster than the soft watchdog timer that
> > updates the interrupt tick the NMI watchdog relies on.
> > 
> > This problem was originally added by commit 58687acba592
> > ("lockup_detector: Combine nmi_watchdog and softlockup detector").
> > Previously the NMI watchdog would always check jiffies, which were
> > ticking fast enough. But now the backing is quite slow so the expire
> > time becomes more sensitive.
> 
> And slapping a factor 3 on the NMI period is the wrong answer to the
> problem. The simple solution would be to increase the hrtimer frequency,
> but that's not really desired either.
> 
> Find an untested patch below, which should cure the issue.

A simple low pass filter.  It compiles. :-) I don't think I have knowledge
to test it.  Kan?

Cheers,
Don

> 
> Thanks,
> 
> 	tglx
> 	
> 8<---------------
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -96,6 +96,7 @@ config X86
>  	select GENERIC_STRNCPY_FROM_USER
>  	select GENERIC_STRNLEN_USER
>  	select GENERIC_TIME_VSYSCALL
> +	select HARDLOCKUP_CHECK_TIMESTAMP	if X86_64
>  	select HAVE_ACPI_APEI			if ACPI
>  	select HAVE_ACPI_APEI_NMI		if ACPI
>  	select HAVE_ALIGNED_STRUCT_PAGE		if SLUB
> --- a/include/linux/nmi.h
> +++ b/include/linux/nmi.h
> @@ -155,6 +155,14 @@ extern int sysctl_hardlockup_all_cpu_bac
>  #define sysctl_softlockup_all_cpu_backtrace 0
>  #define sysctl_hardlockup_all_cpu_backtrace 0
>  #endif
> +
> +#if defined(CONFIG_HARDLOCKUP_CHECK_TIMESTAMP) && \
> +    defined(CONFIG_HARDLOCKUP_DETECTOR)
> +void watchdog_update_hrtimer_threshold(u64 period);
> +#else
> +static inline void watchdog_update_hrtimer_threshold(u64 period) { }
> +#endif
> +
>  extern bool is_hardlockup(void);
>  struct ctl_table;
>  extern int proc_watchdog(struct ctl_table *, int ,
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -161,6 +161,7 @@ static void set_sample_period(void)
>  	 * hardlockup detector generates a warning
>  	 */
>  	sample_period = get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 5);
> +	watchdog_update_hrtimer_threshold(sample_period);
>  }
>  
>  /* Commands for resetting the watchdog */
> --- a/kernel/watchdog_hld.c
> +++ b/kernel/watchdog_hld.c
> @@ -70,6 +70,54 @@ void touch_nmi_watchdog(void)
>  }
>  EXPORT_SYMBOL(touch_nmi_watchdog);
>  
> +#ifdef CONFIG_HARDLOCKUP_CHECK_TIMESTAMP
> +static DEFINE_PER_CPU(ktime_t, last_timestamp);
> +static ktime_t watchdog_hrtimer_sample_threshold __read_mostly;
> +
> +void watchdog_update_hrtimer_threshold(u64 period)
> +{
> +	/*
> +	 * The hrtimer runs with a period of (watchdog_threshold * 2) / 5
> +	 *
> +	 * So it runs effectively with 2.5 times the rate of the NMI
> +	 * watchdog. That means the hrtimer should fire 2-3 times before
> +	 * the NMI watchdog expires. The NMI watchdog on x86 is based on
> +	 * unhalted CPU cycles, so if Turbo-Mode is enabled the CPU cycles
> +	 * might run way faster than expected and the NMI fires in a
> +	 * smaller period than the one deduced from the nominal CPU
> +	 * frequency. Depending on the Turbo-Mode factor this might be fast
> +	 * enough to get the NMI period smaller than the hrtimer watchdog
> +	 * period and trigger false positives.
> +	 *
> +	 * The sample threshold is used to check in the NMI handler whether
> +	 * the minimum time between two NMI samples has elapsed. That
> +	 * prevents false positives.
> +	 *
> +	 * Set this to 4/5 of the actual watchdog threshold period so the
> +	 * hrtimer is guaranteed to fire at least once within the real
> +	 * watchdog threshold.
> +	 */
> +	watchdog_hrtimer_sample_threshold = period * 2;
> +}
> +
> +static bool watchdog_check_timestamp(void)
> +{
> +	ktime_t delta, now = ktime_get_mono_fast_ns();
> +
> +	delta = now - __this_cpu_read(last_timestamp);
> +	if (delta < watchdog_hrtimer_sample_threshold)
> +		return false;
> +	__this_cpu_write(last_timestamp, now);
> +	return true;
> +}
> +#else
> +static inline bool watchdog_check_timestamp(void)
> +{
> +	return true;
> +}
> +#endif
> +
> +
>  static struct perf_event_attr wd_hw_attr = {
>  	.type		= PERF_TYPE_HARDWARE,
>  	.config		= PERF_COUNT_HW_CPU_CYCLES,
> @@ -94,6 +142,9 @@ static void watchdog_overflow_callback(s
>  		return;
>  	}
>  
> +	if (!watchdog_check_timestamp())
> +		return;
> +
>  	/* check for a hardlockup
>  	 * This is done by making sure our timer interrupt
>  	 * is incrementing.  The timer interrupt should have
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -806,6 +806,9 @@ config HARDLOCKUP_DETECTOR
>  	depends on LOCKUP_DETECTOR && !HAVE_NMI_WATCHDOG
>  	depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI
>  
> +config HARDLOCKUP_CHECK_TIMESTAMP
> +	bool
> +
>  config BOOTPARAM_HARDLOCKUP_PANIC
>  	bool "Panic (Reboot) On Hard Lockups"
>  	depends on HARDLOCKUP_DETECTOR

[toc] | [prev] | [next] | [standalone]

#1672768

> Subject: Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups
> 
> On Wed, Jun 21, 2017 at 11:53:57PM +0200, Thomas Gleixner wrote:
> > On Wed, 21 Jun 2017, kan.liang@intel.com wrote:
> > > We now have more and more systems where the Turbo range is wide
> > > enough that the NMI watchdog expires faster than the soft watchdog
> > > timer that updates the interrupt tick the NMI watchdog relies on.
> > >
> > > This problem was originally added by commit 58687acba592
> > > ("lockup_detector: Combine nmi_watchdog and softlockup detector").
> > > Previously the NMI watchdog would always check jiffies, which were
> > > ticking fast enough. But now the backing is quite slow so the expire
> > > time becomes more sensitive.
> >
> > And slapping a factor 3 on the NMI period is the wrong answer to the
> > problem. The simple solution would be to increase the hrtimer
> > frequency, but that's not really desired either.
> >
> > Find an untested patch below, which should cure the issue.
> 
> A simple low pass filter.  It compiles. :-) I don't think I have knowledge to test
> it.  Kan?
> 

Yes, we are doing the test.

Thanks,
Kan

> Cheers,
> Don
> 
> >
> > Thanks,
> >
> > 	tglx
> >
> > 8<---------------
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -96,6 +96,7 @@ config X86
> >  	select GENERIC_STRNCPY_FROM_USER
> >  	select GENERIC_STRNLEN_USER
> >  	select GENERIC_TIME_VSYSCALL
> > +	select HARDLOCKUP_CHECK_TIMESTAMP	if X86_64
> >  	select HAVE_ACPI_APEI			if ACPI
> >  	select HAVE_ACPI_APEI_NMI		if ACPI
> >  	select HAVE_ALIGNED_STRUCT_PAGE		if SLUB
> > --- a/include/linux/nmi.h
> > +++ b/include/linux/nmi.h
> > @@ -155,6 +155,14 @@ extern int sysctl_hardlockup_all_cpu_bac  #define
> > sysctl_softlockup_all_cpu_backtrace 0  #define
> > sysctl_hardlockup_all_cpu_backtrace 0  #endif
> > +
> > +#if defined(CONFIG_HARDLOCKUP_CHECK_TIMESTAMP) && \
> > +    defined(CONFIG_HARDLOCKUP_DETECTOR)
> > +void watchdog_update_hrtimer_threshold(u64 period); #else static
> > +inline void watchdog_update_hrtimer_threshold(u64 period) { } #endif
> > +
> >  extern bool is_hardlockup(void);
> >  struct ctl_table;
> >  extern int proc_watchdog(struct ctl_table *, int ,
> > --- a/kernel/watchdog.c
> > +++ b/kernel/watchdog.c
> > @@ -161,6 +161,7 @@ static void set_sample_period(void)
> >  	 * hardlockup detector generates a warning
> >  	 */
> >  	sample_period = get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 5);
> > +	watchdog_update_hrtimer_threshold(sample_period);
> >  }
> >
> >  /* Commands for resetting the watchdog */
> > --- a/kernel/watchdog_hld.c
> > +++ b/kernel/watchdog_hld.c
> > @@ -70,6 +70,54 @@ void touch_nmi_watchdog(void)  }
> > EXPORT_SYMBOL(touch_nmi_watchdog);
> >
> > +#ifdef CONFIG_HARDLOCKUP_CHECK_TIMESTAMP static
> > +DEFINE_PER_CPU(ktime_t, last_timestamp); static ktime_t
> > +watchdog_hrtimer_sample_threshold __read_mostly;
> > +
> > +void watchdog_update_hrtimer_threshold(u64 period) {
> > +	/*
> > +	 * The hrtimer runs with a period of (watchdog_threshold * 2) / 5
> > +	 *
> > +	 * So it runs effectively with 2.5 times the rate of the NMI
> > +	 * watchdog. That means the hrtimer should fire 2-3 times before
> > +	 * the NMI watchdog expires. The NMI watchdog on x86 is based on
> > +	 * unhalted CPU cycles, so if Turbo-Mode is enabled the CPU cycles
> > +	 * might run way faster than expected and the NMI fires in a
> > +	 * smaller period than the one deduced from the nominal CPU
> > +	 * frequency. Depending on the Turbo-Mode factor this might be fast
> > +	 * enough to get the NMI period smaller than the hrtimer watchdog
> > +	 * period and trigger false positives.
> > +	 *
> > +	 * The sample threshold is used to check in the NMI handler whether
> > +	 * the minimum time between two NMI samples has elapsed. That
> > +	 * prevents false positives.
> > +	 *
> > +	 * Set this to 4/5 of the actual watchdog threshold period so the
> > +	 * hrtimer is guaranteed to fire at least once within the real
> > +	 * watchdog threshold.
> > +	 */
> > +	watchdog_hrtimer_sample_threshold = period * 2; }
> > +
> > +static bool watchdog_check_timestamp(void) {
> > +	ktime_t delta, now = ktime_get_mono_fast_ns();
> > +
> > +	delta = now - __this_cpu_read(last_timestamp);
> > +	if (delta < watchdog_hrtimer_sample_threshold)
> > +		return false;
> > +	__this_cpu_write(last_timestamp, now);
> > +	return true;
> > +}
> > +#else
> > +static inline bool watchdog_check_timestamp(void) {
> > +	return true;
> > +}
> > +#endif
> > +
> > +
> >  static struct perf_event_attr wd_hw_attr = {
> >  	.type		= PERF_TYPE_HARDWARE,
> >  	.config		= PERF_COUNT_HW_CPU_CYCLES,
> > @@ -94,6 +142,9 @@ static void watchdog_overflow_callback(s
> >  		return;
> >  	}
> >
> > +	if (!watchdog_check_timestamp())
> > +		return;
> > +
> >  	/* check for a hardlockup
> >  	 * This is done by making sure our timer interrupt
> >  	 * is incrementing.  The timer interrupt should have
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -806,6 +806,9 @@ config HARDLOCKUP_DETECTOR
> >  	depends on LOCKUP_DETECTOR && !HAVE_NMI_WATCHDOG
> >  	depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI
> >
> > +config HARDLOCKUP_CHECK_TIMESTAMP
> > +	bool
> > +
> >  config BOOTPARAM_HARDLOCKUP_PANIC
> >  	bool "Panic (Reboot) On Hard Lockups"
> >  	depends on HARDLOCKUP_DETECTOR

[toc] | [prev] | [next] | [standalone]

#1673346

On Thu, 22 Jun 2017, Don Zickus wrote:
> On Wed, Jun 21, 2017 at 11:53:57PM +0200, Thomas Gleixner wrote:
> > On Wed, 21 Jun 2017, kan.liang@intel.com wrote:
> > > We now have more and more systems where the Turbo range is wide enough
> > > that the NMI watchdog expires faster than the soft watchdog timer that
> > > updates the interrupt tick the NMI watchdog relies on.
> > > 
> > > This problem was originally added by commit 58687acba592
> > > ("lockup_detector: Combine nmi_watchdog and softlockup detector").
> > > Previously the NMI watchdog would always check jiffies, which were
> > > ticking fast enough. But now the backing is quite slow so the expire
> > > time becomes more sensitive.
> > 
> > And slapping a factor 3 on the NMI period is the wrong answer to the
> > problem. The simple solution would be to increase the hrtimer frequency,
> > but that's not really desired either.
> > 
> > Find an untested patch below, which should cure the issue.
> 
> A simple low pass filter.  It compiles. :-) I don't think I have knowledge
> to test it.  Kan?

Yes, and it has an interesting twist. It's only working once we have
switched to TSC as clocksource.

As long as jiffies are the clocksource, this will miserably fail because
when the hrtimer interrupt is not delivered jiffies wont be incremented
either and the NMI will say: Oh. not enough time elapsed. Lather, rinse and
repeat.

One simple way to fix this is with the delta patch below.

Thanks,

	tglx

8<--------------------------
--- a/kernel/watchdog_hld.c
+++ b/kernel/watchdog_hld.c
@@ -72,6 +72,7 @@ EXPORT_SYMBOL(touch_nmi_watchdog);
 
 #ifdef CONFIG_HARDLOCKUP_CHECK_TIMESTAMP
 static DEFINE_PER_CPU(ktime_t, last_timestamp);
+static DEFINE_PER_CPU(unsigned int, nmi_rearmed);
 static ktime_t watchdog_hrtimer_sample_threshold __read_mostly;
 
 void watchdog_update_hrtimer_threshold(u64 period)
@@ -105,8 +106,11 @@ static bool watchdog_check_timestamp(voi
 	ktime_t delta, now = ktime_get_mono_fast_ns();
 
 	delta = now - __this_cpu_read(last_timestamp);
-	if (delta < watchdog_hrtimer_sample_threshold)
-		return false;
+	if (delta < watchdog_hrtimer_sample_threshold) {
+		if (__this_cpu_inc_return(nmi_rearmed) < 10)
+			return false;
+	}
+	__this_cpu_write(nmi_rearmed, 0);
 	__this_cpu_write(last_timestamp, now);
 	return true;
 }

[toc] | [prev] | [next] | [standalone]

#1673672

On Fri, Jun 23, 2017 at 10:01:55AM +0200, Thomas Gleixner wrote:
> On Thu, 22 Jun 2017, Don Zickus wrote:
> > On Wed, Jun 21, 2017 at 11:53:57PM +0200, Thomas Gleixner wrote:
> > > On Wed, 21 Jun 2017, kan.liang@intel.com wrote:
> > > > We now have more and more systems where the Turbo range is wide enough
> > > > that the NMI watchdog expires faster than the soft watchdog timer that
> > > > updates the interrupt tick the NMI watchdog relies on.
> > > > 
> > > > This problem was originally added by commit 58687acba592
> > > > ("lockup_detector: Combine nmi_watchdog and softlockup detector").
> > > > Previously the NMI watchdog would always check jiffies, which were
> > > > ticking fast enough. But now the backing is quite slow so the expire
> > > > time becomes more sensitive.
> > > 
> > > And slapping a factor 3 on the NMI period is the wrong answer to the
> > > problem. The simple solution would be to increase the hrtimer frequency,
> > > but that's not really desired either.
> > > 
> > > Find an untested patch below, which should cure the issue.
> > 
> > A simple low pass filter.  It compiles. :-) I don't think I have knowledge
> > to test it.  Kan?
> 
> Yes, and it has an interesting twist. It's only working once we have
> switched to TSC as clocksource.
> 
> As long as jiffies are the clocksource, this will miserably fail because
> when the hrtimer interrupt is not delivered jiffies wont be incremented
> either and the NMI will say: Oh. not enough time elapsed. Lather, rinse and
> repeat.
> 
> One simple way to fix this is with the delta patch below.

Hmm, all this work for a temp fix.  Kan, how much longer until the real fix
of having perf count the right cycles?

Cheers,
Don

> 
> Thanks,
> 
> 	tglx
> 
> 8<--------------------------
> --- a/kernel/watchdog_hld.c
> +++ b/kernel/watchdog_hld.c
> @@ -72,6 +72,7 @@ EXPORT_SYMBOL(touch_nmi_watchdog);
>  
>  #ifdef CONFIG_HARDLOCKUP_CHECK_TIMESTAMP
>  static DEFINE_PER_CPU(ktime_t, last_timestamp);
> +static DEFINE_PER_CPU(unsigned int, nmi_rearmed);
>  static ktime_t watchdog_hrtimer_sample_threshold __read_mostly;
>  
>  void watchdog_update_hrtimer_threshold(u64 period)
> @@ -105,8 +106,11 @@ static bool watchdog_check_timestamp(voi
>  	ktime_t delta, now = ktime_get_mono_fast_ns();
>  
>  	delta = now - __this_cpu_read(last_timestamp);
> -	if (delta < watchdog_hrtimer_sample_threshold)
> -		return false;
> +	if (delta < watchdog_hrtimer_sample_threshold) {
> +		if (__this_cpu_inc_return(nmi_rearmed) < 10)
> +			return false;
> +	}
> +	__this_cpu_write(nmi_rearmed, 0);
>  	__this_cpu_write(last_timestamp, now);
>  	return true;
>  }
> 
> 

[toc] | [prev] | [next] | [standalone]

#1673900

On Fri, 23 Jun 2017, Don Zickus wrote:
> Hmm, all this work for a temp fix.  Kan, how much longer until the real fix
> of having perf count the right cycles?

Quite a while. The approach is wilfully breaking the user space ABI, which
is not going to happen.

And there is a simpler solution as well, as I said here:

    http://lkml.kernel.org/r/alpine.DEB.2.20.1706221730520.1885@nanos

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1675082

On Fri, Jun 23, 2017 at 11:50:25PM +0200, Thomas Gleixner wrote:
> On Fri, 23 Jun 2017, Don Zickus wrote:
> > Hmm, all this work for a temp fix.  Kan, how much longer until the real fix
> > of having perf count the right cycles?
> 
> Quite a while. The approach is wilfully breaking the user space ABI, which
> is not going to happen.
> 
> And there is a simpler solution as well, as I said here:
> 
>     http://lkml.kernel.org/r/alpine.DEB.2.20.1706221730520.1885@nanos

Hi Thomas,

So, you are saying instead of slowing down the perf counter, speed up the
hrtimer to sample more frequently like so:

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 03e0b69..8ff49de 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -160,7 +160,7 @@ static void set_sample_period(void)
 	 * and hard thresholds) to increment before the
 	 * hardlockup detector generates a warning
 	 */
-	sample_period = get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 5);
+	sample_period = get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 10);
 }
 
 /* Commands for resetting the watchdog */


That is another way of doing it.  It just hits all the arches.  It does seem
cleaner as the watchdog_thresh value still retains it correct meaning.  Are
the laptop folks going to yell at me some more for waking their systems up
more? :-)

Cheers,
Don

[toc] | [prev] | [next] | [standalone]

#1675092

On Mon, 26 Jun 2017, Don Zickus wrote:
> On Fri, Jun 23, 2017 at 11:50:25PM +0200, Thomas Gleixner wrote:
> > On Fri, 23 Jun 2017, Don Zickus wrote:
> > > Hmm, all this work for a temp fix.  Kan, how much longer until the real fix
> > > of having perf count the right cycles?
> > 
> > Quite a while. The approach is wilfully breaking the user space ABI, which
> > is not going to happen.
> > 
> > And there is a simpler solution as well, as I said here:
> > 
> >     http://lkml.kernel.org/r/alpine.DEB.2.20.1706221730520.1885@nanos
> 
> Hi Thomas,
> 
> So, you are saying instead of slowing down the perf counter, speed up the
> hrtimer to sample more frequently like so:
> 
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 03e0b69..8ff49de 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -160,7 +160,7 @@ static void set_sample_period(void)
>  	 * and hard thresholds) to increment before the
>  	 * hardlockup detector generates a warning
>  	 */
> -	sample_period = get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 5);
> +	sample_period = get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 10);
>  }
> 
>  /* Commands for resetting the watchdog */
> 
> 
> That is another way of doing it.  It just hits all the arches.  It does seem
> cleaner as the watchdog_thresh value still retains it correct meaning.  Are
> the laptop folks going to yell at me some more for waking their systems up
> more? :-)

Yes, that's bound to happen. You might make them less angry if you wake the
softlockup thread only on every second hrtimer expiry, i.e. keeping the
current wakeup rate.  But I can't promise that this will significantly
lower their wrath. :)

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1676150

On Mon, Jun 26, 2017 at 04:19:27PM -0400, Don Zickus wrote:
> On Fri, Jun 23, 2017 at 11:50:25PM +0200, Thomas Gleixner wrote:
> > On Fri, 23 Jun 2017, Don Zickus wrote:
> > > Hmm, all this work for a temp fix.  Kan, how much longer until the real fix
> > > of having perf count the right cycles?
> > 
> > Quite a while. The approach is wilfully breaking the user space ABI, which
> > is not going to happen.
> > 
> > And there is a simpler solution as well, as I said here:
> > 
> >     http://lkml.kernel.org/r/alpine.DEB.2.20.1706221730520.1885@nanos
> 
> Hi Thomas,
> 
> So, you are saying instead of slowing down the perf counter, speed up the
> hrtimer to sample more frequently like so:
> 
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 03e0b69..8ff49de 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -160,7 +160,7 @@ static void set_sample_period(void)
>  	 * and hard thresholds) to increment before the
>  	 * hardlockup detector generates a warning
>  	 */
> -	sample_period = get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 5);
> +	sample_period = get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 10);
>  }

Hi Kan,

Will the above patch work for you?

Cheers,
Don

[toc] | [prev] | [next] | [standalone]

#1676169

> On Mon, Jun 26, 2017 at 04:19:27PM -0400, Don Zickus wrote:
> > On Fri, Jun 23, 2017 at 11:50:25PM +0200, Thomas Gleixner wrote:
> > > On Fri, 23 Jun 2017, Don Zickus wrote:
> > > > Hmm, all this work for a temp fix.  Kan, how much longer until the
> > > > real fix of having perf count the right cycles?
> > >
> > > Quite a while. The approach is wilfully breaking the user space ABI,
> > > which is not going to happen.
> > >
> > > And there is a simpler solution as well, as I said here:
> > >
> > >
> > > http://lkml.kernel.org/r/alpine.DEB.2.20.1706221730520.1885@nanos
> >
> > Hi Thomas,
> >
> > So, you are saying instead of slowing down the perf counter, speed up
> > the hrtimer to sample more frequently like so:
> >
> > diff --git a/kernel/watchdog.c b/kernel/watchdog.c index
> > 03e0b69..8ff49de 100644
> > --- a/kernel/watchdog.c
> > +++ b/kernel/watchdog.c
> > @@ -160,7 +160,7 @@ static void set_sample_period(void)
> >  	 * and hard thresholds) to increment before the
> >  	 * hardlockup detector generates a warning
> >  	 */
> > -	sample_period = get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 5);
> > +	sample_period = get_softlockup_thresh() * ((u64)NSEC_PER_SEC /
> 10);
> >  }
> 
> Hi Kan,
> 
> Will the above patch work for you?
> 

I haven't heard back any test result yet.

The above patch looks good to me.
But I'm not sure if /10 is enough. We may need /15.
Anyway, I think we will test /10 first.

Which workaround do you prefer, the above one or the one checking timestamp?


Thanks,
Kan

[toc] | [prev] | [next] | [standalone]

#1676181

On Tue, Jun 27, 2017 at 08:49:19PM +0000, Liang, Kan wrote:
> 
> > On Mon, Jun 26, 2017 at 04:19:27PM -0400, Don Zickus wrote:
> > > On Fri, Jun 23, 2017 at 11:50:25PM +0200, Thomas Gleixner wrote:
> > > > On Fri, 23 Jun 2017, Don Zickus wrote:
> > > > > Hmm, all this work for a temp fix.  Kan, how much longer until the
> > > > > real fix of having perf count the right cycles?
> > > >
> > > > Quite a while. The approach is wilfully breaking the user space ABI,
> > > > which is not going to happen.
> > > >
> > > > And there is a simpler solution as well, as I said here:
> > > >
> > > >
> > > > http://lkml.kernel.org/r/alpine.DEB.2.20.1706221730520.1885@nanos
> > >
> > > Hi Thomas,
> > >
> > > So, you are saying instead of slowing down the perf counter, speed up
> > > the hrtimer to sample more frequently like so:
> > >
> > > diff --git a/kernel/watchdog.c b/kernel/watchdog.c index
> > > 03e0b69..8ff49de 100644
> > > --- a/kernel/watchdog.c
> > > +++ b/kernel/watchdog.c
> > > @@ -160,7 +160,7 @@ static void set_sample_period(void)
> > >  	 * and hard thresholds) to increment before the
> > >  	 * hardlockup detector generates a warning
> > >  	 */
> > > -	sample_period = get_softlockup_thresh() * ((u64)NSEC_PER_SEC / 5);
> > > +	sample_period = get_softlockup_thresh() * ((u64)NSEC_PER_SEC /
> > 10);
> > >  }
> > 
> > Hi Kan,
> > 
> > Will the above patch work for you?
> > 
> 
> I haven't heard back any test result yet.
> 
> The above patch looks good to me.
> But I'm not sure if /10 is enough. We may need /15.
> Anyway, I think we will test /10 first.
> 
> Which workaround do you prefer, the above one or the one checking timestamp?

Let's go with this one, it is simpler.

Cheers,
Don

[toc] | [prev] | [next] | [standalone]

#1676265

> I haven't heard back any test result yet.
> 
> The above patch looks good to me.

This needs performance testing.  It may slow down performance or latency sensitive workloads.

> Which workaround do you prefer, the above one or the one checking timestamp?

I prefer the earlier patch, it has far less risk of performance issues.

-Andi

[toc] | [prev] | [next] | [standalone]

#1677087

On Tue, Jun 27, 2017 at 04:48:22PM -0700, Andi Kleen wrote:
> > I haven't heard back any test result yet.
> > 
> > The above patch looks good to me.
> 
> This needs performance testing.  It may slow down performance or latency sensitive workloads.

More motivation to work through the issues with the proposed real fix? :-)

> 
> > Which workaround do you prefer, the above one or the one checking timestamp?
> 
> I prefer the earlier patch, it has far less risk of performance issues.

But now you are slowing down the nmi_watchdog so much that the
watchdog_thresh hold becomes meaningless, no? (granted the turbo-mode blows
it out of the water too)  So now folks who depend on the 10/5/1/whatever second
reliability lose that.  I think that might be unfair too.

The hrtimer increase maintains that and just adds a few more
interrupts/second.

Cheers,
Don

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web