On Sun, Mar 15, 2020 at 9:25 PM Commit Bot <[email protected]> wrote:
> From: Waldemar Kozaczuk <[email protected]> > Committer: Waldemar Kozaczuk <[email protected]> > 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]> > > --- > 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]. > 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/CANEVyju5htU3zeUxQJYcPmYcBXpD3%3DPZZ-dWC5etOksbht1r3Q%40mail.gmail.com.
