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

[Liang, Kan]

> On Tue, 15 Aug 2017, Liang, Kan wrote:
> > This patch which speed up the hrtimer
> (https://lkml.org/lkml/2017/6/26/685)
> > is decent to fix the spurious hard lockups.
> > Tested-by: Kan Liang <kan.liang@xxxxxxxxx>
> >
> > Please consider to merge it into both mainline and stable tree.
> 
> Well, it 'fixes' the problem, but at the same time it imposes a higher
> frequency of hrtimer interrupts and a higher frequency of soft lockup
> thread wakeups. I'm not convinced that this is the right thing to do, even
> if the patch itself is simple and small.
> 
> Did you run the patch which implements the low pass filter? Does it fix the
> issue as well? It's slightly larger, but does not come with the downsides
> of the real simple one. Appended for reference.

I just finished the test for the low pass filter patch.
Yes, it also fixes the watchdog false positive issue.

Thanks,
Kan

> 
> Thanks,
> 
> 	tglx
> 
> 8<---------------------
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -100,6 +100,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
> @@ -168,6 +168,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
> @@ -240,6 +240,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
> @@ -37,6 +37,62 @@ void arch_touch_nmi_watchdog(void)
>  }
>  EXPORT_SYMBOL(arch_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)
> +{
> +	/*
> +	 * 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) {
> +		/*
> +		 * If ktime is jiffies based, a stalled timer would prevent
> +		 * jiffies from being incremented and the filter would look
> +		 * at a stale timestamp and never trigger.
> +		 */
> +		if (__this_cpu_inc_return(nmi_rearmed) < 10)
> +			return false;
> +	}
> +	__this_cpu_write(nmi_rearmed, 0);
> +	__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,
> @@ -61,6 +117,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
> @@ -798,6 +798,13 @@ config HARDLOCKUP_DETECTOR_PERF
>  	select SOFTLOCKUP_DETECTOR
> 
>  #
> +# Enables a timestamp based low pass filter to compensate for perf based
> +# hard lockup detection which runs too fast due to turbo modes.
> +#
> +config HARDLOCKUP_CHECK_TIMESTAMP
> +	bool
> +
> +#
>  # arch/ can define HAVE_HARDLOCKUP_DETECTOR_ARCH to provide their
> own hard
>  # lockup detector rather than the perf based detector.
>  #