No worries. The time-related code has always been quite tricky to reason about:-)
On Sunday, March 15, 2020 at 3:33:18 PM UTC-4, Nadav Har'El wrote: > > On Sun, Mar 15, 2020 at 9:25 PM Commit Bot <[email protected] > <javascript:>> wrote: > >> From: Waldemar Kozaczuk <[email protected] <javascript:>> >> Committer: Waldemar Kozaczuk <[email protected] <javascript:>> >> 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 <[email protected] <javascript:>> >> >> --- >> diff --git a/libc/timerfd.cc b/libc/timerfd.cc >> --- 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 + _interval; >> > > 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 [email protected] <javascript:>. >> To view this discussion on the web visit >> https://groups.google.com/d/msgid/osv-dev/0000000000006bfd3c05a0e9a9b8%40google.com >> . >> > -- 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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/ca046b3f-6070-401a-ad04-40645380bdcc%40googlegroups.com.
