Project

General

Profile

Feature #3309

osmo_timer_* should use monotonic clock

Added by neels over 1 year ago. Updated over 1 year ago.

Status:
New
Priority:
Normal
Assignee:
-
Category:
-
Target version:
-
Start date:
05/30/2018
Due date:
% Done:

0%

Spec Reference:

Description

Currently, osmo_timer_* and thus also osmo_fsm_* use osmo_gettimeofday(), meaning that when the user changes the system time, all of our timers glitch. This affects all osmo_timer_* and all osmo_fsm_inst_state_chg(T). I think we should change all of those to monotonic clock.

Would relying on time-of-day be a distinct feature users might rely on? If yes we'd need to add new API to indicate that we want a monotonic clock. I think that is not necessary: the API is only concerned with time durations, and glitching those because of system time change was never intended. We should just change to monotonic clock under the hood without API changes.

Comments welcome.


Related issues

Related to libosmocore - Bug #2586: fix timer duration calculationsResolved10/20/2017

History

#1 Updated by laforge over 1 year ago

Hi Neels,

On Wed, May 30, 2018 at 01:05:47PM +0000, neels [REDMINE] wrote:

Currently, osmo_timer_* and thus also osmo_fsm_* use
osmo_gettimeofday(), meaning that when the user changes the system
time, all of our timers glitch. This affects all osmo_timer_* and
all osmo_fsm_inst_state_chg(T). I think we should change all of
those to monotonic clock.

Agreed. Hoewver, is this possible without breaking ABI compatibility?

Would relying on time-of-day be a distinct feature users might rely
on?

No. I would say we can make that statement.

#2 Updated by neels over 1 year ago

under-the-hood:

ABI/API compatibility wise, we are actually mostly fine, since we can convert the monotonic-clock struct timespec to a struct timeval internally, but.

  • API: osmo_timer_remaining() optionally takes a 'now' argument, where the user can feed the current time.
    The only place we use osmo_timer_remaining() in Osmocom projects seems to be the fsm: a CTRL command returns the remaining time. There we pass now=NULL.
    I would add osmo_timer_remaining2() without the option to pass 'now' and deprecate osmo_timer_remaining().
  • ABI: obviously whoever passes gettimeofday() to osmo_timer_remaining(now) would then get wildly chaotic remaining times.
  • ABI: manually accessing/comparing osmo_timer_list.timeout no longer works.
The implementation could
  • change the old timeval in struct osmo_timer_list to struct timespec (API change that shouldn't affect "proper" API users)
    • or keep the old timeval and add a timespec to the end
  • Or, internally use osmo_clock_gettime() and divide ts_nsec by 1000 to get tv_usec (and if ts_nsec % 1000, tv_usec++ to remain consistent), still using struct timeval to store and compare.

new API:

Another proposal would be to go all the way: add a completely new osmo_timer_monotonic_* API, including a second list traversal in osmo_timers_{prepare,update,check}(). That would be a bit more work, but ensures full compat, and requires moving all callers to the new API (which could be a good thing).

pro/con:

  • in practice, we can move all users to monotonic under the hood with minimal effort, IF we don't give a moist one about users of osmo_timer_remaining(now!=NULL) and accept minimal ABI changes.
  • if we also accept API changes that should not practically affect anyone (TM), we can change the current API to struct timespec with nsec precision to avoid dividing by 1000.
  • full compat would require a full set of new API, and moving all callers individually (basically just because the current structs are not opaque enough).

I would personally choose the middle one, and accept the slight API/ABI dirtyness, and skip changing all callers manually.

If anyone votes against it (a la, we should not stump unknown users), I am very close to accepting that as well and write up a new API from scratch. It would be rather quick to achieve, basically a copy of the current one with timeval replaced by timespec and gettimeofday replaced by osmo_clock_gettime().

#3 Updated by neels over 1 year ago

incompat: we'd also change osmo_timer_nearest() to return struct timespec, and in osmo_select_main() use pselect() instead of select() to feed timespec instead of timeval.

#4 Updated by laforge 5 months ago

  • Related to Bug #2586: fix timer duration calculations added

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)