* [PATCH] core: fix unbound watchdog-notify for timeouts <2s
@ 2014-09-13 13:21 David Herrmann
2014-09-13 16:54 ` Marcel Holtmann
0 siblings, 1 reply; 5+ messages in thread
From: David Herrmann @ 2014-09-13 13:21 UTC (permalink / raw)
To: linux-bluetooth
Cc: Johan Hedberg, Marcel Holtmann, Michael Biebl, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] core: fix unbound watchdog-notify for timeouts <2s
2014-09-13 13:21 [PATCH] core: fix unbound watchdog-notify for timeouts <2s David Herrmann
@ 2014-09-13 16:54 ` Marcel Holtmann
2014-09-13 19:11 ` David Herrmann
0 siblings, 1 reply; 5+ messages in thread
From: Marcel Holtmann @ 2014-09-13 16:54 UTC (permalink / raw)
To: David Herrmann; +Cc: BlueZ development, Johan Hedberg, Michael Biebl
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] core: fix unbound watchdog-notify for timeouts <2s
2014-09-13 16:54 ` Marcel Holtmann
@ 2014-09-13 19:11 ` David Herrmann
2014-09-14 17:41 ` David Herrmann
0 siblings, 1 reply; 5+ messages in thread
From: David Herrmann @ 2014-09-13 19:11 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development, Johan Hedberg, Michael Biebl
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] core: fix unbound watchdog-notify for timeouts <2s
2014-09-13 19:11 ` David Herrmann
@ 2014-09-14 17:41 ` David Herrmann
2014-09-14 17:54 ` Marcel Holtmann
0 siblings, 1 reply; 5+ messages in thread
From: David Herrmann @ 2014-09-14 17:41 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: BlueZ development, Johan Hedberg, Michael Biebl
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] core: fix unbound watchdog-notify for timeouts <2s
2014-09-14 17:41 ` David Herrmann
@ 2014-09-14 17:54 ` Marcel Holtmann
0 siblings, 0 replies; 5+ messages in thread
From: Marcel Holtmann @ 2014-09-14 17:54 UTC (permalink / raw)
To: David Herrmann; +Cc: BlueZ development, Johan Hedberg, Michael Biebl
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-09-14 17:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-13 13:21 [PATCH] core: fix unbound watchdog-notify for timeouts <2s David Herrmann
2014-09-13 16:54 ` Marcel Holtmann
2014-09-13 19:11 ` David Herrmann
2014-09-14 17:41 ` David Herrmann
2014-09-14 17:54 ` Marcel Holtmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.