Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Babu Moger]

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.

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Babu Moger]

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.

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Don Zickus]

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.

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.
> >
> 

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Don Zickus]

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.

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.
> >
> 

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Babu Moger]

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.


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.

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Babu Moger]

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.


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.

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Andrew Morton]

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.

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.

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Andrew Morton]

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.

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.

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Don Zickus]

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?

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.

Thoughts?

Cheers,
Don

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Don Zickus]

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?

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.

Thoughts?

Cheers,
Don

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Andrew Morton]

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?

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Andrew Morton]

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?

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Don Zickus]

On Wed, Oct 19, 2016 at 05:00:12PM -0700, Andrew Morton wrote:
> On Thu, 13 Oct 2016 13:38:01 -0700 Babu Moger  wrote:
> 
> > Currently we do not have a way to enable/disable arch specific
> > watchdog handlers if it was implemented by any of the architectures.
> > 
> > This patch introduces new functions arch_watchdog_nmi_enable and
> > arch_watchdog_nmi_disable which can be used to enable/disable architecture
> > specific NMI watchdog handlers. These functions are defined as weak as
> > architectures can override their definitions to enable/disable nmi
> > watchdog behaviour.
> > 
> > --- a/kernel/watchdog.c
> > +++ b/kernel/watchdog.c
> > @@ -676,8 +660,13 @@ static void watchdog_nmi_disable(unsigned int cpu)
> >  }
> >  
> >  #else
> > -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.

> 
> And I'm not really understanding the interaction with
> CONFIG_HARDLOCKUP_DETECTOR here.  I haven't really worked out why the
> code is all this way but it seems  odd?

If the above explaination doesn't help, then can you point to some examples
where things seem odd?

Cheers,
Don

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Don Zickus]

On Wed, Oct 19, 2016 at 05:00:12PM -0700, Andrew Morton wrote:
> On Thu, 13 Oct 2016 13:38:01 -0700 Babu Moger  wrote:
> 
> > Currently we do not have a way to enable/disable arch specific
> > watchdog handlers if it was implemented by any of the architectures.
> > 
> > This patch introduces new functions arch_watchdog_nmi_enable and
> > arch_watchdog_nmi_disable which can be used to enable/disable architecture
> > specific NMI watchdog handlers. These functions are defined as weak as
> > architectures can override their definitions to enable/disable nmi
> > watchdog behaviour.
> > 
> > --- a/kernel/watchdog.c
> > +++ b/kernel/watchdog.c
> > @@ -676,8 +660,13 @@ static void watchdog_nmi_disable(unsigned int cpu)
> >  }
> >  
> >  #else
> > -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.

> 
> And I'm not really understanding the interaction with
> CONFIG_HARDLOCKUP_DETECTOR here.  I haven't really worked out why the
> code is all this way but it seems  odd?

If the above explaination doesn't help, then can you point to some examples
where things seem odd?

Cheers,
Don

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Andrew Morton]

On Thu, 13 Oct 2016 13:38:01 -0700 Babu Moger  wrote:

> Currently we do not have a way to enable/disable arch specific
> watchdog handlers if it was implemented by any of the architectures.
> 
> This patch introduces new functions arch_watchdog_nmi_enable and
> arch_watchdog_nmi_disable which can be used to enable/disable architecture
> specific NMI watchdog handlers. These functions are defined as weak as
> architectures can override their definitions to enable/disable nmi
> watchdog behaviour.
> 
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -676,8 +660,13 @@ static void watchdog_nmi_disable(unsigned int cpu)
>  }
>  
>  #else
> -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.

And I'm not really understanding the interaction with
CONFIG_HARDLOCKUP_DETECTOR here.  I haven't really worked out why the
code is all this way but it seems  odd?

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Andrew Morton]

On Thu, 13 Oct 2016 13:38:01 -0700 Babu Moger  wrote:

> Currently we do not have a way to enable/disable arch specific
> watchdog handlers if it was implemented by any of the architectures.
> 
> This patch introduces new functions arch_watchdog_nmi_enable and
> arch_watchdog_nmi_disable which can be used to enable/disable architecture
> specific NMI watchdog handlers. These functions are defined as weak as
> architectures can override their definitions to enable/disable nmi
> watchdog behaviour.
> 
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -676,8 +660,13 @@ static void watchdog_nmi_disable(unsigned int cpu)
>  }
>  
>  #else
> -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.

And I'm not really understanding the interaction with
CONFIG_HARDLOCKUP_DETECTOR here.  I haven't really worked out why the
code is all this way but it seems  odd?

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Babu Moger]

Don,

On 10/17/2016 12:31 PM, Don Zickus wrote:

On Thu, Oct 13, 2016 at 01:38:01PM -0700, Babu Moger wrote:

Currently we do not have a way to enable/disable arch specific
watchdog handlers if it was implemented by any of the architectures.

This patch introduces new functions arch_watchdog_nmi_enable and
arch_watchdog_nmi_disable which can be used to enable/disable architecture
specific NMI watchdog handlers. These functions are defined as weak as
architectures can override their definitions to enable/disable nmi
watchdog behaviour.

Hi Babu,

This patch tested fine on my x86 box and I am ok with the changes.

I do have one small cosmetic request below for a failure path.  Other than
that I will give my ack.
Yes. I am testing these changes. If everything goes as expected, I will 
post  v3 version

tomorrow. Thanks Babu



Cheers,
Don


Signed-off-by: Babu Moger 
---
  kernel/watchdog.c |   65 +++-
  1 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 9acb29f..d1e84e6 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -46,7 +46,7 @@
  
  static DEFINE_MUTEX(watchdog_proc_mutex);
  
-#ifdef CONFIG_HARDLOCKUP_DETECTOR

+#if defined(CONFIG_HARDLOCKUP_DETECTOR) || defined(CONFIG_HAVE_NMI_WATCHDOG)
  static unsigned long __read_mostly watchdog_enabled = 
SOFT_WATCHDOG_ENABLED|NMI_WATCHDOG_ENABLED;
  #else
  static unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED;
@@ -585,15 +585,11 @@ static void watchdog(unsigned int cpu)
   */
  static unsigned long cpu0_err;
  
-static int watchdog_nmi_enable(unsigned int cpu)

+static int arch_watchdog_nmi_enable(unsigned int cpu)
  {
struct perf_event_attr *wd_attr;
struct perf_event *event = per_cpu(watchdog_ev, cpu);
  
-	/* nothing to do if the hard lockup detector is disabled */

-   if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
-   goto out;
-
/* is it already setup and enabled? */
if (event && event->state > PERF_EVENT_STATE_OFF)
goto out;
@@ -619,18 +615,6 @@ static int watchdog_nmi_enable(unsigned int cpu)
goto out_save;
}
  
-	/*

-* Disable the hard lockup detector if _any_ CPU fails to set up
-* set up the hardware perf event. The watchdog() function checks
-* the NMI_WATCHDOG_ENABLED bit periodically.
-*
-* The barriers are for syncing up watchdog_enabled across all the
-* cpus, as clear_bit() does not use barriers.
-*/
-   smp_mb__before_atomic();
-   clear_bit(NMI_WATCHDOG_ENABLED_BIT, _enabled);
-   smp_mb__after_atomic();
-
/* skip displaying the same error again */
if (cpu > 0 && (PTR_ERR(event) == cpu0_err))
return PTR_ERR(event);

In the arch_watchdog_nmi_enable code is a pr_info on failure

pr_info("Shutting down hard lockup detector on all cpus\n");

that should be moved to below..



@@ -658,7 +642,7 @@ out:
return 0;
  }
  
-static void watchdog_nmi_disable(unsigned int cpu)

+static void arch_watchdog_nmi_disable(unsigned int cpu)
  {
struct perf_event *event = per_cpu(watchdog_ev, cpu);
  
@@ -676,8 +660,13 @@ static void watchdog_nmi_disable(unsigned int cpu)

  }
  
  #else

-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 */
  
  static struct smp_hotplug_thread watchdog_threads = {

@@ -781,6 +770,40 @@ void lockup_detector_resume(void)
put_online_cpus();
  }
  
+void watchdog_nmi_disable(unsigned int cpu)

+{
+   arch_watchdog_nmi_disable(cpu);
+}
+
+int watchdog_nmi_enable(unsigned int cpu)
+{
+   int err;
+
+   /* nothing to do if the hard lockup detector is disabled */
+   if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
+   return 0;
+
+   err = arch_watchdog_nmi_enable(cpu);
+
+   if (err) {
+   /*
+* Disable the hard lockup detector if _any_ CPU fails to set up
+* set up the hardware perf event. The watchdog() function 
checks
+* the NMI_WATCHDOG_ENABLED bit periodically.
+*
+* The barriers are for syncing up watchdog_enabled across all 
the
+* cpus, as clear_bit() does not use barriers.
+*/
+   smp_mb__before_atomic();
+   clear_bit(NMI_WATCHDOG_ENABLED_BIT, _enabled);
+   smp_mb__after_atomic();

moved to here:

pr_info("Shutting down hard lockup detector on all cpus\n");


This lets the failure message be 

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Babu Moger]

Don,

On 10/17/2016 12:31 PM, Don Zickus wrote:

On Thu, Oct 13, 2016 at 01:38:01PM -0700, Babu Moger wrote:

Currently we do not have a way to enable/disable arch specific
watchdog handlers if it was implemented by any of the architectures.

This patch introduces new functions arch_watchdog_nmi_enable and
arch_watchdog_nmi_disable which can be used to enable/disable architecture
specific NMI watchdog handlers. These functions are defined as weak as
architectures can override their definitions to enable/disable nmi
watchdog behaviour.

Hi Babu,

This patch tested fine on my x86 box and I am ok with the changes.

I do have one small cosmetic request below for a failure path.  Other than
that I will give my ack.
Yes. I am testing these changes. If everything goes as expected, I will 
post  v3 version

tomorrow. Thanks Babu



Cheers,
Don


Signed-off-by: Babu Moger 
---
  kernel/watchdog.c |   65 +++-
  1 files changed, 44 insertions(+), 21 deletions(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 9acb29f..d1e84e6 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -46,7 +46,7 @@
  
  static DEFINE_MUTEX(watchdog_proc_mutex);
  
-#ifdef CONFIG_HARDLOCKUP_DETECTOR

+#if defined(CONFIG_HARDLOCKUP_DETECTOR) || defined(CONFIG_HAVE_NMI_WATCHDOG)
  static unsigned long __read_mostly watchdog_enabled = 
SOFT_WATCHDOG_ENABLED|NMI_WATCHDOG_ENABLED;
  #else
  static unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED;
@@ -585,15 +585,11 @@ static void watchdog(unsigned int cpu)
   */
  static unsigned long cpu0_err;
  
-static int watchdog_nmi_enable(unsigned int cpu)

+static int arch_watchdog_nmi_enable(unsigned int cpu)
  {
struct perf_event_attr *wd_attr;
struct perf_event *event = per_cpu(watchdog_ev, cpu);
  
-	/* nothing to do if the hard lockup detector is disabled */

-   if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
-   goto out;
-
/* is it already setup and enabled? */
if (event && event->state > PERF_EVENT_STATE_OFF)
goto out;
@@ -619,18 +615,6 @@ static int watchdog_nmi_enable(unsigned int cpu)
goto out_save;
}
  
-	/*

-* Disable the hard lockup detector if _any_ CPU fails to set up
-* set up the hardware perf event. The watchdog() function checks
-* the NMI_WATCHDOG_ENABLED bit periodically.
-*
-* The barriers are for syncing up watchdog_enabled across all the
-* cpus, as clear_bit() does not use barriers.
-*/
-   smp_mb__before_atomic();
-   clear_bit(NMI_WATCHDOG_ENABLED_BIT, _enabled);
-   smp_mb__after_atomic();
-
/* skip displaying the same error again */
if (cpu > 0 && (PTR_ERR(event) == cpu0_err))
return PTR_ERR(event);

In the arch_watchdog_nmi_enable code is a pr_info on failure

pr_info("Shutting down hard lockup detector on all cpus\n");

that should be moved to below..



@@ -658,7 +642,7 @@ out:
return 0;
  }
  
-static void watchdog_nmi_disable(unsigned int cpu)

+static void arch_watchdog_nmi_disable(unsigned int cpu)
  {
struct perf_event *event = per_cpu(watchdog_ev, cpu);
  
@@ -676,8 +660,13 @@ static void watchdog_nmi_disable(unsigned int cpu)

  }
  
  #else

-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 */
  
  static struct smp_hotplug_thread watchdog_threads = {

@@ -781,6 +770,40 @@ void lockup_detector_resume(void)
put_online_cpus();
  }
  
+void watchdog_nmi_disable(unsigned int cpu)

+{
+   arch_watchdog_nmi_disable(cpu);
+}
+
+int watchdog_nmi_enable(unsigned int cpu)
+{
+   int err;
+
+   /* nothing to do if the hard lockup detector is disabled */
+   if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
+   return 0;
+
+   err = arch_watchdog_nmi_enable(cpu);
+
+   if (err) {
+   /*
+* Disable the hard lockup detector if _any_ CPU fails to set up
+* set up the hardware perf event. The watchdog() function 
checks
+* the NMI_WATCHDOG_ENABLED bit periodically.
+*
+* The barriers are for syncing up watchdog_enabled across all 
the
+* cpus, as clear_bit() does not use barriers.
+*/
+   smp_mb__before_atomic();
+   clear_bit(NMI_WATCHDOG_ENABLED_BIT, _enabled);
+   smp_mb__after_atomic();

moved to here:

pr_info("Shutting down hard lockup detector on all cpus\n");


This lets the failure message be displayed on all arches 

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Don Zickus]

On Thu, Oct 13, 2016 at 01:38:01PM -0700, Babu Moger wrote:
> Currently we do not have a way to enable/disable arch specific
> watchdog handlers if it was implemented by any of the architectures.
> 
> This patch introduces new functions arch_watchdog_nmi_enable and
> arch_watchdog_nmi_disable which can be used to enable/disable architecture
> specific NMI watchdog handlers. These functions are defined as weak as
> architectures can override their definitions to enable/disable nmi
> watchdog behaviour.

Hi Babu,

This patch tested fine on my x86 box and I am ok with the changes.

I do have one small cosmetic request below for a failure path.  Other than
that I will give my ack.

Cheers,
Don

> 
> Signed-off-by: Babu Moger 
> ---
>  kernel/watchdog.c |   65 +++-
>  1 files changed, 44 insertions(+), 21 deletions(-)
> 
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 9acb29f..d1e84e6 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -46,7 +46,7 @@
>  
>  static DEFINE_MUTEX(watchdog_proc_mutex);
>  
> -#ifdef CONFIG_HARDLOCKUP_DETECTOR
> +#if defined(CONFIG_HARDLOCKUP_DETECTOR) || defined(CONFIG_HAVE_NMI_WATCHDOG)
>  static unsigned long __read_mostly watchdog_enabled = 
> SOFT_WATCHDOG_ENABLED|NMI_WATCHDOG_ENABLED;
>  #else
>  static unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED;
> @@ -585,15 +585,11 @@ static void watchdog(unsigned int cpu)
>   */
>  static unsigned long cpu0_err;
>  
> -static int watchdog_nmi_enable(unsigned int cpu)
> +static int arch_watchdog_nmi_enable(unsigned int cpu)
>  {
>   struct perf_event_attr *wd_attr;
>   struct perf_event *event = per_cpu(watchdog_ev, cpu);
>  
> - /* nothing to do if the hard lockup detector is disabled */
> - if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
> - goto out;
> -
>   /* is it already setup and enabled? */
>   if (event && event->state > PERF_EVENT_STATE_OFF)
>   goto out;
> @@ -619,18 +615,6 @@ static int watchdog_nmi_enable(unsigned int cpu)
>   goto out_save;
>   }
>  
> - /*
> -  * Disable the hard lockup detector if _any_ CPU fails to set up
> -  * set up the hardware perf event. The watchdog() function checks
> -  * the NMI_WATCHDOG_ENABLED bit periodically.
> -  *
> -  * The barriers are for syncing up watchdog_enabled across all the
> -  * cpus, as clear_bit() does not use barriers.
> -  */
> - smp_mb__before_atomic();
> - clear_bit(NMI_WATCHDOG_ENABLED_BIT, _enabled);
> - smp_mb__after_atomic();
> -
>   /* skip displaying the same error again */
>   if (cpu > 0 && (PTR_ERR(event) == cpu0_err))
>   return PTR_ERR(event);

In the arch_watchdog_nmi_enable code is a pr_info on failure

pr_info("Shutting down hard lockup detector on all cpus\n");

that should be moved to below..


> @@ -658,7 +642,7 @@ out:
>   return 0;
>  }
>  
> -static void watchdog_nmi_disable(unsigned int cpu)
> +static void arch_watchdog_nmi_disable(unsigned int cpu)
>  {
>   struct perf_event *event = per_cpu(watchdog_ev, cpu);
>  
> @@ -676,8 +660,13 @@ static void watchdog_nmi_disable(unsigned int cpu)
>  }
>  
>  #else
> -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 */
>  
>  static struct smp_hotplug_thread watchdog_threads = {
> @@ -781,6 +770,40 @@ void lockup_detector_resume(void)
>   put_online_cpus();
>  }
>  
> +void watchdog_nmi_disable(unsigned int cpu)
> +{
> + arch_watchdog_nmi_disable(cpu);
> +}
> +
> +int watchdog_nmi_enable(unsigned int cpu)
> +{
> + int err;
> +
> + /* nothing to do if the hard lockup detector is disabled */
> + if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
> + return 0;
> +
> + err = arch_watchdog_nmi_enable(cpu);
> +
> + if (err) {
> + /*
> +  * Disable the hard lockup detector if _any_ CPU fails to set up
> +  * set up the hardware perf event. The watchdog() function 
> checks
> +  * the NMI_WATCHDOG_ENABLED bit periodically.
> +  *
> +  * The barriers are for syncing up watchdog_enabled across all 
> the
> +  * cpus, as clear_bit() does not use barriers.
> +  */
> + smp_mb__before_atomic();
> + clear_bit(NMI_WATCHDOG_ENABLED_BIT, _enabled);
> + smp_mb__after_atomic();

moved to here:

pr_info("Shutting down hard lockup detector on all cpus\n");


This lets the failure message be displayed on all arches instead of 

Re: [PATCH v2 1/2] watchdog: Introduce arch_watchdog_nmi_enable and arch_watchdog_nmi_disable

[Don Zickus]

On Thu, Oct 13, 2016 at 01:38:01PM -0700, Babu Moger wrote:
> Currently we do not have a way to enable/disable arch specific
> watchdog handlers if it was implemented by any of the architectures.
> 
> This patch introduces new functions arch_watchdog_nmi_enable and
> arch_watchdog_nmi_disable which can be used to enable/disable architecture
> specific NMI watchdog handlers. These functions are defined as weak as
> architectures can override their definitions to enable/disable nmi
> watchdog behaviour.

Hi Babu,

This patch tested fine on my x86 box and I am ok with the changes.

I do have one small cosmetic request below for a failure path.  Other than
that I will give my ack.

Cheers,
Don

> 
> Signed-off-by: Babu Moger 
> ---
>  kernel/watchdog.c |   65 +++-
>  1 files changed, 44 insertions(+), 21 deletions(-)
> 
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index 9acb29f..d1e84e6 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -46,7 +46,7 @@
>  
>  static DEFINE_MUTEX(watchdog_proc_mutex);
>  
> -#ifdef CONFIG_HARDLOCKUP_DETECTOR
> +#if defined(CONFIG_HARDLOCKUP_DETECTOR) || defined(CONFIG_HAVE_NMI_WATCHDOG)
>  static unsigned long __read_mostly watchdog_enabled = 
> SOFT_WATCHDOG_ENABLED|NMI_WATCHDOG_ENABLED;
>  #else
>  static unsigned long __read_mostly watchdog_enabled = SOFT_WATCHDOG_ENABLED;
> @@ -585,15 +585,11 @@ static void watchdog(unsigned int cpu)
>   */
>  static unsigned long cpu0_err;
>  
> -static int watchdog_nmi_enable(unsigned int cpu)
> +static int arch_watchdog_nmi_enable(unsigned int cpu)
>  {
>   struct perf_event_attr *wd_attr;
>   struct perf_event *event = per_cpu(watchdog_ev, cpu);
>  
> - /* nothing to do if the hard lockup detector is disabled */
> - if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
> - goto out;
> -
>   /* is it already setup and enabled? */
>   if (event && event->state > PERF_EVENT_STATE_OFF)
>   goto out;
> @@ -619,18 +615,6 @@ static int watchdog_nmi_enable(unsigned int cpu)
>   goto out_save;
>   }
>  
> - /*
> -  * Disable the hard lockup detector if _any_ CPU fails to set up
> -  * set up the hardware perf event. The watchdog() function checks
> -  * the NMI_WATCHDOG_ENABLED bit periodically.
> -  *
> -  * The barriers are for syncing up watchdog_enabled across all the
> -  * cpus, as clear_bit() does not use barriers.
> -  */
> - smp_mb__before_atomic();
> - clear_bit(NMI_WATCHDOG_ENABLED_BIT, _enabled);
> - smp_mb__after_atomic();
> -
>   /* skip displaying the same error again */
>   if (cpu > 0 && (PTR_ERR(event) == cpu0_err))
>   return PTR_ERR(event);

In the arch_watchdog_nmi_enable code is a pr_info on failure

pr_info("Shutting down hard lockup detector on all cpus\n");

that should be moved to below..


> @@ -658,7 +642,7 @@ out:
>   return 0;
>  }
>  
> -static void watchdog_nmi_disable(unsigned int cpu)
> +static void arch_watchdog_nmi_disable(unsigned int cpu)
>  {
>   struct perf_event *event = per_cpu(watchdog_ev, cpu);
>  
> @@ -676,8 +660,13 @@ static void watchdog_nmi_disable(unsigned int cpu)
>  }
>  
>  #else
> -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 */
>  
>  static struct smp_hotplug_thread watchdog_threads = {
> @@ -781,6 +770,40 @@ void lockup_detector_resume(void)
>   put_online_cpus();
>  }
>  
> +void watchdog_nmi_disable(unsigned int cpu)
> +{
> + arch_watchdog_nmi_disable(cpu);
> +}
> +
> +int watchdog_nmi_enable(unsigned int cpu)
> +{
> + int err;
> +
> + /* nothing to do if the hard lockup detector is disabled */
> + if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
> + return 0;
> +
> + err = arch_watchdog_nmi_enable(cpu);
> +
> + if (err) {
> + /*
> +  * Disable the hard lockup detector if _any_ CPU fails to set up
> +  * set up the hardware perf event. The watchdog() function 
> checks
> +  * the NMI_WATCHDOG_ENABLED bit periodically.
> +  *
> +  * The barriers are for syncing up watchdog_enabled across all 
> the
> +  * cpus, as clear_bit() does not use barriers.
> +  */
> + smp_mb__before_atomic();
> + clear_bit(NMI_WATCHDOG_ENABLED_BIT, _enabled);
> + smp_mb__after_atomic();

moved to here:

pr_info("Shutting down hard lockup detector on all cpus\n");


This lets the failure message be displayed on all arches instead of just
x86.  Though I