Re: [RFC PATCH 12/23] kernel/watchdog: Introduce a struct for NMI watchdog operations
On Thu, Jun 14, 2018 at 12:32:50PM +1000, Nicholas Piggin wrote: > On Wed, 13 Jun 2018 18:31:17 -0700 > Ricardo Neri wrote: > > > On Wed, Jun 13, 2018 at 09:52:25PM +1000, Nicholas Piggin wrote: > > > On Wed, 13 Jun 2018 11:26:49 +0200 (CEST) > > > Thomas Gleixner wrote: > > > > > > > On Wed, 13 Jun 2018, Peter Zijlstra wrote: > > > > > On Wed, Jun 13, 2018 at 05:41:41PM +1000, Nicholas Piggin wrote: > > > > > > On Tue, 12 Jun 2018 17:57:32 -0700 > > > > > > Ricardo Neri wrote: > > > > > > > > > > > > > Instead of exposing individual functions for the operations of > > > > > > > the NMI > > > > > > > watchdog, define a common interface that can be used across > > > > > > > multiple > > > > > > > implementations. > > > > > > > > > > > > > > The struct nmi_watchdog_ops is defined for such operations. These > > > > > > > initial > > > > > > > definitions include the enable, disable, start, stop, and cleanup > > > > > > > operations. > > > > > > > > > > > > > > Only a single NMI watchdog can be used in the system. The > > > > > > > operations of > > > > > > > this NMI watchdog are accessed via the new variable nmi_wd_ops. > > > > > > > This > > > > > > > variable is set to point the operations of the first NMI watchdog > > > > > > > that > > > > > > > initializes successfully. Even though at this moment, the only > > > > > > > available > > > > > > > NMI watchdog is the perf-based hardlockup detector. More > > > > > > > implementations > > > > > > > can be added in the future. > > > > > > > > > > > > Cool, this looks pretty nice at a quick glance. sparc and powerpc at > > > > > > least have their own NMI watchdogs, it would be good to have those > > > > > > converted as well. > > > > > > > > > > Yeah, agreed, this looks like half a patch. > > > > > > > > Though I'm not seeing the advantage of it. That kind of NMI watchdogs > > > > are > > > > low level architecture details so having yet another 'ops' data > > > > structure > > > > with a gazillion of callbacks, checks and indirections does not provide > > > > value over the currently available weak stubs. > > > > > > The other way to go of course is librify the perf watchdog and make an > > > x86 watchdog that selects between perf and hpet... I also probably > > > prefer that for code such as this, but I wouldn't strongly object to > > > ops struct if I'm not writing the code. It's not that bad is it? > > > > My motivation to add the ops was that the hpet and perf watchdog share > > significant portions of code. > > Right, a good motivation. > > > I could look into creating the library for > > common code and relocate the hpet watchdog into arch/x86 for the hpet- > > specific parts. > > If you can investigate that approach, that would be appreciated. I hope > I did not misunderstand you there, Thomas. > > Basically you would have perf infrastructure and hpet infrastructure, > and then the x86 watchdog driver will use one or the other of those. The > generic watchdog driver will be just a simple shim that uses the perf > infrastructure. Then hopefully the powerpc driver would require almost > no change. Sure, I will try to structure code to minimize the changes to the powerpc watchdog... without breaking the sparc one. Thanks and BR, Ricardo ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 12/23] kernel/watchdog: Introduce a struct for NMI watchdog operations
On Thu, 14 Jun 2018, Nicholas Piggin wrote: > On Wed, 13 Jun 2018 18:31:17 -0700 > > I could look into creating the library for > > common code and relocate the hpet watchdog into arch/x86 for the hpet- > > specific parts. > > If you can investigate that approach, that would be appreciated. I hope > I did not misunderstand you there, Thomas. I'm not against cleanups and consolidation, quite the contrary. But this stuff just adds new infrastructure w/o showing that it's actually a cleanup and consolidation. Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 12/23] kernel/watchdog: Introduce a struct for NMI watchdog operations
On Wed, 13 Jun 2018 18:31:17 -0700 Ricardo Neri wrote: > On Wed, Jun 13, 2018 at 09:52:25PM +1000, Nicholas Piggin wrote: > > On Wed, 13 Jun 2018 11:26:49 +0200 (CEST) > > Thomas Gleixner wrote: > > > > > On Wed, 13 Jun 2018, Peter Zijlstra wrote: > > > > On Wed, Jun 13, 2018 at 05:41:41PM +1000, Nicholas Piggin wrote: > > > > > On Tue, 12 Jun 2018 17:57:32 -0700 > > > > > Ricardo Neri wrote: > > > > > > > > > > > Instead of exposing individual functions for the operations of the > > > > > > NMI > > > > > > watchdog, define a common interface that can be used across multiple > > > > > > implementations. > > > > > > > > > > > > The struct nmi_watchdog_ops is defined for such operations. These > > > > > > initial > > > > > > definitions include the enable, disable, start, stop, and cleanup > > > > > > operations. > > > > > > > > > > > > Only a single NMI watchdog can be used in the system. The > > > > > > operations of > > > > > > this NMI watchdog are accessed via the new variable nmi_wd_ops. This > > > > > > variable is set to point the operations of the first NMI watchdog > > > > > > that > > > > > > initializes successfully. Even though at this moment, the only > > > > > > available > > > > > > NMI watchdog is the perf-based hardlockup detector. More > > > > > > implementations > > > > > > can be added in the future. > > > > > > > > > > Cool, this looks pretty nice at a quick glance. sparc and powerpc at > > > > > least have their own NMI watchdogs, it would be good to have those > > > > > converted as well. > > > > > > > > Yeah, agreed, this looks like half a patch. > > > > > > Though I'm not seeing the advantage of it. That kind of NMI watchdogs are > > > low level architecture details so having yet another 'ops' data structure > > > with a gazillion of callbacks, checks and indirections does not provide > > > value over the currently available weak stubs. > > > > The other way to go of course is librify the perf watchdog and make an > > x86 watchdog that selects between perf and hpet... I also probably > > prefer that for code such as this, but I wouldn't strongly object to > > ops struct if I'm not writing the code. It's not that bad is it? > > My motivation to add the ops was that the hpet and perf watchdog share > significant portions of code. Right, a good motivation. > I could look into creating the library for > common code and relocate the hpet watchdog into arch/x86 for the hpet- > specific parts. If you can investigate that approach, that would be appreciated. I hope I did not misunderstand you there, Thomas. Basically you would have perf infrastructure and hpet infrastructure, and then the x86 watchdog driver will use one or the other of those. The generic watchdog driver will be just a simple shim that uses the perf infrastructure. Then hopefully the powerpc driver would require almost no change. Thanks, Nick ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 12/23] kernel/watchdog: Introduce a struct for NMI watchdog operations
On Wed, Jun 13, 2018 at 09:52:25PM +1000, Nicholas Piggin wrote: > On Wed, 13 Jun 2018 11:26:49 +0200 (CEST) > Thomas Gleixner wrote: > > > On Wed, 13 Jun 2018, Peter Zijlstra wrote: > > > On Wed, Jun 13, 2018 at 05:41:41PM +1000, Nicholas Piggin wrote: > > > > On Tue, 12 Jun 2018 17:57:32 -0700 > > > > Ricardo Neri wrote: > > > > > > > > > Instead of exposing individual functions for the operations of the NMI > > > > > watchdog, define a common interface that can be used across multiple > > > > > implementations. > > > > > > > > > > The struct nmi_watchdog_ops is defined for such operations. These > > > > > initial > > > > > definitions include the enable, disable, start, stop, and cleanup > > > > > operations. > > > > > > > > > > Only a single NMI watchdog can be used in the system. The operations > > > > > of > > > > > this NMI watchdog are accessed via the new variable nmi_wd_ops. This > > > > > variable is set to point the operations of the first NMI watchdog that > > > > > initializes successfully. Even though at this moment, the only > > > > > available > > > > > NMI watchdog is the perf-based hardlockup detector. More > > > > > implementations > > > > > can be added in the future. > > > > > > > > Cool, this looks pretty nice at a quick glance. sparc and powerpc at > > > > least have their own NMI watchdogs, it would be good to have those > > > > converted as well. > > > > > > Yeah, agreed, this looks like half a patch. > > > > Though I'm not seeing the advantage of it. That kind of NMI watchdogs are > > low level architecture details so having yet another 'ops' data structure > > with a gazillion of callbacks, checks and indirections does not provide > > value over the currently available weak stubs. > > The other way to go of course is librify the perf watchdog and make an > x86 watchdog that selects between perf and hpet... I also probably > prefer that for code such as this, but I wouldn't strongly object to > ops struct if I'm not writing the code. It's not that bad is it? My motivation to add the ops was that the hpet and perf watchdog share significant portions of code. I could look into creating the library for common code and relocate the hpet watchdog into arch/x86 for the hpet- specific parts. Thanks and BR, Ricardo ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 12/23] kernel/watchdog: Introduce a struct for NMI watchdog operations
On Wed, Jun 13, 2018 at 10:42:19AM +0200, Peter Zijlstra wrote: > On Wed, Jun 13, 2018 at 05:41:41PM +1000, Nicholas Piggin wrote: > > On Tue, 12 Jun 2018 17:57:32 -0700 > > Ricardo Neri wrote: > > > > > Instead of exposing individual functions for the operations of the NMI > > > watchdog, define a common interface that can be used across multiple > > > implementations. > > > > > > The struct nmi_watchdog_ops is defined for such operations. These initial > > > definitions include the enable, disable, start, stop, and cleanup > > > operations. > > > > > > Only a single NMI watchdog can be used in the system. The operations of > > > this NMI watchdog are accessed via the new variable nmi_wd_ops. This > > > variable is set to point the operations of the first NMI watchdog that > > > initializes successfully. Even though at this moment, the only available > > > NMI watchdog is the perf-based hardlockup detector. More implementations > > > can be added in the future. > > > > Cool, this looks pretty nice at a quick glance. sparc and powerpc at > > least have their own NMI watchdogs, it would be good to have those > > converted as well. > > Yeah, agreed, this looks like half a patch. I planned to look into the conversion of sparc and powerpc. I just wanted to see the reception to these patches before jumping and do potentially useless work. Comments in this thread lean towards keep using the weak stubs. Thanks and BR, Ricardo ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 12/23] kernel/watchdog: Introduce a struct for NMI watchdog operations
On Wed, 13 Jun 2018 11:26:49 +0200 (CEST) Thomas Gleixner wrote: > On Wed, 13 Jun 2018, Peter Zijlstra wrote: > > On Wed, Jun 13, 2018 at 05:41:41PM +1000, Nicholas Piggin wrote: > > > On Tue, 12 Jun 2018 17:57:32 -0700 > > > Ricardo Neri wrote: > > > > > > > Instead of exposing individual functions for the operations of the NMI > > > > watchdog, define a common interface that can be used across multiple > > > > implementations. > > > > > > > > The struct nmi_watchdog_ops is defined for such operations. These > > > > initial > > > > definitions include the enable, disable, start, stop, and cleanup > > > > operations. > > > > > > > > Only a single NMI watchdog can be used in the system. The operations of > > > > this NMI watchdog are accessed via the new variable nmi_wd_ops. This > > > > variable is set to point the operations of the first NMI watchdog that > > > > initializes successfully. Even though at this moment, the only available > > > > NMI watchdog is the perf-based hardlockup detector. More implementations > > > > can be added in the future. > > > > > > Cool, this looks pretty nice at a quick glance. sparc and powerpc at > > > least have their own NMI watchdogs, it would be good to have those > > > converted as well. > > > > Yeah, agreed, this looks like half a patch. > > Though I'm not seeing the advantage of it. That kind of NMI watchdogs are > low level architecture details so having yet another 'ops' data structure > with a gazillion of callbacks, checks and indirections does not provide > value over the currently available weak stubs. The other way to go of course is librify the perf watchdog and make an x86 watchdog that selects between perf and hpet... I also probably prefer that for code such as this, but I wouldn't strongly object to ops struct if I'm not writing the code. It's not that bad is it? Thanks, Nick ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 12/23] kernel/watchdog: Introduce a struct for NMI watchdog operations
On Wed, 13 Jun 2018, Peter Zijlstra wrote: > On Wed, Jun 13, 2018 at 05:41:41PM +1000, Nicholas Piggin wrote: > > On Tue, 12 Jun 2018 17:57:32 -0700 > > Ricardo Neri wrote: > > > > > Instead of exposing individual functions for the operations of the NMI > > > watchdog, define a common interface that can be used across multiple > > > implementations. > > > > > > The struct nmi_watchdog_ops is defined for such operations. These initial > > > definitions include the enable, disable, start, stop, and cleanup > > > operations. > > > > > > Only a single NMI watchdog can be used in the system. The operations of > > > this NMI watchdog are accessed via the new variable nmi_wd_ops. This > > > variable is set to point the operations of the first NMI watchdog that > > > initializes successfully. Even though at this moment, the only available > > > NMI watchdog is the perf-based hardlockup detector. More implementations > > > can be added in the future. > > > > Cool, this looks pretty nice at a quick glance. sparc and powerpc at > > least have their own NMI watchdogs, it would be good to have those > > converted as well. > > Yeah, agreed, this looks like half a patch. Though I'm not seeing the advantage of it. That kind of NMI watchdogs are low level architecture details so having yet another 'ops' data structure with a gazillion of callbacks, checks and indirections does not provide value over the currently available weak stubs. > > Is hpet a cross platform thing, or just x86? We should avoid > > proliferation of files under kernel/ I think, so with these watchdog > > driver structs then maybe implementations could go in drivers/ or > > arch/ > > HPET is mostly an x86 thing (altough it can be found elsewhere), but the On ia64 and I doubt that anyone wants to take on the task of underwater welding it to Itanic. > whole thing relies on the x86 NMI mechanism and is thus firmly arch/ > material (like the sparc and ppc thing). Right. Trying to make this 'generic' is not really solving anything. Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 12/23] kernel/watchdog: Introduce a struct for NMI watchdog operations
On Wed, Jun 13, 2018 at 05:41:41PM +1000, Nicholas Piggin wrote: > On Tue, 12 Jun 2018 17:57:32 -0700 > Ricardo Neri wrote: > > > Instead of exposing individual functions for the operations of the NMI > > watchdog, define a common interface that can be used across multiple > > implementations. > > > > The struct nmi_watchdog_ops is defined for such operations. These initial > > definitions include the enable, disable, start, stop, and cleanup > > operations. > > > > Only a single NMI watchdog can be used in the system. The operations of > > this NMI watchdog are accessed via the new variable nmi_wd_ops. This > > variable is set to point the operations of the first NMI watchdog that > > initializes successfully. Even though at this moment, the only available > > NMI watchdog is the perf-based hardlockup detector. More implementations > > can be added in the future. > > Cool, this looks pretty nice at a quick glance. sparc and powerpc at > least have their own NMI watchdogs, it would be good to have those > converted as well. Yeah, agreed, this looks like half a patch. > Is hpet a cross platform thing, or just x86? We should avoid > proliferation of files under kernel/ I think, so with these watchdog > driver structs then maybe implementations could go in drivers/ or > arch/ HPET is mostly an x86 thing (altough it can be found elsewhere), but the whole thing relies on the x86 NMI mechanism and is thus firmly arch/ material (like the sparc and ppc thing). ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 12/23] kernel/watchdog: Introduce a struct for NMI watchdog operations
On Tue, 12 Jun 2018 17:57:32 -0700 Ricardo Neri wrote: > Instead of exposing individual functions for the operations of the NMI > watchdog, define a common interface that can be used across multiple > implementations. > > The struct nmi_watchdog_ops is defined for such operations. These initial > definitions include the enable, disable, start, stop, and cleanup > operations. > > Only a single NMI watchdog can be used in the system. The operations of > this NMI watchdog are accessed via the new variable nmi_wd_ops. This > variable is set to point the operations of the first NMI watchdog that > initializes successfully. Even though at this moment, the only available > NMI watchdog is the perf-based hardlockup detector. More implementations > can be added in the future. Cool, this looks pretty nice at a quick glance. sparc and powerpc at least have their own NMI watchdogs, it would be good to have those converted as well. Is hpet a cross platform thing, or just x86? We should avoid proliferation of files under kernel/ I think, so with these watchdog driver structs then maybe implementations could go in drivers/ or arch/ Thanks, Nick ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
