| Message ID | 20220614135414.37746-3-ldufour@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Extending NMI watchdog during LPM | expand |
Laurent Dufour <ldufour@linux.ibm.com> writes: > In some cricunstances it may be interesting to reconfigure the watchdog > from inside the kernel. > > On PowerPC, this may helpful before and after a LPAR migration (LPM) is > initiated, because it implies some latencies, watchdog, and especially NMI > watchdog is expected to be triggered during this operation. Reconfiguring > the watchdog, would prevent it to happen too frequently during LPM. > > The watchdog_mutex is exported to allow some variable to be changed under > its protection and prevent any conflict. > The lockup_detector_reconfigure() function is exported and is expected to > be called under the protection of watchdog_mutex. > > Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> > --- > include/linux/nmi.h | 3 +++ > kernel/watchdog.c | 6 +++--- > 2 files changed, 6 insertions(+), 3 deletions(-) Is there a maintainer for kernel/watchdog.c ? There's Wim & Guenter at linux-watchdog@vger but I think that's only for drivers/watchdog? Maybe we should Cc that list anyway? > diff --git a/include/linux/nmi.h b/include/linux/nmi.h > index 750c7f395ca9..84300fb0f90a 100644 > --- a/include/linux/nmi.h > +++ b/include/linux/nmi.h > @@ -122,6 +122,9 @@ int watchdog_nmi_probe(void); > int watchdog_nmi_enable(unsigned int cpu); > void watchdog_nmi_disable(unsigned int cpu); > > +extern struct mutex watchdog_mutex; > +void lockup_detector_reconfigure(void); It would be preferable if we didn't export the mutex. I think you could arrange that by ... Renaming lockup_detector_configure() to __lockup_detector_configure() and then adding a new lockup_detector_configure() that is non-static and takes the lock around __lockup_detector_configure(). cheers
On 24/06/2022, 08:31:55, Michael Ellerman wrote: > Laurent Dufour <ldufour@linux.ibm.com> writes: >> In some cricunstances it may be interesting to reconfigure the watchdog >> from inside the kernel. >> >> On PowerPC, this may helpful before and after a LPAR migration (LPM) is >> initiated, because it implies some latencies, watchdog, and especially NMI >> watchdog is expected to be triggered during this operation. Reconfiguring >> the watchdog, would prevent it to happen too frequently during LPM. >> >> The watchdog_mutex is exported to allow some variable to be changed under >> its protection and prevent any conflict. >> The lockup_detector_reconfigure() function is exported and is expected to >> be called under the protection of watchdog_mutex. >> >> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> >> --- >> include/linux/nmi.h | 3 +++ >> kernel/watchdog.c | 6 +++--- >> 2 files changed, 6 insertions(+), 3 deletions(-) > > Is there a maintainer for kernel/watchdog.c ? Nothing clearly identified AFAICT. I'll add the commit signers reported by get_maintainer.pl. > There's Wim & Guenter at linux-watchdog@vger but I think that's only for > drivers/watchdog? > > Maybe we should Cc that list anyway? Yes, that's a good idea. > > >> diff --git a/include/linux/nmi.h b/include/linux/nmi.h >> index 750c7f395ca9..84300fb0f90a 100644 >> --- a/include/linux/nmi.h >> +++ b/include/linux/nmi.h >> @@ -122,6 +122,9 @@ int watchdog_nmi_probe(void); >> int watchdog_nmi_enable(unsigned int cpu); >> void watchdog_nmi_disable(unsigned int cpu); >> >> +extern struct mutex watchdog_mutex; >> +void lockup_detector_reconfigure(void); > > It would be preferable if we didn't export the mutex. > > I think you could arrange that by ... > > Renaming lockup_detector_configure() to __lockup_detector_configure() > and then adding a new lockup_detector_configure() that is non-static and > takes the lock around __lockup_detector_configure(). Unfortunately, that will not be enough, because this mutex is also used to protect wd_watchdog, to ensure it is not changed while another operation is in progress. I may try finding another way to protect that value, may be using WRITE/READ_ONCE(). Indeed, the only requirement is to read a stable value in watchdog_calc_timeouts(). Thanks, Laurent.
On Tue, Jun 14, 2022 at 03:54:12PM +0200, Laurent Dufour wrote: > The watchdog_mutex is exported to allow some variable to be changed under > its protection and prevent any conflict. > The lockup_detector_reconfigure() function is exported and is expected to > be called under the protection of watchdog_mutex. Please provide an actual function accessor instead of directly touching a global lock.
On 24/06/2022, 11:37:23, Christoph Hellwig wrote: > On Tue, Jun 14, 2022 at 03:54:12PM +0200, Laurent Dufour wrote: >> The watchdog_mutex is exported to allow some variable to be changed under >> its protection and prevent any conflict. >> The lockup_detector_reconfigure() function is exported and is expected to >> be called under the protection of watchdog_mutex. > > Please provide an actual function accessor instead of directly touching > a global lock. Thanks Christoph, I'll try to not touch to that mutex, if that's not doable, I'll create function accessor as you're suggesting.
diff --git a/include/linux/nmi.h b/include/linux/nmi.h index 750c7f395ca9..84300fb0f90a 100644 --- a/include/linux/nmi.h +++ b/include/linux/nmi.h @@ -122,6 +122,9 @@ int watchdog_nmi_probe(void); int watchdog_nmi_enable(unsigned int cpu); void watchdog_nmi_disable(unsigned int cpu); +extern struct mutex watchdog_mutex; +void lockup_detector_reconfigure(void); + /** * touch_nmi_watchdog - restart NMI watchdog timeout. * diff --git a/kernel/watchdog.c b/kernel/watchdog.c index 20a7a55e62b6..0a67a2dd1258 100644 --- a/kernel/watchdog.c +++ b/kernel/watchdog.c @@ -27,7 +27,7 @@ #include <asm/irq_regs.h> #include <linux/kvm_para.h> -static DEFINE_MUTEX(watchdog_mutex); +DEFINE_MUTEX(watchdog_mutex); #if defined(CONFIG_HARDLOCKUP_DETECTOR) || defined(CONFIG_HAVE_NMI_WATCHDOG) # define WATCHDOG_DEFAULT (SOFT_WATCHDOG_ENABLED | NMI_WATCHDOG_ENABLED) @@ -541,7 +541,7 @@ int lockup_detector_offline_cpu(unsigned int cpu) return 0; } -static void lockup_detector_reconfigure(void) +void lockup_detector_reconfigure(void) { cpus_read_lock(); watchdog_nmi_stop(); @@ -583,7 +583,7 @@ static __init void lockup_detector_setup(void) } #else /* CONFIG_SOFTLOCKUP_DETECTOR */ -static void lockup_detector_reconfigure(void) +void lockup_detector_reconfigure(void) { cpus_read_lock(); watchdog_nmi_stop();
In some cricunstances it may be interesting to reconfigure the watchdog from inside the kernel. On PowerPC, this may helpful before and after a LPAR migration (LPM) is initiated, because it implies some latencies, watchdog, and especially NMI watchdog is expected to be triggered during this operation. Reconfiguring the watchdog, would prevent it to happen too frequently during LPM. The watchdog_mutex is exported to allow some variable to be changed under its protection and prevent any conflict. The lockup_detector_reconfigure() function is exported and is expected to be called under the protection of watchdog_mutex. Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com> --- include/linux/nmi.h | 3 +++ kernel/watchdog.c | 6 +++--- 2 files changed, 6 insertions(+), 3 deletions(-)