Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934366AbbDVIV4 (ORCPT ); Wed, 22 Apr 2015 04:21:56 -0400 Received: from mx4-phx2.redhat.com ([209.132.183.25]:56647 "EHLO mx4-phx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756062AbbDVIVx (ORCPT ); Wed, 22 Apr 2015 04:21:53 -0400 Date: Wed, 22 Apr 2015 04:20:55 -0400 (EDT) From: Ulrich Obergfell To: Chris Metcalf Cc: Frederic Weisbecker , Don Zickus , Ingo Molnar , Andrew Morton , Andrew Jones , chai wen , Fabian Frederick , Aaron Tomlin , Ben Zhang , Christoph Lameter , Gilad Ben-Yossef , Steven Rostedt , linux-kernel@vger.kernel.org, Jonathan Corbet , linux-doc@vger.kernel.org, Thomas Gleixner , Peter Zijlstra Message-ID: <2132240307.4651379.1429690855807.JavaMail.zimbra@redhat.com> In-Reply-To: <5531299C.60600@ezchip.com> References: <20150413215423.GA6121@lerouge> <1429040253-7054-1-git-send-email-cmetcalf@ezchip.com> <1429040253-7054-2-git-send-email-cmetcalf@ezchip.com> <755157528.1109591.1429181212104.JavaMail.zimbra@redhat.com> <5531299C.60600@ezchip.com> Subject: Re: [PATCH v8 2/3] watchdog: add watchdog_cpumask sysctl to assist nohz MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [10.3.224.7] X-Mailer: Zimbra 8.0.6_GA_5922 (ZimbraWebClient - FF22 (Linux)/8.0.6_GA_5922) Thread-Topic: watchdog: add watchdog_cpumask sysctl to assist nohz Thread-Index: fWpMmR6yEH2i5EI119H+rSvO3FRxww== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4617 Lines: 136 Chris, in principle the change looks o.k. to me, even though I'm not really familiar with the watchdog_nmi_disable_all() and watchdog_nmi_enable_all() functions. It is my understanding that those functions are only called once via 'initcall' early during kernel startup as shown in the following flow of execution: kernel_init { kernel_init_freeable { lockup_detector_init { cpumask_andnot(watchdog_cpumask, cpu_possible_mask,tick_nohz_full_mask) watchdog_enable_all_cpus smpboot_register_percpu_thread(&watchdog_threads) smpboot_update_cpumask_percpu_thread(&watchdog_threads,watchdog_cpumask) // here we make sure that watchdog threads don't run on nohz_full CPUs // only the watchdog threads of housekeeping CPUs keep on running } do_basic_setup do_initcalls do_initcall_level do_one_initcall fixup_ht_bug // subsys_initcall(fixup_ht_bug) { watchdog_nmi_disable_all // here we disable NMI watchdog only on housekeeping CPUs for_each_cpu_and(cpu,cpu_online_mask,watchdog_cpumask) watchdog_nmi_disable watchdog_nmi_enable_all // here we enable NMI watchdog only on housekeeping CPUs for_each_cpu_and(cpu,cpu_online_mask,watchdog_cpumask) watchdog_nmi_enable } } } It seems crucial that lockup_detector_init() is executed before fixup_ht_bug(). Regards, Uli On 04/16/2015 06:46 AM, Ulrich Obergfell wrote: > if a user changes watchdog parameters in /proc/sys/kernel, the watchdog threads > are not stopped and restarted in all cases. Parameters can also be changed 'on > the fly', for example like 'watchdog_thresh' in the following flow of execution: > > proc_watchdog_thresh > proc_watchdog_update > if (watchdog_enabled && watchdog_thresh) > watchdog_enable_all_cpus > if (!watchdog_running) { > // watchdog threads are already running so we don't get here > } else { > update_watchdog_all_cpus > for_each_online_cpu <-----------------------------. > update_watchdog | > watchdog_nmi_disable | > watchdog_nmi_enable | > } | > | > I think we would not want to call watchdog_nmi_enable() for each_online_ CPU, > but rather for each CPU that has an_unparked_ watchdog thread (i.e. where the > watchdog mechanism is actually enabled). How about something like this? I'll fold it into v9 of the patchset. Thanks! diff --git a/kernel/watchdog.c b/kernel/watchdog.c index 0c5a37cdbedd..a4e1c9a2e769 100644 --- a/kernel/watchdog.c +++ b/kernel/watchdog.c @@ -61,6 +61,10 @@ static cpumask_var_t watchdog_cpumask_for_smpboot; static cpumask_var_t watchdog_cpumask; unsigned long *watchdog_cpumask_bits; +/* Helper for online, unparked cpus. */ +#define for_each_watchdog_cpu(cpu) \ + for_each_cpu_and((cpu), cpu_online_mask, watchdog_cpumask) + static int __read_mostly watchdog_running; static u64 __read_mostly sample_period; @@ -209,7 +213,7 @@ void touch_all_softlockup_watchdogs(void) * do we care if a 0 races with a timestamp? * all it means is the softlock check starts one cycle later */ - for_each_online_cpu(cpu) + for_each_watchdog_cpu(cpu) per_cpu(watchdog_touch_ts, cpu) = 0; } @@ -616,7 +620,7 @@ void watchdog_nmi_enable_all(void) return; get_online_cpus(); - for_each_online_cpu(cpu) + for_each_watchdog_cpu(cpu) watchdog_nmi_enable(cpu); put_online_cpus(); } @@ -629,7 +633,7 @@ void watchdog_nmi_disable_all(void) return; get_online_cpus(); - for_each_online_cpu(cpu) + for_each_watchdog_cpu(cpu) watchdog_nmi_disable(cpu); put_online_cpus(); } @@ -688,7 +692,7 @@ static void update_watchdog_all_cpus(void) int cpu; get_online_cpus(); - for_each_online_cpu(cpu) + for_each_watchdog_cpu(cpu) update_watchdog(cpu); put_online_cpus(); } -- Chris Metcalf, EZChip Semiconductor http://www.ezchip.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/