[PATCH 1/2] timer/linux: lower rounding of tsc estimation to 10KHz

[PATCH 1/2] timer/linux: lower rounding of tsc estimation to 10KHz

[Isaac Boukris]

In practice, the estimation result is just a couple of KHz
away from the kernel's tsc_khz value, so it should suffice.

Roundin to 10MHz can cause a significant drift from real time,
up to a second per 10 minutes.

See also bugzilla: 959

Signed-off-by: Isaac Boukris <iboukris@gmail.com>
---
 lib/eal/linux/eal_timer.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/eal/linux/eal_timer.c b/lib/eal/linux/eal_timer.c
index 1cb1e92193..241b20d416 100644
--- a/lib/eal/linux/eal_timer.c
+++ b/lib/eal/linux/eal_timer.c
@@ -192,7 +192,7 @@ get_tsc_freq(void)
 {
 #ifdef CLOCK_MONOTONIC_RAW
 #define NS_PER_SEC 1E9
-#define CYC_PER_10MHZ 1E7
+#define CYC_PER_10KHZ 1E4
 
 	struct timespec sleeptime = {.tv_nsec = NS_PER_SEC / 10 }; /* 1/10 second */
 
@@ -209,8 +209,8 @@ get_tsc_freq(void)
 
 		double secs = (double)ns/NS_PER_SEC;
 		tsc_hz = (uint64_t)((end - start)/secs);
-		/* Round up to 10Mhz. 1E7 ~ 10Mhz */
-		return RTE_ALIGN_MUL_NEAR(tsc_hz, CYC_PER_10MHZ);
+		/* Round up to 10Khz. 1E4 ~ 10Khz */
+		return RTE_ALIGN_MUL_NEAR(tsc_hz, CYC_PER_10KHZ);
 	}
 #endif
 	return 0;
-- 
2.45.0

[PATCH 2/2] timer/linux: override TSC freq if no tsc_known_freq

[Isaac Boukris]

If the tsc_known_freq cpu flag is missing, it means the kernel doesn't
trust it and calculates its own. We should do the same to avoid drift.

Signed-off-by: Isaac Boukris <iboukris@gmail.com>
---
 lib/eal/common/eal_common_timer.c |  3 +-
 lib/eal/common/eal_private.h      |  2 +-
 lib/eal/freebsd/eal_timer.c       |  5 +++-
 lib/eal/linux/eal_timer.c         | 48 +++++++++++++++++++++++++++++--
 lib/eal/windows/eal_timer.c       |  5 +++-
 5 files changed, 56 insertions(+), 7 deletions(-)

diff --git a/lib/eal/common/eal_common_timer.c b/lib/eal/common/eal_common_timer.c
index c5c4703f15..e00be0a5c8 100644
--- a/lib/eal/common/eal_common_timer.c
+++ b/lib/eal/common/eal_common_timer.c
@@ -66,8 +66,7 @@ set_tsc_freq(void)
 	}
 
 	freq = get_tsc_freq_arch();
-	if (!freq)
-		freq = get_tsc_freq();
+	freq = get_tsc_freq(freq);
 	if (!freq)
 		freq = estimate_tsc_freq();
 
diff --git a/lib/eal/common/eal_private.h b/lib/eal/common/eal_private.h
index af09620426..bb315dab04 100644
--- a/lib/eal/common/eal_private.h
+++ b/lib/eal/common/eal_private.h
@@ -374,7 +374,7 @@ void set_tsc_freq(void);
  *
  * This function is private to the EAL.
  */
-uint64_t get_tsc_freq(void);
+uint64_t get_tsc_freq(uint64_t arch_hz);
 
 /**
  * Get TSC frequency if the architecture supports.
diff --git a/lib/eal/freebsd/eal_timer.c b/lib/eal/freebsd/eal_timer.c
index 3dd70e24ba..5a8aea03e1 100644
--- a/lib/eal/freebsd/eal_timer.c
+++ b/lib/eal/freebsd/eal_timer.c
@@ -26,12 +26,15 @@
 enum timer_source eal_timer_source = EAL_TIMER_TSC;
 
 uint64_t
-get_tsc_freq(void)
+get_tsc_freq(uint64_t arch_hz)
 {
 	size_t sz;
 	int tmp;
 	uint64_t tsc_hz;
 
+	if (arch_hz)
+		return arch_hz;
+
 	sz = sizeof(tmp);
 	tmp = 0;
 
diff --git a/lib/eal/linux/eal_timer.c b/lib/eal/linux/eal_timer.c
index 241b20d416..cdbd191828 100644
--- a/lib/eal/linux/eal_timer.c
+++ b/lib/eal/linux/eal_timer.c
@@ -5,9 +5,9 @@
 
 #include <stdio.h>
 #include <stdint.h>
+#include <inttypes.h>
 #ifdef RTE_LIBEAL_USE_HPET
 #include <fcntl.h>
-#include <inttypes.h>
 #include <sys/mman.h>
 #include <unistd.h>
 #endif
@@ -187,8 +187,38 @@ rte_eal_hpet_init(int make_default)
 }
 #endif
 
+/* Check if the kernel deems the arch provided TSC frequency trustworthy. */
+
+static bool
+is_tsc_known_freq(void)
+{
+	char line[2048];
+	FILE *stream;
+	bool ret = true; /* Assume tsc_known_freq */
+
+	stream = fopen("/proc/cpuinfo", "r");
+	if (!stream) {
+		EAL_LOG(WARNING, "Unable to open /proc/cpuinfo");
+		return ret;
+	}
+
+	while (fgets(line, sizeof(line), stream)) {
+		if (strncmp(line, "flags", 5) != 0)
+			continue;
+
+		if (!strstr(line, "tsc_known_freq"))
+			ret = false;
+
+		break;
+	}
+
+	fclose(stream);
+
+	return ret;
+}
+
 uint64_t
-get_tsc_freq(void)
+get_tsc_freq(uint64_t arch_hz)
 {
 #ifdef CLOCK_MONOTONIC_RAW
 #define NS_PER_SEC 1E9
@@ -199,6 +229,9 @@ get_tsc_freq(void)
 	struct timespec t_start, t_end;
 	uint64_t tsc_hz;
 
+	if (arch_hz && is_tsc_known_freq())
+		return arch_hz;
+
 	if (clock_gettime(CLOCK_MONOTONIC_RAW, &t_start) == 0) {
 		uint64_t ns, end, start = rte_rdtsc();
 		nanosleep(&sleeptime,NULL);
@@ -209,6 +242,17 @@ get_tsc_freq(void)
 
 		double secs = (double)ns/NS_PER_SEC;
 		tsc_hz = (uint64_t)((end - start)/secs);
+
+		if (arch_hz) {
+			/* Make sure we're within 1% for sanity check */
+			if (arch_hz - tsc_hz > arch_hz / 100)
+				return arch_hz;
+
+			EAL_LOG(DEBUG, "Refined architecture frequency %"PRIu64
+				       " to actual frequency %"PRIu64".",
+					arch_hz, tsc_hz);
+		}
+
 		/* Round up to 10Khz. 1E4 ~ 10Khz */
 		return RTE_ALIGN_MUL_NEAR(tsc_hz, CYC_PER_10KHZ);
 	}
diff --git a/lib/eal/windows/eal_timer.c b/lib/eal/windows/eal_timer.c
index b070cb7751..cfd6c267ac 100644
--- a/lib/eal/windows/eal_timer.c
+++ b/lib/eal/windows/eal_timer.c
@@ -49,13 +49,16 @@ rte_delay_us_sleep(unsigned int us)
 }
 
 uint64_t
-get_tsc_freq(void)
+get_tsc_freq(uint64_t arch_hz)
 {
 	LARGE_INTEGER t_start, t_end, elapsed_us;
 	LARGE_INTEGER frequency;
 	uint64_t tsc_hz;
 	uint64_t end, start;
 
+	if (arch_hz)
+		return arch_hz;
+
 	QueryPerformanceFrequency(&frequency);
 
 	QueryPerformanceCounter(&t_start);
-- 
2.45.0

Re: [PATCH 2/2] timer/linux: override TSC freq if no tsc_known_freq

[Isaac Boukris]

Perhaps this was a bit hastily, I got an automated failure report on
"arm64-native-linuxapp-gcc":

https://lab.dpdk.org/results/dashboard/patchsets/31106/

Not sure which patch caused the problem, looking at the kernel code it
looks like 'tsc_known_freq' is only set for x86 arch, but it could be
the other patch and maybe lowering the rounding to 10KHz is too
aggressive, maybe better 100KHz or 1MHz.

As an alternative to 'tsc_known_freq' detection, maybe just provide an
init parameter to set the frequency manually, along with some
known-issue documentation.

Maybe we should just allow to specify the frequency as a parameter at init