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 <b...@cloudius-systems.com > <javascript:>> wrote: > >> From: Waldemar Kozaczuk <jwkoz...@gmail.com <javascript:>> >> Committer: Waldemar Kozaczuk <jwkoz...@gmail.com <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 <jwkoz...@gmail.com <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 osv...@googlegroups.com <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 osv-dev+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/ca046b3f-6070-401a-ad04-40645380bdcc%40googlegroups.com.
