2013-04-17 21:24:12

by Don Zickus

[permalink] [raw]
Subject: [PATCH v3] watchdog: Add hook for kicking in kdump path

A common problem with kdump is that during the boot up of the
second kernel, the hardware watchdog times out and reboots the
machine before a vmcore can be captured.

Instead of tellling customers to disable their hardware watchdog
timers, I hacked up a hook to put in the kdump path that provides
one last kick before jumping into the second kernel.

The assumption is the watchdog timeout is at least 10-30 seconds
long, enough to get the second kernel to userspace to kick the watchdog
again, if needed.

Of course kdump is usually executed on a panic path, so grabbing the
watchdog mutexes to communicate with the hardware won't work. For now,
I use trylock, otherwise fail.

I have tested this with a machine using iTCO_wdt and the 'watchdog' app.
The extra kicked happened as expected.

v2: based on feedback, implemented a linked list of watchdog references.
added trylock in watchdog_ping and used that function for kicking.
renamed export function to be more generic.

v3: small cleanups, remove mutex_safe variable from EXPORT_SYMBOL

Signed-off-by: Don Zickus <[email protected]>
---
drivers/watchdog/watchdog_dev.c | 74 +++++++++++++++++++++++++++++++++++---
include/linux/watchdog.h | 9 +++++
kernel/kexec.c | 6 +++
3 files changed, 83 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 08b48bb..52cb465 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -49,6 +49,16 @@ static dev_t watchdog_devt;
/* the watchdog device behind /dev/watchdog */
static struct watchdog_device *old_wdd;

+/* link list of all watchdog devices */
+struct watchdog_list {
+ spinlock_t lock;
+ struct list_head head;
+};
+static struct watchdog_list wdlist = {
+ .lock = __SPIN_LOCK_UNLOCKED(wdlist.lock),
+ .head = LIST_HEAD_INIT(wdlist.head),
+};
+
/*
* watchdog_ping: ping the watchdog.
* @wddev: the watchdog device to ping
@@ -59,11 +69,18 @@ static struct watchdog_device *old_wdd;
* We only ping when the watchdog device is running.
*/

-static int watchdog_ping(struct watchdog_device *wddev)
+static int watchdog_ping(struct watchdog_device *wddev, bool mutex_safe)
{
int err = 0;

- mutex_lock(&wddev->lock);
+ if (mutex_safe) {
+ mutex_lock(&wddev->lock);
+ } else {
+ if (!mutex_trylock(&wddev->lock)) {
+ pr_warn("watchdog%d: Unable to lock mutex\n", wddev->id);
+ return -EAGAIN;
+ }
+ }

if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
err = -ENODEV;
@@ -83,6 +100,38 @@ out_ping:
return err;
}

+/**
+ * watchdog_kick_all: kick all the watchdogs
+ *
+ * There are times when the kernel needs to kick all the
+ * watchdogs at once without the use of references. For
+ * example in the kdump path, when the kernel is about
+ * to jump into the second kernel.
+ *
+ * The 'false' variable is for contextes that can not
+ * sleep, therefore try to kick the watchdog with trylock
+ * instead.
+ *
+ * Walk the link list locklessly using RCU to handle various
+ * contexts this could be called in. Should support irq and
+ * NMI contexts correctly.
+ */
+
+void watchdog_kick_all(void)
+{
+ struct watchdog_device *wddev;
+
+ rcu_read_lock();
+
+ list_for_each_entry_rcu(wddev, &wdlist.head, list)
+ watchdog_ping(wddev, false);
+
+ rcu_read_unlock();
+
+ return;
+}
+EXPORT_SYMBOL_GPL(watchdog_kick_all);
+
/*
* watchdog_start: wrapper to start the watchdog.
* @wddev: the watchdog device to start
@@ -314,7 +363,7 @@ static ssize_t watchdog_write(struct file *file, const char __user *data,
}

/* someone wrote to us, so we send the watchdog a keepalive ping */
- watchdog_ping(wdd);
+ watchdog_ping(wdd, true);

return len;
}
@@ -370,7 +419,7 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
case WDIOC_KEEPALIVE:
if (!(wdd->info->options & WDIOF_KEEPALIVEPING))
return -EOPNOTSUPP;
- watchdog_ping(wdd);
+ watchdog_ping(wdd, true);
return 0;
case WDIOC_SETTIMEOUT:
if (get_user(val, p))
@@ -381,7 +430,7 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
/* If the watchdog is active then we send a keepalive ping
* to make sure that the watchdog keep's running (and if
* possible that it takes the new timeout) */
- watchdog_ping(wdd);
+ watchdog_ping(wdd, true);
/* Fall */
case WDIOC_GETTIMEOUT:
/* timeout == 0 means that we don't know the timeout */
@@ -479,7 +528,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
if (!test_bit(WDOG_UNREGISTERED, &wdd->status))
dev_crit(wdd->dev, "watchdog did not stop!\n");
mutex_unlock(&wdd->lock);
- watchdog_ping(wdd);
+ watchdog_ping(wdd, true);
}

/* Allow the owner module to be unloaded again */
@@ -550,7 +599,14 @@ int watchdog_dev_register(struct watchdog_device *watchdog)
misc_deregister(&watchdog_miscdev);
old_wdd = NULL;
}
+ return err;
}
+
+ /* no need for save/restore here, not in an irq context */
+ spin_lock_irq(&wdlist.lock);
+ list_add_tail_rcu(&watchdog->list, &wdlist.head);
+ spin_unlock_irq(&wdlist.lock);
+
return err;
}

@@ -572,6 +628,12 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog)
misc_deregister(&watchdog_miscdev);
old_wdd = NULL;
}
+
+ /* no need for save/restore here, not in an irq context */
+ spin_lock_irq(&wdlist.lock);
+ list_del_rcu(&watchdog->list);
+ spin_unlock_irq(&wdlist.lock);
+
return 0;
}

diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 2a3038e..d33e209 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -65,6 +65,7 @@ struct watchdog_ops {
* @driver-data:Pointer to the drivers private data.
* @lock: Lock for watchdog core internal use only.
* @status: Field that contains the devices internal status bits.
+ * @list: Link list of all watchdog devices
*
* The watchdog_device structure contains all information about a
* watchdog timer device.
@@ -95,6 +96,7 @@ struct watchdog_device {
#define WDOG_ALLOW_RELEASE 2 /* Did we receive the magic char ? */
#define WDOG_NO_WAY_OUT 3 /* Is 'nowayout' feature set ? */
#define WDOG_UNREGISTERED 4 /* Has the device been unregistered */
+ struct list_head list;
};

#ifdef CONFIG_WATCHDOG_NOWAYOUT
@@ -142,4 +144,11 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd,
extern int watchdog_register_device(struct watchdog_device *);
extern void watchdog_unregister_device(struct watchdog_device *);

+#ifdef CONFIG_WATCHDOG_CORE
+/* drivers/watchdog/watchdog_dev.c */
+extern void watchdog_kick_all(void);
+#else
+static inline void watchdog_kick_all(void) { };
+#endif
+
#endif /* ifndef _LINUX_WATCHDOG_H */
diff --git a/kernel/kexec.c b/kernel/kexec.c
index bddd3d7..24787f1 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -32,6 +32,7 @@
#include <linux/vmalloc.h>
#include <linux/swap.h>
#include <linux/syscore_ops.h>
+#include <linux/watchdog.h>

#include <asm/page.h>
#include <asm/uaccess.h>
@@ -1094,6 +1095,11 @@ void crash_kexec(struct pt_regs *regs)
if (kexec_crash_image) {
struct pt_regs fixed_regs;

+ /*
+ * Give second kernel a chance to boot
+ */
+ watchdog_kick_all();
+
crash_setup_regs(&fixed_regs, regs);
crash_save_vmcoreinfo();
machine_crash_shutdown(&fixed_regs);
--
1.7.1


2013-04-17 21:33:38

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

On Wed, Apr 17, 2013 at 05:19:56PM -0400, Don Zickus wrote:
> A common problem with kdump is that during the boot up of the
> second kernel, the hardware watchdog times out and reboots the
> machine before a vmcore can be captured.
>
> Instead of tellling customers to disable their hardware watchdog
> timers, I hacked up a hook to put in the kdump path that provides
> one last kick before jumping into the second kernel.
>
> The assumption is the watchdog timeout is at least 10-30 seconds
> long, enough to get the second kernel to userspace to kick the watchdog
> again, if needed.
>
> Of course kdump is usually executed on a panic path, so grabbing the
> watchdog mutexes to communicate with the hardware won't work. For now,
> I use trylock, otherwise fail.
>
> I have tested this with a machine using iTCO_wdt and the 'watchdog' app.
> The extra kicked happened as expected.
>
> v2: based on feedback, implemented a linked list of watchdog references.
> added trylock in watchdog_ping and used that function for kicking.
> renamed export function to be more generic.
>
> v3: small cleanups, remove mutex_safe variable from EXPORT_SYMBOL
>
> Signed-off-by: Don Zickus <[email protected]>

I know, I am nitpicking. Just a couple of small issues.

> ---
> drivers/watchdog/watchdog_dev.c | 74 +++++++++++++++++++++++++++++++++++---
> include/linux/watchdog.h | 9 +++++
> kernel/kexec.c | 6 +++
> 3 files changed, 83 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 08b48bb..52cb465 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -49,6 +49,16 @@ static dev_t watchdog_devt;
> /* the watchdog device behind /dev/watchdog */
> static struct watchdog_device *old_wdd;
>
> +/* link list of all watchdog devices */
> +struct watchdog_list {
> + spinlock_t lock;
> + struct list_head head;
> +};
> +static struct watchdog_list wdlist = {
> + .lock = __SPIN_LOCK_UNLOCKED(wdlist.lock),
> + .head = LIST_HEAD_INIT(wdlist.head),
> +};
> +
> /*
> * watchdog_ping: ping the watchdog.
> * @wddev: the watchdog device to ping
> @@ -59,11 +69,18 @@ static struct watchdog_device *old_wdd;
> * We only ping when the watchdog device is running.
> */
>
> -static int watchdog_ping(struct watchdog_device *wddev)
> +static int watchdog_ping(struct watchdog_device *wddev, bool mutex_safe)
> {
> int err = 0;
>
> - mutex_lock(&wddev->lock);
> + if (mutex_safe) {
> + mutex_lock(&wddev->lock);
> + } else {
> + if (!mutex_trylock(&wddev->lock)) {
> + pr_warn("watchdog%d: Unable to lock mutex\n", wddev->id);
> + return -EAGAIN;
> + }
> + }
>
> if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
> err = -ENODEV;
> @@ -83,6 +100,38 @@ out_ping:
> return err;
> }
>
> +/**
> + * watchdog_kick_all: kick all the watchdogs
> + *
> + * There are times when the kernel needs to kick all the
> + * watchdogs at once without the use of references. For
> + * example in the kdump path, when the kernel is about
> + * to jump into the second kernel.
> + *
> + * The 'false' variable is for contextes that can not
> + * sleep, therefore try to kick the watchdog with trylock
> + * instead.

The variable no longer exists.

> + *
> + * Walk the link list locklessly using RCU to handle various
> + * contexts this could be called in. Should support irq and
> + * NMI contexts correctly.
> + */
> +
> +void watchdog_kick_all(void)
> +{
> + struct watchdog_device *wddev;
> +
> + rcu_read_lock();
> +
> + list_for_each_entry_rcu(wddev, &wdlist.head, list)
> + watchdog_ping(wddev, false);
> +
> + rcu_read_unlock();
> +
> + return;

Unnecessary return statement.

> +}
> +EXPORT_SYMBOL_GPL(watchdog_kick_all);
> +
> /*
> * watchdog_start: wrapper to start the watchdog.
> * @wddev: the watchdog device to start
> @@ -314,7 +363,7 @@ static ssize_t watchdog_write(struct file *file, const char __user *data,
> }
>
> /* someone wrote to us, so we send the watchdog a keepalive ping */
> - watchdog_ping(wdd);
> + watchdog_ping(wdd, true);
>
> return len;
> }
> @@ -370,7 +419,7 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
> case WDIOC_KEEPALIVE:
> if (!(wdd->info->options & WDIOF_KEEPALIVEPING))
> return -EOPNOTSUPP;
> - watchdog_ping(wdd);
> + watchdog_ping(wdd, true);
> return 0;
> case WDIOC_SETTIMEOUT:
> if (get_user(val, p))
> @@ -381,7 +430,7 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
> /* If the watchdog is active then we send a keepalive ping
> * to make sure that the watchdog keep's running (and if
> * possible that it takes the new timeout) */
> - watchdog_ping(wdd);
> + watchdog_ping(wdd, true);
> /* Fall */
> case WDIOC_GETTIMEOUT:
> /* timeout == 0 means that we don't know the timeout */
> @@ -479,7 +528,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
> if (!test_bit(WDOG_UNREGISTERED, &wdd->status))
> dev_crit(wdd->dev, "watchdog did not stop!\n");
> mutex_unlock(&wdd->lock);
> - watchdog_ping(wdd);
> + watchdog_ping(wdd, true);
> }
>
> /* Allow the owner module to be unloaded again */
> @@ -550,7 +599,14 @@ int watchdog_dev_register(struct watchdog_device *watchdog)
> misc_deregister(&watchdog_miscdev);
> old_wdd = NULL;
> }
> + return err;
> }
> +
> + /* no need for save/restore here, not in an irq context */
> + spin_lock_irq(&wdlist.lock);
> + list_add_tail_rcu(&watchdog->list, &wdlist.head);
> + spin_unlock_irq(&wdlist.lock);
> +
> return err;

No error here, so return 0;

> }
>
> @@ -572,6 +628,12 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog)
> misc_deregister(&watchdog_miscdev);
> old_wdd = NULL;
> }
> +
> + /* no need for save/restore here, not in an irq context */
> + spin_lock_irq(&wdlist.lock);
> + list_del_rcu(&watchdog->list);
> + spin_unlock_irq(&wdlist.lock);
> +
> return 0;
> }
>
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 2a3038e..d33e209 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -65,6 +65,7 @@ struct watchdog_ops {
> * @driver-data:Pointer to the drivers private data.
> * @lock: Lock for watchdog core internal use only.
> * @status: Field that contains the devices internal status bits.
> + * @list: Link list of all watchdog devices
> *
> * The watchdog_device structure contains all information about a
> * watchdog timer device.
> @@ -95,6 +96,7 @@ struct watchdog_device {
> #define WDOG_ALLOW_RELEASE 2 /* Did we receive the magic char ? */
> #define WDOG_NO_WAY_OUT 3 /* Is 'nowayout' feature set ? */
> #define WDOG_UNREGISTERED 4 /* Has the device been unregistered */
> + struct list_head list;
> };
>
> #ifdef CONFIG_WATCHDOG_NOWAYOUT
> @@ -142,4 +144,11 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd,
> extern int watchdog_register_device(struct watchdog_device *);
> extern void watchdog_unregister_device(struct watchdog_device *);
>
> +#ifdef CONFIG_WATCHDOG_CORE
> +/* drivers/watchdog/watchdog_dev.c */
> +extern void watchdog_kick_all(void);
> +#else
> +static inline void watchdog_kick_all(void) { };
> +#endif
> +
> #endif /* ifndef _LINUX_WATCHDOG_H */
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index bddd3d7..24787f1 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -32,6 +32,7 @@
> #include <linux/vmalloc.h>
> #include <linux/swap.h>
> #include <linux/syscore_ops.h>
> +#include <linux/watchdog.h>
>
> #include <asm/page.h>
> #include <asm/uaccess.h>
> @@ -1094,6 +1095,11 @@ void crash_kexec(struct pt_regs *regs)
> if (kexec_crash_image) {
> struct pt_regs fixed_regs;
>
> + /*
> + * Give second kernel a chance to boot
> + */
> + watchdog_kick_all();
> +
> crash_setup_regs(&fixed_regs, regs);
> crash_save_vmcoreinfo();
> machine_crash_shutdown(&fixed_regs);
> --
> 1.7.1
>
>

2013-04-17 21:50:18

by Eric W. Biederman

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

Don Zickus <[email protected]> writes:

> A common problem with kdump is that during the boot up of the
> second kernel, the hardware watchdog times out and reboots the
> machine before a vmcore can be captured.
>
> Instead of tellling customers to disable their hardware watchdog
> timers, I hacked up a hook to put in the kdump path that provides
> one last kick before jumping into the second kernel.
>
> The assumption is the watchdog timeout is at least 10-30 seconds
> long, enough to get the second kernel to userspace to kick the watchdog
> again, if needed.

Why not double the watchdog timeout? and/or pet the watchdog a little
more frequently.

This is the least icky hook I have seen proposed to be put on the kexec
on panic path, but I still suspect this may reduce the ability to take a
crash dump. What happens if it was the watchdog timer that panic'd for
example.

Eric

2013-04-18 03:02:44

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

On Wed, Apr 17, 2013 at 02:49:59PM -0700, Eric W. Biederman wrote:
> Don Zickus <[email protected]> writes:
>
> > A common problem with kdump is that during the boot up of the
> > second kernel, the hardware watchdog times out and reboots the
> > machine before a vmcore can be captured.
> >
> > Instead of tellling customers to disable their hardware watchdog
> > timers, I hacked up a hook to put in the kdump path that provides
> > one last kick before jumping into the second kernel.
> >
> > The assumption is the watchdog timeout is at least 10-30 seconds
> > long, enough to get the second kernel to userspace to kick the watchdog
> > again, if needed.
>
> Why not double the watchdog timeout? and/or pet the watchdog a little
> more frequently.
>
> This is the least icky hook I have seen proposed to be put on the kexec
> on panic path, but I still suspect this may reduce the ability to take a
> crash dump. What happens if it was the watchdog timer that panic'd for
> example.
>
That should be addressed by the use of mutex_trylock(), which should fail in
that case.

Guenter

2013-04-18 13:00:55

by Don Zickus

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

On Wed, Apr 17, 2013 at 02:49:59PM -0700, Eric W. Biederman wrote:
> Don Zickus <[email protected]> writes:
>
> > A common problem with kdump is that during the boot up of the
> > second kernel, the hardware watchdog times out and reboots the
> > machine before a vmcore can be captured.
> >
> > Instead of tellling customers to disable their hardware watchdog
> > timers, I hacked up a hook to put in the kdump path that provides
> > one last kick before jumping into the second kernel.
> >
> > The assumption is the watchdog timeout is at least 10-30 seconds
> > long, enough to get the second kernel to userspace to kick the watchdog
> > again, if needed.
>
> Why not double the watchdog timeout? and/or pet the watchdog a little
> more frequently.

I am not sure if the watchdog timeouts can be doubled. I think Guenter
was saying some have a max of a couple seconds?? Petting a little more
frequently might be an option. Guenter can that be done with a softdog
option?

Cheers,
Don

2013-04-18 13:48:46

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

On Thu, Apr 18, 2013 at 09:00:09AM -0400, Don Zickus wrote:
> On Wed, Apr 17, 2013 at 02:49:59PM -0700, Eric W. Biederman wrote:
> > Don Zickus <[email protected]> writes:
> >
> > > A common problem with kdump is that during the boot up of the
> > > second kernel, the hardware watchdog times out and reboots the
> > > machine before a vmcore can be captured.
> > >
> > > Instead of tellling customers to disable their hardware watchdog
> > > timers, I hacked up a hook to put in the kdump path that provides
> > > one last kick before jumping into the second kernel.
> > >
> > > The assumption is the watchdog timeout is at least 10-30 seconds
> > > long, enough to get the second kernel to userspace to kick the watchdog
> > > again, if needed.
> >
> > Why not double the watchdog timeout? and/or pet the watchdog a little
> > more frequently.
>
> I am not sure if the watchdog timeouts can be doubled. I think Guenter
> was saying some have a max of a couple seconds?? Petting a little more
> frequently might be an option. Guenter can that be done with a softdog
> option?
>
Most watchdog driver permit at least a minute. Some are more limited.
Worst I have seen is the BookE watchdog timer (non-Freescale version)
which has a maximum of three seconds. But that is broken anyway.

Most hardware watchdogs implement a softdog on top of the hardware watchdog
if the hardware needs to be pinged faster than every 60 seconds.

So, yes, for the most common case you should actually be able to live with a,
say, 30-60 second timeout which is pinged at least every 5-10 seconds. I thought
that somehow did not work in your case. Maybe a misunderstanding ?

If you have a customer with a specific problem on a specific watchdog which has
a too-short maximum interval, maybe another solution sould be to look into that
specific watchdog driver and see if it can be fixed.

Thanks,
Guenter

2013-04-18 13:53:10

by Don Zickus

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

On Thu, Apr 18, 2013 at 06:49:04AM -0700, Guenter Roeck wrote:
> On Thu, Apr 18, 2013 at 09:00:09AM -0400, Don Zickus wrote:
> > On Wed, Apr 17, 2013 at 02:49:59PM -0700, Eric W. Biederman wrote:
> > > Don Zickus <[email protected]> writes:
> > >
> > > > A common problem with kdump is that during the boot up of the
> > > > second kernel, the hardware watchdog times out and reboots the
> > > > machine before a vmcore can be captured.
> > > >
> > > > Instead of tellling customers to disable their hardware watchdog
> > > > timers, I hacked up a hook to put in the kdump path that provides
> > > > one last kick before jumping into the second kernel.
> > > >
> > > > The assumption is the watchdog timeout is at least 10-30 seconds
> > > > long, enough to get the second kernel to userspace to kick the watchdog
> > > > again, if needed.
> > >
> > > Why not double the watchdog timeout? and/or pet the watchdog a little
> > > more frequently.
> >
> > I am not sure if the watchdog timeouts can be doubled. I think Guenter
> > was saying some have a max of a couple seconds?? Petting a little more
> > frequently might be an option. Guenter can that be done with a softdog
> > option?
> >
> Most watchdog driver permit at least a minute. Some are more limited.
> Worst I have seen is the BookE watchdog timer (non-Freescale version)
> which has a maximum of three seconds. But that is broken anyway.
>
> Most hardware watchdogs implement a softdog on top of the hardware watchdog
> if the hardware needs to be pinged faster than every 60 seconds.
>
> So, yes, for the most common case you should actually be able to live with a,
> say, 30-60 second timeout which is pinged at least every 5-10 seconds. I thought
> that somehow did not work in your case. Maybe a misunderstanding ?

No, that will probably work. It is my misunderstanding. Is there a
common way to check the timeout length and the ping frequency?

Cheers,
Don

2013-04-18 14:53:57

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

On Thu, Apr 18, 2013 at 09:52:57AM -0400, Don Zickus wrote:
> On Thu, Apr 18, 2013 at 06:49:04AM -0700, Guenter Roeck wrote:
> > On Thu, Apr 18, 2013 at 09:00:09AM -0400, Don Zickus wrote:
> > > On Wed, Apr 17, 2013 at 02:49:59PM -0700, Eric W. Biederman wrote:
> > > > Don Zickus <[email protected]> writes:
> > > >
> > > > > A common problem with kdump is that during the boot up of the
> > > > > second kernel, the hardware watchdog times out and reboots the
> > > > > machine before a vmcore can be captured.
> > > > >
> > > > > Instead of tellling customers to disable their hardware watchdog
> > > > > timers, I hacked up a hook to put in the kdump path that provides
> > > > > one last kick before jumping into the second kernel.
> > > > >
> > > > > The assumption is the watchdog timeout is at least 10-30 seconds
> > > > > long, enough to get the second kernel to userspace to kick the watchdog
> > > > > again, if needed.
> > > >
> > > > Why not double the watchdog timeout? and/or pet the watchdog a little
> > > > more frequently.
> > >
> > > I am not sure if the watchdog timeouts can be doubled. I think Guenter
> > > was saying some have a max of a couple seconds?? Petting a little more
> > > frequently might be an option. Guenter can that be done with a softdog
> > > option?
> > >
> > Most watchdog driver permit at least a minute. Some are more limited.
> > Worst I have seen is the BookE watchdog timer (non-Freescale version)
> > which has a maximum of three seconds. But that is broken anyway.
> >
> > Most hardware watchdogs implement a softdog on top of the hardware watchdog
> > if the hardware needs to be pinged faster than every 60 seconds.
> >
> > So, yes, for the most common case you should actually be able to live with a,
> > say, 30-60 second timeout which is pinged at least every 5-10 seconds. I thought
> > that somehow did not work in your case. Maybe a misunderstanding ?
>
> No, that will probably work. It is my misunderstanding. Is there a
> common way to check the timeout length and the ping frequency?
>
Usually it is configured in /etc/watchdog.conf if the watchdog package
is installed. The standard ping interval is "interval", the timeout is
"watchdog-timeout". See "man watchdog.conf" for details.

Minimum and maximum values for a given watchdog driver are not exported
to user space, so you would have to look into the driver sources to find
out what they are.

Guenter

2013-04-18 16:35:24

by Eric W. Biederman

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

Don Zickus <[email protected]> writes:

> A common problem with kdump is that during the boot up of the
> second kernel, the hardware watchdog times out and reboots the
> machine before a vmcore can be captured.
>
> Instead of tellling customers to disable their hardware watchdog
> timers, I hacked up a hook to put in the kdump path that provides
> one last kick before jumping into the second kernel.

Having thought about this a little more this patch is actively wrong.

The problem is you can easily be petting the watchdog in violation of
whatever policy is normally in place. Which means that this extra
petting can result in a system that is unavailable for an unacceptably
long period of time.

I expect most watchdog policies are not that strict, but this patch
would preclude using those that are.

And like is being discussed in another subthread it does look like
changing the timeout and the interval should be enough all on it's own.

Eric

2013-04-18 17:44:42

by Don Zickus

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

On Thu, Apr 18, 2013 at 09:35:05AM -0700, Eric W. Biederman wrote:
> Don Zickus <[email protected]> writes:
>
> > A common problem with kdump is that during the boot up of the
> > second kernel, the hardware watchdog times out and reboots the
> > machine before a vmcore can be captured.
> >
> > Instead of tellling customers to disable their hardware watchdog
> > timers, I hacked up a hook to put in the kdump path that provides
> > one last kick before jumping into the second kernel.
>
> Having thought about this a little more this patch is actively wrong.
>
> The problem is you can easily be petting the watchdog in violation of
> whatever policy is normally in place. Which means that this extra
> petting can result in a system that is unavailable for an unacceptably
> long period of time.

Not really, just an extra period which isn't that much. This would only
be noticable if kdump is setup and enabled and then _hung_, otherwise it
just quickly reboots and noone notices. :-)

>
> I expect most watchdog policies are not that strict, but this patch
> would preclude using those that are.

I would assume most of those users would not enable kdump and would not be
affected.

>
> And like is being discussed in another subthread it does look like
> changing the timeout and the interval should be enough all on it's own.

Probably and I will pursue that. Thanks for the suggestion.

Cheers,
Don

2013-04-18 18:09:44

by Eric W. Biederman

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

Don Zickus <[email protected]> writes:

> On Thu, Apr 18, 2013 at 09:35:05AM -0700, Eric W. Biederman wrote:
>> Don Zickus <[email protected]> writes:
>>
>> > A common problem with kdump is that during the boot up of the
>> > second kernel, the hardware watchdog times out and reboots the
>> > machine before a vmcore can be captured.
>> >
>> > Instead of tellling customers to disable their hardware watchdog
>> > timers, I hacked up a hook to put in the kdump path that provides
>> > one last kick before jumping into the second kernel.
>>
>> Having thought about this a little more this patch is actively wrong.
>>
>> The problem is you can easily be petting the watchdog in violation of
>> whatever policy is normally in place. Which means that this extra
>> petting can result in a system that is unavailable for an unacceptably
>> long period of time.
>
> Not really, just an extra period which isn't that much. This would only
> be noticable if kdump is setup and enabled and then _hung_, otherwise it
> just quickly reboots and noone notices. :-)

For the folks who care the definition of acceptable unavailability would
look like: watchdog timeout + max boot time + margin of error. So it
is possible for an extra watchdog pet to eat up or exceed your margin
of error.

You are more likely to cause a how in the world did that happen than
something more extreme, but even playing invalidating peoples mental
model can be a problem sometimes.

>> I expect most watchdog policies are not that strict, but this patch
>> would preclude using those that are.
>
> I would assume most of those users would not enable kdump and would not be
> affected.

I have seen it be the case that the goal is to record what went wrong
if there is time, but to get back into service in a timely manner
regardless.

>> And like is being discussed in another subthread it does look like
>> changing the timeout and the interval should be enough all on it's own.
>
> Probably and I will pursue that. Thanks for the suggestion.

Eric

2013-04-24 14:42:40

by Don Zickus

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

On Thu, Apr 18, 2013 at 07:54:13AM -0700, Guenter Roeck wrote:
> > No, that will probably work. It is my misunderstanding. Is there a
> > common way to check the timeout length and the ping frequency?
> >
> Usually it is configured in /etc/watchdog.conf if the watchdog package
> is installed. The standard ping interval is "interval", the timeout is
> "watchdog-timeout". See "man watchdog.conf" for details.
>
> Minimum and maximum values for a given watchdog driver are not exported
> to user space, so you would have to look into the driver sources to find
> out what they are.

Hi Guenter,

Is there an easy way to determine which driver is loaded for each
/dev/watchdogN device (from a script perspective).

Basically, I wanted to determine the module that needs to be included in
the kdump initrd image.

Thanks,
Don

2013-04-24 15:21:15

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

On Wed, Apr 24, 2013 at 10:42:26AM -0400, Don Zickus wrote:
> On Thu, Apr 18, 2013 at 07:54:13AM -0700, Guenter Roeck wrote:
> > > No, that will probably work. It is my misunderstanding. Is there a
> > > common way to check the timeout length and the ping frequency?
> > >
> > Usually it is configured in /etc/watchdog.conf if the watchdog package
> > is installed. The standard ping interval is "interval", the timeout is
> > "watchdog-timeout". See "man watchdog.conf" for details.
> >
> > Minimum and maximum values for a given watchdog driver are not exported
> > to user space, so you would have to look into the driver sources to find
> > out what they are.
>
> Hi Guenter,
>
> Is there an easy way to determine which driver is loaded for each
> /dev/watchdogN device (from a script perspective).
>
> Basically, I wanted to determine the module that needs to be included in
> the kdump initrd image.
>
Sometimes. For example, the iTCO_wdt driver has an entry in /sys/devices, and
/sys/class/watchdog/watchdogX/device points to it. That is not always the case,
howewver. In my system, for example, the MEI watchdog is active, but there is
nothing I can find that would give me an indication that /dev/watchdog0 actually
points to the MEI watchdog driver.

Of course I might be missing something, and there might be some other means
to identify the driver from userspace. Would be great, actually, as I am having
the same problem.

Guenter

2013-05-27 19:16:25

by Wim Van Sebroeck

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

Hi Guenter,

> > Hi Guenter,
> >
> > Is there an easy way to determine which driver is loaded for each
> > /dev/watchdogN device (from a script perspective).
> >
> > Basically, I wanted to determine the module that needs to be included in
> > the kdump initrd image.
> >
> Sometimes. For example, the iTCO_wdt driver has an entry in /sys/devices, and
> /sys/class/watchdog/watchdogX/device points to it. That is not always the case,
> howewver. In my system, for example, the MEI watchdog is active, but there is
> nothing I can find that would give me an indication that /dev/watchdog0 actually
> points to the MEI watchdog driver.
>
> Of course I might be missing something, and there might be some other means
> to identify the driver from userspace. Would be great, actually, as I am having
> the same problem.

Only when you set the .parent field in the watchdog_device data you will get the
referal in the /sys/class/... tree. In iTCO_wdt.c you have the following code
that does that: iTCO_wdt_watchdog_dev.parent = dev->dev.parent;

>From the watchdog-kernel-api documentation:
* parent: set this to the parent device (or NULL) before calling
watchdog_register_device.

Kind regards,
Wim.

2013-05-28 01:10:25

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

On Mon, May 27, 2013 at 09:16:18PM +0200, Wim Van Sebroeck wrote:
> Hi Guenter,
>
> > > Hi Guenter,
> > >
> > > Is there an easy way to determine which driver is loaded for each
> > > /dev/watchdogN device (from a script perspective).
> > >
> > > Basically, I wanted to determine the module that needs to be included in
> > > the kdump initrd image.
> > >
> > Sometimes. For example, the iTCO_wdt driver has an entry in /sys/devices, and
> > /sys/class/watchdog/watchdogX/device points to it. That is not always the case,
> > howewver. In my system, for example, the MEI watchdog is active, but there is
> > nothing I can find that would give me an indication that /dev/watchdog0 actually
> > points to the MEI watchdog driver.
> >
> > Of course I might be missing something, and there might be some other means
> > to identify the driver from userspace. Would be great, actually, as I am having
> > the same problem.
>
> Only when you set the .parent field in the watchdog_device data you will get the
> referal in the /sys/class/... tree. In iTCO_wdt.c you have the following code
> that does that: iTCO_wdt_watchdog_dev.parent = dev->dev.parent;
>
> From the watchdog-kernel-api documentation:
> * parent: set this to the parent device (or NULL) before calling
> watchdog_register_device.
>
Hi Wim,

We live and learn ... thanks a lot for the hint!

Guenter

2013-05-28 15:34:31

by Guenter Roeck

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

On Mon, May 27, 2013 at 09:16:18PM +0200, Wim Van Sebroeck wrote:
> Hi Guenter,
>
> > > Hi Guenter,
> > >
> > > Is there an easy way to determine which driver is loaded for each
> > > /dev/watchdogN device (from a script perspective).
> > >
> > > Basically, I wanted to determine the module that needs to be included in
> > > the kdump initrd image.
> > >
> > Sometimes. For example, the iTCO_wdt driver has an entry in /sys/devices, and
> > /sys/class/watchdog/watchdogX/device points to it. That is not always the case,
> > howewver. In my system, for example, the MEI watchdog is active, but there is
> > nothing I can find that would give me an indication that /dev/watchdog0 actually
> > points to the MEI watchdog driver.
> >
> > Of course I might be missing something, and there might be some other means
> > to identify the driver from userspace. Would be great, actually, as I am having
> > the same problem.
>
> Only when you set the .parent field in the watchdog_device data you will get the
> referal in the /sys/class/... tree. In iTCO_wdt.c you have the following code
> that does that: iTCO_wdt_watchdog_dev.parent = dev->dev.parent;
>
> From the watchdog-kernel-api documentation:
> * parent: set this to the parent device (or NULL) before calling
> watchdog_register_device.
>
Hi Wim,

unfortunately that only works if the driver is instantiated as platform driver,
PCI driver, or similar, where a parent device exists. Some drivers are, however,
instantiated directly from the init function (the BookE driver is an example).
What is the recommended action in this case ? Convert it to a platform driver ?

Thanks,
Guenter

2013-05-30 20:37:28

by Wim Van Sebroeck

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

Hi Guenter,

> > Only when you set the .parent field in the watchdog_device data you will get the
> > referal in the /sys/class/... tree. In iTCO_wdt.c you have the following code
> > that does that: iTCO_wdt_watchdog_dev.parent = dev->dev.parent;
> >
> > From the watchdog-kernel-api documentation:
> > * parent: set this to the parent device (or NULL) before calling
> > watchdog_register_device.
> >
> Hi Wim,
>
> We live and learn ... thanks a lot for the hint!

If it's about learning: the old way was via miscdevice.parent ... I think some "old"
drivers still do this.

Kind regards,
Wim.

2013-05-30 21:54:30

by Wim Van Sebroeck

[permalink] [raw]
Subject: Re: [PATCH v3] watchdog: Add hook for kicking in kdump path

Hi Guenter,

> > Only when you set the .parent field in the watchdog_device data you will get the
> > referal in the /sys/class/... tree. In iTCO_wdt.c you have the following code
> > that does that: iTCO_wdt_watchdog_dev.parent = dev->dev.parent;
> >
> > From the watchdog-kernel-api documentation:
> > * parent: set this to the parent device (or NULL) before calling
> > watchdog_register_device.
> >
> Hi Wim,
>
> unfortunately that only works if the driver is instantiated as platform driver,
> PCI driver, or similar, where a parent device exists. Some drivers are, however,
> instantiated directly from the init function (the BookE driver is an example).
> What is the recommended action in this case ? Convert it to a platform driver ?

If you don't have a bus related driver (like a pci-driver) then indeed it's a
conversion to a platform driver.

Kind regards,
Wim.