Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use monotonic clock. #281

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use monotonic clock. #281

wants to merge 1 commit into from

Conversation

dajohi
Copy link
Contributor

@dajohi dajohi commented Dec 6, 2018

This avoids a jumping system clock causing issues.

@rewolff
Copy link
Collaborator

rewolff commented Dec 7, 2018

I seriously doubt that all systems that we support will have the function that you use to get the monotonic clock. So, you've done things nicely that there is now one "getmonotime" function whose implementation could be what you've written, OR just "gettimeofday" when clock_gettime or TIMESPEC_TO_TIMEVAL does not exist.

Ideally a compile time check would determine if clock_gettime is available and use the "better" interface when available.

If you can't hack autoconf to make it that way I need proof that this causes problems in practice before I commit such a change that is sure to cause problems on systems that don't have clock_gettime in the way that you expect....

This avoids a jumping system clock causing issues.
gettimeofday(&lasttime, NULL);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use getmonotime here?

gettimeofday(&thistime, NULL);

#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use getmonotime here?

@rewolff
Copy link
Collaborator

rewolff commented Aug 9, 2019

Look, I don't understand what the problem is that this solves. The systemtime is normally monotonic, it doesn't jump around when it wants to. Or say, "well, on tuesdays it is often really bad". No, time just goes forward all the time.
Now when DOES the system time jump? Well if you go on vacation, turn off your computer for a week and your bios clock is really slow, so it has lost say 5 seconds. You start her up and almost immediately start an mtr to a host with one of the first hops having a 10 second delay.

So while this is running, ntp finally trusts the measurements enough to jump the system time. from 10:05:23 time suddenly jumps to 10:05:28. Now packets sent at 10:05:21, suddenly arrive only at 10:05:36. getmonotime does nothing to correct this.

If ntp jumps the time the other way, there might be some "faster" packets that get a corrected, but still wrong timestamp when they get back. On the long packets, you simply get wrong measurements again. If mtr does see and display these absurd values, the user can clearly see that there are measurement errors. Suppose this jump happens when I'm not looking. When I get back, I see a nicely progressing "minimum time" going from 0 to 1 second. That minimum time is what I often use to guess the "no congestion" delay of a path. When there are negative numbers in the list I know there is a measurement error. If all looks seemingly OK, I might be convinced that those numbers are real.

getmonotime is meant for situations where time going backwards is really bad, Or when two consecutive timestamps should in no circumstance be the same, because the order in which things happened is very important for you.

So..... I'm afraid such a change will break compatibility with some existing platforms that we support. Secondly I don't see the problem that it is supposed to fix.

#ifdef HAVE_CLOCK_GETTIME
#include <time.h>
#endif

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to add #include <stddef.h> otherwise NULL is undefined.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants