LKML Archive on lore.kernel.org
 help / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Kan Liang <kan.liang@intel.com>
Cc: linux-kernel@vger.kernel.org, dzickus@redhat.com,
	mingo@kernel.org, akpm@linux-foundation.org,
	babu.moger@oracle.com, atomlin@redhat.com, prarit@redhat.com,
	torvalds@linux-foundation.org, peterz@infradead.org,
	eranian@google.com, acme@redhat.com, ak@linux.intel.com,
	stable@vger.kernel.org
Subject: Re: [PATCH V2] kernel/watchdog: fix spurious hard lockups
Date: Wed, 21 Jun 2017 23:53:57 +0200 (CEST)
Message-ID: <alpine.DEB.2.20.1706212235550.2152@nanos> (raw)
In-Reply-To: <20170621144118.5939-1-kan.liang@intel.com>

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

  parent reply index

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-06-21 14:41 kan.liang
2017-06-21 15:12 ` Thomas Gleixner
2017-06-21 15:47   ` Liang, Kan
2017-06-21 17:40     ` Prarit Bhargava
2017-06-21 17:07   ` Andi Kleen
2017-06-21 19:59     ` Thomas Gleixner
2017-06-21 21:53 ` Thomas Gleixner [this message]
2017-06-22 15:33   ` Thomas Gleixner
2017-06-22 15:44   ` Don Zickus
2017-06-22 15:48     ` Liang, Kan
2017-06-23  8:01     ` Thomas Gleixner
2017-06-23 16:29       ` Don Zickus
2017-06-23 21:50         ` Thomas Gleixner
2017-06-26 20:19           ` Don Zickus
2017-06-26 20:30             ` Thomas Gleixner
2017-06-27 20:12             ` Don Zickus
2017-06-27 20:49               ` Liang, Kan
2017-06-27 21:09                 ` Don Zickus
2017-06-27 23:48                 ` Andi Kleen
2017-06-28 19:00                   ` Don Zickus
2017-06-28 20:14                     ` Andi Kleen
2017-06-29 15:44                       ` Don Zickus
2017-06-29 16:12                         ` Andi Kleen
2017-06-29 16:26                           ` Don Zickus
2017-06-29 16:36                             ` Andi Kleen
2017-07-17  1:24               ` Liang, Kan
2017-07-17  7:14                 ` Thomas Gleixner
2017-07-17 12:18                   ` Liang, Kan
2017-07-17 13:13                     ` Thomas Gleixner
2017-07-17 14:46                       ` Liang, Kan
2017-07-17 15:00                         ` Thomas Gleixner
2017-07-17 14:46                 ` Don Zickus
2017-08-15  1:16                   ` Liang, Kan
2017-08-15  1:28                     ` Linus Torvalds
2017-08-15  7:50                     ` Thomas Gleixner
2017-08-17 15:45                       ` Liang, Kan
2017-08-18 10:39                       ` [tip:core/urgent] kernel/watchdog: Prevent false positives with turbo modes tip-bot for Thomas Gleixner

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.20.1706212235550.2152@nanos \
    --to=tglx@linutronix.de \
    --cc=acme@redhat.com \
    --cc=ak@linux.intel.com \
    --cc=akpm@linux-foundation.org \
    --cc=atomlin@redhat.com \
    --cc=babu.moger@oracle.com \
    --cc=dzickus@redhat.com \
    --cc=eranian@google.com \
    --cc=kan.liang@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=peterz@infradead.org \
    --cc=prarit@redhat.com \
    --cc=stable@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox