hwclock: hctosys drift compensation

Allowing hctosys to drift compensate facilitates:

More precise setting of the System Clock early in the boot process when --adjust cannot be used because the file system is not writeable.

Applies sub second drift corrections immediately, where as --adjust cannot.

Reduces boot time by not calling hwclock multiple times, e.g., --hctosys early before fsck when the file system is read-only, then --adjust later when the file system is read-write and --hctosys again for drift correction.

Use of --adjust elsewhere may no longer be necessary.

---
 sys-utils/hwclock.c | 99 +++++++++++++++++++++++++++++------------------------
 1 file changed, 55 insertions(+), 44 deletions(-)

index 474e04f..ef7bd89 100644
@@ -437,6 +437,45 @@ mktime_tz(struct tm tm, const bool universal,
 }
 
 /*
+ * Do the drift adjustment calculation.
+ *
+ * The way we have to set the clock, we need the adjustment in two parts:
+ *
+ *	1) an integer number of seconds (return as *adjustment_p)
+ *	2) a positive fraction of a second (less than 1) (return as *retro_p)
+ *
+ * The sum of these two values is the adjustment needed. Positive means to
+ * advance the clock or insert seconds. Negative means to retard the clock
+ * or remove seconds.
+ */
+static void
+calculate_adjustment(const double factor,
+		     const time_t last_time,
+		     const double not_adjusted,
+		     const time_t systime, int *adjustment_p, double *retro_p)
+{
+	double exact_adjustment;
+
+	exact_adjustment =
+	    ((double)(systime - last_time)) * factor / (24 * 60 * 60)
+	    + not_adjusted;
+	*adjustment_p = FLOOR(exact_adjustment);
+
+	*retro_p = exact_adjustment - (double)*adjustment_p;
+	if (debug) {
+		printf(P_("Time since last adjustment is %d second\n",
+			"Time since last adjustment is %d seconds\n",
+		       (int)(systime - last_time)),
+		       (int)(systime - last_time));
+		printf(P_("Need to insert %d second and refer time back "
+			 "%.6f seconds ago\n",
+			 "Need to insert %d seconds and refer time back "
+			 "%.6f seconds ago\n", *adjustment_p),
+			 *adjustment_p, *retro_p);
+	}
+}
+
+/*
  * Read the hardware clock and return the current time via <tm> argument.
  *
  * Use the method indicated by <method> argument to access the hardware

@@ -793,7 +832,7 @@ static int interpret_date_string(const char *date_opt, time_t * const time_p)
 }
 
 /*
- * Set the System Clock to time 'newtime'.
+ * Set the System Clock to time 'newtime' plus any drift correction.
  *
  * Also set the kernel time zone value to the value indicated by the TZ
  * environment variable and/or /usr/lib/zoneinfo/, interpreted as tzset()

@@ -807,8 +846,8 @@ static int interpret_date_string(const char *date_opt, time_t * const time_p)
  * have.
  */
 static int
-set_system_clock(const bool hclock_valid, const time_t newtime,
-		 const bool testing)
+set_system_clock(struct adjtime *adjtime_p, const bool hclock_valid,
+		 const time_t newtime, const bool testing)
 {
 	int retcode;
 

@@ -834,6 +873,16 @@ set_system_clock(const bool hclock_valid, const time_t newtime,
 		if (broken->tm_isdst)
 			minuteswest -= 60;
 #endif
+		int adjustment;
+		/* Number of seconds the Hardware Clock has drifted. */
+		double retro;
+		/* Fraction of second the Hardware Clock has drifted. */
+		calculate_adjustment(adjtime_p->drift_factor,
+				     adjtime_p->last_adj_time,
+				     adjtime_p->not_adjusted,
+				     newtime, &adjustment, &retro);
+		tv.tv_sec += adjustment;
+		tv.tv_usec += retro * 1E6;
 
 		if (debug) {
 			printf(_("Calling settimeofday:\n"));

@@ -1072,45 +1121,6 @@ adjust_drift_factor(struct adjtime *adjtime_p,
 }
 
 /*
- * Do the drift adjustment calculation.
- *
- * The way we have to set the clock, we need the adjustment in two parts:
- *
- *	1) an integer number of seconds (return as *adjustment_p)
- *	2) a positive fraction of a second (less than 1) (return as *retro_p)
- *
- * The sum of these two values is the adjustment needed. Positive means to
- * advance the clock or insert seconds. Negative means to retard the clock
- * or remove seconds.
- */
-static void
-calculate_adjustment(const double factor,
-		     const time_t last_time,
-		     const double not_adjusted,
-		     const time_t systime, int *adjustment_p, double *retro_p)
-{
-	double exact_adjustment;
-
-	exact_adjustment =
-	    ((double)(systime - last_time)) * factor / (24 * 60 * 60)
-	    + not_adjusted;
-	*adjustment_p = FLOOR(exact_adjustment);
-
-	*retro_p = exact_adjustment - (double)*adjustment_p;
-	if (debug) {
-		printf(P_("Time since last adjustment is %d second\n",
-			"Time since last adjustment is %d seconds\n",
-		       (int)(systime - last_time)),
-		       (int)(systime - last_time));
-		printf(P_("Need to insert %d second and refer time back "
-			 "%.6f seconds ago\n",
-			 "Need to insert %d seconds and refer time back "
-			 "%.6f seconds ago\n", *adjustment_p),
-			 *adjustment_p, *retro_p);
-	}
-}
-
-/*
  * Write the contents of the <adjtime> structure to its disk file.
  *
  * But if the contents are clean (unchanged since read from disk), don't

@@ -1313,7 +1323,8 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
 	}
 
 	if (!noadjfile
-	    && (adjust || set || systohc || (!utc && !local_opt) || predict)) {
+	    && (adjust || set || systohc ||
+	        hctosys || (!utc && !local_opt) || predict)) {
 		rc = read_adjtime(&adjtime);
 		if (rc)
 			return rc;

@@ -1393,7 +1404,7 @@ manipulate_clock(const bool show, const bool adjust, const bool noadjfile,
 					    hclock_valid, hclocktime, (double)
 					    read_time.tv_usec / 1E6);
 	} else if (hctosys) {
-		rc = set_system_clock(hclock_valid, hclocktime, testing);
+		rc = set_system_clock(&adjtime, hclock_valid, hclocktime, testing);
 		if (rc) {
 			printf(_("Unable to set system clock.\n"));
 			return rc;

Update hwclock man page for the
hwclock: hctosys drift compensation patch.

---
 sys-utils/hwclock.8.in | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

index b11b45c..b42a201 100644
@@ -62,7 +62,9 @@ option.
 Showing the Hardware Clock time is the default when no function is specified.
 .TP
 .BR \-s , \ \-\-hctosys
-Set the System Time from the Hardware Clock.
+Set the System Time from the Hardware Clock.  The time read from the Hardware
+Clock is compensated to account for systematic drift before using it to set the
+System Clock.  See the discussion below, under \fBThe Adjust Function\fR.
 .PP
 Also set the kernel's timezone value to the local timezone
 as indicated by the TZ environment variable and/or

@@ -484,8 +486,9 @@ The Hardware Clock is usually not very accurate.  However, much of its
 inaccuracy is completely predictable - it gains or loses the same amount
 of time every day.  This is called systematic drift.
 .BR hwclock 's
-"adjust" function lets you make systematic corrections to correct the
-systematic drift.
+.I \-\-adjust
+function lets you apply systematic drift corrections to the
+Hardware Clock.
 .PP
 It works like this:
 .B hwclock

@@ -529,20 +532,22 @@ since the last calibration, how long it has been since the last
 adjustment, what drift rate was assumed in any intervening
 adjustments, and the amount by which the clock is presently off.
 .PP
-A small amount of error creeps in any time
-.B hwclock
-sets the clock, so it refrains from making an adjustment that would be
-less than 1 second.  Later on, when you request an adjustment again,
-the accumulated drift will be more than a second and
-.B hwclock
-will do the adjustment then.
-.PP
-It is good to do a
-.I hwclock \-\-adjust
-just before the
-.I hwclock \-\-hctosys
-at system startup time, and maybe periodically while the system is
-running via cron.
+A small amount of error creeps in when
+the Hardware Clock is set, so
+.I \-\-adjust
+refrains from making any adjustment that is less
+than 1 second.  Later on, when you request an adjustment again, the accumulated
+drift will be more than 1 second and
+.I \-\-adjust
+will make the adjustment including any fractional amount.
+.PP
+.IR "hwclock \-\-hctosys"
+also uses the adjtime file data to compensate the value read from the Hardware
+Clock before using it to set the System Time.  It does not share the 1 second
+limitation of --adjust, and will correct sub-second drift values immediately.
+It does not change the Hardware Clock time or the adjtime file.  This may
+eliminate the need to use --adjust, unless something else on the system needs
+the Hardware Clock to be compensated.
 .PP
 The adjtime file, while named for its historical purpose of controlling
 adjustments only, actually contains other information for use by hwclock

Hello,

So, also on my hwclock todo list is drift correcting --show. I believe that doing them together will yield a more eloquent patch, and streamline the code in general. Therefore, please disregard this patch for the time being. I apologize for submitting it prematurely.

Of course, comments on the concepts or the code are welcome.

I read in the howto-contribute.txt file to announce when extended work is being done. As already mentioned, I have a hwclock todo list in progress.

Thank you,
Will

OK, the idea to read adjtime file when we apply HW clock to sys time makes sense.

Something else:

See Documentation/TODO, the writable /etc/adjtime sucks, because in many cases we want to keep /etc read-only. I see two possible ways:

1. move the file to /var/lib/hwclock/adjtime

This is already possible by "./configure ADJTIME_PATH=/var/lib/hwclock/adjtime" (used for example by Linux-from-scratch project).

-- the problem is that the file (specially last UTC/LOCAL line)
may be expected by another tools

2. create another independent /var/lib/hwclock/drift file with info about HW clock drift numbers and keep only zeros and and UTC/LOCAL setting in /etc/adjtime.

This could be implemented backwardly compatible, if there is no /var/lib/hwclock then always use /etc/adjtime for drift numbers.

Karel

 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com

Hello Karel,

Yes, I saw that and have it on my todo list. However, it is down my list a ways. I had not planned on attempting to integrate it into the hctosys/show patch.

True, init scripts depend upon that information too. So it is an important consideration.

My initial thought when I read u-l's todo list was that adjtimex [ -c | -a ] depend upon /etc/adjtime *drift* data. Do we care about breaking that?

BJH's hwclock uses /var/state/adjtime, that seems like a good choice as well. ?

I have often thought that hwclock should have a separate 'configuration' file, for example, to allow specific init behavior customization without requiring system admins to modify the init scripts.

Will

/var/state is not in the Filesystem Hierarchy Standard.

"Applications must generally not add directories to the top level of /var. Such directories should only be added if they have some system-wide implication, and in consultation with the FHS mailing list."

-- Bruce

I see that FHS specifies the path that Karel wrote, fair enough.

I'll be researching all the ramifications further before attempting to write a patch for this todo item.

Thank you for the feedback Bruce.

Will

I'm not sure, but I guess adjtimex reads /etc/adjtime to get info about UTC/LOCAL only. Well, it seems we need to check adjtimex code.

Anyway, if adjtimex depends on drift data then we can change it too to use /var.

Note that we have hwclock --compare to provide adjtimex -c functionality with in hwclock.

The old RHEL/Fedora has /etc/sysconfig/hwclock where is possible to specify hwclock command line, so no one is forced to modify init scripts. All you need is to read the file from your init scripts.

BTW, slowly growing number of systemd based distributions where hwclock --hctosys and --adjust is no more used and we usually assume that NTP + kernel is enough to update CMOS. From this point of view hwclock(8) is more about very basic low-level HW clock manipulation than about systime and hwtime relationship :-)

(but yes, we (upstream) still support the original hwclock(8) use-cases
and non-systemd installations)

Karel

 Karel Zak  <[email protected]>
 http://karelzak.blogspot.com

man 8 adjtimex
-c[count], --compare[=count]
... CMOS clock readings are adjusted
for systematic drift using using the correction in /etc/adjtime -- see hwclock(8)...

I have not had a chance to examine it closely yet, but it appears to be quite rudimentary at this point when compared to adjtimex -c. I'm not sure I understand the purpose of trying to build adjtimex into hwclock anyway. Why not just use adjtimex to do adjtimex things?

But that is distro specific. Rather than having everyone baking their own solution it seems more practical for hwclock to have its own config file. This todo item seems like a good opportunity to do so.

I'm glad you included this comment. I wouldn't wasting my time trying to improve hwclock otherwise. I have no interest in dragging the systemd war into this conversation, but my distro of choice is pushing back against it.

Also with IoT putting Linux into everything from automobiles to light bulbs, and some of them interested in keeping good date-time, I think hwclock's init capabilities could remain relevant for some time to come. Linux is about more then just distros, yes?

Will