Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
From: Guenter Roeck <linux@roeck-us.net>
To: Dmitry Safonov <dima@arista.com>
Cc: linux-kernel@vger.kernel.org,
	Dmitry Safonov <0x7f454c46@gmail.com>,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	linux-watchdog@vger.kernel.org
Subject: Re: [PATCH 1/2] watchdog: Check WDOG_STOP_ON_REBOOT in reboot notifier
Date: Thu, 13 Feb 2020 11:12:30 -0800
Message-ID: <20200213191230.GA17448@roeck-us.net> (raw)
In-Reply-To: <20200213175958.105914-2-dima@arista.com>

On Thu, Feb 13, 2020 at 05:59:57PM +0000, Dmitry Safonov wrote:
> Many watchdog drivers use watchdog_stop_on_reboot() helper in order
> to stop the watchdog on system reboot. Unfortunately, this logic is
> coded in driver's probe function and doesn't allows user to decide what
> to do during shutdown/reboot.
> 
> On the other side, Xen and Qemu watchdog drivers (xen_wdt and i6300esb)
> may be configured to either send NMI or turn off/reboot VM as
> the watchdog action. As the kernel may stuck at any state, sending NMIs
> can't reliably reboot the VM.
> 
> At Arista, we benefited from the following set-up: the emulated watchdogs
> trigger VM reset and softdog is set to catch less severe conditions to
> generate vmcore. Just before reboot watchdog's timeout is increased
> to some good-enough value (3 mins). That keeps watchdog always running
> and guarantees that VM doesn't stuck.
> 
> As a preparation to move the watchdog's decision to stop on reboot or
> not in userspace, allow WDOG_STOP_ON_REBOOT to be set during runtime,
> not only on driver's probing. Always register reboot notifier and check
> WDOG_STOP_ON_REBOOT inside it (on actual reboot).
> 

Does that really have to be decided at runtime, by the user ?
How about doing it with a module parameter ?

Also, I am not sure if an ioctl is the best means to do this, if it indeed
makes sense to decide it at runtime. ioctl implies an open watchdog device,
which interferes with the watchdog daemon. This means that the watchdog
daemon would have to be modified to support this, making this a quite expensive
change. It also implies that the action would have to be known when the
watchdog daemon is started, suggesting that a module parameter should be
sufficient.

Guenter

> Signed-off-by: Dmitry Safonov <dima@arista.com>
> ---
>  drivers/watchdog/watchdog_core.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 861daf4f37b2..ebf80ff3e8ce 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -153,6 +153,10 @@ static int watchdog_reboot_notifier(struct notifier_block *nb,
>  	struct watchdog_device *wdd;
>  
>  	wdd = container_of(nb, struct watchdog_device, reboot_nb);
> +
> +	if (!test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
> +		return NOTIFY_DONE;
> +
>  	if (code == SYS_DOWN || code == SYS_HALT) {
>  		if (watchdog_active(wdd)) {
>  			int ret;
> @@ -254,17 +258,14 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>  		}
>  	}
>  
> -	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
> -		wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
> -
> -		ret = register_reboot_notifier(&wdd->reboot_nb);
> -		if (ret) {
> -			pr_err("watchdog%d: Cannot register reboot notifier (%d)\n",
> -			       wdd->id, ret);
> -			watchdog_dev_unregister(wdd);
> -			ida_simple_remove(&watchdog_ida, id);
> -			return ret;
> -		}
> +	wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
> +	ret = register_reboot_notifier(&wdd->reboot_nb);
> +	if (ret) {
> +		pr_err("watchdog%d: Cannot register reboot notifier (%d)\n",
> +				wdd->id, ret);
> +		watchdog_dev_unregister(wdd);
> +		ida_simple_remove(&watchdog_ida, id);
> +		return ret;
>  	}
>  
>  	if (wdd->ops->restart) {
> @@ -321,9 +322,7 @@ static void __watchdog_unregister_device(struct watchdog_device *wdd)
>  	if (wdd->ops->restart)
>  		unregister_restart_handler(&wdd->restart_nb);
>  
> -	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
> -		unregister_reboot_notifier(&wdd->reboot_nb);
> -
> +	unregister_reboot_notifier(&wdd->reboot_nb);
>  	watchdog_dev_unregister(wdd);
>  	ida_simple_remove(&watchdog_ida, wdd->id);
>  }
> -- 
> 2.25.0
> 

  reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-13 17:59 [PATCH 0/2] watchdog: Provide user control over WDOG_STOP_ON_REBOOT Dmitry Safonov
2020-02-13 17:59 ` [PATCH 1/2] watchdog: Check WDOG_STOP_ON_REBOOT in reboot notifier Dmitry Safonov
2020-02-13 19:12   ` Guenter Roeck [this message]
2020-02-13 20:23     ` Dmitry Safonov
2020-02-13 17:59 ` [PATCH 2/2] watchdog/uapi: Add WDIOS_{RUN,STOP}_ON_REBOOT Dmitry Safonov

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200213191230.GA17448@roeck-us.net \
    --to=linux@roeck-us.net \
    --cc=0x7f454c46@gmail.com \
    --cc=dima@arista.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-watchdog@vger.kernel.org \
    --cc=wim@linux-watchdog.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git