Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752034AbbHEIK3 (ORCPT ); Wed, 5 Aug 2015 04:10:29 -0400 Received: from mx6-phx2.redhat.com ([209.132.183.39]:39120 "EHLO mx6-phx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751468AbbHEIKN (ORCPT ); Wed, 5 Aug 2015 04:10:13 -0400 Date: Wed, 5 Aug 2015 04:10:00 -0400 (EDT) From: Ulrich Obergfell To: Andrew Morton Cc: linux-kernel@vger.kernel.org, dzickus@redhat.com, atomlin@redhat.com, jolsa@kernel.org, mhocko@suse.cz, eranian@google.com, cmetcalf@ezchip.com, fweisbec@gmail.com Message-ID: <1748046785.4567339.1438762200771.JavaMail.zimbra@redhat.com> In-Reply-To: <20150804155848.97bfe8bdf86a97e166bc32d9@linux-foundation.org> References: <1438433365-2979-1-git-send-email-uobergfe@redhat.com> <1438433365-2979-3-git-send-email-uobergfe@redhat.com> <20150804155848.97bfe8bdf86a97e166bc32d9@linux-foundation.org> Subject: Re: [PATCH 2/4] watchdog: introduce watchdog_suspend() and watchdog_resume() MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Originating-IP: [10.36.5.188] X-Mailer: Zimbra 8.0.6_GA_5922 (ZimbraWebClient - FF22 (Linux)/8.0.6_GA_5922) Thread-Topic: watchdog: introduce watchdog_suspend() and watchdog_resume() Thread-Index: +yGWBK4HcZ2kF9ngW4rMR09KtjFsqA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3458 Lines: 85 > ----- Original Message ----- > From: "Andrew Morton" > ... > On Sat, 1 Aug 2015 14:49:23 +0200 Ulrich Obergfell wrote: > >> This interface can be utilized to deactivate the hard and soft lockup >> detector temporarily. Callers are expected to minimize the duration of >> deactivation. Multiple deactivations are allowed to occur in parallel >> but should be rare in practice. >> >> ... >> >> --- a/include/linux/nmi.h >> +++ b/include/linux/nmi.h >> @@ -80,6 +80,8 @@ extern int proc_watchdog_thresh(struct ctl_table *, int , >> void __user *, size_t *, loff_t *); >> extern int proc_watchdog_cpumask(struct ctl_table *, int, >> void __user *, size_t *, loff_t *); >> +extern int watchdog_suspend(void); >> +extern void watchdog_resume(void); >> #endif >> >> #ifdef CONFIG_HAVE_ACPI_APEI_NMI >> diff --git a/kernel/watchdog.c b/kernel/watchdog.c >> index 5571f20..98d44b1 100644 >> --- a/kernel/watchdog.c >> +++ b/kernel/watchdog.c >> @@ -67,6 +67,7 @@ unsigned long *watchdog_cpumask_bits = cpumask_bits(&watchdog_cpumask); >> #define for_each_watchdog_cpu(cpu) \ >> for_each_cpu_and((cpu), cpu_online_mask, &watchdog_cpumask) >> >> +static int __read_mostly watchdog_suspended = 0; > > With my compiler the "= 0" increases the size of watchdog.o data. For > some reason by 16 bytes(!). I see that you already fixed this. Many Thanks. >> static int __read_mostly watchdog_running; >> static u64 __read_mostly sample_period; > > The relationship between watchdog_running and watchdog_suspended hurts > my brain a bit. It appears that all watchdog_running transitions > happen under watchdog_proc_mutex so I don't think it's racy, but I > wonder if things would be simpler if these were folded into a single > up/down counter. The 'watchdog_running' variable indicates whether watchdog threads exist. [Whether they have been launched via smpboot_register_percpu_thread().] The 'watchdog_suspended' variable indicates whether existing threads are currently parked, and whether multiple requests to suspend the watchdog have been made by different callers. I think folding them into one variable would not improve the readability of the code, because instead of two variables with distinct semantics we would then have one variable with multiple semantics (which I think would also make code more complex). If we would want to improve the readability I'd prefer to either just add a comment block to explain the semantics of the variables or to rename them -for example- like: watchdog_running to watchdog_threads_exist watchdog_suspended to watchdog_suspend_count Please let me know if you would want any changes in this regard. I also received these suggestions: - rename the watchdog_{suspend|resume} functions as discussed in http://marc.info/?l=linux-kernel&m=143844050610220&w=2 - use pr_debug() instead of pr_info() as discussed in http://marc.info/?l=linux-kernel&m=143869949229461&w=2 Please let me know if you want me to post a 'version 2' of the patch set or if you want me to post these changes as separate follow-up patches. Regards, Uli -- 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/