Issues with adjtime implementation / Build failure.

[patacongo]

adjtime() is the semi-standard method used in NuttX, Linux, and FreeBSD to gradually adapt the system time so that it is in agreement with RTC time. It differs from clock_synchronize() or clock_resynchronize() in that it is intended to be used during normal runtime without disrupting ongoing timing operations rather than at exceptional times like power up or wake-up from sleep.

adjtime() is implemented in sched/clock/clock_timekeeping. A quick review reveals several problems:

1. Missing system call. The implemented of adjtime() is provided, but there is no call gate implemented in syscall/ and include/sys [NOT a problem. See comment below].

2. Adjustment has limited range. The logic calculates a separate adjustment that is added to the current time (a timespec). The adjustment value is a type long with an LSB of 1 nonosecond. So the total range is only +/-2 seconds (assuming that long is 32-bits). That is an insufficient range for systems that are always on.

This is because adjtime() does not follow the man page: "If the adjustment in delta is positive, then the system clock is speeded up by some small percentage (i.e., by adding a small amount of time to the clock value in each second) until the adjustment has been completed. If the adjustment in delta is negative, then the clock is slowed down in a similar fashion."

If small adjustments were added to a full timespec or if the clock rate is modified instead, overflow would not occur.

3. Notice that the man page quote above does not talk about adjustments to the time, but rather about adjustments to the rate of the clock (speeded up or slowed down. Hmmm.. There is no such word as speeded up, is there? Isn't the past tense of speed sped, not speeded.)

4. No guarantee of monotonically increasing time. The requirement is: "The adjustment that adjtime() makes to the clock is carried out in such a manner that the clock is always monotonically increasing." But I see nothing that guarantees this. If adjtime() were implemented correctly so that the clock rate is adjusted, not the current time, then that would meet this requirement.

If the rate of the clock source is set even just a little too fast, it would be impossible to both adjust the time downward and to also guarantee a strictly monotonically increasing time. The only way to accomplish that would be to adjust the clock rate downward.

Forward time steps should also be small. The corrections are suppose to work gradually over time.

If system time were to "go backward" that could result in threads locking up waiting under certain circumstances.: Calculated time delays would be enormous.

Reference: https://man7.org/linux/man-pages/man3/adjtime.3.html (Linux) and https://man.freebsd.org/cgi/man.cgi?query=adjtime&sektion=2&n=1 (FreeBSD)

6. Build Failure. If I try building a configuration with CONFIG_CLOCK_TIMEKEEPING enabled, the build fails with the error:

   clock/clock_initialize.c:340:35 error 'g_basetime' undeclared (first use in this function): ...

This is because g_basetime is conditionally compiled out when CONFIG_CLOCK_TIMEKEEPING is enabled. However the following commit added references to g_basetime that are not guarded by CONFIG_CLOCK_TIMEKEEPING:

46bd8798c55 sched/clock/clock_initialize.c (Xiang Xiao      2018-11-12 06:50:37 -0600 371)       rtc_diff->tv_sec  = rtc_time.tv_sec  - curr_ts.tv_sec;
46bd8798c55 sched/clock/clock_initialize.c (Xiang Xiao      2018-11-12 06:50:37 -0600 372)       rtc_diff->tv_nsec = rtc_time.tv_nsec - curr_ts.tv_nsec;

commit 46bd8798c551c2fb5f26d57559214c02e8946637
Author: Xiang Xiao <xiaoxiang@xiaomi.com>
Date:   Mon Nov 12 06:50:37 2018 -0600

    sched/clock/:  Remove g_monotonic_basetime and g_clock_monotonic_time since we don't need ensure monotonic time start from zero as state here: http://pubs.opengroup.org/onlinepubs/009696899/functions/clock_getres.html

To replicate this failure:

a. make distclean
b. tools/configure.sh stm3240g-eval:nsh
c. make menuconfig (enable CONFIG_CLOCK_TIMEKEEPING)
d. make

[patacongo]

1. Missing system call. The implemented of adjtime() is provided, but there is no call gate implemented in syscall/ and include/sys

Nevermind. I am mistaken. system call support for adjtime() is indeed in place. I must have had a typo in the command line when I searched for adjtime in syscall/

[cederom]

Here are some references :-)

https://github.com/freebsd/freebsd-src/blob/master/contrib/ntp/libntp/adjtime.c

https://github.com/freebsd/freebsd-src/blob/master/contrib/ntp/include/ntp_syscall.h

https://github.com/freebsd/freebsd-src/blob/main/sys/kern/kern_time.c

And the man page :-)

https://man.freebsd.org/cgi/man.cgi?query=adjtime&sektion=2&apropos=0&manpath=FreeBSD+13.1-RELEASE+and+Ports

 The adjtime() system call makes small adjustments to the system time, as
 returned by gettimeofday(2), advancing or retarding it by the time    speci-
 fied by the timeval delta.     If delta is negative, the clock is slowed
 down by incrementing it more slowly than normal until the correction is
 complete.    If delta is positive, a    larger increment than normal is    used.
 The skew used to perform the correction is    generally a fraction of    one
 percent.  Thus, the time is always    a monotonically    increasing function.
 A time correction from an earlier call to adjtime() may not be finished
 when adjtime() is called again.  If olddelta is not a null    pointer, the
 structure pointed to will contain,    upon return, the number    of microsec-
 onds still    to be corrected    from the earlier call.

 This call may be used by time servers that    synchronize the    clocks of com-
 puters in a local area network.  Such time    servers    would slow down    the
 clocks of some machines and speed up the clocks of    others to bring    them
 to    the average network time.

 The adjtime() system call is restricted to    the super-user.

Hope that helps :-)

[michallenc]

Thank you Greg @patacongo for detailed issue description. I agree clock rate adjustment would be better solution and would solve possible problems with time jumps. This is also how time synchronization is handled in RTEMS implementation of adjtime() function.

I will take a look into possible implementation to NuttX mainline.

[patacongo]

I will take a look into possible implementation to NuttX mainline.

There are many implementations of algorithms out there, so fixing the NuttX adjtime should not be too difficult (although it doesn't even compile now if you enable TIMEKEEPING).

There are many that are much simpler than Linux or FreeBSD because they are self-contained. Just Google for "adjtime site:github.com ". I have looked at this one: https://github.com/aosm/ntp/blob/master/libntp/adjtime.c

It will require special architecture-specific hooks to modify the clock rate, but these can be conditioned on CONFIG_HAVE_CLOCK_TIMEKEEPING as the logic is now, so it does not require modification of all architectures. There is already ARCH_HAVE_TIMEKEEPING support for several MCUs including some STM32's, PIC32, RISC-V, ESP32:

$ grep -r TIMEKEEPING arch

I think you could just remove the existing TIMEKEEPING support for those MCUs provided that you also remove the setting of CONFIG_ARCH_HAVE_TIMEKEEPING.

Currently CONFIG_ARCH_HAVE_TIMEKEEPING is set only in arch/arm/Kconfig and only for STM32, STM32F7, and STM32H7. So certainly TIMEKEEPING could never have been used in the other architectures: That has to be untested, unused, dead code that was just brought along with the port from STM32. That includes: STM32WB, PIC32MZ, RISC-V ESP32C3, and Extensa ESP32.

Those platforms the select CONFIG_ARCH_HAVE_TIMEKEEP must provide the following architecture-specific interfaces as prototyped in include/nuttx/arch.h:

#if defined(CONFIG_SCHED_TICKLESS_TICK_ARGUMENT) || defined(CONFIG_CLOCK_TIMEKEEPING)
int up_timer_gettick(FAR clock_t *ticks);
#endif

#ifdef CONFIG_CLOCK_TIMEKEEPING
void up_timer_getmask(FAR clock_t *mask);
#endif

That doesn't feel right: It doesn't address clock rate adjustments and looks like it depends on some specifics of a particular clock implementation. That simpler libntp version that I reference above supports many different architecture hooks. For QNX, It requires these two interface:

/*
 * Get the current clock period (nanoseconds)
 */
if (ClockPeriod (CLOCK_REALTIME, 0, &period, 0) < 0)
    return -1;

and

if (ClockAdjust (CLOCK_REALTIME, &adj, &oldadj) < 0)
    return -1;

[michallenc]

Hi,

I've finished the first take of possible adjtime implementation. The commit can be found here. There are few things I would like to discuss before going into PR.

1. Licensing issue: While major part of the implementation is my own and therefore can be released under Apache License, the idea of the algorithm itself is taken from libntp @patacongo refered to. I am not sure about licensing in this case. Do we need the original author agreement?
2. I implemented adjtime function separately from TIMEKEEPING support as that seems to be implementing not only adjtime but also some other stuffs. I think it is ok to keep it separate, what do you think?

Other than that, the adjustment seems to be working fine based on the tests I made.

[patacongo]

• Licensing issue: While major part of the implementation is my own and therefore can be released under Apache License, the idea of the algorithm itself is taken from libntp @patacongo refered to. I am not sure about licensing in this case. Do we need the original author agreement?

@michallenc I haven't looked at your implementation, but if you ONLY took the algorithm and the implementation is wholly yours, then there is no copyright issues.

Think of it this way: The copyright protects the implementation. It protects the actual code. If you re-use code, or even copy-paste sections of the code, you may be violating a copyright. If you look at the code to understand the algorithmic content, then write you own code that implements that algorithm, you have not violated any copyrights. People do this all of the time. That is why we can have work alike versions of some things. This is the whole basis of IBM's clean room design that has led to thousands of lines of opened code. https://en.wikipedia.org/wiki/Clean_room_design

There can be an issue with using if the underlying algorithm is patented, however. Unlike copyrights, patents can protect the content of the code. Think for examples of the problems that people had with writing their own open source MPEG4/5 algorithms. MPEG4/5 is patented so although there is no copyright violation from distributing the code under any any source license, there is a patent violation if you use the code in a product without permission. I doubt that applies here; the adjtime() algorithm is not that complex and probably not patentable.

• I implemented adjtime function separately from TIMEKEEPING support as that seems to be implementing not only adjtime but also some other stuffs. I think it is ok to keep it separate, what do you think?

adjtime() is a standard application interface. It is not POSIX but still a part of standards that NuttX follows. I think that adjtime() should be available without being conditioned on a configuration variable. That should not result in any size increase since adjtim() is a leaf function in a library; in the FLAT build it will not be drawn in unless it is specifically referenced. (It will get pulled into PROTECTED and KERNEL builds because the system call logic will reference it.

adjtime() does depend on the architecture-specific code providing interfaces to adjust the clock rate, so perhaps it would need to depend on CONFIG_ARCH_HAVE_TIMEKEEPING

[michallenc]

@patacongo Thanks for the license clarification.

adjtime() does depend on the architecture-specific code providing interfaces to adjust the clock rate, so perhaps it would need to depend on CONFIG_ARCH_HAVE_TIMEKEEPING

That is the current state. I just changed it that adjtime() would depend on CONFIG_ARCH_HAVE_ADJTIME and CONFIG_CLOCK_ADJTIME. Mostly because current timekeeping implementation does not use traditional basetime structure

#ifndef CONFIG_CLOCK_TIMEKEEPING
struct timespec   g_basetime;
#endif

As can be seen here https://github.com/apache/nuttx/blob/master/sched/clock/clock_initialize.c#L58. Therefore there is a different approach for settime and gettime. I think maybe we should keep the implementation of adjtime (under CONFIG_CLOCK_ADJTIME) and other timekeeping function (wall time etc. under CONFIG_CLOCK_TIMEKEEPING) separately. This way it will be possible to use adjtime just for clock period adjustment when required by the user.