From e147f2f11e1bc208db9053fa1df05d9b4857a545 Mon Sep 17 00:00:00 2001
From: Miroslav Lichvar <mlichvar@redhat.com>
Date: Thu, 22 May 2014 16:28:20 +0200
Subject: [PATCH] sys: drop frequency scaling in Linux driver

Since the kernel USER_HZ constant was introduced and the internal HZ
can't be reliably detected in user-space, the frequency scaling constant
used with older kernels is just a random guess.

Remove the scaling completely and let the closed loop compensate for the
error. To prevent thrashing between two states when the system's
frequency error is close to a multiple of USER_HZ, stick to the current
tick value if it's next to the new required tick. This is used only on
archs where USER_HZ is 100 as the frequency adjustment is limited to 500
ppm.

The linux_hz and linux_freq_scale directives are no longer supported,
but allowed by the config parser.
---
 chrony.texi.in |  34 ------------
 conf.c         |  32 +----------
 conf.h         |   2 -
 sys_linux.c    | 141 +++++++++++++++----------------------------------
 4 files changed, 45 insertions(+), 164 deletions(-)

diff --git a/chrony.texi.in b/chrony.texi.in
index f1e938d..6503ca8 100644
--- a/chrony.texi.in
+++ b/chrony.texi.in
@@ -1141,8 +1141,6 @@ the configuration file is ignored.
 * initstepslew directive::      Trim the system clock on boot-up
 * keyfile directive::           Specify location of file containing keys
 * leapsectz directive::         Read leap second data from tz database
-* linux_freq_scale directive::  Define a non-standard value to compensate the kernel frequency bias
-* linux_hz directive::          Define a non-standard value of the kernel USER_HZ constant
 * local directive::             Allow unsynchronised machine to act as server
 * lock_all directive::          Require that chronyd be locked into RAM
 * log directive::               Make daemon log certain sets of information
@@ -1818,38 +1816,6 @@ $ TZ=right/UTC date -d 'Dec 31 2008 23:59:60'
 Wed Dec 31 23:59:60 UTC 2008
 @end example
 
-@c }}}
-@c {{{ linux_freq_scale
-@node linux_freq_scale directive
-@subsection linux_freq_scale
-(This option only applies to Linux).
-
-This option sets a scale factor needed to control the frequency of the clock by
-the @code{adjtimex()} system call exactly.  By default, the value is determined
-by the version of the running kernel.  In recent kernels it is always 1.0 (i.e.
-no scaling is needed).
-
-An example of the command is
-
-@example
-linux_freq_scale 0.99902439
-@end example
-@c }}}
-@c {{{ linux_hz
-@node linux_hz directive
-@subsection linux_hz
-(This option only applies to Linux).
-
-This option defines the value of the kernel @code{USER_HZ} constant, which is
-needed to use the @code{adjtimex()} system call correctly.  By default, its
-value is determined from the running kernel automatically and there should
-rarely be a need to use this option.
-
-An example of the command is
-
-@example
-linux_hz 100
-@end example
 @c }}}
 @c {{{ local
 @node local directive
diff --git a/conf.c b/conf.c
index 26fd605..50ed760 100644
--- a/conf.c
+++ b/conf.c
@@ -187,16 +187,6 @@ static char *tempcomp_file = NULL;
 static double tempcomp_interval;
 static double tempcomp_T0, tempcomp_k0, tempcomp_k1, tempcomp_k2;
 
-/* Boolean for whether the Linux HZ value has been overridden, and the
- * new value. */
-static int set_linux_hz = 0;
-static int linux_hz;
-
-/* Boolean for whether the Linux frequency scaling value (i.e. the one that's
- * approx (1<<SHIFT_HZ)/HZ) has been overridden, and the new value. */
-static int set_linux_freq_scale = 0;
-static double linux_freq_scale;
-
 static int sched_priority = 0;
 static int lock_memory = 0;
 
@@ -396,9 +386,9 @@ CNF_ParseLine(const char *filename, int number, char *line)
   } else if (!strcasecmp(command, "leapsectz")) {
     parse_string(p, &leapsec_tz);
   } else if (!strcasecmp(command, "linux_freq_scale")) {
-    set_linux_freq_scale = parse_double(p, &linux_freq_scale);
+    LOG(LOGS_WARN, LOGF_Configure, "%s directive is no longer supported", command);
   } else if (!strcasecmp(command, "linux_hz")) {
-    set_linux_hz = parse_int(p, &linux_hz);
+    LOG(LOGS_WARN, LOGF_Configure, "%s directive is no longer supported", command);
   } else if (!strcasecmp(command, "local")) {
     parse_local(p);
   } else if (!strcasecmp(command, "lock_all")) {
@@ -1616,24 +1606,6 @@ CNF_GetLeapSecTimezone(void)
 
 /* ================================================== */
 
-void
-CNF_GetLinuxHz(int *set, int *hz)
-{
-  *set = set_linux_hz;
-  *hz = linux_hz;
-}
-
-/* ================================================== */
-
-void
-CNF_GetLinuxFreqScale(int *set, double *freq_scale)
-{
-  *set = set_linux_freq_scale;
-  *freq_scale = linux_freq_scale ;
-}
-
-/* ================================================== */
-
 int
 CNF_GetSchedPriority(void)
 {
diff --git a/conf.h b/conf.h
index 7b98bcf..049991e 100644
--- a/conf.h
+++ b/conf.h
@@ -74,8 +74,6 @@ extern void CNF_GetBindAcquisitionAddress(int family, IPAddr *addr);
 extern void CNF_GetBindCommandAddress(int family, IPAddr *addr);
 extern char *CNF_GetPidFile(void);
 extern char *CNF_GetLeapSecTimezone(void);
-extern void CNF_GetLinuxHz(int *set, int *hz);
-extern void CNF_GetLinuxFreqScale(int *set, double *freq_scale);
 
 /* Value returned in ppm, as read from file */
 extern double CNF_GetMaxUpdateSkew(void);
diff --git a/sys_linux.c b/sys_linux.c
index b1041ff..3c0f042 100644
--- a/sys_linux.c
+++ b/sys_linux.c
@@ -61,25 +61,14 @@ int LockAll = 0;
 /* This is the uncompensated system tick value */
 static int nominal_tick;
 
+/* Current tick value */
+static int current_delta_tick;
+
 /* The maximum amount by which 'tick' can be biased away from 'nominal_tick'
    (sys_adjtimex() in the kernel bounds this to 10%) */
 static int max_tick_bias;
 
-/* This is the scaling required to go between absolute ppm and the
-   scaled ppm used as an argument to adjtimex.  Because chronyd is to an extent
-   'closed loop' maybe it doesn't matter if this is wrongly determined, UNLESS
-   the system's ppm error is close to a multiple of HZ, in which case the
-   relationship between changing the frequency and changing the value of 'tick'
-   will be wrong.  This would (I imagine) cause the system to thrash between
-   two states.
-   
-   However..., if this effect was not corrected, and the system is left offline
-   for a long period, a substantial error would build up.  e.g. with HZ==100,
-   the correction required is 128/128.125, giving a drift of about 84 seconds
-   per day). */
-static double freq_scale;
-
-/* The kernel HZ constant (USER_HZ in recent kernels). */
+/* The kernel USER_HZ constant */
 static int hz;
 static double dhz; /* And dbl prec version of same for arithmetic */
 
@@ -117,29 +106,44 @@ apply_step_offset(double offset)
 /* ================================================== */
 /* This call sets the Linux kernel frequency to a given value in parts
    per million relative to the nominal running frequency.  Nominal is taken to
-   be tick=10000, freq=0 (for a HZ==100 system, other values otherwise).  The
-   convention is that this is called with a positive argument if the local
+   be tick=10000, freq=0 (for a USER_HZ==100 system, other values otherwise).
+   The convention is that this is called with a positive argument if the local
    clock runs fast when uncompensated.  */
 
 static double
 set_frequency(double freq_ppm)
 {
   long required_tick;
-  double required_freq; /* what we use */
-  double scaled_freq; /* what adjtimex & the kernel use */
+  double required_freq;
   int required_delta_tick;
 
   required_delta_tick = our_round(freq_ppm / dhz);
-  required_freq = -(freq_ppm - dhz * required_delta_tick);
-  required_tick = nominal_tick - required_delta_tick;
-  scaled_freq = freq_scale * required_freq;
 
-  if (TMX_SetFrequency(&scaled_freq, required_tick) < 0) {
-    LOG_FATAL(LOGF_SysLinux, "adjtimex failed for set_frequency, freq_ppm=%10.4e scaled_freq=%10.4e required_tick=%ld",
-        freq_ppm, scaled_freq, required_tick);
+  /* Older kernels (pre-2.6.18) don't apply the frequency offset exactly as
+     set by adjtimex() and a scaling constant (that depends on the internal
+     kernel HZ constant) would be needed to compensate for the error. Because
+     chronyd is closed loop it doesn't matter much if we don't scale the
+     required frequency, but we want to prevent thrashing between two states
+     when the system's frequency error is close to a multiple of USER_HZ.  With
+     USER_HZ <= 250, the maximum frequency adjustment of 500 ppm overlaps at
+     least two ticks and we can stick to the current tick if it's next to the
+     required tick. */
+  if (hz <= 250 && (required_delta_tick + 1 == current_delta_tick ||
+                    required_delta_tick - 1 == current_delta_tick)) {
+    required_delta_tick = current_delta_tick;
   }
 
-  return dhz * (nominal_tick - required_tick) - scaled_freq / freq_scale;
+  required_freq = -(freq_ppm - dhz * required_delta_tick);
+  required_tick = nominal_tick - required_delta_tick;
+
+  if (TMX_SetFrequency(&required_freq, required_tick) < 0) {
+    LOG_FATAL(LOGF_SysLinux, "adjtimex failed for set_frequency, freq_ppm=%10.4e required_freq=%10.4e required_tick=%ld",
+        freq_ppm, required_freq, required_tick);
+  }
+
+  current_delta_tick = required_delta_tick;
+
+  return dhz * current_delta_tick - required_freq;
 }
 
 /* ================================================== */
@@ -148,24 +152,16 @@ set_frequency(double freq_ppm)
 static double
 read_frequency(void)
 {
-  double tick_term;
-  double unscaled_freq;
-  double freq_term;
   long tick;
+  double freq;
 
-  if (TMX_GetFrequency(&unscaled_freq, &tick) < 0) {
+  if (TMX_GetFrequency(&freq, &tick) < 0) {
     LOG_FATAL(LOGF_SysLinux, "adjtimex() failed");
   }
 
-  tick_term = dhz * (double)(nominal_tick - tick);
-  freq_term = unscaled_freq / freq_scale;
+  current_delta_tick = nominal_tick - tick;
   
-#if 0
-  LOG(LOGS_INFO, LOGF_SysLinux, "txc.tick=%ld txc.freq=%ld tick_term=%f freq_term=%f",
-      txc.tick, txc.freq, tick_term, freq_term);
-#endif
-
-  return tick_term - freq_term;
+  return dhz * current_delta_tick - freq;
 }
 
 /* ================================================== */
@@ -183,10 +179,10 @@ set_leap(int leap)
 
 /* ================================================== */
 
-/* Estimate the value of HZ given the value of txc.tick that chronyd finds when
+/* Estimate the value of USER_HZ given the value of txc.tick that chronyd finds when
  * it starts.  The only credible values are 100 (Linux/x86) or powers of 2.
  * Also, the bounds checking inside the kernel's adjtimex system call enforces
- * a +/- 10% movement of tick away from the nominal value 1e6/HZ. */
+ * a +/- 10% movement of tick away from the nominal value 1e6/USER_HZ. */
 
 static void
 guess_hz_and_shift_hz(int tick, int *hz, int *shift_hz)
@@ -264,11 +260,6 @@ get_version_specific_details(void)
 {
   int major, minor, patch;
   int shift_hz;
-  double dshift_hz;
-  double basic_freq_scale; /* what to use if HZ!=100 */
-  int config_hz, set_config_hz; /* values of HZ from conf file */
-  int set_config_freq_scale;
-  double config_freq_scale;
   struct tmx_params tmx_params;
   struct utsname uts;
   
@@ -288,14 +279,7 @@ get_version_specific_details(void)
     }
   }
 
-  CNF_GetLinuxHz(&set_config_hz, &config_hz);
-  if (set_config_hz) hz = config_hz;
-  /* (If true, presumably freq_scale will be overridden anyway, making shift_hz
-     redundant too.) */
-
   dhz = (double) hz;
-  dshift_hz = (double)(1UL << shift_hz);
-  basic_freq_scale = dshift_hz / dhz;
   nominal_tick = (1000000L + (hz/2))/hz; /* Mirror declaration in kernel */
   max_tick_bias = nominal_tick / 10;
 
@@ -303,33 +287,6 @@ get_version_specific_details(void)
      (CONFIG_NO_HZ aka tickless), assume the lowest commonly used fixed rate */
   tick_update_hz = 100;
 
-  /* The basic_freq_scale comes from:
-     * the kernel increments the usec counter HZ times per second (if the timer
-       interrupt period were perfect)
-     * the following code in the kernel
-
-       time_adj (+/-)= ltemp >>
-         (SHIFT_USEC + SHIFT_HZ - SHIFT_SCALE);
-
-       causes the adjtimex 'freq' value to be divided down by 1<<SHIFT_HZ.
-
-      The net effect is that we have to scale up the value we want by the
-      reciprocal of all this, i.e. multiply by (1<<SHIFT_HZ)/HZ.
-
-     If HZ==100, this code in the kernel comes into play too:
-#if HZ == 100
-    * Compensate for (HZ==100) != (1 << SHIFT_HZ).
-     * Add 25% and 3.125% to get 128.125; => only 0.125% error (p. 14)
-     *
-    if (time_adj < 0)
-        time_adj -= (-time_adj >> 2) + (-time_adj >> 5);
-    else
-        time_adj += (time_adj >> 2) + (time_adj >> 5);
-#endif
-
-    Special case that later.
-   */
-
   if (uname(&uts) < 0) {
     LOG_FATAL(LOGF_SysLinux, "Cannot uname(2) to get kernel version, sorry.");
   }
@@ -345,17 +302,11 @@ get_version_specific_details(void)
     LOG_FATAL(LOGF_SysLinux, "Kernel version not supported, sorry.");
   }
 
-  if (kernelvercmp(major, minor, patch, 2, 6, 27) < 0) {
-    freq_scale = (hz == 100) ? (128.0 / 128.125) : basic_freq_scale;
-  } else {
-    /* These don't seem to need scaling */
-    freq_scale = 1.0;
-
-    if (kernelvercmp(major, minor, patch, 2, 6, 33) < 0) {
-      /* Tickless kernels before 2.6.33 accumulated ticks only in
-         half-second intervals */
-      tick_update_hz = 2;
-    }
+  if (kernelvercmp(major, minor, patch, 2, 6, 27) >= 0 &&
+      kernelvercmp(major, minor, patch, 2, 6, 33) < 0) {
+    /* Tickless kernels before 2.6.33 accumulated ticks only in
+       half-second intervals */
+    tick_update_hz = 2;
   }
 
   /* ADJ_SETOFFSET support */
@@ -365,14 +316,8 @@ get_version_specific_details(void)
     have_setoffset = 1;
   }
 
-  /* Override freq_scale if it appears in conf file */
-  CNF_GetLinuxFreqScale(&set_config_freq_scale, &config_freq_scale);
-  if (set_config_freq_scale) {
-    freq_scale = config_freq_scale;
-  }
-
-  DEBUG_LOG(LOGF_SysLinux, "hz=%d shift_hz=%d freq_scale=%.8f nominal_tick=%d max_tick_bias=%d",
-      hz, shift_hz, freq_scale, nominal_tick, max_tick_bias);
+  DEBUG_LOG(LOGF_SysLinux, "hz=%d nominal_tick=%d max_tick_bias=%d",
+      hz, nominal_tick, max_tick_bias);
 }
 
 /* ================================================== */