Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650304 > unrolled thread

[PATCH 4/4] watchdog: provide watchdog_reconfigure() for arch watchdogs

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/4] watchdog: provide watchdog_reconfigure() for arch watchdogs Nicholas Piggin <npiggin@gmail.com> - 2017-05-25 10:30 +0200
    Re: [PATCH 4/4] watchdog: provide watchdog_reconfigure() for arch  watchdogs Don Zickus <dzickus@redhat.com> - 2017-05-25 16:10 +0200
      Re: [PATCH 4/4] watchdog: provide watchdog_reconfigure() for arch  watchdogs Nicholas Piggin <npiggin@gmail.com> - 2017-05-26 02:50 +0200
        Re: [PATCH 4/4] watchdog: provide watchdog_reconfigure() for arch  watchdogs Don Zickus <dzickus@redhat.com> - 2017-05-26 16:30 +0200

#1650304 — [PATCH 4/4] watchdog: provide watchdog_reconfigure() for arch watchdogs

After reconfiguring watchdog sysctls etc., architecture specific
watchdogs may not get all their parameters updated.

watchdog_reconfigure() can be implemented to pull the new values
in and set the arch NMI watchdog.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 kernel/watchdog.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index deb010505646..d2996f5bf551 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -120,6 +120,11 @@ void __weak watchdog_nmi_disable(unsigned int cpu)
 {
 }
 
+void __weak watchdog_nmi_reconfigure(void)
+{
+}
+
+
 #ifdef CONFIG_SOFTLOCKUP_DETECTOR
 
 /* Helper for online, unparked cpus. */
@@ -597,6 +602,12 @@ static void watchdog_disable_all_cpus(void)
 	}
 }
 
+static int watchdog_update_cpus(void)
+{
+	return smpboot_update_cpumask_percpu_thread(
+		    &watchdog_threads, &watchdog_cpumask);
+}
+
 #else /* SOFTLOCKUP */
 static int watchdog_park_threads(void)
 {
@@ -616,6 +627,10 @@ static void watchdog_disable_all_cpus(void)
 {
 }
 
+static int watchdog_update_cpus(void)
+{
+}
+
 static void set_sample_period(void)
 {
 }
@@ -648,6 +663,8 @@ int lockup_detector_suspend(void)
 		watchdog_enabled = 0;
 	}
 
+	watchdog_nmi_reconfigure();
+
 	mutex_unlock(&watchdog_proc_mutex);
 
 	return ret;
@@ -668,6 +685,8 @@ void lockup_detector_resume(void)
 	if (watchdog_running && !watchdog_suspended)
 		watchdog_unpark_threads();
 
+	watchdog_nmi_reconfigure();
+
 	mutex_unlock(&watchdog_proc_mutex);
 	put_online_cpus();
 }
@@ -693,6 +712,8 @@ static int proc_watchdog_update(void)
 	else
 		watchdog_disable_all_cpus();
 
+	watchdog_nmi_reconfigure();
+
 	return err;
 
 }
@@ -878,12 +899,11 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
 			 * a temporary cpumask, so we are likely not in a
 			 * position to do much else to make things better.
 			 */
-#ifdef CONFIG_SOFTLOCKUP_DETECTOR
-			if (smpboot_update_cpumask_percpu_thread(
-				    &watchdog_threads, &watchdog_cpumask) != 0)
+			if (watchdog_update_cpus() != 0)
 				pr_err("cpumask update failed\n");
-#endif
 		}
+
+		watchdog_nmi_reconfigure();
 	}
 out:
 	mutex_unlock(&watchdog_proc_mutex);
-- 
2.11.0

[toc] | [next] | [standalone]

#1650535 — Re: [PATCH 4/4] watchdog: provide watchdog_reconfigure() for arch watchdogs

On Thu, May 25, 2017 at 06:28:56PM +1000, Nicholas Piggin wrote:
> After reconfiguring watchdog sysctls etc., architecture specific
> watchdogs may not get all their parameters updated.
> 
> watchdog_reconfigure() can be implemented to pull the new values
> in and set the arch NMI watchdog.

I understand the reason for this patch and I don't have any real objection
on how it was implemented within the constraints of all the current logic.

I just wonder if the current logic should be adjusted to make the hardlockup
detector, namely the perf implementation more separate so it can handle what
you would like more cleanly.

The watchdog_nmi_reconfigure is sort of hackish, but it is hard to fault you
based on how things are designed.  I am going to poke at it a little bit,
but I will probably not find time to do much and accept what you have for
now.

Cheers,
Don

> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  kernel/watchdog.c | 28 ++++++++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> index deb010505646..d2996f5bf551 100644
> --- a/kernel/watchdog.c
> +++ b/kernel/watchdog.c
> @@ -120,6 +120,11 @@ void __weak watchdog_nmi_disable(unsigned int cpu)
>  {
>  }
>  
> +void __weak watchdog_nmi_reconfigure(void)
> +{
> +}
> +
> +
>  #ifdef CONFIG_SOFTLOCKUP_DETECTOR
>  
>  /* Helper for online, unparked cpus. */
> @@ -597,6 +602,12 @@ static void watchdog_disable_all_cpus(void)
>  	}
>  }
>  
> +static int watchdog_update_cpus(void)
> +{
> +	return smpboot_update_cpumask_percpu_thread(
> +		    &watchdog_threads, &watchdog_cpumask);
> +}
> +
>  #else /* SOFTLOCKUP */
>  static int watchdog_park_threads(void)
>  {
> @@ -616,6 +627,10 @@ static void watchdog_disable_all_cpus(void)
>  {
>  }
>  
> +static int watchdog_update_cpus(void)
> +{
> +}
> +
>  static void set_sample_period(void)
>  {
>  }
> @@ -648,6 +663,8 @@ int lockup_detector_suspend(void)
>  		watchdog_enabled = 0;
>  	}
>  
> +	watchdog_nmi_reconfigure();
> +
>  	mutex_unlock(&watchdog_proc_mutex);
>  
>  	return ret;
> @@ -668,6 +685,8 @@ void lockup_detector_resume(void)
>  	if (watchdog_running && !watchdog_suspended)
>  		watchdog_unpark_threads();
>  
> +	watchdog_nmi_reconfigure();
> +
>  	mutex_unlock(&watchdog_proc_mutex);
>  	put_online_cpus();
>  }
> @@ -693,6 +712,8 @@ static int proc_watchdog_update(void)
>  	else
>  		watchdog_disable_all_cpus();
>  
> +	watchdog_nmi_reconfigure();
> +
>  	return err;
>  
>  }
> @@ -878,12 +899,11 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write,
>  			 * a temporary cpumask, so we are likely not in a
>  			 * position to do much else to make things better.
>  			 */
> -#ifdef CONFIG_SOFTLOCKUP_DETECTOR
> -			if (smpboot_update_cpumask_percpu_thread(
> -				    &watchdog_threads, &watchdog_cpumask) != 0)
> +			if (watchdog_update_cpus() != 0)
>  				pr_err("cpumask update failed\n");
> -#endif
>  		}
> +
> +		watchdog_nmi_reconfigure();
>  	}
>  out:
>  	mutex_unlock(&watchdog_proc_mutex);
> -- 
> 2.11.0
> 

[toc] | [prev] | [next] | [standalone]

#1651008 — Re: [PATCH 4/4] watchdog: provide watchdog_reconfigure() for arch watchdogs

On Thu, 25 May 2017 10:08:33 -0400
Don Zickus <dzickus@redhat.com> wrote:

> On Thu, May 25, 2017 at 06:28:56PM +1000, Nicholas Piggin wrote:
> > After reconfiguring watchdog sysctls etc., architecture specific
> > watchdogs may not get all their parameters updated.
> > 
> > watchdog_reconfigure() can be implemented to pull the new values
> > in and set the arch NMI watchdog.  
> 
> I understand the reason for this patch and I don't have any real objection
> on how it was implemented within the constraints of all the current logic.
> 
> I just wonder if the current logic should be adjusted to make the hardlockup
> detector, namely the perf implementation more separate so it can handle what
> you would like more cleanly.
> 
> The watchdog_nmi_reconfigure is sort of hackish, but it is hard to fault you
> based on how things are designed.  I am going to poke at it a little bit,
> but I will probably not find time to do much and accept what you have for
> now.

I actually agree with you. These patches are basically an initial bridge
to get us to decoupling hld-perf from hld-arch, but the code could
definitely use several more passes to clean things up.

One thing we want to be mindful of is some watchdogs are very light weight,
minimal, and some may not even want to call C code (at least from the NMI
and touch-watchdog paths). But having said that, it may not be a bad idea
to have implementations provide a watchdog driver struct with some of the
methods and reconfiguration they support. E.g., suspend/resume, stop/start
on CPUs, adjust timeouts, etc.).

I didn't want to go the whole hog and over-engineer something that doesn't
work though, so I'm hoping we can get the powerpc watchdog in, and then
keep working on the apis.

Let me know what you think after you poke at it though.

Thanks,
Nick

[toc] | [prev] | [next] | [standalone]

#1651409 — Re: [PATCH 4/4] watchdog: provide watchdog_reconfigure() for arch watchdogs

On Fri, May 26, 2017 at 10:39:09AM +1000, Nicholas Piggin wrote:
> On Thu, 25 May 2017 10:08:33 -0400
> Don Zickus <dzickus@redhat.com> wrote:
> 
> > On Thu, May 25, 2017 at 06:28:56PM +1000, Nicholas Piggin wrote:
> > > After reconfiguring watchdog sysctls etc., architecture specific
> > > watchdogs may not get all their parameters updated.
> > > 
> > > watchdog_reconfigure() can be implemented to pull the new values
> > > in and set the arch NMI watchdog.  
> > 
> > I understand the reason for this patch and I don't have any real objection
> > on how it was implemented within the constraints of all the current logic.
> > 
> > I just wonder if the current logic should be adjusted to make the hardlockup
> > detector, namely the perf implementation more separate so it can handle what
> > you would like more cleanly.
> > 
> > The watchdog_nmi_reconfigure is sort of hackish, but it is hard to fault you
> > based on how things are designed.  I am going to poke at it a little bit,
> > but I will probably not find time to do much and accept what you have for
> > now.
> 
> I actually agree with you. These patches are basically an initial bridge
> to get us to decoupling hld-perf from hld-arch, but the code could
> definitely use several more passes to clean things up.

Makes sense. :-)

> 
> One thing we want to be mindful of is some watchdogs are very light weight,
> minimal, and some may not even want to call C code (at least from the NMI

Agreed.

> and touch-watchdog paths). But having said that, it may not be a bad idea
> to have implementations provide a watchdog driver struct with some of the
> methods and reconfiguration they support. E.g., suspend/resume, stop/start
> on CPUs, adjust timeouts, etc.).

Hehe.  I was hoping to avoid doing that, but it may lead there over time.

> 
> I didn't want to go the whole hog and over-engineer something that doesn't
> work though, so I'm hoping we can get the powerpc watchdog in, and then
> keep working on the apis.

Agreed. 

> 
> Let me know what you think after you poke at it though.

I will.

Cheers,
Don

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web