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

runtime: use CLOCK_BOOTIME, not CLOCK_MONOTONIC, when possible #24595

Open
ianlancetaylor opened this Issue Mar 29, 2018 · 19 comments

Comments

Projects
None yet
10 participants
@ianlancetaylor
Contributor

ianlancetaylor commented Mar 29, 2018

On GNU/Linux since 2.6.39 clock_gettime supports CLOCK_BOOTTIME, which is equivalent to CLOCK_MONOTONIC except that it also accounts for time when the computer is asleep. We should prefer CLOCK_BOOTTIME to CLOCK_MONOTONIC when it is available. That will permit time.Time.Sub to return the correct value across periods when the computer has gone to sleep.

See #23178.

@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 29, 2018

@rsc

This comment has been minimized.

Contributor

rsc commented Mar 30, 2018

Define "correct". I am not convinced.

@rsc

This comment has been minimized.

Contributor

rsc commented Mar 30, 2018

Sorry. That was too short.

I think it's plausible that some people will want to exclude the time when the computer is asleep. I will also note that - I believe - the monotonic time always matches runtime.nanotime, and I'm not sure the runtime timers necessarily want to include the time when the computer is asleep.

Or imagine calling time.Ticker(1*time.Hour) to do some kind of cleanup once per hour. If the computer was off for 50 minutes, do you want the tick after 10 minutes of program execution or not? I'm not sure you do.

I can also imagine programs depending on one or the other behavior. If all current systems agree that monotonic time does not include time the computer is asleep, then I'm not sure we should introduce a Linux-specific variation. (I'm assuming - maybe incorrectly - that other systems typically don't support this.)

My point is only that I don't think it's a slam dunk we should definitely do this. I'd like to know more about what other systems do. For example, do Python and Java's monotonic time measurements include time spent asleep? How many operating systems provide access to time spent asleep?

@mpx

This comment has been minimized.

Contributor

mpx commented Apr 1, 2018

There is an interesting comparison to be made with suspending a single Go process (SIGSTOP/delay/SIGCONT). Over a shorter time frame an overloaded system might look similar as well - real time passes without any opportunities to process events due to cpu starvation.

Processes can't control whether they experience some form of cpu starvation, either via system suspend, process suspend, or overload. They probably need to handle events occuring without much processing time to be reliable. Eg, a process doing an hourly cleanup needs to handle the cleanup occurring "too soon" otherwise it should track against amount processed, not real time.

It is surprising that t1.Sub(t0) doesn't return duration in realtime, but an ambiguous "time spent while system (not necessarily process) was running". Other languages without Go's monotonic aware time.Now will return the expected realtime duration in a similar situation.

@mpx

This comment has been minimized.

Contributor

mpx commented Apr 16, 2018

CLOCK_MONOTONIC will behave the same as CLOCK_BOOTTIME as of Linux 4.17 (advancing time while suspended). Details here.

This may be a good argument to use CLOCK_BOOTTIME for consistency between kernels.

@ash2k

This comment has been minimized.

ash2k commented Apr 16, 2018

It is surprising that t1.Sub(t0) doesn't return duration in realtime, but an ambiguous "time spent while system (not necessarily process) was running". Other languages without Go's monotonic aware time.Now will return the expected realtime duration in a similar situation.

If I understand correctly, this is also a change in behaviour compared to the previous implementation when time.Now was not monotonic aware.

@zx2c4

This comment has been minimized.

Contributor

zx2c4 commented May 4, 2018

Oh, I just opened a duplicate of this. Yes, please make this change. Without it, implementing certain network protocols reliably for Android devices is really miserable.

@gopherbot

This comment has been minimized.

gopherbot commented May 4, 2018

Change https://golang.org/cl/111356 mentions this issue: runtime/linux: use CLOCK_BOOTTIME in nanotime()

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 4, 2018

https://github.com/golang/go/wiki/MinimumRequirements#linux says we currently require Kernel version 2.6.23.

Would this require bumping our minimum kernel version?

Or would we use this conditionally at runtime?

@zx2c4

This comment has been minimized.

Contributor

zx2c4 commented May 4, 2018

BOOTTIME requires 2.6.39 indeed. I just checked old kernels, and fortunately clock_gettime returns -EINVAL if you pass it too new of a value, which means you could easily try BOOTTIME and fall back to MONOTONIC, if you're on a kernel that doesn't support it. Or you could just raise the minimum kernel requirement. Either way is fine with me. (It's worth noting that the oldest kernel supported by kernel.org is 3.2.)

@slrz

This comment has been minimized.

slrz commented May 7, 2018

Note that the change to make CLOCK_MONOTONIC behave like BOOTTIME in Linux 4.17 got reverted quickly due to causing compatibility issues. It won't happen, at least not for a very long time, if ever.

https://www.spinics.net/lists/linux-tip-commits/msg43709.html

@zx2c4

This comment has been minimized.

Contributor

zx2c4 commented May 7, 2018

Bummer, though still not a reason for us to inherit Linux's legacy problems.

@slrz

This comment has been minimized.

slrz commented May 7, 2018

Agreed. It's just that the same kind of issue (like the systemd watchdog timeouts triggering on resume) may very well lurk in Go programs, too. It's probably worth a try, reverting the change before release if things blow up.

@zx2c4

This comment has been minimized.

Contributor

zx2c4 commented May 9, 2018

Welp, this is unfortunate, but I'm doing this now downstream: https://git.zx2c4.com/wireguard-android/commit/?id=7f8799a4d44058d2fe0981841b8b6d825f97aee7

(The good news is that it actually works pretty well.)

@eliasnaur

This comment has been minimized.

Contributor

eliasnaur commented May 20, 2018

FWIW, macOS added mach_continuous_time in macOS 10.12. It is like mach_absolute_time but advances during sleep.

@FiloSottile FiloSottile added NeedsDecision and removed NeedsFix labels May 22, 2018

@FiloSottile

This comment has been minimized.

Member

FiloSottile commented May 22, 2018

Tagging this NeedsDecision as the CL is blocked on @rsc approving.

While I see the risk of having the same timeout issues the Linux kernel had, I think our case is different because it’s exposed through an interface that normally refers to real time. The best argument I’ve seen for this change is that without it time.Since can return very surprising results.

@zx2c4

This comment has been minimized.

Contributor

zx2c4 commented May 22, 2018

The best argument I’ve seen

It also makes stateful network protocols extremely difficult to implement without this, particularly cryptographic ones, where cleaning up old keys is important.

@rsc

This comment has been minimized.

Contributor

rsc commented Jun 25, 2018

OK, feel free to try this early in Go 1.12.

@rsc rsc modified the milestones: Go1.11, Go1.12 Jun 25, 2018

@zx2c4

This comment has been minimized.

Contributor

zx2c4 commented Jun 25, 2018

Okay. I'll set a reminder for myself to do this when the 1.12 tree opens in August.

@zx2c4

This comment has been minimized.

Contributor

zx2c4 commented Sep 4, 2018

OK, feel free to try this early in Go 1.12.

This has now been submitted at https://go-review.googlesource.com/c/go/+/111356 for merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment