[PATCH 1/2] hwclock: hctosys drift compensation

[PATCH 1/2] hwclock: hctosys drift compensation

[JWP]

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.

Signed-off-by: J William Piggott <elseifthen@gmx.com>
---
 sys-utils/hwclock.c | 99 +++++++++++++++++++++++++++++------------------------
 1 file changed, 55 insertions(+), 44 deletions(-)

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 474e04f..ef7bd89 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -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;

[PATCH 2/2] hwclock: hctosys drift compensation man page

[JWP]

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

Signed-off-by: J William Piggott <elseifthen@gmx.com>
---
 sys-utils/hwclock.8.in | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/sys-utils/hwclock.8.in b/sys-utils/hwclock.8.in
index b11b45c..b42a201 100644
--- a/sys-utils/hwclock.8.in
+++ b/sys-utils/hwclock.8.in
@@ -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

Re: * RECALL * [PATCH 1/2] hwclock: hctosys drift compensation

[JWP]

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

Re: * RECALL * [PATCH 1/2] hwclock: hctosys drift compensation

[Karel Zak]

On Mon, Sep 15, 2014 at 10:03:56AM -0400, JWP wrote:
> 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.

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

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

 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  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: * RECALL * [PATCH 1/2] hwclock: hctosys drift compensation

[JWP]

Hello Karel,

On 09/16/2014 05:35 AM, Karel Zak wrote:  
>  See Documentation/TODO, the writable /etc/adjtime sucks, because in
>  many cases we want to keep /etc read-only. I see two possible ways:

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.

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

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

Re: * RECALL * [PATCH 1/2] hwclock: hctosys drift compensation

[Bruce Dubbs]

JWP wrote:
> Hello Karel,
>
> On 09/16/2014 05:35 AM, Karel Zak wrote:
>>   See Documentation/TODO, the writable /etc/adjtime sucks, because in
>>   many cases we want to keep /etc read-only. I see two possible ways:
>
> 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.
>
>>       -- the problem is that the file (specially last UTC/LOCAL line)
>>          may be expected by another tools
>
> 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. ?

/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

Re: * RECALL * [PATCH 1/2] hwclock: hctosys drift compensation

[JWP]

On 09/16/2014 12:32 PM, Bruce Dubbs wrote:
>
>/var/state is not in the Filesystem Hierarchy Standard.

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

Re: * RECALL * [PATCH 1/2] hwclock: hctosys drift compensation

[Karel Zak]

On Tue, Sep 16, 2014 at 09:08:23AM -0400, JWP wrote:
> Hello Karel,
> 
> On 09/16/2014 05:35 AM, Karel Zak wrote:  
> >  See Documentation/TODO, the writable /etc/adjtime sucks, because in
> >  many cases we want to keep /etc read-only. I see two possible ways:
> 
> 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.
> 
> >      -- the problem is that the file (specially last UTC/LOCAL line)
> >         may be expected by another tools
> 
> 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?

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.

> 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.

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  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: * RECALL * [PATCH 1/2] hwclock: hctosys drift compensation

[elseifthen]

On 09/17/2014 05:55 AM, Karel Zak wrote:

>> 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?
> 
> 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.

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)...

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

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? 
 
>> 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.
> 
> 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.

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.

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

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