* [PATCH] watchdog: core: make sure the watchdog_worker always works
@ 2017-12-07 10:38 Christophe Leroy
2017-12-07 14:45 ` Guenter Roeck
0 siblings, 1 reply; 3+ messages in thread
From: Christophe Leroy @ 2017-12-07 10:38 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck
Cc: linux-kernel, linuxppc-dev, linux-watchdog
When running a command like 'chrt -f 99 dd if=/dev/zero of=/dev/null',
the watchdog_worker fails to service the HW watchdog and the
HW watchdog fires long before the watchdog soft timeout.
At the moment, the watchdog_worker is invoked as a delayed work.
Delayed works are handled by non realtime kernel threads. The
WQ_HIGHPRI flag only increases the niceness of that threads.
This patchs replaces the delayed work logic by hrtimer, in order to
ensure that the watchdog_worker will already have priority.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
drivers/watchdog/watchdog_dev.c | 87 +++++++++++++++++++----------------------
1 file changed, 41 insertions(+), 46 deletions(-)
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 1e971a50d7fb..e9b234c4ff67 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -36,7 +36,7 @@
#include <linux/errno.h> /* For the -ENODEV/... values */
#include <linux/fs.h> /* For file operations */
#include <linux/init.h> /* For __init/__exit/... */
-#include <linux/jiffies.h> /* For timeout functions */
+#include <linux/hrtimer.h>
#include <linux/kernel.h> /* For printk/panic/... */
#include <linux/kref.h> /* For data references */
#include <linux/miscdevice.h> /* For handling misc devices */
@@ -46,7 +46,6 @@
#include <linux/slab.h> /* For memory functions */
#include <linux/types.h> /* For standard types (like size_t) */
#include <linux/watchdog.h> /* For watchdog specific items */
-#include <linux/workqueue.h> /* For workqueue */
#include <linux/uaccess.h> /* For copy_to_user/put_user/... */
#include "watchdog_core.h"
@@ -65,9 +64,9 @@ struct watchdog_core_data {
struct cdev cdev;
struct watchdog_device *wdd;
struct mutex lock;
- unsigned long last_keepalive;
- unsigned long last_hw_keepalive;
- struct delayed_work work;
+ ktime_t last_keepalive;
+ ktime_t last_hw_keepalive;
+ struct hrtimer timer;
unsigned long status; /* Internal status bits */
#define _WDOG_DEV_OPEN 0 /* Opened ? */
#define _WDOG_ALLOW_RELEASE 1 /* Did we receive the magic char ? */
@@ -79,8 +78,6 @@ static dev_t watchdog_devt;
/* Reference to watchdog device behind /dev/watchdog */
static struct watchdog_core_data *old_wd_data;
-static struct workqueue_struct *watchdog_wq;
-
static bool handle_boot_enabled =
IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED);
@@ -107,18 +104,19 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
(t && !watchdog_active(wdd) && watchdog_hw_running(wdd));
}
-static long watchdog_next_keepalive(struct watchdog_device *wdd)
+static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd)
{
struct watchdog_core_data *wd_data = wdd->wd_data;
unsigned int timeout_ms = wdd->timeout * 1000;
- unsigned long keepalive_interval;
- unsigned long last_heartbeat;
- unsigned long virt_timeout;
+ ktime_t keepalive_interval;
+ ktime_t last_heartbeat, latest_heartbeat;
+ ktime_t virt_timeout;
unsigned int hw_heartbeat_ms;
- virt_timeout = wd_data->last_keepalive + msecs_to_jiffies(timeout_ms);
+ virt_timeout = ktime_add(wd_data->last_keepalive,
+ ms_to_ktime(timeout_ms));
hw_heartbeat_ms = min_not_zero(timeout_ms, wdd->max_hw_heartbeat_ms);
- keepalive_interval = msecs_to_jiffies(hw_heartbeat_ms / 2);
+ keepalive_interval = ms_to_ktime(hw_heartbeat_ms / 2);
if (!watchdog_active(wdd))
return keepalive_interval;
@@ -128,8 +126,11 @@ static long watchdog_next_keepalive(struct watchdog_device *wdd)
* after the most recent ping from userspace, the last
* worker ping has to come in hw_heartbeat_ms before this timeout.
*/
- last_heartbeat = virt_timeout - msecs_to_jiffies(hw_heartbeat_ms);
- return min_t(long, last_heartbeat - jiffies, keepalive_interval);
+ last_heartbeat = ktime_sub(virt_timeout, ms_to_ktime(hw_heartbeat_ms));
+ latest_heartbeat = ktime_sub(last_heartbeat, ktime_get());
+ if (ktime_before(latest_heartbeat, keepalive_interval))
+ return latest_heartbeat;
+ return keepalive_interval;
}
static inline void watchdog_update_worker(struct watchdog_device *wdd)
@@ -137,29 +138,33 @@ static inline void watchdog_update_worker(struct watchdog_device *wdd)
struct watchdog_core_data *wd_data = wdd->wd_data;
if (watchdog_need_worker(wdd)) {
- long t = watchdog_next_keepalive(wdd);
+ ktime_t t = watchdog_next_keepalive(wdd);
if (t > 0)
- mod_delayed_work(watchdog_wq, &wd_data->work, t);
+ hrtimer_start(&wd_data->timer, t, HRTIMER_MODE_REL);
} else {
- cancel_delayed_work(&wd_data->work);
+ hrtimer_cancel(&wd_data->timer);
}
}
static int __watchdog_ping(struct watchdog_device *wdd)
{
struct watchdog_core_data *wd_data = wdd->wd_data;
- unsigned long earliest_keepalive = wd_data->last_hw_keepalive +
- msecs_to_jiffies(wdd->min_hw_heartbeat_ms);
+ ktime_t earliest_keepalive, now;
int err;
- if (time_is_after_jiffies(earliest_keepalive)) {
- mod_delayed_work(watchdog_wq, &wd_data->work,
- earliest_keepalive - jiffies);
+ earliest_keepalive = ktime_add(wd_data->last_hw_keepalive,
+ ms_to_ktime(wdd->min_hw_heartbeat_ms));
+ now = ktime_get();
+
+ if (ktime_after(earliest_keepalive, now)) {
+ hrtimer_start(&wd_data->timer,
+ ktime_sub(earliest_keepalive, now),
+ HRTIMER_MODE_REL);
return 0;
}
- wd_data->last_hw_keepalive = jiffies;
+ wd_data->last_hw_keepalive = now;
if (wdd->ops->ping)
err = wdd->ops->ping(wdd); /* ping the watchdog */
@@ -192,7 +197,7 @@ static int watchdog_ping(struct watchdog_device *wdd)
set_bit(_WDOG_KEEPALIVE, &wd_data->status);
- wd_data->last_keepalive = jiffies;
+ wd_data->last_keepalive = ktime_get();
return __watchdog_ping(wdd);
}
@@ -203,17 +208,18 @@ static bool watchdog_worker_should_ping(struct watchdog_core_data *wd_data)
return wdd && (watchdog_active(wdd) || watchdog_hw_running(wdd));
}
-static void watchdog_ping_work(struct work_struct *work)
+enum hrtimer_restart watchdog_ping_work(struct hrtimer *timer)
{
struct watchdog_core_data *wd_data;
- wd_data = container_of(to_delayed_work(work), struct watchdog_core_data,
- work);
+ wd_data = container_of(timer, struct watchdog_core_data, timer);
mutex_lock(&wd_data->lock);
if (watchdog_worker_should_ping(wd_data))
__watchdog_ping(wd_data->wdd);
mutex_unlock(&wd_data->lock);
+
+ return HRTIMER_NORESTART;
}
/*
@@ -230,7 +236,7 @@ static void watchdog_ping_work(struct work_struct *work)
static int watchdog_start(struct watchdog_device *wdd)
{
struct watchdog_core_data *wd_data = wdd->wd_data;
- unsigned long started_at;
+ ktime_t started_at;
int err;
if (watchdog_active(wdd))
@@ -238,7 +244,7 @@ static int watchdog_start(struct watchdog_device *wdd)
set_bit(_WDOG_KEEPALIVE, &wd_data->status);
- started_at = jiffies;
+ started_at = ktime_get();
if (watchdog_hw_running(wdd) && wdd->ops->ping)
err = wdd->ops->ping(wdd);
else
@@ -919,10 +925,8 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
wd_data->wdd = wdd;
wdd->wd_data = wd_data;
- if (!watchdog_wq)
- return -ENODEV;
-
- INIT_DELAYED_WORK(&wd_data->work, watchdog_ping_work);
+ hrtimer_init(&wd_data->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+ wd_data->timer.function = watchdog_ping_work;
if (wdd->id == 0) {
old_wd_data = wd_data;
@@ -958,7 +962,7 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
}
/* Record time of most recent heartbeat as 'just before now'. */
- wd_data->last_hw_keepalive = jiffies - 1;
+ wd_data->last_hw_keepalive = ktime_get();
/*
* If the watchdog is running, prevent its driver from being unloaded,
@@ -968,7 +972,7 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
if (handle_boot_enabled) {
__module_get(wdd->ops->owner);
kref_get(&wd_data->kref);
- queue_delayed_work(watchdog_wq, &wd_data->work, 0);
+ hrtimer_start(&wd_data->timer, 0, HRTIMER_MODE_REL);
} else {
pr_info("watchdog%d running and kernel based pre-userspace handler disabled\n",
wdd->id);
@@ -1006,7 +1010,7 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd)
watchdog_stop(wdd);
}
- cancel_delayed_work_sync(&wd_data->work);
+ hrtimer_cancel(&wd_data->timer);
kref_put(&wd_data->kref, watchdog_core_data_release);
}
@@ -1111,13 +1115,6 @@ int __init watchdog_dev_init(void)
{
int err;
- watchdog_wq = alloc_workqueue("watchdogd",
- WQ_HIGHPRI | WQ_MEM_RECLAIM, 0);
- if (!watchdog_wq) {
- pr_err("Failed to create watchdog workqueue\n");
- return -ENOMEM;
- }
-
err = class_register(&watchdog_class);
if (err < 0) {
pr_err("couldn't register class\n");
@@ -1135,7 +1132,6 @@ int __init watchdog_dev_init(void)
err_alloc:
class_unregister(&watchdog_class);
err_register:
- destroy_workqueue(watchdog_wq);
return err;
}
@@ -1149,7 +1145,6 @@ void __exit watchdog_dev_exit(void)
{
unregister_chrdev_region(watchdog_devt, MAX_DOGS);
class_unregister(&watchdog_class);
- destroy_workqueue(watchdog_wq);
}
module_param(handle_boot_enabled, bool, 0444);
--
2.13.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] watchdog: core: make sure the watchdog_worker always works
2017-12-07 10:38 [PATCH] watchdog: core: make sure the watchdog_worker always works Christophe Leroy
@ 2017-12-07 14:45 ` Guenter Roeck
2017-12-08 10:33 ` Christophe LEROY
0 siblings, 1 reply; 3+ messages in thread
From: Guenter Roeck @ 2017-12-07 14:45 UTC (permalink / raw)
To: Christophe Leroy, Wim Van Sebroeck
Cc: linux-kernel, linuxppc-dev, linux-watchdog
On 12/07/2017 02:38 AM, Christophe Leroy wrote:
> When running a command like 'chrt -f 99 dd if=/dev/zero of=/dev/null',
> the watchdog_worker fails to service the HW watchdog and the
> HW watchdog fires long before the watchdog soft timeout.
>
> At the moment, the watchdog_worker is invoked as a delayed work.
> Delayed works are handled by non realtime kernel threads. The
> WQ_HIGHPRI flag only increases the niceness of that threads.
>
> This patchs replaces the delayed work logic by hrtimer, in order to
s/patchs/patch/
> ensure that the watchdog_worker will already have priority.
always ?
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> drivers/watchdog/watchdog_dev.c | 87 +++++++++++++++++++----------------------
> 1 file changed, 41 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 1e971a50d7fb..e9b234c4ff67 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -36,7 +36,7 @@
> #include <linux/errno.h> /* For the -ENODEV/... values */
> #include <linux/fs.h> /* For file operations */
> #include <linux/init.h> /* For __init/__exit/... */
> -#include <linux/jiffies.h> /* For timeout functions */
> +#include <linux/hrtimer.h>
> #include <linux/kernel.h> /* For printk/panic/... */
> #include <linux/kref.h> /* For data references */
> #include <linux/miscdevice.h> /* For handling misc devices */
> @@ -46,7 +46,6 @@
> #include <linux/slab.h> /* For memory functions */
> #include <linux/types.h> /* For standard types (like size_t) */
> #include <linux/watchdog.h> /* For watchdog specific items */
> -#include <linux/workqueue.h> /* For workqueue */
> #include <linux/uaccess.h> /* For copy_to_user/put_user/... */
>
> #include "watchdog_core.h"
> @@ -65,9 +64,9 @@ struct watchdog_core_data {
> struct cdev cdev;
> struct watchdog_device *wdd;
> struct mutex lock;
> - unsigned long last_keepalive;
> - unsigned long last_hw_keepalive;
> - struct delayed_work work;
> + ktime_t last_keepalive;
> + ktime_t last_hw_keepalive;
> + struct hrtimer timer;
> unsigned long status; /* Internal status bits */
> #define _WDOG_DEV_OPEN 0 /* Opened ? */
> #define _WDOG_ALLOW_RELEASE 1 /* Did we receive the magic char ? */
> @@ -79,8 +78,6 @@ static dev_t watchdog_devt;
> /* Reference to watchdog device behind /dev/watchdog */
> static struct watchdog_core_data *old_wd_data;
>
> -static struct workqueue_struct *watchdog_wq;
> -
> static bool handle_boot_enabled =
> IS_ENABLED(CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED);
>
> @@ -107,18 +104,19 @@ static inline bool watchdog_need_worker(struct watchdog_device *wdd)
> (t && !watchdog_active(wdd) && watchdog_hw_running(wdd));
> }
>
> -static long watchdog_next_keepalive(struct watchdog_device *wdd)
> +static ktime_t watchdog_next_keepalive(struct watchdog_device *wdd)
> {
> struct watchdog_core_data *wd_data = wdd->wd_data;
> unsigned int timeout_ms = wdd->timeout * 1000;
> - unsigned long keepalive_interval;
> - unsigned long last_heartbeat;
> - unsigned long virt_timeout;
> + ktime_t keepalive_interval;
> + ktime_t last_heartbeat, latest_heartbeat;
> + ktime_t virt_timeout;
> unsigned int hw_heartbeat_ms;
>
> - virt_timeout = wd_data->last_keepalive + msecs_to_jiffies(timeout_ms);
> + virt_timeout = ktime_add(wd_data->last_keepalive,
> + ms_to_ktime(timeout_ms));
> hw_heartbeat_ms = min_not_zero(timeout_ms, wdd->max_hw_heartbeat_ms);
> - keepalive_interval = msecs_to_jiffies(hw_heartbeat_ms / 2);
> + keepalive_interval = ms_to_ktime(hw_heartbeat_ms / 2);
>
> if (!watchdog_active(wdd))
> return keepalive_interval;
> @@ -128,8 +126,11 @@ static long watchdog_next_keepalive(struct watchdog_device *wdd)
> * after the most recent ping from userspace, the last
> * worker ping has to come in hw_heartbeat_ms before this timeout.
> */
> - last_heartbeat = virt_timeout - msecs_to_jiffies(hw_heartbeat_ms);
> - return min_t(long, last_heartbeat - jiffies, keepalive_interval);
> + last_heartbeat = ktime_sub(virt_timeout, ms_to_ktime(hw_heartbeat_ms));
> + latest_heartbeat = ktime_sub(last_heartbeat, ktime_get());
> + if (ktime_before(latest_heartbeat, keepalive_interval))
> + return latest_heartbeat;
> + return keepalive_interval;
> }
>
> static inline void watchdog_update_worker(struct watchdog_device *wdd)
> @@ -137,29 +138,33 @@ static inline void watchdog_update_worker(struct watchdog_device *wdd)
> struct watchdog_core_data *wd_data = wdd->wd_data;
>
> if (watchdog_need_worker(wdd)) {
> - long t = watchdog_next_keepalive(wdd);
> + ktime_t t = watchdog_next_keepalive(wdd);
>
> if (t > 0)
> - mod_delayed_work(watchdog_wq, &wd_data->work, t);
> + hrtimer_start(&wd_data->timer, t, HRTIMER_MODE_REL);
> } else {
> - cancel_delayed_work(&wd_data->work);
> + hrtimer_cancel(&wd_data->timer);
> }
> }
>
> static int __watchdog_ping(struct watchdog_device *wdd)
> {
> struct watchdog_core_data *wd_data = wdd->wd_data;
> - unsigned long earliest_keepalive = wd_data->last_hw_keepalive +
> - msecs_to_jiffies(wdd->min_hw_heartbeat_ms);
> + ktime_t earliest_keepalive, now;
> int err;
>
> - if (time_is_after_jiffies(earliest_keepalive)) {
> - mod_delayed_work(watchdog_wq, &wd_data->work,
> - earliest_keepalive - jiffies);
> + earliest_keepalive = ktime_add(wd_data->last_hw_keepalive,
> + ms_to_ktime(wdd->min_hw_heartbeat_ms));
> + now = ktime_get();
> +
> + if (ktime_after(earliest_keepalive, now)) {
> + hrtimer_start(&wd_data->timer,
> + ktime_sub(earliest_keepalive, now),
> + HRTIMER_MODE_REL);
> return 0;
> }
>
> - wd_data->last_hw_keepalive = jiffies;
> + wd_data->last_hw_keepalive = now;
>
> if (wdd->ops->ping)
> err = wdd->ops->ping(wdd); /* ping the watchdog */
> @@ -192,7 +197,7 @@ static int watchdog_ping(struct watchdog_device *wdd)
>
> set_bit(_WDOG_KEEPALIVE, &wd_data->status);
>
> - wd_data->last_keepalive = jiffies;
> + wd_data->last_keepalive = ktime_get();
> return __watchdog_ping(wdd);
> }
>
> @@ -203,17 +208,18 @@ static bool watchdog_worker_should_ping(struct watchdog_core_data *wd_data)
> return wdd && (watchdog_active(wdd) || watchdog_hw_running(wdd));
> }
>
> -static void watchdog_ping_work(struct work_struct *work)
> +enum hrtimer_restart watchdog_ping_work(struct hrtimer *timer)
static
> {
> struct watchdog_core_data *wd_data;
>
> - wd_data = container_of(to_delayed_work(work), struct watchdog_core_data,
> - work);
> + wd_data = container_of(timer, struct watchdog_core_data, timer);
>
> mutex_lock(&wd_data->lock);
> if (watchdog_worker_should_ping(wd_data))
> __watchdog_ping(wd_data->wdd);
> mutex_unlock(&wd_data->lock);
> +
> + return HRTIMER_NORESTART;
I am not sure if it is a good idea to execute the ping in this context.
After all, this code may block (eg if the ping is sent to an i2c device,
but also due to the use of a mutex). Looking into other drivers using
high resolution timers, it is common to have the actual work done by
a worker.
Of course, that might defeat the purpose of this exercise, so the
question is if it is possible to have a realtime worker. Can you explore ?
Thanks,
Guenter
> }
>
> /*
> @@ -230,7 +236,7 @@ static void watchdog_ping_work(struct work_struct *work)
> static int watchdog_start(struct watchdog_device *wdd)
> {
> struct watchdog_core_data *wd_data = wdd->wd_data;
> - unsigned long started_at;
> + ktime_t started_at;
> int err;
>
> if (watchdog_active(wdd))
> @@ -238,7 +244,7 @@ static int watchdog_start(struct watchdog_device *wdd)
>
> set_bit(_WDOG_KEEPALIVE, &wd_data->status);
>
> - started_at = jiffies;
> + started_at = ktime_get();
> if (watchdog_hw_running(wdd) && wdd->ops->ping)
> err = wdd->ops->ping(wdd);
> else
> @@ -919,10 +925,8 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
> wd_data->wdd = wdd;
> wdd->wd_data = wd_data;
>
> - if (!watchdog_wq)
> - return -ENODEV;
> -
> - INIT_DELAYED_WORK(&wd_data->work, watchdog_ping_work);
> + hrtimer_init(&wd_data->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> + wd_data->timer.function = watchdog_ping_work;
>
> if (wdd->id == 0) {
> old_wd_data = wd_data;
> @@ -958,7 +962,7 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
> }
>
> /* Record time of most recent heartbeat as 'just before now'. */
> - wd_data->last_hw_keepalive = jiffies - 1;
> + wd_data->last_hw_keepalive = ktime_get();
Please assign ktime_sub(ktime_get(), 1);
Thanks,
Guenter
>
> /*
> * If the watchdog is running, prevent its driver from being unloaded,
> @@ -968,7 +972,7 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
> if (handle_boot_enabled) {
> __module_get(wdd->ops->owner);
> kref_get(&wd_data->kref);
> - queue_delayed_work(watchdog_wq, &wd_data->work, 0);
> + hrtimer_start(&wd_data->timer, 0, HRTIMER_MODE_REL);
> } else {
> pr_info("watchdog%d running and kernel based pre-userspace handler disabled\n",
> wdd->id);
> @@ -1006,7 +1010,7 @@ static void watchdog_cdev_unregister(struct watchdog_device *wdd)
> watchdog_stop(wdd);
> }
>
> - cancel_delayed_work_sync(&wd_data->work);
> + hrtimer_cancel(&wd_data->timer);
>
> kref_put(&wd_data->kref, watchdog_core_data_release);
> }
> @@ -1111,13 +1115,6 @@ int __init watchdog_dev_init(void)
> {
> int err;
>
> - watchdog_wq = alloc_workqueue("watchdogd",
> - WQ_HIGHPRI | WQ_MEM_RECLAIM, 0);
> - if (!watchdog_wq) {
> - pr_err("Failed to create watchdog workqueue\n");
> - return -ENOMEM;
> - }
> -
> err = class_register(&watchdog_class);
> if (err < 0) {
> pr_err("couldn't register class\n");
> @@ -1135,7 +1132,6 @@ int __init watchdog_dev_init(void)
> err_alloc:
> class_unregister(&watchdog_class);
> err_register:
> - destroy_workqueue(watchdog_wq);
> return err;
> }
>
> @@ -1149,7 +1145,6 @@ void __exit watchdog_dev_exit(void)
> {
> unregister_chrdev_region(watchdog_devt, MAX_DOGS);
> class_unregister(&watchdog_class);
> - destroy_workqueue(watchdog_wq);
> }
>
> module_param(handle_boot_enabled, bool, 0444);
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] watchdog: core: make sure the watchdog_worker always works
2017-12-07 14:45 ` Guenter Roeck
@ 2017-12-08 10:33 ` Christophe LEROY
0 siblings, 0 replies; 3+ messages in thread
From: Christophe LEROY @ 2017-12-08 10:33 UTC (permalink / raw)
To: Guenter Roeck, Wim Van Sebroeck
Cc: linux-kernel, linuxppc-dev, linux-watchdog
Le 07/12/2017 à 15:45, Guenter Roeck a écrit :
> On 12/07/2017 02:38 AM, Christophe Leroy wrote:
>> When running a command like 'chrt -f 99 dd if=/dev/zero of=/dev/null',
>> the watchdog_worker fails to service the HW watchdog and the
>> HW watchdog fires long before the watchdog soft timeout.
>>
>> At the moment, the watchdog_worker is invoked as a delayed work.
>> Delayed works are handled by non realtime kernel threads. The
>> WQ_HIGHPRI flag only increases the niceness of that threads.
>>
>> This patchs replaces the delayed work logic by hrtimer, in order to
>
> s/patchs/patch/
>
>> ensure that the watchdog_worker will already have priority.
>
> always ?
>
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>> drivers/watchdog/watchdog_dev.c | 87
>> +++++++++++++++++++----------------------
>> 1 file changed, 41 insertions(+), 46 deletions(-)
>>
[snip]
>
> I am not sure if it is a good idea to execute the ping in this context.
> After all, this code may block (eg if the ping is sent to an i2c device,
> but also due to the use of a mutex). Looking into other drivers using
> high resolution timers, it is common to have the actual work done by
> a worker.
> Of course, that might defeat the purpose of this exercise, so the
> question is if it is possible to have a realtime worker. Can you explore ?
kthread_delayed_work is likely our solution:
-
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b56c0d8937e665a27d90517ee7a746d0aa05af46
-
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=22597dc3d97b1ead2aca201397415a1a84bf2b26
I submitted v2 of the patch based on that instead of hrtimer. Also
implies less modifications to the code.
Christophe
>
> Thanks,
> Guenter
>
>> }
>> /*
>> @@ -230,7 +236,7 @@ static void watchdog_ping_work(struct work_struct
>> *work)
>> static int watchdog_start(struct watchdog_device *wdd)
>> {
>> struct watchdog_core_data *wd_data = wdd->wd_data;
>> - unsigned long started_at;
>> + ktime_t started_at;
>> int err;
>> if (watchdog_active(wdd))
>> @@ -238,7 +244,7 @@ static int watchdog_start(struct watchdog_device
>> *wdd)
>> set_bit(_WDOG_KEEPALIVE, &wd_data->status);
>> - started_at = jiffies;
>> + started_at = ktime_get();
>> if (watchdog_hw_running(wdd) && wdd->ops->ping)
>> err = wdd->ops->ping(wdd);
>> else
>> @@ -919,10 +925,8 @@ static int watchdog_cdev_register(struct
>> watchdog_device *wdd, dev_t devno)
>> wd_data->wdd = wdd;
>> wdd->wd_data = wd_data;
>> - if (!watchdog_wq)
>> - return -ENODEV;
>> -
>> - INIT_DELAYED_WORK(&wd_data->work, watchdog_ping_work);
>> + hrtimer_init(&wd_data->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>> + wd_data->timer.function = watchdog_ping_work;
>> if (wdd->id == 0) {
>> old_wd_data = wd_data;
>> @@ -958,7 +962,7 @@ static int watchdog_cdev_register(struct
>> watchdog_device *wdd, dev_t devno)
>> }
>> /* Record time of most recent heartbeat as 'just before now'. */
>> - wd_data->last_hw_keepalive = jiffies - 1;
>> + wd_data->last_hw_keepalive = ktime_get();
>
> Please assign ktime_sub(ktime_get(), 1);
>
> Thanks,
> Guenter
>
>> /*
>> * If the watchdog is running, prevent its driver from being
>> unloaded,
>> @@ -968,7 +972,7 @@ static int watchdog_cdev_register(struct
>> watchdog_device *wdd, dev_t devno)
>> if (handle_boot_enabled) {
>> __module_get(wdd->ops->owner);
>> kref_get(&wd_data->kref);
>> - queue_delayed_work(watchdog_wq, &wd_data->work, 0);
>> + hrtimer_start(&wd_data->timer, 0, HRTIMER_MODE_REL);
>> } else {
>> pr_info("watchdog%d running and kernel based
>> pre-userspace handler disabled\n",
>> wdd->id);
>> @@ -1006,7 +1010,7 @@ static void watchdog_cdev_unregister(struct
>> watchdog_device *wdd)
>> watchdog_stop(wdd);
>> }
>> - cancel_delayed_work_sync(&wd_data->work);
>> + hrtimer_cancel(&wd_data->timer);
>> kref_put(&wd_data->kref, watchdog_core_data_release);
>> }
>> @@ -1111,13 +1115,6 @@ int __init watchdog_dev_init(void)
>> {
>> int err;
>> - watchdog_wq = alloc_workqueue("watchdogd",
>> - WQ_HIGHPRI | WQ_MEM_RECLAIM, 0);
>> - if (!watchdog_wq) {
>> - pr_err("Failed to create watchdog workqueue\n");
>> - return -ENOMEM;
>> - }
>> -
>> err = class_register(&watchdog_class);
>> if (err < 0) {
>> pr_err("couldn't register class\n");
>> @@ -1135,7 +1132,6 @@ int __init watchdog_dev_init(void)
>> err_alloc:
>> class_unregister(&watchdog_class);
>> err_register:
>> - destroy_workqueue(watchdog_wq);
>> return err;
>> }
>> @@ -1149,7 +1145,6 @@ void __exit watchdog_dev_exit(void)
>> {
>> unregister_chrdev_region(watchdog_devt, MAX_DOGS);
>> class_unregister(&watchdog_class);
>> - destroy_workqueue(watchdog_wq);
>> }
>> module_param(handle_boot_enabled, bool, 0444);
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-12-08 10:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-07 10:38 [PATCH] watchdog: core: make sure the watchdog_worker always works Christophe Leroy
2017-12-07 14:45 ` Guenter Roeck
2017-12-08 10:33 ` Christophe LEROY
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).