On Sun, Mar 15, 2020 at 5:36 PM Waldek Kozaczuk <[email protected]> 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. > 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/CANEVyjsLBaxEmfEjiNZQW4E%3Dq_dXk%3DXHfMtLFqpOi%3DyWe5Oszg%40mail.gmail.com.
