Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] watchdog: prevent deferral of watchdogd wakeup on RT
@ 2019-11-05 14:45 Sebastian Andrzej Siewior
  2019-11-05 16:02 ` Guenter Roeck
  0 siblings, 1 reply; 3+ messages in thread
From: Sebastian Andrzej Siewior @ 2019-11-05 14:45 UTC (permalink / raw)
  To: linux-watchdog
  Cc: Wim Van Sebroeck, Guenter Roeck, Steffen Trumtrar, Tim Sander,
	Julia Cartwright, Sebastian Andrzej Siewior, Thomas Gleixner

From: Julia Cartwright <julia@ni.com>

When PREEMPT_RT is enabled, all hrtimer expiry functions are
deferred for execution into the context of ksoftirqd unless otherwise
annotated.

Deferring the expiry of the hrtimer used by the watchdog core, however,
is a waste, as the callback does nothing but queue a kthread work item
and wakeup watchdogd.

It's worst then that, too: the deferral through ksoftirqd also means
that for correct behavior a user must adjust the scheduling parameters
of both watchdogd _and_ ksoftirqd, which is unnecessary and has other
side effects (like causing unrelated expiry functions to execute at
potentially elevated priority).

Instead, mark the hrtimer used by the watchdog core as being _HARD to
allow it's execution directly from hardirq context.  The work done in
this expiry function is well-bounded and minimal.

A user still must adjust the scheduling parameters of the watchdogd
to be correct w.r.t. their application needs.

Link: https://lkml.kernel.org/r/0e02d8327aeca344096c246713033887bc490dd7.1538089180.git.julia@ni.com
Cc: Guenter Roeck <linux@roeck-us.net>
Reported-and-tested-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
Reported-by: Tim Sander <tim@krieglstein.org>
Signed-off-by: Julia Cartwright <julia@ni.com>
Acked-by: Guenter Roeck <linux@roeck-us.net>
[bigeasy: use only HRTIMER_MODE_REL_HARD]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/watchdog/watchdog_dev.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index dbd2ad4c92948..d3acc0a7256ca 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -158,7 +158,8 @@ static inline void watchdog_update_worker(struct watchdog_device *wdd)
 		ktime_t t = watchdog_next_keepalive(wdd);
 
 		if (t > 0)
-			hrtimer_start(&wd_data->timer, t, HRTIMER_MODE_REL);
+			hrtimer_start(&wd_data->timer, t,
+				      HRTIMER_MODE_REL_HARD);
 	} else {
 		hrtimer_cancel(&wd_data->timer);
 	}
@@ -177,7 +178,7 @@ static int __watchdog_ping(struct watchdog_device *wdd)
 	if (ktime_after(earliest_keepalive, now)) {
 		hrtimer_start(&wd_data->timer,
 			      ktime_sub(earliest_keepalive, now),
-			      HRTIMER_MODE_REL);
+			      HRTIMER_MODE_REL_HARD);
 		return 0;
 	}
 
@@ -971,7 +972,7 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
 		return -ENODEV;
 
 	kthread_init_work(&wd_data->work, watchdog_ping_work);
-	hrtimer_init(&wd_data->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	hrtimer_init(&wd_data->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_HARD);
 	wd_data->timer.function = watchdog_timer_expired;
 
 	if (wdd->id == 0) {
@@ -1019,7 +1020,8 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
 		__module_get(wdd->ops->owner);
 		kref_get(&wd_data->kref);
 		if (handle_boot_enabled)
-			hrtimer_start(&wd_data->timer, 0, HRTIMER_MODE_REL);
+			hrtimer_start(&wd_data->timer, 0,
+				      HRTIMER_MODE_REL_HARD);
 		else
 			pr_info("watchdog%d running and kernel based pre-userspace handler disabled\n",
 				wdd->id);
-- 
2.24.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] watchdog: prevent deferral of watchdogd wakeup on RT
  2019-11-05 14:45 [PATCH] watchdog: prevent deferral of watchdogd wakeup on RT Sebastian Andrzej Siewior
@ 2019-11-05 16:02 ` Guenter Roeck
  2019-11-05 16:19   ` Sebastian Andrzej Siewior
  0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2019-11-05 16:02 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: linux-watchdog, Wim Van Sebroeck, Steffen Trumtrar, Tim Sander,
	Julia Cartwright, Thomas Gleixner

On Tue, Nov 05, 2019 at 03:45:06PM +0100, Sebastian Andrzej Siewior wrote:
> From: Julia Cartwright <julia@ni.com>
> 
> When PREEMPT_RT is enabled, all hrtimer expiry functions are
> deferred for execution into the context of ksoftirqd unless otherwise
> annotated.
> 
> Deferring the expiry of the hrtimer used by the watchdog core, however,
> is a waste, as the callback does nothing but queue a kthread work item
> and wakeup watchdogd.
> 
> It's worst then that, too: the deferral through ksoftirqd also means
> that for correct behavior a user must adjust the scheduling parameters
> of both watchdogd _and_ ksoftirqd, which is unnecessary and has other
> side effects (like causing unrelated expiry functions to execute at
> potentially elevated priority).
> 
> Instead, mark the hrtimer used by the watchdog core as being _HARD to
> allow it's execution directly from hardirq context.  The work done in
> this expiry function is well-bounded and minimal.
> 
> A user still must adjust the scheduling parameters of the watchdogd
> to be correct w.r.t. their application needs.
> 
> Link: https://lkml.kernel.org/r/0e02d8327aeca344096c246713033887bc490dd7.1538089180.git.julia@ni.com
> Cc: Guenter Roeck <linux@roeck-us.net>
> Reported-and-tested-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> Reported-by: Tim Sander <tim@krieglstein.org>
> Signed-off-by: Julia Cartwright <julia@ni.com>
> Acked-by: Guenter Roeck <linux@roeck-us.net>
> [bigeasy: use only HRTIMER_MODE_REL_HARD]
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Looks ok for mainline.

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/watchdog_dev.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index dbd2ad4c92948..d3acc0a7256ca 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -158,7 +158,8 @@ static inline void watchdog_update_worker(struct watchdog_device *wdd)
>  		ktime_t t = watchdog_next_keepalive(wdd);
>  
>  		if (t > 0)
> -			hrtimer_start(&wd_data->timer, t, HRTIMER_MODE_REL);
> +			hrtimer_start(&wd_data->timer, t,
> +				      HRTIMER_MODE_REL_HARD);
>  	} else {
>  		hrtimer_cancel(&wd_data->timer);
>  	}
> @@ -177,7 +178,7 @@ static int __watchdog_ping(struct watchdog_device *wdd)
>  	if (ktime_after(earliest_keepalive, now)) {
>  		hrtimer_start(&wd_data->timer,
>  			      ktime_sub(earliest_keepalive, now),
> -			      HRTIMER_MODE_REL);
> +			      HRTIMER_MODE_REL_HARD);
>  		return 0;
>  	}
>  
> @@ -971,7 +972,7 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
>  		return -ENODEV;
>  
>  	kthread_init_work(&wd_data->work, watchdog_ping_work);
> -	hrtimer_init(&wd_data->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	hrtimer_init(&wd_data->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_HARD);
>  	wd_data->timer.function = watchdog_timer_expired;
>  
>  	if (wdd->id == 0) {
> @@ -1019,7 +1020,8 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
>  		__module_get(wdd->ops->owner);
>  		kref_get(&wd_data->kref);
>  		if (handle_boot_enabled)
> -			hrtimer_start(&wd_data->timer, 0, HRTIMER_MODE_REL);
> +			hrtimer_start(&wd_data->timer, 0,
> +				      HRTIMER_MODE_REL_HARD);
>  		else
>  			pr_info("watchdog%d running and kernel based pre-userspace handler disabled\n",
>  				wdd->id);
> -- 
> 2.24.0
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] watchdog: prevent deferral of watchdogd wakeup on RT
  2019-11-05 16:02 ` Guenter Roeck
@ 2019-11-05 16:19   ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 3+ messages in thread
From: Sebastian Andrzej Siewior @ 2019-11-05 16:19 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: linux-watchdog, Wim Van Sebroeck, Steffen Trumtrar, Tim Sander,
	Julia Cartwright, Thomas Gleixner

On 2019-11-05 08:02:50 [-0800], Guenter Roeck wrote:
> > Acked-by: Guenter Roeck <linux@roeck-us.net>
> > [bigeasy: use only HRTIMER_MODE_REL_HARD]
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> Looks ok for mainline.

Good. HRTIMER_MODE_REL_HARD is mainline this time.

> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

thanks. I kept/added your Acked-by from last time.

Sebastian

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-05 14:45 [PATCH] watchdog: prevent deferral of watchdogd wakeup on RT Sebastian Andrzej Siewior
2019-11-05 16:02 ` Guenter Roeck
2019-11-05 16:19   ` Sebastian Andrzej Siewior

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