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/CANEVyjsFktF1-d1SMBv1mqoTx4R9aABDDJrG82e6oX%2BFsAKZ8g%40mail.gmail.com.
