Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1200088 > unrolled thread

Re: [PATCH 2/8] watchdog: Introduce hardware maximum timeout in watchdog core

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/8] watchdog: Introduce hardware maximum timeout in watchdog  core Guenter Roeck <linux@roeck-us.net> - 2015-08-04 17:40 +0200
    Re: [PATCH 2/8] watchdog: Introduce hardware maximum timeout in  watchdog core Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-08-04 18:00 +0200
      Re: [PATCH 2/8] watchdog: Introduce hardware maximum timeout in watchdog  core Guenter Roeck <linux@roeck-us.net> - 2015-08-04 18:10 +0200
        Re: [PATCH 2/8] watchdog: Introduce hardware maximum timeout in  watchdog core Uwe Kleine-König   <u.kleine-koenig@pengutronix.de> - 2015-08-05 10:30 +0200
          Re: [PATCH 2/8] watchdog: Introduce hardware maximum timeout in watchdog  core Guenter Roeck <linux@roeck-us.net> - 2015-08-05 11:20 +0200

#1200088 — Re: [PATCH 2/8] watchdog: Introduce hardware maximum timeout in watchdog core

Hi Uwe,

On 08/04/2015 05:18 AM, Uwe Kleine-König wrote:
> On Mon, Aug 03, 2015 at 07:13:28PM -0700, Guenter Roeck wrote:
>> Introduce an optional hardware maximum timeout in the watchdog core.
>> The hardware maximum timeout can be lower than the maximum timeout.
> Is this only until all drivers are converted to make use of the central
> worker? Otherwise this doesn't make sense, right?
>
>> Drivers can set the maximum hardare timeout value in the watchdog data
> s/hardare/hardware/
>
Always those fat fingers ;-)

>> structure. If the configured timeout exceeds half the value of the
>> maximum hardware timeout, the watchdog core enables a timer function
>> to assist sending keepalive requests to the watchdog driver.
> I don't understand why you want to halve the maximum hw-timeout. If my
> watchdog has hw-max-timeout = 5s and userspace sets it to 3s there
> should be no need for assistance?! I think the implementation is the
> other way round?
>
It is supposed to reflect the _maximum_ timeout. That is different to
the time between heartbeats, which is supposed to be less; using half
the value of the maximum hardware timeout seemed to be a safe number.
It is supposed to be a constant after initialization and should not change
afterwards if the (soft) timeout is changed. Not sure how to explain
that better.

>> ---
>>   Documentation/watchdog/watchdog-kernel-api.txt |  14 +++
>>   drivers/watchdog/watchdog_dev.c                | 121 +++++++++++++++++++++----
>>   include/linux/watchdog.h                       |  21 ++++-
>>   3 files changed, 135 insertions(+), 21 deletions(-)
>>
>> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
>> index d8b0d3367706..5fa085276874 100644
>> --- a/Documentation/watchdog/watchdog-kernel-api.txt
>> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
>> @@ -53,9 +53,12 @@ struct watchdog_device {
>>   	unsigned int timeout;
>>   	unsigned int min_timeout;
>>   	unsigned int max_timeout;
>> +	unsigned int max_hw_timeout_ms;
>> +	unsigned long last_keepalive;
>>   	void *driver_data;
>>   	struct mutex lock;
>>   	unsigned long status;
>> +	struct delayed_work work;
>>   	struct list_head deferred;
>>   };
>>
>> @@ -73,8 +76,18 @@ It contains following fields:
>>     additional information about the watchdog timer itself. (Like it's unique name)
>>   * ops: a pointer to the list of watchdog operations that the watchdog supports.
>>   * timeout: the watchdog timer's timeout value (in seconds).
>> +  This is the time after which the system will reboot if user space does
>> +  not send a heartbeat request if the watchdog device is opened.
>> +  This may or may not be the hardware watchdog timeout. See max_hw_timeout_ms
>> +  for more details.
> Hmm, what is timeout then? Is this the value that the driver currently
> handles? Or the framework with the automatic pings? Probably the
> former?! This needs better wording.
>

As I say above, "This is the time after which the system will reboot if user
space does not send a heartbeat request if the watchdog device is opened".
Not sure how to express that better. Any idea ?

>>   * min_timeout: the watchdog timer's minimum timeout value (in seconds).
>>   * max_timeout: the watchdog timer's maximum timeout value (in seconds).
>> +* max_hw_timeout_ms: Maximum hardware timeout, in milli-seconds. May differ
>> +  from max_timeout. If set, the infrastructure will send a heartbeat to the
>> +  watchdog driver if 'timeout' is larger than 'max_hw_timeout / 2',
>> +  unless user space failed to ping the watchdog for 'timeout' seconds.
> In the long run max_timeout should be removed, right?
>
It could be removed, yes, though that would require each of the drivers
to set max_hw_timeout_ms. That is a much larger task, though, and might be
difficult to accomplish since each driver handles it differently (in some
cases it is a hard limit, in others it is an arbitrary number).

>> +* last_keepalive: Time of most recent keepalive triggered from user space,
>> +  in jiffies.
>>   * bootstatus: status of the device after booting (reported with watchdog
>>     WDIOF_* status bits).
>>   * driver_data: a pointer to the drivers private data of a watchdog device.
>> @@ -85,6 +98,7 @@ It contains following fields:
>>     information about the status of the device (Like: is the watchdog timer
>>     running/active, is the nowayout bit set, is the device opened via
>>     the /dev/watchdog interface or not, ...).
>> +* work: Worker data structure for WatchDog Timer Driver Core internal use only.
>>   * deferred: entry in wtd_deferred_reg_list which is used to
>>     register early initialized watchdogs.
>>
>> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
>> index 06171c73daf5..25849c1d6dc1 100644
>> --- a/drivers/watchdog/watchdog_dev.c
>> +++ b/drivers/watchdog/watchdog_dev.c
>> @@ -37,7 +37,9 @@
>>   #include <linux/errno.h>	/* For the -ENODEV/... values */
>>   #include <linux/kernel.h>	/* For printk/panic/... */
>>   #include <linux/fs.h>		/* For file operations */
>> +#include <linux/jiffies.h>	/* For timeout functions */
>>   #include <linux/watchdog.h>	/* For watchdog specific items */
>> +#include <linux/workqueue.h>	/* For workqueue */
>>   #include <linux/miscdevice.h>	/* For handling misc devices */
>>   #include <linux/init.h>		/* For __init/__exit/... */
>>   #include <linux/uaccess.h>	/* For copy_to_user/put_user/... */
>> @@ -49,6 +51,53 @@ static dev_t watchdog_devt;
>>   /* the watchdog device behind /dev/watchdog */
>>   static struct watchdog_device *old_wdd;
>>
>> +static struct workqueue_struct *watchdog_wq;
>> +
>> +static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>> +{
>> +	unsigned int hm = wdd->max_hw_timeout_ms;
>> +	unsigned int m = wdd->max_timeout * 1000;
>> +
>> +	return watchdog_active(wdd) && hm && hm != m &&
>> +		wdd->timeout * 500 > hm;
>
> I don't understand what max_timeout is now that there is max_hw_timeout.
> So I don't understand why you need hm != m either.
>

Backward compatibility. A driver which does not set max_hw_timeout_ms,
or sets both to the same value, by definition expects to handle everything
internally, and thus no worker is configured.

> Taking the example from above (hw-maxtimeout = 5000ms, current timeout =
> 3s) this doesn't trigger.
>
This is intentional. The idea here is that the driver set max_timeout
(here to a low value), and thus doesn't expect additional internal
heartbeats generated from the kernel. In the above example, user space
would be expected to send heartbeats after less than 3s, which does
not require kernel assistance. Even if the current timeout is set to 5s,
user space would be expected to send heartbeats much more often than that,
say every 2 or 3 seconds. Again, this does not require kernel assistance.

> And the other way round:
>   - hw-max-timeout = 3s
>   - timeout = 5s
>
> In this case userspace might send a ping only after 4 seconds, but
> watchdog_need_worker will be false.
>

Yep, that is wrong. The condition should be
	wdd->timeout * 1000 > hm
to trigger internal heartbeats every 1.5 seconds.

> What is the meaning of WDOG_ACTIVE now? does it mean userspace has the
> device open? Then this looks wrong, too.
>
Yes, it is, and always was. Why is that wrong ? It indicates if the
keepalive worker needs to run or not, and in this state it won't need
to run if the watchdog is not active (that state is added in the next patch).

> /me wonders if he understood that function correctly?!
>
>> @@ -88,6 +96,8 @@ struct watchdog_device {
>>   	unsigned int timeout;
>>   	unsigned int min_timeout;
>>   	unsigned int max_timeout;
>> +	unsigned int max_hw_timeout_ms;
>> +	unsigned long last_keepalive;
>>   	void *driver_data;
>>   	struct mutex lock;
>>   	unsigned long status;
> It would be nice to group this a bit to make it more clear which members
> are supposed to be set by driver and which are not.
>
Good idea. I'll do that.

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1200115 — Re: [PATCH 2/8] watchdog: Introduce hardware maximum timeout in watchdog core

On Tue, Aug 04, 2015 at 08:31:43AM -0700, Guenter Roeck wrote:
> Hi Uwe,
> 
> On 08/04/2015 05:18 AM, Uwe Kleine-König wrote:
> >On Mon, Aug 03, 2015 at 07:13:28PM -0700, Guenter Roeck wrote:
> >>Introduce an optional hardware maximum timeout in the watchdog core.
> >>The hardware maximum timeout can be lower than the maximum timeout.
> >Is this only until all drivers are converted to make use of the central
> >worker? Otherwise this doesn't make sense, right?
> >
> >>Drivers can set the maximum hardare timeout value in the watchdog data
> >s/hardare/hardware/
> >
> Always those fat fingers ;-)
> 
> >>structure. If the configured timeout exceeds half the value of the
> >>maximum hardware timeout, the watchdog core enables a timer function
> >>to assist sending keepalive requests to the watchdog driver.
> >I don't understand why you want to halve the maximum hw-timeout. If my
> >watchdog has hw-max-timeout = 5s and userspace sets it to 3s there
> >should be no need for assistance?! I think the implementation is the
> >other way round?
> >
> It is supposed to reflect the _maximum_ timeout. That is different to
> the time between heartbeats, which is supposed to be less; using half
> the value of the maximum hardware timeout seemed to be a safe number.
Right, I got that. With hw-max-timeout = 5s the machine resets after 5s
not caring for the device. And so pinging repeatedly after 2.5s is fine.
But if userspace sets a timeout of 3s (probably with the intention to
ping with a frequency of 1/1.5s) there is no need for worker-assistance,
because the pings coming in each 1.5s provided by userspace are good
enough.

> >>+static inline bool watchdog_need_worker(struct watchdog_device *wdd)
> >>+{
> >>+	unsigned int hm = wdd->max_hw_timeout_ms;
> >>+	unsigned int m = wdd->max_timeout * 1000;
> >>+
> >>+	return watchdog_active(wdd) && hm && hm != m &&
> >>+		wdd->timeout * 500 > hm;
> >
> >I don't understand what max_timeout is now that there is max_hw_timeout.
> >So I don't understand why you need hm != m either.
> >
> 
> Backward compatibility. A driver which does not set max_hw_timeout_ms,
> or sets both to the same value, by definition expects to handle everything
> internally, and thus no worker is configured.
And a driver that does

	max_timeout = 5
	max_hw_timeout = 5125

falls through the cracks.
 
Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200122

Hi Uwe,

On 08/04/2015 08:52 AM, Uwe Kleine-König wrote:
> On Tue, Aug 04, 2015 at 08:31:43AM -0700, Guenter Roeck wrote:
>> Hi Uwe,
>>
>> On 08/04/2015 05:18 AM, Uwe Kleine-König wrote:
>>> On Mon, Aug 03, 2015 at 07:13:28PM -0700, Guenter Roeck wrote:
>>>> Introduce an optional hardware maximum timeout in the watchdog core.
>>>> The hardware maximum timeout can be lower than the maximum timeout.
>>> Is this only until all drivers are converted to make use of the central
>>> worker? Otherwise this doesn't make sense, right?
>>>
>>>> Drivers can set the maximum hardare timeout value in the watchdog data
>>> s/hardare/hardware/
>>>
>> Always those fat fingers ;-)
>>
>>>> structure. If the configured timeout exceeds half the value of the
>>>> maximum hardware timeout, the watchdog core enables a timer function
>>>> to assist sending keepalive requests to the watchdog driver.
>>> I don't understand why you want to halve the maximum hw-timeout. If my
>>> watchdog has hw-max-timeout = 5s and userspace sets it to 3s there
>>> should be no need for assistance?! I think the implementation is the
>>> other way round?
>>>
>> It is supposed to reflect the _maximum_ timeout. That is different to
>> the time between heartbeats, which is supposed to be less; using half
>> the value of the maximum hardware timeout seemed to be a safe number.
> Right, I got that. With hw-max-timeout = 5s the machine resets after 5s
> not caring for the device. And so pinging repeatedly after 2.5s is fine.
> But if userspace sets a timeout of 3s (probably with the intention to
> ping with a frequency of 1/1.5s) there is no need for worker-assistance,
> because the pings coming in each 1.5s provided by userspace are good
> enough.
>
Yes, that is how it is supposed to work.

>>>> +static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>>>> +{
>>>> +	unsigned int hm = wdd->max_hw_timeout_ms;
>>>> +	unsigned int m = wdd->max_timeout * 1000;
>>>> +
>>>> +	return watchdog_active(wdd) && hm && hm != m &&
>>>> +		wdd->timeout * 500 > hm;
>>>
>>> I don't understand what max_timeout is now that there is max_hw_timeout.
>>> So I don't understand why you need hm != m either.
>>>
>>
>> Backward compatibility. A driver which does not set max_hw_timeout_ms,
>> or sets both to the same value, by definition expects to handle everything
>> internally, and thus no worker is configured.
> And a driver that does
>
> 	max_timeout = 5
> 	max_hw_timeout = 5125
>
> falls through the cracks.
>
Hmm - not that this configuration makes any sense, but you are right.
I'll make it "hm < m".

Thanks,
Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200522 — Re: [PATCH 2/8] watchdog: Introduce hardware maximum timeout in watchdog core

Hello Guenter,

On Tue, Aug 04, 2015 at 09:03:27AM -0700, Guenter Roeck wrote:
> On 08/04/2015 08:52 AM, Uwe Kleine-König wrote:
> >On Tue, Aug 04, 2015 at 08:31:43AM -0700, Guenter Roeck wrote:
> >>On 08/04/2015 05:18 AM, Uwe Kleine-König wrote:
> >>>On Mon, Aug 03, 2015 at 07:13:28PM -0700, Guenter Roeck wrote:
> >>>>structure. If the configured timeout exceeds half the value of the
> >>>>maximum hardware timeout, the watchdog core enables a timer function
> >>>>to assist sending keepalive requests to the watchdog driver.
> >>>I don't understand why you want to halve the maximum hw-timeout. If my
> >>>watchdog has hw-max-timeout = 5s and userspace sets it to 3s there
> >>>should be no need for assistance?! I think the implementation is the
> >>>other way round?
> >>>
> >>It is supposed to reflect the _maximum_ timeout. That is different to
> >>the time between heartbeats, which is supposed to be less; using half
> >>the value of the maximum hardware timeout seemed to be a safe number.
> >Right, I got that. With hw-max-timeout = 5s the machine resets after 5s
> >not caring for the device. And so pinging repeatedly after 2.5s is fine.
> >But if userspace sets a timeout of 3s (probably with the intention to
> >ping with a frequency of 1/1.5s) there is no need for worker-assistance,
> >because the pings coming in each 1.5s provided by userspace are good
> >enough.
> >
> Yes, that is how it is supposed to work.
So for the changelog you want:

	If the configured timeout exceeds the maximum hardware timeout
	the watchdog core enables a timer function ...

right?

> >>>>+static inline bool watchdog_need_worker(struct watchdog_device *wdd)
> >>>>+{
> >>>>+	unsigned int hm = wdd->max_hw_timeout_ms;
> >>>>+	unsigned int m = wdd->max_timeout * 1000;
> >>>>+
> >>>>+	return watchdog_active(wdd) && hm && hm != m &&
> >>>>+		wdd->timeout * 500 > hm;
One problem with the worker I see is that the reset will probably be
delayed with your worker. Consider userspace sets timeout = 10 s because
if the main application doesn't work for 12 s something dangerous can
happen. (Consider a guillotine where the blade can only be hold up for
12 s when not locked. :-) Now if the hw-max-timeout is 9s you setup a
timer to ping at $last_keepalive + 4.5 s and $last_keepalive + 9 s (not
taking timer and system latency into account). That means the system
only resets 18 s after the last userspace ping. Oops.

So ideally you send the last auto-ping at $last_keepalive +
$configured_timeout - $hw-max-timeout (assuming the hardware is
configured for $hw-max-timeout).

> >>>I don't understand what max_timeout is now that there is max_hw_timeout.
> >>>So I don't understand why you need hm != m either.
> >>>
> >>
> >>Backward compatibility. A driver which does not set max_hw_timeout_ms,
> >>or sets both to the same value, by definition expects to handle everything
> >>internally, and thus no worker is configured.
> >And a driver that does
> >
> >	max_timeout = 5
> >	max_hw_timeout = 5125
> >
> >falls through the cracks.
> >
> Hmm - not that this configuration makes any sense, but you are right.
> I'll make it "hm < m".
It does not? What do you expect max_timeout to be set to if the maximal
hw-timeout is 5125 ms? 0 would work, but IMHO you need some more
documentation then.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200566

On 08/05/2015 01:22 AM, Uwe Kleine-König wrote:
> Hello Guenter,
>
> On Tue, Aug 04, 2015 at 09:03:27AM -0700, Guenter Roeck wrote:
>> On 08/04/2015 08:52 AM, Uwe Kleine-König wrote:
>>> On Tue, Aug 04, 2015 at 08:31:43AM -0700, Guenter Roeck wrote:
>>>> On 08/04/2015 05:18 AM, Uwe Kleine-König wrote:
>>>>> On Mon, Aug 03, 2015 at 07:13:28PM -0700, Guenter Roeck wrote:
>>>>>> structure. If the configured timeout exceeds half the value of the
>>>>>> maximum hardware timeout, the watchdog core enables a timer function
>>>>>> to assist sending keepalive requests to the watchdog driver.
>>>>> I don't understand why you want to halve the maximum hw-timeout. If my
>>>>> watchdog has hw-max-timeout = 5s and userspace sets it to 3s there
>>>>> should be no need for assistance?! I think the implementation is the
>>>>> other way round?
>>>>>
>>>> It is supposed to reflect the _maximum_ timeout. That is different to
>>>> the time between heartbeats, which is supposed to be less; using half
>>>> the value of the maximum hardware timeout seemed to be a safe number.
>>> Right, I got that. With hw-max-timeout = 5s the machine resets after 5s
>>> not caring for the device. And so pinging repeatedly after 2.5s is fine.
>>> But if userspace sets a timeout of 3s (probably with the intention to
>>> ping with a frequency of 1/1.5s) there is no need for worker-assistance,
>>> because the pings coming in each 1.5s provided by userspace are good
>>> enough.
>>>
>> Yes, that is how it is supposed to work.
> So for the changelog you want:
>
> 	If the configured timeout exceeds the maximum hardware timeout
> 	the watchdog core enables a timer function ...
>
> right?
>
Something like that. You are right, the changelog needs an update.

>>>>>> +static inline bool watchdog_need_worker(struct watchdog_device *wdd)
>>>>>> +{
>>>>>> +	unsigned int hm = wdd->max_hw_timeout_ms;
>>>>>> +	unsigned int m = wdd->max_timeout * 1000;
>>>>>> +
>>>>>> +	return watchdog_active(wdd) && hm && hm != m &&
>>>>>> +		wdd->timeout * 500 > hm;
> One problem with the worker I see is that the reset will probably be
> delayed with your worker. Consider userspace sets timeout = 10 s because
> if the main application doesn't work for 12 s something dangerous can
> happen. (Consider a guillotine where the blade can only be hold up for
> 12 s when not locked. :-) Now if the hw-max-timeout is 9s you setup a
> timer to ping at $last_keepalive + 4.5 s and $last_keepalive + 9 s (not
> taking timer and system latency into account). That means the system
> only resets 18 s after the last userspace ping. Oops.
>
> So ideally you send the last auto-ping at $last_keepalive +
> $configured_timeout - $hw-max-timeout (assuming the hardware is
> configured for $hw-max-timeout).
>

Yes, you are right, and makes sense. In practice that means I would
schedule an auto-ping 1s after the most recent user ping in your
example above, not at fixed intervals of 4.5s. I'll look into it.
Thinking about it, it is better anyway to reschedule the auto-ping
after a user ping, to avoid unnecessary pings.

>>>>> I don't understand what max_timeout is now that there is max_hw_timeout.
>>>>> So I don't understand why you need hm != m either.
>>>>>
>>>>
>>>> Backward compatibility. A driver which does not set max_hw_timeout_ms,
>>>> or sets both to the same value, by definition expects to handle everything
>>>> internally, and thus no worker is configured.
>>> And a driver that does
>>>
>>> 	max_timeout = 5
>>> 	max_hw_timeout = 5125
>>>
>>> falls through the cracks.
>>>
>> Hmm - not that this configuration makes any sense, but you are right.
>> I'll make it "hm < m".
> It does not? What do you expect max_timeout to be set to if the maximal
> hw-timeout is 5125 ms? 0 would work, but IMHO you need some more
> documentation then.
>

0 would work, or anything larger than 5.125s.

If you want to set max_timeout to 0, there is no need to set max_hw_timeout.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web