Brian Norris has uploaded this change for review.

View Change

udelay: clock_getres() does not provide clock_gettime() resolution

POSIX doesn't give a clear definition of how clock_getres() and
clock_gettime() relate, but Linux makes this clearer. See:

https://git.kernel.org/linus/01679b5db7172b898be325ff272e10aebd412911
posix-timers: Document sys_clock_getres() correctly

The deleted comments are quite clear:

  "Clock resolution is used to round up timer and interval times, NOT to
   report clock times"

The new comments instead tell how the resolution is only related to
clock_settime() and to the precision of *timers* -- not to the precision
of clock_gettime() itself.

This is particularly relevant depending on the Linux sysctl `sysctl
kernel.timer_highres`. When enabled, resolution is always 1 nanosecond;
when disabled, it follow the CONFIG_HZ of the kernel in question. But in
neither case does this result actually assist determining the quality of
the use case in question: busy-looping over clock_gettime().

In practice, any system with a typical CPU clock will have good enough
clock_gettime() resolution, and trusting clock_getres() for this is
actively misleading (and, wasteful on systems with
kernel.timer_highres==0). Any system that doesn't trust the precision of
clock_gettime() should simply be building with HAS_CLOCK_GETTIME=no.

Therefore, avoid ever looking at clock_getres(), and instead always use
clock_gettime() if we're allowed to. This saves a bunch of wasted time
in "calibration", which is itself not actually accurate in the presence
of hybrid CPUs or CPU DFS. Notably, calibration time-wastage has been
the subject of a few past stalled efforts:

https://review.coreboot.org/c/flashrom/+/39864
https://review.coreboot.org/c/flashrom/+/44517

BUG=b:325539928
TEST=flashrom never resorts to "Calibrating delay loop" when
     clock_gettime is available
TEST=flashrom works at the same speed (wall clock time) with
     `sysctl kernel.timer_highres=1` and `sysctl kernel.timer_highres=0`

Change-Id: Icdc6e184eacb14d3bd27029e3fc75be4431d82b4
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
M udelay.c
1 file changed, 5 insertions(+), 22 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/06/80806/1

diff --git a/udelay.c b/udelay.c
index 82ddcbd..0005d39 100644
--- a/udelay.c
+++ b/udelay.c
@@ -27,10 +27,10 @@
 #include "flash.h"
 #include "programmer.h"
 
-static bool use_clock_gettime = false;
-
 #if HAVE_CLOCK_GETTIME == 1
 
+static const bool use_clock_gettime = true;
+
 #ifdef _POSIX_MONOTONIC_CLOCK
 static clockid_t clock_id = CLOCK_MONOTONIC;
 #else
@@ -51,28 +51,11 @@
 		clock_gettime(clock_id, &now);
 	} while (now.tv_sec < end.tv_sec || (now.tv_sec == end.tv_sec && now.tv_nsec < end.tv_nsec));
 }
-
-static int clock_check_res(void)
-{
-	struct timespec res;
-	if (!clock_getres(clock_id, &res)) {
-		if (res.tv_sec == 0 && res.tv_nsec <= 100) {
-			msg_pinfo("Using clock_gettime for delay loops (clk_id: %d, resolution: %ldns).\n",
-				  (int)clock_id, res.tv_nsec);
-			use_clock_gettime = true;
-			return 1;
-		}
-	} else if (clock_id != CLOCK_REALTIME && errno == EINVAL) {
-		/* Try again with CLOCK_REALTIME. */
-		clock_id = CLOCK_REALTIME;
-		return clock_check_res();
-	}
-	return 0;
-}
 #else
 
+static const bool use_clock_gettime = false;
+
 static inline void clock_usec_delay(int usecs) {}
-static inline int clock_check_res(void) { return 0; }
 
 #endif /* HAVE_CLOCK_GETTIME == 1 */
 
@@ -135,7 +118,7 @@
 
 void myusec_calibrate_delay(void)
 {
-	if (clock_check_res())
+	if (use_clock_gettime)
 		return;
 
 	unsigned long count = 1000;

To view, visit change 80806. To unsubscribe, or for help writing mail filters, visit settings.