Redefine CLOCK_BOOTTIME to alias CLOCK_MONOTONIC, not CLOCK_UPTIME

[val_packett.cool]

Summary

The suspend-awareness situation with monotonic clocks across platforms
is kind of a mess, let's try not making it worse.

On Linux, CLOCK_MONOTONIC does NOT count suspended time, and
CLOCK_BOOTTIME was introduced to INCLUDE suspended time.

On OpenBSD, CLOCK_MONOTONIC DOES count suspended time, and
CLOCK_UPTIME was introduced to EXCLUDE suspended time.

On macOS, it's the same as OpenBSD, but with CLOCK_UPTIME_RAW.

Right now, we do not have a monotonic clock that counts suspended time.
We have CLOCK_UPTIME as a distinct ID alias, and CLOCK_BOOTTIME as
a preprocessor alias, both being effectively CLOCK_MONOTONIC for now.

When we introduce a suspend-aware clock in the future, it would make
a lot more sense to do it the OpenBSD/macOS way, i.e. to make
CLOCK_MONOTONIC include suspended time and make CLOCK_UPTIME exclude it,
because that's what the name CLOCK_UPTIME implies: a deviation from the
default intended for the uptime command to allow it to only show the
time the system was actually up and not suspended.

Let's change the define right now to make sure software using the define
would not end up using the ID of the wrong clock in the future, and
fix the IDs in the Linux compat code to match the expected changes too.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1824084
for more discussion.

Fixes: 155f15118a77 ("clock_gettime: Add Linux aliases for CLOCK_*")
Fixes: 25ada637362d ("Map Linux CLOCK_BOOTTIME to native CLOCK_UPTIME.")
Sponsored by: https://www.patreon.com/valpackett

[imp]

I'm not so sure...

This undoes:

commit 155f15118a77d2aeab7b177ada78c848778d7d80
Author: Warner Losh <[e-mail]>
Date: Fri Jul 30 17:11:43 2021 -0600

clock_gettime: Add Linux aliases for CLOCK_*

Linux standardized what we call CLOCK_{REALTIME,MONOTONIC}_FAST as
CLOCK_{REALTIME,MONOTONIC}_COARSE. In addition, Linux spells
CLOCK_UPTIME as CLOCK_BOOTTIME.

Add aliases to time.h and document these new aliases in
clock_gettime(2).

Reviewed by:            vangyzen, kib (prior), dchagin (prior)
Sponsored by:           Netflix
Differential Revision:  https://reviews.freebsd.org/D30988

At least in spirit... CLOCK_UPTIME and CLOCK_MONOTONIC are generally treated as aliases today, so this has no practical effecet. POSIX is neutral on the issue.

I'm not sure this is a good idea. "fixes" seems a bit strong... we specifically aligned with Linux here because more software is written for linux and we had a lot of patches in the ports tree to emulate the linux behavior prior to my changes.

[val_packett.cool]

In D39270#894014, @imp wrote:

we specifically aligned with Linux here because more software is written for linux and we had a lot of patches in the ports tree to emulate the linux behavior prior to my changes.

Okay, actually looking at what we're discussing in mozilla another time with fresh eyes, about the correct cross-platform code…

#  ifdef CLOCK_UPTIME
#    define CLOCK_EXCLUDING_SUSPEND CLOCK_UPTIME
#  else
#    define CLOCK_EXCLUDING_SUSPEND CLOCK_MONOTONIC
#  endif

#  ifdef CLOCK_BOOTTIME
#    define CLOCK_INCLUDING_SUSPEND CLOCK_BOOTTIME
#  else
#    define CLOCK_INCLUDING_SUSPEND CLOCK_MONOTONIC
#  endif

…so because we define both UPTIME and BOOTTIME it *is* possible after all to go either way with whether MONOTONIC would include or exclude suspend. The only thing is that—still—UPTIME and BOOTTIME cannot point to each other like rG155f15118a77 made it. One cannot be an alias of the other because they have the polar opposite meanings: UPTIME excludes suspend (says OpenBSD and macOS), BOOTTIME includes it (says Linux, and OpenBSD agrees). The choice is purely "what would (the suspend-behavior-undefined) MONOTONIC be".

What's pointing us in the BSD-like "MONOTONIC includes suspend" direction is the fact that we've had UPTIME as the one with a distinct ID for ages (i.e. this is the direction implied by rGf6232df7a4366 !) and BOOTTIME has been added as a #define.

So basically this is the tradeoff:

future behavior we decide on	+	-
MONOTONIC excludes suspend like Linux	suspend-awareness works in code that's only aware of Linux-like BOOTTIME but not OpenBSD-like UPTIME	we introduce a new clock ID, which isn't great for binary *backwards* compat and leaves the UPTIME clock ID as a silly alias one
MONOTONIC includes suspend like OpenBSD	clock IDs stay neat, only-OpenBSD-like suspend-aware code will be suspend-aware	only-Linux-like suspend-aware code won't be suspend-aware

(note the "-like": I'm talking specifically about code that checks for only one of the BOOTTIME and UPTIME defines; anything that e.g. checks __linux__ or isn't written in C/C++ would need patching for suspend-awareness anyway)

(also, the future might not be far away, I'm going to look into how OpenBSD implements the suspend-awareness, it's probably not rocket science and I could implement it in a patch…)

[val_packett.cool]

Oh, what's also pretty neat about the "MONOTONIC includes suspend like OpenBSD" choice is that software doing some security timeouts like sudo does would default to the secure behavior (not prolonging timeouts across suspends) even without doing anything for suspend-awareness / while being just POSIX without any OS-specific code!! :)

[imp]

BOOTTIME was added for Linux compatibility, so it's following Linux convention. Most of the changes to ports for BOOTTIME changed it to follow Linux conventions before my change anyway. So if Linux's BOOTTIME includes suspend time, let's have it be its own thing and not an alias.

UPTIME and MONOTONIC almost certainly have way too much code depending on their current behavior to change.... Perhaps we should have a clock that is some kind of elapsed time since boot, including suspend time, but redefining what we have today to do that is a non-starter given the existing code that depends on the current definitions. Since there's no universal convention, and since POSIX doesn't define it, our best option to include it is for a new type, alas. We likely can't be compatible with everything... At least not with out much careful study of the existing code and some inkling about what will or won't work after a change like that.

[imp]

CLOCK_MONOTONIC is defined as "The identifier for the system-wide monotonic clock, which is defined as a clock whose value cannot be set via clock_settime() and which cannot have backward clock jumps. The maximum possible clock jump is implementation-defined."

POSIX Issue 8 seems to monkey a lot with things. Since at one point it says "CLOCK_MONOTONIC is guaranteed not to jump backwards and must also advance in real time" and similar in places without clearly defining what that means (possibly suggesting that it should have suspend time added). Since we also have "Using CLOCK_MONOTONIC rather than CLOCK_REALTIME means that the timeout is not influenced by the system clock being changed." which suggests it should NOT have suspend time added since the system time jumps after a resume. It is also used for timeouts in many places for timeout parameters to timed wait primitives. I think I need to more carefully study the forthcoming standard, and see if I can raise the issue about any contradictory requirements so we can get clarity on this, possibly, from them. I also think I need to find/read Defect 1346 to see if it can shed any more coherent light than its effects scattered in the standard... ah, just found "All implementations shall support a clock_id of CLOCK_MONOTONIC defined in <time.h>. This clock represents the monotonic clock for the system. For this clock, the value returned by clock_gettime ( ) represents the amount of time (in seconds and nanoseconds) since an unspecified point in the past (for example, system start-up time, or the Epoch). This point does not change after system start-up time." which is more explicit and fairly close to requiring suspend time be counted.... On the whole it looks clear that CLOCK_MONOTONIC likely should separate from CLOCK_UPTIME to conform to issue 8 when it comes out and CLOCK_UPTIME will continue to be the old meaning of CLOCK_MONOTONIC. How that aligns with Linux and other OSes is going to be messy, I fear.

I think the change is a bit premature, since we likely need to agree among a wider audience than this code review what the right course of action is for posix issue 8 compliance, how we communicate this to users and what changes we need to make in upstreams that use CLOCK_MONOTONIC today that really want the proposed CLOCK_UPTIME semantics. We've had this without suspend time for so long I am sure there's some applications that depend on it, but without looking at the issue, we don't know if a change like this is a yawn or a big deal.

So rather starting with code changes, we should start with a discussion on arch@ I think. I don't think that OpenBSD nor MacOS behaviors should trump Linux... but Linux behaviors shouldn't trump POSIX here since from what I'm reading CLOCK_MONOTONIC is fairly robustly defined there and likely is also the right way to define it that violates POLA the least (in the global sense, not in the FreeBSD traditional sense). I think the focus should be focused on POSIX and/or the most desirable behavior if designed from scratch first, Linux second and OpenBSD/MacOS third. I think that will lead to MONOTONIC including suspend time for POSIX compliance, UPTIME not and a need to change from MONOTONIC to UPTIME for those applications where this pedantic difference matters. snmpd in base should be examined because it publishes either MONOTONIC or UPTIME as a way to catch system crashes and the suspend time may pop up there in people's monitoring scripts... Otherwise for 'normal' things, including suspend time is likely the right answer though some 'non-normal' things that are trying to be nice that are nice today may create a thundering heard of timeouts on resume...

[val_packett.cool]

In D39270#894574, @imp wrote:

How that aligns with Linux and other OSes is going to be messy, I fear.

Not really…? Again, there's currently no misalignment between OSes on what UPTIME and BOOTTIME mean, and we shouldn't add any: regardless of which way we decide on MONOTONIC, we shouldn't contradict the established convention of

• UPTIME meaning no suspend
• BOOTTIME meaning yes suspend
• and MONOTONIC meaning implementation-defined :)

I think the change is a bit premature, since we likely need to agree among a wider audience than this code review

Well, yeah, sparking the discussion was my goal most of all right now! Can we also discuss getting discussions off of mailing lists already please haha

But the other goal was to disentangle the #defines ASAP to prevent binaries compiled currently from becoming wrong in the future:

• #define CLOCK_BOOTTIME CLOCK_MONOTONIC will be fully correct if we do end up with a suspend-including MONOTONIC, and otherwise would be a fallback to non-awareness.
• #define CLOCK_BOOTTIME CLOCK_UPTIME is definitionally wrong by the aforementioned convention that holds across all three mentioned OSes (modulo some *not having* some definitions, but there are no contradictions) so it would become incorrect no matter what (unless we intentionally break the convention which, like, we shouldn't).

[val_packett.cool]

Found some interesting discussion from almost a decade ago: https://lists.freebsd.org/pipermail/freebsd-arch/2013-August/014707.html confirming that MONOTONIC should include suspended time:

The monotonic clock has always been broken across suspend/resume in FreeBSD

About POSIX:

Note that the "unspecified point in the past" "does not change after system start-up [bogus word time]".

[mohd.akram_outlook.com]

What happened with this change? The current situation is really confusing. Linux is wrong IMO about MONOTONIC not increasing during sleep time. They tried to fix it but then reverted it. Regardless, CLOCK_BOOTTIME cannot be relied on in FreeBSD if it does something different than on Linux. It needs to do the same thing.

[imp]

I'm convinced this is good. I'll push it in.

[olce]

In D39270#1035983, @imp wrote:

I'm convinced this is good. I'll push it in.

After having read all the discussion here, I agree too.

In D39270#894574, @imp wrote:

Since we also have "Using CLOCK_MONOTONIC rather than CLOCK_REALTIME means that the timeout is not influenced by the system clock being changed." which suggests it should NOT have suspend time added since the system time jumps after a resume.

I think it's the opposite here, this suggests that CLOCK_MONOTONIC rather *should* include suspend time. With that in mind, in your review above, everything now hints at including suspend time in CLOCK_MONOTONIC (and arguably your last point shows it's actually a requirement).

[imp]

This breaks wpa

diff --git a/contrib/wpa/src/utils/os_unix.c b/contrib/wpa/src/utils/os_unix.c
index 315c973f3228..1a0cefbbb188 100644
--- a/contrib/wpa/src/utils/os_unix.c
+++ b/contrib/wpa/src/utils/os_unix.c
@@ -97,12 +97,12 @@ int os_get_reltime(struct os_reltime *t)
                        return 0;
                }
                switch (clock_id) {
-#ifdef CLOCK_BOOTTIME
+#if (defined(CLOCK_BOOTTIME) && defined(CLOCK_MONOTONIC)) && (CLOCK_MONOTONIC != CLOCK_BOOTTIME)
                case CLOCK_BOOTTIME:
                        clock_id = CLOCK_MONOTONIC;
                        break;
 #endif
-#ifdef CLOCK_MONOTONIC
+#if defined(CLOCK_MONOTONIC) && (!defined(CLOCK_BOOTTIME) || CLOCK_MONOTONIC != CLOCK_BOOTTIME)
                case CLOCK_MONOTONIC:
                        clock_id = CLOCK_REALTIME;
                        break;

[imp]

https://reviews.freebsd.org/D45418

[imp]

FreeBSD version bump?

[imp]

I'll bump as a separate commit.

[olce]

In D39270#1036114, @imp wrote:

FreeBSD version bump?

Not too experienced with that personally, and learning here. I've seen bumps done when there are things to be recompiled, which is the case here, so I guess it's the prudent approach.