From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758123AbcJYA4Z (ORCPT ); Mon, 24 Oct 2016 20:56:25 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:38020 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757984AbcJYA4X (ORCPT ); Mon, 24 Oct 2016 20:56:23 -0400 Subject: Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable To: Don Zickus References: <1476391082-77928-1-git-send-email-babu.moger@oracle.com> <1476391082-77928-2-git-send-email-babu.moger@oracle.com> <20161019170012.6006d06d9326e62a8059fd08@linux-foundation.org> <20161020161414.GE35881@redhat.com> <20161020202527.b01839356c6d34ed0cba3569@linux-foundation.org> <20161021151114.GC35881@redhat.com> <20161021121914.b7e541782dc18868c87de068@linux-foundation.org> <8898d56a-3344-f37f-2697-ce327323042c@oracle.com> <20161024151950.GS35881@redhat.com> Cc: Andrew Morton , mingo@kernel.org, ak@linux.intel.com, jkosina@suse.cz, baiyaowei@cmss.chinamobile.com, atomlin@redhat.com, uobergfe@redhat.com, tj@kernel.org, hidehiro.kawai.ez@hitachi.com, johunt@akamai.com, davem@davemloft.net, sparclinux@vger.kernel.org, linux-kernel@vger.kernel.org, sam@ravnborg.org From: Babu Moger Organization: Oracle Corporation Message-ID: <30bb7aea-5bcc-31b6-5a8b-b99162b1c35a@oracle.com> Date: Mon, 24 Oct 2016 19:55:50 -0500 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161024151950.GS35881@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Source-IP: aserv0021.oracle.com [141.146.126.233] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/24/2016 10:19 AM, Don Zickus wrote: > On Fri, Oct 21, 2016 at 04:50:21PM -0500, Babu Moger wrote: >> Don, >> >> On 10/21/2016 2:19 PM, Andrew Morton wrote: >>> On Fri, 21 Oct 2016 11:11:14 -0400 Don Zickus wrote: >>> >>>> On Thu, Oct 20, 2016 at 08:25:27PM -0700, Andrew Morton wrote: >>>>> On Thu, 20 Oct 2016 12:14:14 -0400 Don Zickus wrote: >>>>> >>>>>>>> -static int watchdog_nmi_enable(unsigned int cpu) { return 0; } >>>>>>>> -static void watchdog_nmi_disable(unsigned int cpu) { return; } >>>>>>>> +/* >>>>>>>> + * These two functions are mostly architecture specific >>>>>>>> + * defining them as weak here. >>>>>>>> + */ >>>>>>>> +int __weak arch_watchdog_nmi_enable(unsigned int cpu) { return 0; } >>>>>>>> +void __weak arch_watchdog_nmi_disable(unsigned int cpu) { return; } >>>>>>>> + >>>>>>>> #endif /* CONFIG_HARDLOCKUP_DETECTOR */ >>>>>>> This is a strange way of using __weak. >>>>>>> >>>>>>> Take a look at (one of many examples) kernel/module.c:module_alloc(). >>>>>>> We simply provide a default implementation and some other compilation >>>>>>> unit can override (actually replace) that at link time. No strange >>>>>>> ifdeffing needed. >>>>>> Yeah, this is mostly because of how we enable the hardlockup detector. >>>>>> >>>>>> Some arches use the perf hw and enable CONFIG_HARDLOCKUP_DETECTOR. Other >>>>>> arches just use their own variant of nmi and set CONFIG_HAVE_NMI_WATCHDOG and >>>>>> the rest of the arches do not use this. >>>>>> >>>>>> So the thought was if CONFIG_HARDLOCKUP_DETECTOR use that implementation, >>>>>> everyone else use the __weak version. Then the arches like sparc can override >>>>>> the weak version with their own nmi enablement. >>>>>> >>>>>> I don't know how to represent those 3 states correctly and the above is what >>>>>> we end up with. >>>>> >>>>> >>>>> Is there a suitable site where we could capture these considerations in >>>>> a code comment? >>>> Hi Andrew, >>>> >>>> I am not sure I understand your question. When you say 'site', are you >>>> referring to the kernel/watchdog.c file? >>> Yes, somewhere in there I guess. >>> >>> The problem with this sort of thing is that the implementation is >>> splattered over multiple places in one file or in several files so >>> there's no clear place to document what's happening. But I think this >>> situation *should* be documented somewhere. Or maybe that just isn't >>> worthwhile - feel free to disagree! >>> >>>> The other approach that might help de-clutter this file, is to pull out the >>>> HARDLOCKUP_DETECTOR changes (as they are arch specific) and move it to say >>>> kernel/watchdog_hw_ld.c. Then all the nmi hooks in kernel/watchdog.c can be >>>> __weak and overridden by the kernel_watchdog_hw_ld.c file or the sparc >>>> files. >>>> >>>> This would leave kernel/watchdog.c with just a framework and the >>>> arch-agnostic softlockup detector. Probably easier to read and digest. >> Don, Yes. I am fine with your idea. Let me know if you need any help here. >> If you want I can >> start working this cleanup myself. I might take sometime as I need to spend >> sometime >> understanding the whole watchdog stuff first. If you have already started >> working on this >> then I will let you continue. > Hi Babu, > > Feel free to start looking at it. I am trying to wrap up a couple of things > here and will only be able to little poke at it the next couple of days. > But for the most part you might be able to rip out anything with > CONFIG_HARDLOCKUP_DETECTOR and put it into another file. Then just clean up > the pieces. Don. Sure. I have started on this. Will send RFC version sometime this week. > > Cheers, > Don > >>> Well, it depends how the code ends up looking. It's best to separate >>> functional changes from cleanups. Generally I think it's best to do >>> "cleanup comes first", because it's then simpler to revert the >>> functional change if it has problems. Plus people are more >>> *interested* in the functional change so it's best to have that at >>> top-of-tree. >>> From mboxrd@z Thu Jan 1 00:00:00 1970 From: Babu Moger Date: Tue, 25 Oct 2016 00:55:50 +0000 Subject: Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable Message-Id: <30bb7aea-5bcc-31b6-5a8b-b99162b1c35a@oracle.com> List-Id: References: <1476391082-77928-1-git-send-email-babu.moger@oracle.com> <1476391082-77928-2-git-send-email-babu.moger@oracle.com> <20161019170012.6006d06d9326e62a8059fd08@linux-foundation.org> <20161020161414.GE35881@redhat.com> <20161020202527.b01839356c6d34ed0cba3569@linux-foundation.org> <20161021151114.GC35881@redhat.com> <20161021121914.b7e541782dc18868c87de068@linux-foundation.org> <8898d56a-3344-f37f-2697-ce327323042c@oracle.com> <20161024151950.GS35881@redhat.com> In-Reply-To: <20161024151950.GS35881@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Don Zickus Cc: Andrew Morton , mingo@kernel.org, ak@linux.intel.com, jkosina@suse.cz, baiyaowei@cmss.chinamobile.com, atomlin@redhat.com, uobergfe@redhat.com, tj@kernel.org, hidehiro.kawai.ez@hitachi.com, johunt@akamai.com, davem@davemloft.net, sparclinux@vger.kernel.org, linux-kernel@vger.kernel.org, sam@ravnborg.org On 10/24/2016 10:19 AM, Don Zickus wrote: > On Fri, Oct 21, 2016 at 04:50:21PM -0500, Babu Moger wrote: >> Don, >> >> On 10/21/2016 2:19 PM, Andrew Morton wrote: >>> On Fri, 21 Oct 2016 11:11:14 -0400 Don Zickus wrote: >>> >>>> On Thu, Oct 20, 2016 at 08:25:27PM -0700, Andrew Morton wrote: >>>>> On Thu, 20 Oct 2016 12:14:14 -0400 Don Zickus wrote: >>>>> >>>>>>>> -static int watchdog_nmi_enable(unsigned int cpu) { return 0; } >>>>>>>> -static void watchdog_nmi_disable(unsigned int cpu) { return; } >>>>>>>> +/* >>>>>>>> + * These two functions are mostly architecture specific >>>>>>>> + * defining them as weak here. >>>>>>>> + */ >>>>>>>> +int __weak arch_watchdog_nmi_enable(unsigned int cpu) { return 0; } >>>>>>>> +void __weak arch_watchdog_nmi_disable(unsigned int cpu) { return; } >>>>>>>> + >>>>>>>> #endif /* CONFIG_HARDLOCKUP_DETECTOR */ >>>>>>> This is a strange way of using __weak. >>>>>>> >>>>>>> Take a look at (one of many examples) kernel/module.c:module_alloc(). >>>>>>> We simply provide a default implementation and some other compilation >>>>>>> unit can override (actually replace) that at link time. No strange >>>>>>> ifdeffing needed. >>>>>> Yeah, this is mostly because of how we enable the hardlockup detector. >>>>>> >>>>>> Some arches use the perf hw and enable CONFIG_HARDLOCKUP_DETECTOR. Other >>>>>> arches just use their own variant of nmi and set CONFIG_HAVE_NMI_WATCHDOG and >>>>>> the rest of the arches do not use this. >>>>>> >>>>>> So the thought was if CONFIG_HARDLOCKUP_DETECTOR use that implementation, >>>>>> everyone else use the __weak version. Then the arches like sparc can override >>>>>> the weak version with their own nmi enablement. >>>>>> >>>>>> I don't know how to represent those 3 states correctly and the above is what >>>>>> we end up with. >>>>> >>>>> >>>>> Is there a suitable site where we could capture these considerations in >>>>> a code comment? >>>> Hi Andrew, >>>> >>>> I am not sure I understand your question. When you say 'site', are you >>>> referring to the kernel/watchdog.c file? >>> Yes, somewhere in there I guess. >>> >>> The problem with this sort of thing is that the implementation is >>> splattered over multiple places in one file or in several files so >>> there's no clear place to document what's happening. But I think this >>> situation *should* be documented somewhere. Or maybe that just isn't >>> worthwhile - feel free to disagree! >>> >>>> The other approach that might help de-clutter this file, is to pull out the >>>> HARDLOCKUP_DETECTOR changes (as they are arch specific) and move it to say >>>> kernel/watchdog_hw_ld.c. Then all the nmi hooks in kernel/watchdog.c can be >>>> __weak and overridden by the kernel_watchdog_hw_ld.c file or the sparc >>>> files. >>>> >>>> This would leave kernel/watchdog.c with just a framework and the >>>> arch-agnostic softlockup detector. Probably easier to read and digest. >> Don, Yes. I am fine with your idea. Let me know if you need any help here. >> If you want I can >> start working this cleanup myself. I might take sometime as I need to spend >> sometime >> understanding the whole watchdog stuff first. If you have already started >> working on this >> then I will let you continue. > Hi Babu, > > Feel free to start looking at it. I am trying to wrap up a couple of things > here and will only be able to little poke at it the next couple of days. > But for the most part you might be able to rip out anything with > CONFIG_HARDLOCKUP_DETECTOR and put it into another file. Then just clean up > the pieces. Don. Sure. I have started on this. Will send RFC version sometime this week. > > Cheers, > Don > >>> Well, it depends how the code ends up looking. It's best to separate >>> functional changes from cleanups. Generally I think it's best to do >>> "cleanup comes first", because it's then simpler to revert the >>> functional change if it has problems. Plus people are more >>> *interested* in the functional change so it's best to have that at >>> top-of-tree. >>>