(adding Uli)
On Fri, May 19, 2017 at 01:50:26AM +1000, Nicholas Piggin wrote:
> I'd like to make it easier for architectures that have their own NMI /
> hard lockup detector to reuse various configuration interfaces that are
> provided by generic detectors (cmdline, sysctl, suspend/resume calls).
>
> I'd also like to remove the dependency of arch hard lockup detectors
> on the softlockup detector. The reason being these watchdogs can be
> very small (sparc's is like a page of core code that does not use any
> big subsystem like kthreads or timers).
>
> So I do this by adding a separate CONFIG_SOFTLOCKUP_DETECTOR, and
> juggling around what goes under config options. HAVE_NMI_WATCHDOG
> continues to be the config for arch to override the hard lockup
> detector, which is expanded to cover a few more cases.
Basically you are trying to remove the heavy HARDLOCKUP pieces to minimize
the SOFTLOCKUP piece and use your own NMI detector, right?
I am guessing you would then disable SOFTLOCKUP to remove all the kthread
and timer stuff but continue to use the generic infrastructure to help
manager your own NMI detector?
A lot of the code is just re-organizing things and adding an explicit
ifdef on SOFTLOCKUP, which seems fine to me.
I just need to spend some time on some of your #else clauses to see what
functionality is dropped when you use your approach.
Cheers,
Don
>
> These config options are pretty ugly, but they already were. I don't
> think they've become a lot worse after this.
>
> Architectures can then use watchdog_nmi_reconfigure() which gets
> called every time something changes (sysctls, suspend/resume, etc) and
> adjust according to relevant parameters (watchdog_enabled,
> watchdog_thresh, watchdog_cpumask, watchdog_suspended).
>
> I have something for powerpc but I want to make sure this generic code
> change is reasonable and could work for other archs.
>
> Thanks,
> Nick
>
> diff --git a/include/linux/nmi.h b/include/linux/nmi.h
> index aa3cd0878270..5c2265f84fa3 100644
> --- a/include/linux/nmi.h
> +++ b/include/linux/nmi.h
> @@ -8,16 +8,25 @@
> #include <asm/irq.h>
>
> #ifdef CONFIG_LOCKUP_DETECTOR
> +void lockup_detector_init(void);
> +#else
> +static inline void lockup_detector_init(void)
> +{
> +}
> +#endif
> +
> +#ifdef CONFIG_SOFTLOCKUP_DETECTOR
> +extern int proc_dowatchdog_thresh(struct ctl_table *table, int write,
> + void __user *buffer,
> + size_t *lenp, loff_t *ppos);
> +
> extern void touch_softlockup_watchdog_sched(void);
> extern void touch_softlockup_watchdog(void);
> extern void touch_softlockup_watchdog_sync(void);
> extern void touch_all_softlockup_watchdogs(void);
> -extern int proc_dowatchdog_thresh(struct ctl_table *table, int write,
> - void __user *buffer,
> - size_t *lenp, loff_t *ppos);
> extern unsigned int softlockup_panic;
> -extern unsigned int hardlockup_panic;
> -void lockup_detector_init(void);
> +extern int soft_watchdog_enabled;
> +extern atomic_t watchdog_park_in_progress;
> #else
> static inline void touch_softlockup_watchdog_sched(void)
> {
> @@ -31,9 +40,6 @@ static inline void touch_softlockup_watchdog_sync(void)
> static inline void touch_all_softlockup_watchdogs(void)
> {
> }
> -static inline void lockup_detector_init(void)
> -{
> -}
> #endif
>
> #ifdef CONFIG_DETECT_HUNG_TASK
> @@ -70,17 +76,14 @@ static inline void reset_hung_task_detector(void)
> */
> #if defined(CONFIG_HAVE_NMI_WATCHDOG) || defined(CONFIG_HARDLOCKUP_DETECTOR)
> #include <asm/nmi.h>
> +extern unsigned int hardlockup_panic;
> extern void touch_nmi_watchdog(void);
> +extern void hardlockup_detector_disable(void);
> #else
> static inline void touch_nmi_watchdog(void)
> {
> touch_softlockup_watchdog();
> }
> -#endif
> -
> -#if defined(CONFIG_HARDLOCKUP_DETECTOR)
> -extern void hardlockup_detector_disable(void);
> -#else
> static inline void hardlockup_detector_disable(void) {}
> #endif
>
> @@ -139,15 +142,18 @@ static inline bool trigger_single_cpu_backtrace(int cpu)
> }
> #endif
>
> -#ifdef CONFIG_LOCKUP_DETECTOR
> +#ifdef CONFIG_HARDLOCKUP_DETECTOR
> u64 hw_nmi_get_sample_period(int watchdog_thresh);
> +#endif
> +
> +#ifdef CONFIG_LOCKUP_DETECTOR
> extern int nmi_watchdog_enabled;
> -extern int soft_watchdog_enabled;
> extern int watchdog_user_enabled;
> extern int watchdog_thresh;
> extern unsigned long watchdog_enabled;
> +extern struct cpumask watchdog_cpumask;
> extern unsigned long *watchdog_cpumask_bits;
> -extern atomic_t watchdog_park_in_progress;
> +extern int __read_mostly watchdog_suspended;
> #ifdef CONFIG_SMP
> extern int sysctl_softlockup_all_cpu_backtrace;
> extern int sysctl_hardlockup_all_cpu_backtrace;
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 4dfba1a76cc3..bb747d8f8521 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -880,6 +880,14 @@ static struct ctl_table kern_table[] = {
> #endif
> },
> {
> + .procname = "watchdog_cpumask",
> + .data = &watchdog_cpumask_bits,
> + .maxlen = NR_CPUS,
> + .mode = 0644,
> + .proc_handler = proc_watchdog_cpumask,
> + },
> +#ifdef CONFIG_SOFTLOCKUP_DETECTOR
> + {
> .procname = "soft_watchdog",
> .data = &soft_watchdog_enabled,
> .maxlen = sizeof (int),
> @@ -889,13 +897,6 @@ static struct ctl_table kern_table[] = {
> .extra2 = &one,
> },
> {
> - .procname = "watchdog_cpumask",
> - .data = &watchdog_cpumask_bits,
> - .maxlen = NR_CPUS,
> - .mode = 0644,
> - .proc_handler = proc_watchdog_cpumask,
> - },
> - {
> .procname = "softlockup_panic",
> .data = &softlockup_panic,
> .maxlen = sizeof(int),
> @@ -904,7 +905,8 @@ static struct ctl_table kern_table[] = {
> .extra1 = &zero,
> .extra2 = &one,
> },
> -#ifdef CONFIG_HARDLOCKUP_DETECTOR
> +#endif
> +#if defined(CONFIG_HAVE_NMI_WATCHDOG) || defined(CONFIG_HARDLOCKUP_DETECTOR)
> {
> .procname = "hardlockup_panic",
> .data = &hardlockup_panic,
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 03e0b69bb5bf..192125f3f8aa 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -29,15 +29,54 @@
> #include <linux/kvm_para.h>
> #include <linux/kthread.h>
>
> +/* Watchdog configuration */
> static DEFINE_MUTEX(watchdog_proc_mutex);
>
> #if defined(CONFIG_HAVE_NMI_WATCHDOG) || defined(CONFIG_HARDLOCKUP_DETECTOR)
> unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED|NMI_WATCHDOG_ENABLED;
> +int __read_mostly nmi_watchdog_enabled;
> +
> +/* boot commands */
> +/*
> + * Should we panic when a soft-lockup or hard-lockup occurs:
> + */
> +unsigned int __read_mostly hardlockup_panic =
> + CONFIG_BOOTPARAM_HARDLOCKUP_PANIC_VALUE;
> +/*
> + * 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 hardlockup_detector_disable(void)
> +{
> + watchdog_enabled &= ~NMI_WATCHDOG_ENABLED;
> +}
> +
> +static int __init hardlockup_panic_setup(char *str)
> +{
> + if (!strncmp(str, "panic", 5))
> + hardlockup_panic = 1;
> + else if (!strncmp(str, "nopanic", 7))
> + hardlockup_panic = 0;
> + else if (!strncmp(str, "0", 1))
> + watchdog_enabled &= ~NMI_WATCHDOG_ENABLED;
> + else if (!strncmp(str, "1", 1))
> + watchdog_enabled |= NMI_WATCHDOG_ENABLED;
> + return 1;
> +}
> +__setup("nmi_watchdog=", hardlockup_panic_setup);
> +
> #else
> unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED;
> #endif
> -int __read_mostly nmi_watchdog_enabled;
> +
> +#ifdef CONFIG_SOFTLOCKUP_DETECTOR
> int __read_mostly soft_watchdog_enabled;
> +#endif
> +
> int __read_mostly watchdog_user_enabled;
> int __read_mostly watchdog_thresh = 10;
>
> @@ -45,15 +84,9 @@ int __read_mostly watchdog_thresh = 10;
> int __read_mostly sysctl_softlockup_all_cpu_backtrace;
> int __read_mostly sysctl_hardlockup_all_cpu_backtrace;
> #endif
> -static struct cpumask watchdog_cpumask __read_mostly;
> +struct cpumask watchdog_cpumask __read_mostly;
> unsigned long *watchdog_cpumask_bits = cpumask_bits(&watchdog_cpumask);
>
> -/* Helper for online, unparked cpus. */
> -#define for_each_watchdog_cpu(cpu) \
> - for_each_cpu_and((cpu), cpu_online_mask, &watchdog_cpumask)
> -
> -atomic_t watchdog_park_in_progress = ATOMIC_INIT(0);
> -
> /*
> * The 'watchdog_running' variable is set to 1 when the watchdog threads
> * are registered/started and is set to 0 when the watchdog threads are
> @@ -72,7 +105,32 @@ static int __read_mostly watchdog_running;
> * of 'watchdog_running' cannot change while the watchdog is deactivated
> * temporarily (see related code in 'proc' handlers).
> */
> -static int __read_mostly watchdog_suspended;
> +int __read_mostly watchdog_suspended;
> +
> +/*
> + * These functions can be overridden if an architecture implements its
> + * own hardlockup detector.
> + */
> +int __weak watchdog_nmi_enable(unsigned int cpu)
> +{
> + return 0;
> +}
> +void __weak watchdog_nmi_disable(unsigned int cpu)
> +{
> +}
> +
> +void __weak watchdog_nmi_reconfigure(void)
> +{
> +}
> +
> +
> +#ifdef CONFIG_SOFTLOCKUP_DETECTOR
> +
> +/* Helper for online, unparked cpus. */
> +#define for_each_watchdog_cpu(cpu) \
> + for_each_cpu_and((cpu), cpu_online_mask, &watchdog_cpumask)
> +
> +atomic_t watchdog_park_in_progress = ATOMIC_INIT(0);
>
> static u64 __read_mostly sample_period;
>
> @@ -120,6 +178,7 @@ static int __init softlockup_all_cpu_backtrace_setup(char *str)
> return 1;
> }
> __setup("softlockup_all_cpu_backtrace=", softlockup_all_cpu_backtrace_setup);
> +#ifdef CONFIG_HARDLOCKUP_DETECTOR
> static int __init hardlockup_all_cpu_backtrace_setup(char *str)
> {
> sysctl_hardlockup_all_cpu_backtrace =
> @@ -128,6 +187,7 @@ static int __init hardlockup_all_cpu_backtrace_setup(char *str)
> }
> __setup("hardlockup_all_cpu_backtrace=", hardlockup_all_cpu_backtrace_setup);
> #endif
> +#endif
>
> /*
> * Hard-lockup warnings should be triggered after just a few seconds. Soft-
> @@ -213,18 +273,6 @@ void touch_softlockup_watchdog_sync(void)
> __this_cpu_write(watchdog_touch_ts, 0);
> }
>
> -/* watchdog detector functions */
> -bool is_hardlockup(void)
> -{
> - unsigned long hrint = __this_cpu_read(hrtimer_interrupts);
> -
> - if (__this_cpu_read(hrtimer_interrupts_saved) == hrint)
> - return true;
> -
> - __this_cpu_write(hrtimer_interrupts_saved, hrint);
> - return false;
> -}
> -
> static int is_softlockup(unsigned long touch_ts)
> {
> unsigned long now = get_timestamp();
> @@ -237,21 +285,21 @@ static int is_softlockup(unsigned long touch_ts)
> return 0;
> }
>
> -static void watchdog_interrupt_count(void)
> +/* watchdog detector functions */
> +bool is_hardlockup(void)
> {
> - __this_cpu_inc(hrtimer_interrupts);
> -}
> + unsigned long hrint = __this_cpu_read(hrtimer_interrupts);
>
> -/*
> - * These two functions are mostly architecture specific
> - * defining them as weak here.
> - */
> -int __weak watchdog_nmi_enable(unsigned int cpu)
> -{
> - return 0;
> + if (__this_cpu_read(hrtimer_interrupts_saved) == hrint)
> + return true;
> +
> + __this_cpu_write(hrtimer_interrupts_saved, hrint);
> + return false;
> }
> -void __weak watchdog_nmi_disable(unsigned int cpu)
> +
> +static void watchdog_interrupt_count(void)
> {
> + __this_cpu_inc(hrtimer_interrupts);
> }
>
> static int watchdog_enable_all_cpus(void);
> @@ -502,57 +550,6 @@ static void watchdog_unpark_threads(void)
> kthread_unpark(per_cpu(softlockup_watchdog, cpu));
> }
>
> -/*
> - * Suspend the hard and soft lockup detector by parking the watchdog threads.
> - */
> -int lockup_detector_suspend(void)
> -{
> - int ret = 0;
> -
> - get_online_cpus();
> - mutex_lock(&watchdog_proc_mutex);
> - /*
> - * Multiple suspend requests can be active in parallel (counted by
> - * the 'watchdog_suspended' variable). If the watchdog threads are
> - * running, the first caller takes care that they will be parked.
> - * The state of 'watchdog_running' cannot change while a suspend
> - * request is active (see related code in 'proc' handlers).
> - */
> - if (watchdog_running && !watchdog_suspended)
> - ret = watchdog_park_threads();
> -
> - if (ret == 0)
> - watchdog_suspended++;
> - else {
> - watchdog_disable_all_cpus();
> - pr_err("Failed to suspend lockup detectors, disabled\n");
> - watchdog_enabled = 0;
> - }
> -
> - mutex_unlock(&watchdog_proc_mutex);
> -
> - return ret;
> -}
> -
> -/*
> - * Resume the hard and soft lockup detector by unparking the watchdog threads.
> - */
> -void lockup_detector_resume(void)
> -{
> - mutex_lock(&watchdog_proc_mutex);
> -
> - watchdog_suspended--;
> - /*
> - * The watchdog threads are unparked if they were previously running
> - * and if there is no more active suspend request.
> - */
> - if (watchdog_running && !watchdog_suspended)
> - watchdog_unpark_threads();
> -
> - mutex_unlock(&watchdog_proc_mutex);
> - put_online_cpus();
> -}
> -
> static int update_watchdog_all_cpus(void)
> {
> int ret;
> @@ -604,6 +601,96 @@ static void watchdog_disable_all_cpus(void)
> }
> }
>
> +static int watchdog_update_cpus(void)
> +{
> + return smpboot_update_cpumask_percpu_thread(
> + &watchdog_threads, &watchdog_cpumask);
> +}
> +
> +#else /* SOFTLOCKUP */
> +static int watchdog_park_threads(void)
> +{
> + return 0;
> +}
> +
> +static void watchdog_unpark_threads(void)
> +{
> +}
> +
> +static int watchdog_enable_all_cpus(void)
> +{
> + return 0;
> +}
> +
> +static void watchdog_disable_all_cpus(void)
> +{
> +}
> +
> +static int watchdog_update_cpus(void)
> +{
> + return 0;
> +}
> +
> +static void set_sample_period(void)
> +{
> +}
> +#endif /* SOFTLOCKUP */
> +
> +/*
> + * Suspend the hard and soft lockup detector by parking the watchdog threads.
> + */
> +int lockup_detector_suspend(void)
> +{
> + int ret = 0;
> +
> + get_online_cpus();
> + mutex_lock(&watchdog_proc_mutex);
> + /*
> + * Multiple suspend requests can be active in parallel (counted by
> + * the 'watchdog_suspended' variable). If the watchdog threads are
> + * running, the first caller takes care that they will be parked.
> + * The state of 'watchdog_running' cannot change while a suspend
> + * request is active (see related code in 'proc' handlers).
> + */
> + if (watchdog_running && !watchdog_suspended)
> + ret = watchdog_park_threads();
> +
> + if (ret == 0)
> + watchdog_suspended++;
> + else {
> + watchdog_disable_all_cpus();
> + pr_err("Failed to suspend lockup detectors, disabled\n");
> + watchdog_enabled = 0;
> + }
> +
> + watchdog_nmi_reconfigure();
> +
> + mutex_unlock(&watchdog_proc_mutex);
> +
> + return ret;
> +}
> +
> +/*
> + * Resume the hard and soft lockup detector by unparking the watchdog threads.
> + */
> +void lockup_detector_resume(void)
> +{
> + mutex_lock(&watchdog_proc_mutex);
> +
> + watchdog_suspended--;
> + /*
> + * The watchdog threads are unparked if they were previously running
> + * and if there is no more active suspend request.
> + */
> + if (watchdog_running && !watchdog_suspended)
> + watchdog_unpark_threads();
> +
> + watchdog_nmi_reconfigure();
> +
> + mutex_unlock(&watchdog_proc_mutex);
> + put_online_cpus();
> +}
> +
> #ifdef CONFIG_SYSCTL
>
> /*
> @@ -625,6 +712,8 @@ static int proc_watchdog_update(void)
> else
> watchdog_disable_all_cpus();
>
> + watchdog_nmi_reconfigure();
> +
> return err;
>
> }
> @@ -810,10 +899,11 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
> * a temporary cpumask, so we are likely not in a
> * position to do much else to make things better.
> */
> - if (smpboot_update_cpumask_percpu_thread(
> - &watchdog_threads, &watchdog_cpumask) != 0)
> + if (watchdog_update_cpus() != 0)
> pr_err("cpumask update failed\n");
> }
> +
> + watchdog_nmi_reconfigure();
> }
> out:
> mutex_unlock(&watchdog_proc_mutex);
> diff --git a/kernel/watchdog_hld.c b/kernel/watchdog_hld.c
> index 54a427d1f344..2b328d6c0bc8 100644
> --- a/kernel/watchdog_hld.c
> +++ b/kernel/watchdog_hld.c
> @@ -22,39 +22,7 @@ static DEFINE_PER_CPU(bool, hard_watchdog_warn);
> static DEFINE_PER_CPU(bool, watchdog_nmi_touch);
> static DEFINE_PER_CPU(struct perf_event *, watchdog_ev);
>
> -/* boot commands */
> -/*
> - * Should we panic when a soft-lockup or hard-lockup occurs:
> - */
> -unsigned int __read_mostly hardlockup_panic =
> - CONFIG_BOOTPARAM_HARDLOCKUP_PANIC_VALUE;
> static unsigned long hardlockup_allcpu_dumped;
> -/*
> - * 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 hardlockup_detector_disable(void)
> -{
> - watchdog_enabled &= ~NMI_WATCHDOG_ENABLED;
> -}
> -
> -static int __init hardlockup_panic_setup(char *str)
> -{
> - if (!strncmp(str, "panic", 5))
> - hardlockup_panic = 1;
> - else if (!strncmp(str, "nopanic", 7))
> - hardlockup_panic = 0;
> - else if (!strncmp(str, "0", 1))
> - watchdog_enabled &= ~NMI_WATCHDOG_ENABLED;
> - else if (!strncmp(str, "1", 1))
> - watchdog_enabled |= NMI_WATCHDOG_ENABLED;
> - return 1;
> -}
> -__setup("nmi_watchdog=", hardlockup_panic_setup);
>
> void touch_nmi_watchdog(void)
> {
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index e4587ebe52c7..68685d3ddac0 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -801,14 +801,18 @@ config LOCKUP_DETECTOR
> The frequency of hrtimer and NMI events and the soft and hard lockup
> thresholds can be controlled through the sysctl watchdog_thresh.
>
> +config SOFTLOCKUP_DETECTOR
> + bool "Detect Soft Lockups"
> + depends on LOCKUP_DETECTOR
> +
> config HARDLOCKUP_DETECTOR
> - def_bool y
> - depends on LOCKUP_DETECTOR && !HAVE_NMI_WATCHDOG
> + bool "Detect Hard Lockups"
> + depends on SOFTLOCKUP_DETECTOR && !HAVE_NMI_WATCHDOG
> depends on PERF_EVENTS && HAVE_PERF_EVENTS_NMI
>
> config BOOTPARAM_HARDLOCKUP_PANIC
> bool "Panic (Reboot) On Hard Lockups"
> - depends on HARDLOCKUP_DETECTOR
> + depends on HARDLOCKUP_DETECTOR || HAVE_NMI_WATCHDOG
> help
> Say Y here to enable the kernel to panic on "hard lockups",
> which are bugs that cause the kernel to loop in kernel
> @@ -819,14 +823,14 @@ config BOOTPARAM_HARDLOCKUP_PANIC
>
> config BOOTPARAM_HARDLOCKUP_PANIC_VALUE
> int
> - depends on HARDLOCKUP_DETECTOR
> + depends on HARDLOCKUP_DETECTOR || HAVE_NMI_WATCHDOG
> range 0 1
> default 0 if !BOOTPARAM_HARDLOCKUP_PANIC
> default 1 if BOOTPARAM_HARDLOCKUP_PANIC
>
> config BOOTPARAM_SOFTLOCKUP_PANIC
> bool "Panic (Reboot) On Soft Lockups"
> - depends on LOCKUP_DETECTOR
> + depends on SOFTLOCKUP_DETECTOR
> help
> Say Y here to enable the kernel to panic on "soft lockups",
> which are bugs that cause the kernel to loop in kernel
> @@ -843,7 +847,7 @@ config BOOTPARAM_SOFTLOCKUP_PANIC
>
> config BOOTPARAM_SOFTLOCKUP_PANIC_VALUE
> int
> - depends on LOCKUP_DETECTOR
> + depends on SOFTLOCKUP_DETECTOR
> range 0 1
> default 0 if !BOOTPARAM_SOFTLOCKUP_PANIC
> default 1 if BOOTPARAM_SOFTLOCKUP_PANIC
> @@ -851,7 +855,7 @@ config BOOTPARAM_SOFTLOCKUP_PANIC_VALUE
> config DETECT_HUNG_TASK
> bool "Detect Hung Tasks"
> depends on DEBUG_KERNEL
> - default LOCKUP_DETECTOR
> + default SOFTLOCKUP_DETECTOR
> help
> Say Y here to enable the kernel to detect "hung tasks",
> which are bugs that cause the task to be stuck in
> --
> 2.11.0
>
