CLOCK_ADJUST_REALTIME

[kib]

Summary

Steps realtime clock by specified delta.

Requested by: cy

Test Plan

Not tested.

[imp]

sys/kern/kern_time.c

490

Why an hour?

501

Why this value? I know it is 8000 years, but why? It was in the old code, but deserves at least a comment

[phk]

sys/sys/time.h

483

Have you checked if Linux has already used 16 ?

No need to make a collision if we can avoid it...

[kib]

sys/kern/kern_time.c

490

My feel was that adjustments ought to be some not too large values. This is how I defined it when writing the code.

If you have another proposal, I am open for suggestions.

501

I do not have an idea about the comment, except that this value 'was there for minor sanity check'. If you can provide text, please commit after me, or I can add it there.

sys/sys/time.h

483

How would numeric value collisions there affect anything ? You cannot run Linux ABI code inside FreeBSD ABI process, at least without translating syscalls.

Anyway, Linux seems to define the following:

/*
 * The IDs of the various system clocks (for POSIX.1b interval timers):
 */
#define CLOCK_REALTIME                  0
#define CLOCK_MONOTONIC                 1
#define CLOCK_PROCESS_CPUTIME_ID        2
#define CLOCK_THREAD_CPUTIME_ID         3
#define CLOCK_MONOTONIC_RAW             4
#define CLOCK_REALTIME_COARSE           5
#define CLOCK_MONOTONIC_COARSE          6
#define CLOCK_BOOTTIME                  7
#define CLOCK_REALTIME_ALARM            8
#define CLOCK_BOOTTIME_ALARM            9
/*
 * The driver implementing this got removed. The clock ID is kept as a
 * place holder. Do not reuse!
 */
#define CLOCK_SGI_CYCLE                 10
#define CLOCK_TAI                       11

#define MAX_CLOCKS                      16

i.e. we already diversed too much to care about one more value.

[imp]

sys/kern/kern_time.c

490

Limiting is fine. It's the magic hard coded value I'm having heartburn with. At the very least it should be #define with a comment saying that an hour was chosen arbitrarily as a bound that seemed sane. I've had all kinds of issues with magic, hard coded, hard to find limits with NTP over the years and would like this to be easier to find / change should it prove to be a bit too arbitrary and people don't want to open it up to any value.

So my proposal is to use a #define and add a comment saying it's arbitrary, or giving the reason for that choice vs others if there's a more nuanced articulation of the limit I've missed somewhere in the long email chain.

501

Yea, I was hoping more that phk or Ian would say something so we can justify the cap and explain why 8000 is any better than 7000 or 3000 or whatever. I have a very dim memory of this matching some sanity code put into libc to keep the date < 10,000 AD. It would be nice to spell that out a bit if phk or Ian can recall why better than I. If not, it's fine as it is and I can update things if I find the actual answer.

[cy]

nwtime.org need it to return the current time, i.e. clock_gettime() output atomically to avoid the expense of making another system call. Adding a clock_id satisfies one of the two requirements. I suppose current time could be returned in *ts upon return.

[cy]

sys/kern/kern_tc.c

1260

Correct me if I'm wrong, it appears all callers to tc_setclock() set bool abs to true. I see only two callers of tc_setclock(). One in kern/kern_time.c and the other in kern/subr_rtc.c, both of which call tc_setclock() with abs = true.

[kib]

In D27571#616178, @cy wrote:

nwtime.org need it to return the current time, i.e. clock_gettime() output atomically to avoid the expense of making another system call. Adding a clock_id satisfies one of the two requirements. I suppose current time could be returned in *ts upon return.

Ok I did this hack, the time fetched right after tc_windup(), under the same spinlock region, is copyied out to *tp. One wrinkle is that clock_settime() takes const struct timespec *, not sure how wrong it feels.

sys/kern/kern_tc.c

1260

Look at steptime() call from kern_clock_settime(case CLOCK_ADJUST_REALTIME).

[kib]

Make limit for adjust a define.
Fetch and copy out time after adjustment.

[cy]

I expect Harlan Stenn and Juergen Perlinger to be satisfied with this. I'll check in with them and cc you.

sys/kern/kern_tc.c

1260

Thank you.

[imp]

Thanks kib! I think I'm happy with this now. I think cy is right, but I'll let him chase down the ntp folks.

[cy]

I started a new thread with the participants of the original email thread and added kib@ to it. Waiting for their replies.

[kib]

Add clock_settime_adjust(3).

It is a trick to hide constness of the struct timespec * argument of clock_settime(2). It is implemented as a trivial wrapper over clock_settime(CLOCK_ADJUST_REALTIME).

[cy]

Looks good.

[kib]

Extract the limit checks into steplimit() and apply them in the relative case as well.
Add locking to steplimit(), otherwise parallel calls have undefined consequences.

[cy]

Looks good. But I suggest holding off committing this until Juergen or Harlan sign off on this. They're the people who asked for it. They're trying to work around issues where VMware VMs are starved of resources and ntp needs to make big adjustments to catch up the clock after a VM has not executed for a number of seconds or minutes.

[kib]

CLOCK_ADJUST_REALTIME now returns clamped adjustment instead of abs time.
Make all sanity and clamp limits adjustable.
Some logic errors fixed.

[kib]

Update comments in steplimit() to take knobs into account.