[PATCH] libc: make timerfd_* functions handle wall clock jumps

[Waldemar Kozaczuk]

When wall clock jumps backwards triggered for example by host clock
change and propagated through kvmclock synchronization mechanism
implemented by the commit https://github.com/cloudius-systems/osv/commit/e694d066d20d87897d4a9b6dc721c0926d0fdc74,
timerfd::read() may see 'now' timepoint before the 'expiration' upon
wakeup event which is opposite to what the current implementation
expects and leads to broken assert crash.

This patch fixes the implementation of timerfd::read() by
differentiating between CLOCK_MONOTONIC and CLOCK_REALTIME
and handling the case when clock is realtime (wall clock)
and 'now' is before the 'expiration'. In which case we
simply re-arm to wake up at the same 'expiration' timepoint
which is still ahead.

Partially addresses #1076. Please note that we need to adjust
tst-timerfd to account for clock jumping forwards or backwards.

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---
libc/timerfd.cc | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/libc/timerfd.cc b/libc/timerfd.cc
index 5d382fa3..70500dca 100644
--- a/libc/timerfd.cc
+++ b/libc/timerfd.cc
@@ -179,6 +179,7 @@ int timerfd::read(uio *data, int flags)
}

WITH_LOCK(_mutex) {
+again:
while (!_expiration || _wakeup_due) {
if (f_flags & O_NONBLOCK) {
return EAGAIN;
@@ -193,14 +194,21 @@ int timerfd::read(uio *data, int flags)
_expiration = 0;
} else {
auto now = time_now();
- // set next wakeup for the next multiple of interval from
- // _expiration which is after "now".
- assert (now >= _expiration);
- u64 count = (now - _expiration) / _interval;
- _expiration = _expiration + (count+1) * _interval;
- _wakeup_due = _expiration;
- _wakeup_change_cond.wake_one();
- ret = 1 + count;
+ if (_clockid == CLOCK_MONOTONIC || now >= _expiration) {
+ // set next wakeup for the next multiple of interval from
+ // _expiration which is after "now".
+ assert (now >= _expiration);
+ u64 count = (now - _expiration) / _interval;
+ _expiration = _expiration + (count+1) * _interval;
+ _wakeup_due = _expiration;
+ _wakeup_change_cond.wake_one();
+ ret = 1 + count;
+ } else {
+ // Clock is REALTIME and now < _expiration (clock may have jumped backwards)
+ _wakeup_due = _expiration;
+ _wakeup_change_cond.wake_one();
+ goto again;
+ }
}
copy_to_uio((const char *)&ret, sizeof(ret), data);
return 0;
--
2.20.1

[Nadav Har'El]

I wonder if this shouldn't be  _expiration + interval -

_expiration was the time of the *previous* expiration, which already happened. The next expiration is supposed to happen at _expiration + interval.

For example, if the interval is 1 second, and we had a wakeup at wall clock 7.0, and suddenly wallclock went back to 6.999, the next wakeup should still be at 8.0 - not at 7.0 again.

+                _wakeup_change_cond.wake_one();
+                goto again;

+            }
         }
         copy_to_uio((const char *)&ret, sizeof(ret), data);
         return 0;
--
2.20.1

--
You received this message because you are subscribed to the Google Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/20200313014725.14928-1-jwkozaczuk%40gmail.com.

[Waldek Kozaczuk]

You might be right. But does it mean we can count on wake up mechanism to wake up up every _interval according to monotonic or real-time clock. I am honestly a bit at loss in my reasoning:-)

[Nadav Har'El]

On Sun, Mar 15, 2020 at 5:36 PM Waldek Kozaczuk <jwkoz...@gmail.com> wrote:

You might be right. But does it mean we can count on wake up mechanism to wake up up every _interval according to monotonic or real-time clock.

I don't understand what your version guarantees - imagine we ask to be woken up every *second* (in wall clock, I don't know why anybody would want to do that...), and we were last woken up on 10:00:00. Now the clock back moved a full minute (wow!) to 9:59:00. In your code it will now wait until 10:00:00 again - a whole minute. In my code it waits for a minute and a second 10:00:01. Neither version would wait just one second...

I have to be honest, I don't know what Linux do in this case. They have a flag TFD_TIMER_CANCEL_ON_SET which causes a special failure of the read() if the time was set. But what happens without this flag?

I don't think you should spend too much effort on this. Just pick whatever you think is most reasonable, and we can commit it.

[Commit Bot]

From: Waldemar Kozaczuk <jwkoz...@gmail.com>
Committer: Waldemar Kozaczuk <jwkoz...@gmail.com>
Branch: master

libc: make timerfd_* functions handle wall clock jumps

When wall clock jumps backwards triggered for example by host clock
change and propagated through kvmclock synchronization mechanism
implemented by the commit https://github.com/cloudius-systems/osv/commit/e694d066d20d87897d4a9b6dc721c0926d0fdc74,
timerfd::read() may see 'now' timepoint before the 'expiration' upon
wakeup event which is opposite to what the current implementation
expects and leads to broken assert crash.

This patch fixes the implementation of timerfd::read() by
differentiating between CLOCK_MONOTONIC and CLOCK_REALTIME
and handling the case when clock is realtime (wall clock)
and 'now' is before the 'expiration'. In which case we

simply re-arm to wake up at the 'expiration' + 'interval' timepoint.

Partially addresses #1076. Please note that we need to adjust
tst-timerfd to account for clock jumping forwards or backwards.

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>

---

diff --git a/libc/timerfd.cc b/libc/timerfd.cc

+ _wakeup_due = _expiration + _interval;

[Nadav Har'El]

I think I might have given you bad advice :-(

I'm starting to realize that _expiration maybe wasn't the *previous* expiration, rather it was the planned

expiration. Usually we discover that now is a little over expiration, and get count = 0 (now - expiration < interval)

and return 1. If now is before expiration, we should probably wait again until expiration, and we will eventually

return 1 then. If we wait until _expiration + _interval like I asked you to do, when we wake up a little *over*

_expiration + _interval, we will have count = 1 and return 2.

So now I think that your original code was the correct one. Sorry about that..

 

+                _wakeup_change_cond.wake_one();
+                goto again;
+            }
         }
         copy_to_uio((const char *)&ret, sizeof(ret), data);
         return 0;

--
You received this message because you are subscribed to the Google Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/0000000000006bfd3c05a0e9a9b8%40google.com.

[Waldek Kozaczuk]

No worries. The time-related code has always been quite tricky to reason about:-)

To unsubscribe from this group and stop receiving emails from it, send an email to osv...@googlegroups.com.