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:-)
On Sun, Mar 15, 2020 at 11:30 Nadav Har'El <[email protected]> wrote: > On Fri, Mar 13, 2020 at 3:47 AM Waldemar Kozaczuk <[email protected]> > wrote: > >> 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 <[email protected]> >> --- >> 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; >> > > 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 [email protected]. >> To view this discussion on the web visit >> https://groups.google.com/d/msgid/osv-dev/20200313014725.14928-1-jwkozaczuk%40gmail.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/CAL9cFfNm5PzRG1XcsOcMALaK%2BN0QpmgUUOGG%2B%3DcSQkHhNGLFnw%40mail.gmail.com.
