[PATCH] core: fix unbound watchdog-notify for timeouts <2s

[PATCH] core: fix unbound watchdog-notify for timeouts <2s

[David Herrmann]

If the watchdog timeout is below 2s, we end up with a timeout of 0s as
glib event source. This causes unbound watchdog notifications. Avoid this
by never using event-timeouts below 1s.

Reported by Michael Biebl.

---
 src/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/main.c b/src/main.c
index e6bac6e..da0bd12 100644
--- a/src/main.c
+++ b/src/main.c
@@ -596,9 +596,10 @@ int main(int argc, char *argv[])
 
 		seconds = atoi(watchdog_usec) / (1000 * 1000);
 		info("Watchdog timeout is %d seconds", seconds);
+		seconds = (seconds < 4) ? 1 : seconds / 2;
 
 		watchdog = g_timeout_add_seconds_full(G_PRIORITY_HIGH,
-							seconds / 2,
+							seconds,
 							watchdog_callback,
 							NULL, NULL);
 	} else
-- 
2.1.0

Re: [PATCH] core: fix unbound watchdog-notify for timeouts <2s

[Marcel Holtmann]

Hi David,

> If the watchdog timeout is below 2s, we end up with a timeout of 0s as
> glib event source. This causes unbound watchdog notifications. Avoid this
> by never using event-timeouts below 1s.
> 
> Reported by Michael Biebl.
> 
> ---
> src/main.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/src/main.c b/src/main.c
> index e6bac6e..da0bd12 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -596,9 +596,10 @@ int main(int argc, char *argv[])
> 
> 		seconds = atoi(watchdog_usec) / (1000 * 1000);
> 		info("Watchdog timeout is %d seconds", seconds);
> +		seconds = (seconds < 4) ? 1 : seconds / 2;
> 
> 		watchdog = g_timeout_add_seconds_full(G_PRIORITY_HIGH,
> -							seconds / 2,
> +							seconds,
> 							watchdog_callback,
> 							NULL, NULL);

while setting WatchdogSec=1 is pretty insane interval anyway, this now also means that setting it that low will cause a race condition between systemd's watchdog killing us and we are able to be quickly enough to respond with a keep alive message.

I get the feeling that if > 2s, then we should use g_timeout_add_seconds to get the advantage of being woken up with all other timeouts in our daemon. And when it is <= 2s, then we better use a high precision g_timeout_add.

One alternative is to actually use timerfd natively here and use kernel based range timers.

Regards

Marcel

Re: [PATCH] core: fix unbound watchdog-notify for timeouts <2s

[David Herrmann]

Hi

On Sat, Sep 13, 2014 at 6:54 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> while setting WatchdogSec=1 is pretty insane interval anyway, this now also means that setting it that low will cause a race condition between systemd's watchdog killing us and we are able to be quickly enough to respond with a keep alive message.

Whoops, indeed. The patch is stupid. And yes, WatchdogSec=1 is insane,
but I guess we should at least have a sane behavior. I mean, it's
user-config so someone will (and currently is!) trigger this.

> I get the feeling that if > 2s, then we should use g_timeout_add_seconds to get the advantage of being woken up with all other timeouts in our daemon. And when it is <= 2s, then we better use a high precision g_timeout_add.

This sounds actually good. I will do that in v2.

Thanks
David

> One alternative is to actually use timerfd natively here and use kernel based range timers.
>
> Regards
>
> Marcel
>

Re: [PATCH] core: fix unbound watchdog-notify for timeouts <2s

[David Herrmann]

Hi

On Sat, Sep 13, 2014 at 9:11 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> I get the feeling that if > 2s, then we should use g_timeout_add_seconds to get the advantage of being woken up with all other timeouts in our daemon. And when it is <= 2s, then we better use a high precision g_timeout_add.
>
> This sounds actually good. I will do that in v2.

Ok, glib only supports millisecond precision, so whatever we do, there
will be a WATCHDOG_USEC range that we cannot react to properly. Ok, to
be honest, if WATCHDOG_USEC is below 1ms, we're screwed anyway, so
maybe we should just keep our current behavior.. I mean there is
nothing sane to do for use if users specify such low watchdog ranges.
At some point, we have to resort to either run watchdogs without a
timeout or bail out and tells users to stop using such low values.
Whether this is at 1s or at 1ms doesn't matter much, does it?

So I think we can just keep the current behavior.

Thanks
David

Re: [PATCH] core: fix unbound watchdog-notify for timeouts <2s

[Marcel Holtmann]

Hi David,

>>> I get the feeling that if > 2s, then we should use g_timeout_add_seconds to get the advantage of being woken up with all other timeouts in our daemon. And when it is <= 2s, then we better use a high precision g_timeout_add.
>> 
>> This sounds actually good. I will do that in v2.
> 
> Ok, glib only supports millisecond precision, so whatever we do, there
> will be a WATCHDOG_USEC range that we cannot react to properly. Ok, to
> be honest, if WATCHDOG_USEC is below 1ms, we're screwed anyway, so
> maybe we should just keep our current behavior.. I mean there is
> nothing sane to do for use if users specify such low watchdog ranges.
> At some point, we have to resort to either run watchdogs without a
> timeout or bail out and tells users to stop using such low values.
> Whether this is at 1s or at 1ms doesn't matter much, does it?

I am fine with just bailing out and terminating the daemon with an error exit code in case the watchdog timeout is < 2 seconds.

Regards

Marcel