Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752519AbdHQPqA convert rfc822-to-8bit (ORCPT ); Thu, 17 Aug 2017 11:46:00 -0400 Received: from mga06.intel.com ([134.134.136.31]:38861 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751561AbdHQPp6 (ORCPT ); Thu, 17 Aug 2017 11:45:58 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,388,1498546800"; d="scan'208";a="301448548" From: "Liang, Kan" To: Thomas Gleixner CC: "'Don Zickus'" , "linux-kernel@vger.kernel.org" , "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 Thread-Topic: [PATCH V2] kernel/watchdog: fix spurious hard lockups Thread-Index: AQHS6pyX93nZMscYGUu+rlSjZMkkxKIvVm6AgAErMwCAARD/gIAAjbWAgABZxoCABJ2VAIABkHmAgB6zMiCAAGBLgIAc+e0wgBAlWICABCzIcA== Date: Thu, 17 Aug 2017 15:45:54 +0000 Message-ID: <37D7C6CF3E00A74B8858931C1DB2F07753786CA3@SHSMSX103.ccr.corp.intel.com> References: <20170621144118.5939-1-kan.liang@intel.com> <20170622154450.2lua7fdmigcixldw@redhat.com> <20170623162907.l6inpxgztwwkeaoi@redhat.com> <20170626201927.3ak7fk3yvdzbb4ay@redhat.com> <20170627201249.ll34ecwhpme3vh2u@redhat.com> <37D7C6CF3E00A74B8858931C1DB2F0775371D43E@SHSMSX103.ccr.corp.intel.com> <20170717144637.34umykrccvjma3fl@redhat.com> <37D7C6CF3E00A74B8858931C1DB2F07753784A2B@SHSMSX103.ccr.corp.intel.com> In-Reply-To: Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZDNiMmQ3NjUtNjczNC00ZWE2LWIyODctZGUxZGIwNGI4MzMxIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6Ik9Nbk1hb0VXa0tTM3pjbHllcWN5YlAwc1lQZFV5cEcrczZGUjNjYWRUcDg9In0= x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5348 Lines: 160 > 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 > > > > 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. > #