[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

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

[Bruce Richardson]

On Tue, Sep 24, 2024 at 08:04:00PM +0300, Isaac Boukris wrote:
> 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/
> 

That might be a false positive failure. I don't see how this patch could
cause issues for a single ip reassembly test case.

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

I don't see why we can't put an #ifdef in the code to always return true on
non-x86 platforms. [Or if we don't want an #ifdef, we can always use 'if
(strcmp(RTE_ARCH,"x86_64") && strcmp(RTE_ARCH,"i686"))' :-)]

Don't really have a strong opinion on the rounding, maybe others do. [Since
you give 3 options, I'm going to suggest going for the middle one -
100KHz!]

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

I don't think that is a great solution, better to detect the frequency if
we can.

An interesting future enhancement would be to update DPDK to work with [1],
where we read the tsc value directly from the kernel. According to that
project, this is already available on Google production kernels, though
not generally elsewhere. For those who care about having accurate TSC values,
or faster startup times, having the extra kernel module to read the TSC
value directly might be worthwhile.

Regards,
/Bruce

[1] https://github.com/trailofbits/tsc_freq_khz

[PATCH v2 0/2] Improve TSC frequency accuracy on Linux

[Isaac Boukris]

Changes in V2:
 - changed rounding to 100KHz which should be pretty safe.
 - limited the tsc_known_freq flag check to x86 arch.

Isaac Boukris (2):
  timer/linux: lower rounding of tsc estimation to 100KHz
  timer/linux/x86: override TSC freq if no tsc_known_freq

 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         | 56 ++++++++++++++++++++++++++++---
 lib/eal/windows/eal_timer.c       |  5 ++-
 5 files changed, 61 insertions(+), 10 deletions(-)

-- 
2.45.0

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

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

Rounding 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..f56a7ae15b 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_100KHZ 1E5
 
 	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 100Khz. 1E5 ~ 100Khz */
+		return RTE_ALIGN_MUL_NEAR(tsc_hz, CYC_PER_100KHZ);
 	}
 #endif
 	return 0;
-- 
2.45.0

[PATCH v2 2/2] timer/linux/x86: 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         | 50 +++++++++++++++++++++++++++++--
 lib/eal/windows/eal_timer.c       |  5 +++-
 5 files changed, 58 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 f56a7ae15b..9a38e47ba3 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,40 @@ 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 */
+
+#if defined(RTE_ARCH_X86)
+	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);
+#endif
+
+	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 +231,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 +244,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 measured frequency %"PRIu64".",
+					arch_hz, tsc_hz);
+		}
+
 		/* Round up to 100Khz. 1E5 ~ 100Khz */
 		return RTE_ALIGN_MUL_NEAR(tsc_hz, CYC_PER_100KHZ);
 	}
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 v2 2/2] timer/linux/x86: override TSC freq if no tsc_known_freq

[Stephen Hemminger]

On Tue,  1 Oct 2024 01:08:32 +0300
Isaac Boukris <iboukris@gmail.com> wrote:

> +
> +			EAL_LOG(DEBUG, "Refined architecture frequency %"PRIu64
> +				       " to measured frequency %"PRIu64".",
> +					arch_hz, tsc_hz);
> +		}

Best not to break messages, it make's it harder for users to search.
Don't need period either.


			EAL_LOG(DEBUG,
				"Refined architecture frequency %"PRIu64" to measured frequency %"PRIu64,

[PATCH v3 0/2] Improve TSC frequency accuracy on Linux

[Isaac Boukris]

Changes in V3:
 - fixed ifdef logic.
 - avoid breaking a debug line.

Isaac Boukris (2):
  timer/linux: lower rounding of tsc estimation to 100KHz
  timer/linux/x86: override TSC freq if no tsc_known_freq

 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         | 59 +++++++++++++++++++++++++++----
 lib/eal/windows/eal_timer.c       |  5 ++-
 5 files changed, 63 insertions(+), 11 deletions(-)

-- 
2.45.0

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

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

Rounding 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..f56a7ae15b 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_100KHZ 1E5
 
 	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 100Khz. 1E5 ~ 100Khz */
+		return RTE_ALIGN_MUL_NEAR(tsc_hz, CYC_PER_100KHZ);
 	}
 #endif
 	return 0;
-- 
2.45.0

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

[Stephen Hemminger]

On Tue,  1 Oct 2024 03:22:50 +0300
Isaac Boukris <iboukris@gmail.com> wrote:

> In practice, the estimation result is just a couple of KHz
> away from the kernel's tsc_khz value, so it should suffice.
> 
> Rounding 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>
> ---

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

[PATCH v3 2/2] timer/linux/x86: 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         | 53 +++++++++++++++++++++++++++++--
 lib/eal/windows/eal_timer.c       |  5 ++-
 5 files changed, 60 insertions(+), 8 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 f56a7ae15b..2c5fc9ff3e 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,41 @@ 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)
+{
+	bool ret = true; /* Assume tsc_known_freq */
+
+#if defined(RTE_ARCH_X86)
+	char line[2048];
+	FILE *stream;
+
+	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);
+#endif
+
+	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 +232,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,11 +245,22 @@ 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 arch frequency %"PRIu64" to measured frequency %"PRIu64,
+				arch_hz, tsc_hz);
+		}
+
 		/* Round up to 100Khz. 1E5 ~ 100Khz */
 		return RTE_ALIGN_MUL_NEAR(tsc_hz, CYC_PER_100KHZ);
 	}
 #endif
-	return 0;
+	return arch_hz;
 }
 
 int
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 v3 2/2] timer/linux/x86: override TSC freq if no tsc_known_freq

[Stephen Hemminger]

On Tue,  1 Oct 2024 03:22:51 +0300
Isaac Boukris <iboukris@gmail.com> wrote:

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

On Windows, I would not use arch_hz at all, since it is opaque how
the Windows kernel determines the frequency, and best not to get
skew.

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

[Isaac Boukris]

On Tue, Oct 1, 2024 at 6:22 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Tue,  1 Oct 2024 03:22:51 +0300
> Isaac Boukris <iboukris@gmail.com> wrote:
>
> > 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);
> > --
>
> On Windows, I would not use arch_hz at all, since it is opaque how
> the Windows kernel determines the frequency, and best not to get
> skew.

Not sure I follow, currently the patch doesn't change the behavior for
Windows, and the Windows code is quite similar to the Linux one,
should we always prefer the measured value in Windows?

On the other hand, I think I should update the rounding commit to also
change the Windows code, there is no sense in 10MHz rounding there
either.

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

[Bruce Richardson]

On Tue, Oct 01, 2024 at 03:22:51AM +0300, Isaac Boukris wrote:
> 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         | 53 +++++++++++++++++++++++++++++--
>  lib/eal/windows/eal_timer.c       |  5 ++-
>  5 files changed, 60 insertions(+), 8 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;
>  

On FreeBSD I'm not sure this is the best behaviour. On BSD we read the TSC
value from the kernel, which, one assumes, has measured it accurately.
Therefore I'd tend toward just using the kernel value in all cases, maybe
check the arch value (if non-zero) against that and warning if they have
significant divergence. WDYT?

/Bruce

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

[Isaac Boukris]

On Tue, Oct 1, 2024 at 11:01 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Tue, Oct 01, 2024 at 03:22:51AM +0300, Isaac Boukris wrote:
> > 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         | 53 +++++++++++++++++++++++++++++--
> >  lib/eal/windows/eal_timer.c       |  5 ++-
> >  5 files changed, 60 insertions(+), 8 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;
> >
>
> On FreeBSD I'm not sure this is the best behaviour. On BSD we read the TSC
> value from the kernel, which, one assumes, has measured it accurately.
> Therefore I'd tend toward just using the kernel value in all cases, maybe
> check the arch value (if non-zero) against that and warning if they have
> significant divergence. WDYT?

Makes sense, I'll add a patch for that. We could also use the arch
value if for some reason the sysctlbyname() failed.

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

[Bruce Richardson]

On Wed, Oct 02, 2024 at 12:59:58AM +0300, Isaac Boukris wrote:
> On Tue, Oct 1, 2024 at 11:01 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Tue, Oct 01, 2024 at 03:22:51AM +0300, Isaac Boukris wrote:
> > > 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         | 53 +++++++++++++++++++++++++++++--
> > >  lib/eal/windows/eal_timer.c       |  5 ++-
> > >  5 files changed, 60 insertions(+), 8 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;
> > >
> >
> > On FreeBSD I'm not sure this is the best behaviour. On BSD we read the TSC
> > value from the kernel, which, one assumes, has measured it accurately.
> > Therefore I'd tend toward just using the kernel value in all cases, maybe
> > check the arch value (if non-zero) against that and warning if they have
> > significant divergence. WDYT?
> 
> Makes sense, I'll add a patch for that. We could also use the arch
> value if for some reason the sysctlbyname() failed.

Yep, +1 to that.

/Bruce

[PATCH v4 0/2] Improve TSC frequency accuracy

[Isaac Boukris]

Changes in V4:
 - lower ronding on Windows too
 - on freebsd prefer system tsc_hz

Isaac Boukris (2):
  timer: lower rounding of TSC estimation to 100KHz
  timer: allow platform to override cpu TSC frequency

 lib/eal/common/eal_common_timer.c |  3 +-
 lib/eal/common/eal_private.h      |  2 +-
 lib/eal/freebsd/eal_timer.c       |  8 +++--
 lib/eal/linux/eal_timer.c         | 59 +++++++++++++++++++++++++++----
 lib/eal/windows/eal_timer.c       | 11 +++---
 5 files changed, 68 insertions(+), 15 deletions(-)

-- 
2.45.0

[PATCH v4 1/2] timer: lower rounding of TSC estimation to 100KHz

[Isaac Boukris]

In practice, the frequency is often not a nice round number, while
the estimation results are rather accurate, just a couple of KHz
away from the kernel's tsc_khz value, so it should suffice.

Rounding 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 +++---
 lib/eal/windows/eal_timer.c | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/eal/linux/eal_timer.c b/lib/eal/linux/eal_timer.c
index 1cb1e92193..f56a7ae15b 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_100KHZ 1E5
 
 	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 100Khz. 1E5 ~ 100Khz */
+		return RTE_ALIGN_MUL_NEAR(tsc_hz, CYC_PER_100KHZ);
 	}
 #endif
 	return 0;
diff --git a/lib/eal/windows/eal_timer.c b/lib/eal/windows/eal_timer.c
index b070cb7751..4003541b08 100644
--- a/lib/eal/windows/eal_timer.c
+++ b/lib/eal/windows/eal_timer.c
@@ -12,7 +12,7 @@
 #include "eal_private.h"
 
 #define US_PER_SEC 1E6
-#define CYC_PER_10MHZ 1E7
+#define CYC_PER_100KHZ 1E5
 
 void
 rte_delay_us_sleep(unsigned int us)
@@ -81,8 +81,8 @@ get_tsc_freq(void)
 	double secs = ((double)elapsed_us.QuadPart)/US_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 100Khz. 1E5 ~ 100Khz */
+	return RTE_ALIGN_MUL_NEAR(tsc_hz, CYC_PER_100KHZ);
 }
 
 
-- 
2.45.0

[PATCH v4 2/2] timer: allow platform to override cpu TSC frequency

[Isaac Boukris]

The CPU value is often not accurate, allow overriding it based
on info from the host OS.

On Linux X86, 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.

On freebsd we have access to the kernel tsc_hz value, just use it.

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       |  8 +++--
 lib/eal/linux/eal_timer.c         | 53 +++++++++++++++++++++++++++++--
 lib/eal/windows/eal_timer.c       |  5 ++-
 5 files changed, 62 insertions(+), 9 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..19fc9a7228 100644
--- a/lib/eal/freebsd/eal_timer.c
+++ b/lib/eal/freebsd/eal_timer.c
@@ -26,7 +26,7 @@
 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;
@@ -50,9 +50,13 @@ get_tsc_freq(void)
 	sz = sizeof(tsc_hz);
 	if (sysctlbyname("machdep.tsc_freq", &tsc_hz, &sz, NULL, 0)) {
 		EAL_LOG(WARNING, "%s", strerror(errno));
-		return 0;
+		return arch_hz;
 	}
 
+	if (arch_hz && arch_hz - tsc_hz > arch_hz / 100)
+		EAL_LOG(WARNING, "Host tsc_freq %"PRIu64" at odds with cpu value %"PRIu64,
+			tsc_hz, arch_hz);
+
 	return tsc_hz;
 }
 
diff --git a/lib/eal/linux/eal_timer.c b/lib/eal/linux/eal_timer.c
index f56a7ae15b..2c5fc9ff3e 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,41 @@ 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)
+{
+	bool ret = true; /* Assume tsc_known_freq */
+
+#if defined(RTE_ARCH_X86)
+	char line[2048];
+	FILE *stream;
+
+	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);
+#endif
+
+	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 +232,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,11 +245,22 @@ 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 arch frequency %"PRIu64" to measured frequency %"PRIu64,
+				arch_hz, tsc_hz);
+		}
+
 		/* Round up to 100Khz. 1E5 ~ 100Khz */
 		return RTE_ALIGN_MUL_NEAR(tsc_hz, CYC_PER_100KHZ);
 	}
 #endif
-	return 0;
+	return arch_hz;
 }
 
 int
diff --git a/lib/eal/windows/eal_timer.c b/lib/eal/windows/eal_timer.c
index 4003541b08..020035c4cc 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 v4 2/2] timer: allow platform to override cpu TSC frequency

[Bruce Richardson]

On Wed, Oct 02, 2024 at 07:56:36PM +0300, Isaac Boukris wrote:
> The CPU value is often not accurate, allow overriding it based
> on info from the host OS.
> 
> On Linux X86, 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.
> 
> On freebsd we have access to the kernel tsc_hz value, just use it.
> 
> Signed-off-by: Isaac Boukris <iboukris@gmail.com>
> ---

Looks good to me. Hope to test it out soon on my systems to see how it
goes.

One comment inline below.

/Bruce

>  lib/eal/common/eal_common_timer.c |  3 +-
>  lib/eal/common/eal_private.h      |  2 +-
>  lib/eal/freebsd/eal_timer.c       |  8 +++--
>  lib/eal/linux/eal_timer.c         | 53 +++++++++++++++++++++++++++++--
>  lib/eal/windows/eal_timer.c       |  5 ++-
>  5 files changed, 62 insertions(+), 9 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..19fc9a7228 100644
> --- a/lib/eal/freebsd/eal_timer.c
> +++ b/lib/eal/freebsd/eal_timer.c
> @@ -26,7 +26,7 @@
>  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;
> @@ -50,9 +50,13 @@ get_tsc_freq(void)
>  	sz = sizeof(tsc_hz);
>  	if (sysctlbyname("machdep.tsc_freq", &tsc_hz, &sz, NULL, 0)) {
>  		EAL_LOG(WARNING, "%s", strerror(errno));
> -		return 0;
> +		return arch_hz;
>  	}
>  
> +	if (arch_hz && arch_hz - tsc_hz > arch_hz / 100)

Do we not need an "abs()" call on the "arch_hz - tsc_hz" in case tsc_hz is
the bigger value?

Re: [PATCH v4 2/2] timer: allow platform to override cpu TSC frequency

[Isaac Boukris]

On Wed, Oct 2, 2024 at 8:11 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Wed, Oct 02, 2024 at 07:56:36PM +0300, Isaac Boukris wrote:
> > The CPU value is often not accurate, allow overriding it based
> > on info from the host OS.
> >
> > On Linux X86, 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.
> >
> > On freebsd we have access to the kernel tsc_hz value, just use it.
> >
> > Signed-off-by: Isaac Boukris <iboukris@gmail.com>
> > ---
>
> Looks good to me. Hope to test it out soon on my systems to see how it
> goes.
>
> One comment inline below.
>
> /Bruce
>
> >  lib/eal/common/eal_common_timer.c |  3 +-
> >  lib/eal/common/eal_private.h      |  2 +-
> >  lib/eal/freebsd/eal_timer.c       |  8 +++--
> >  lib/eal/linux/eal_timer.c         | 53 +++++++++++++++++++++++++++++--
> >  lib/eal/windows/eal_timer.c       |  5 ++-
> >  5 files changed, 62 insertions(+), 9 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..19fc9a7228 100644
> > --- a/lib/eal/freebsd/eal_timer.c
> > +++ b/lib/eal/freebsd/eal_timer.c
> > @@ -26,7 +26,7 @@
> >  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;
> > @@ -50,9 +50,13 @@ get_tsc_freq(void)
> >       sz = sizeof(tsc_hz);
> >       if (sysctlbyname("machdep.tsc_freq", &tsc_hz, &sz, NULL, 0)) {
> >               EAL_LOG(WARNING, "%s", strerror(errno));
> > -             return 0;
> > +             return arch_hz;
> >       }
> >
> > +     if (arch_hz && arch_hz - tsc_hz > arch_hz / 100)
>
> Do we not need an "abs()" call on the "arch_hz - tsc_hz" in case tsc_hz is
> the bigger value?

As they are unsigned it will overflow and result the absolute value, I
actually did use abs() at first, and the compiler yelled at me:
warning: taking the absolute value of unsigned type ‘uint64_t’ {aka
‘long unsigned int’} has no effect [-Wabsolute-value]

Re: [PATCH v4 2/2] timer: allow platform to override cpu TSC frequency

[Bruce Richardson]

On Wed, Oct 02, 2024 at 10:14:57PM +0300, Isaac Boukris wrote:
> On Wed, Oct 2, 2024 at 8:11 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Wed, Oct 02, 2024 at 07:56:36PM +0300, Isaac Boukris wrote:
> > > The CPU value is often not accurate, allow overriding it based
> > > on info from the host OS.
> > >
> > > On Linux X86, 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.
> > >
> > > On freebsd we have access to the kernel tsc_hz value, just use it.
> > >
> > > Signed-off-by: Isaac Boukris <iboukris@gmail.com>
> > > ---
> >
> > Looks good to me. Hope to test it out soon on my systems to see how it
> > goes.
> >
> > One comment inline below.
> >
> > /Bruce
> >
> > >  lib/eal/common/eal_common_timer.c |  3 +-
> > >  lib/eal/common/eal_private.h      |  2 +-
> > >  lib/eal/freebsd/eal_timer.c       |  8 +++--
> > >  lib/eal/linux/eal_timer.c         | 53 +++++++++++++++++++++++++++++--
> > >  lib/eal/windows/eal_timer.c       |  5 ++-
> > >  5 files changed, 62 insertions(+), 9 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..19fc9a7228 100644
> > > --- a/lib/eal/freebsd/eal_timer.c
> > > +++ b/lib/eal/freebsd/eal_timer.c
> > > @@ -26,7 +26,7 @@
> > >  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;
> > > @@ -50,9 +50,13 @@ get_tsc_freq(void)
> > >       sz = sizeof(tsc_hz);
> > >       if (sysctlbyname("machdep.tsc_freq", &tsc_hz, &sz, NULL, 0)) {
> > >               EAL_LOG(WARNING, "%s", strerror(errno));
> > > -             return 0;
> > > +             return arch_hz;
> > >       }
> > >
> > > +     if (arch_hz && arch_hz - tsc_hz > arch_hz / 100)
> >
> > Do we not need an "abs()" call on the "arch_hz - tsc_hz" in case tsc_hz is
> > the bigger value?
> 
> As they are unsigned it will overflow and result the absolute value, I
> actually did use abs() at first, and the compiler yelled at me:
> warning: taking the absolute value of unsigned type ‘uint64_t’ {aka
> ‘long unsigned int’} has no effect [-Wabsolute-value]

Yes, the compiler is right about them being unsigned. However, without
using signed arithmetic and using abs, tsc_hz being even 1Hz greater than
arch_hz will lead to the condition failing, so it is actually checking for
tsc_hz being within 1% only on the low side.

/Bruce

Re: [PATCH v4 2/2] timer: allow platform to override cpu TSC frequency

[Isaac Boukris]

On Thu, Oct 3, 2024 at 12:31 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Wed, Oct 02, 2024 at 10:14:57PM +0300, Isaac Boukris wrote:
> > On Wed, Oct 2, 2024 at 8:11 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > On Wed, Oct 02, 2024 at 07:56:36PM +0300, Isaac Boukris wrote:
> > > > The CPU value is often not accurate, allow overriding it based
> > > > on info from the host OS.
> > > >
> > > > On Linux X86, 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.
> > > >
> > > > On freebsd we have access to the kernel tsc_hz value, just use it.
> > > >
> > > > Signed-off-by: Isaac Boukris <iboukris@gmail.com>
> > > > ---
> > >
> > > Looks good to me. Hope to test it out soon on my systems to see how it
> > > goes.
> > >
> > > One comment inline below.
> > >
> > > /Bruce
> > >
> > > >  lib/eal/common/eal_common_timer.c |  3 +-
> > > >  lib/eal/common/eal_private.h      |  2 +-
> > > >  lib/eal/freebsd/eal_timer.c       |  8 +++--
> > > >  lib/eal/linux/eal_timer.c         | 53 +++++++++++++++++++++++++++++--
> > > >  lib/eal/windows/eal_timer.c       |  5 ++-
> > > >  5 files changed, 62 insertions(+), 9 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..19fc9a7228 100644
> > > > --- a/lib/eal/freebsd/eal_timer.c
> > > > +++ b/lib/eal/freebsd/eal_timer.c
> > > > @@ -26,7 +26,7 @@
> > > >  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;
> > > > @@ -50,9 +50,13 @@ get_tsc_freq(void)
> > > >       sz = sizeof(tsc_hz);
> > > >       if (sysctlbyname("machdep.tsc_freq", &tsc_hz, &sz, NULL, 0)) {
> > > >               EAL_LOG(WARNING, "%s", strerror(errno));
> > > > -             return 0;
> > > > +             return arch_hz;
> > > >       }
> > > >
> > > > +     if (arch_hz && arch_hz - tsc_hz > arch_hz / 100)
> > >
> > > Do we not need an "abs()" call on the "arch_hz - tsc_hz" in case tsc_hz is
> > > the bigger value?
> >
> > As they are unsigned it will overflow and result the absolute value, I
> > actually did use abs() at first, and the compiler yelled at me:
> > warning: taking the absolute value of unsigned type ‘uint64_t’ {aka
> > ‘long unsigned int’} has no effect [-Wabsolute-value]
>
> Yes, the compiler is right about them being unsigned. However, without
> using signed arithmetic and using abs, tsc_hz being even 1Hz greater than
> arch_hz will lead to the condition failing, so it is actually checking for
> tsc_hz being within 1% only on the low side.

Ah right, I guess the 'has no effect' warning confused me.. V5 patch on its way.

Re: [PATCH v4 0/2] Improve TSC frequency accuracy

[Bruce Richardson]

On Wed, Oct 02, 2024 at 07:56:34PM +0300, Isaac Boukris wrote:
> Changes in V4:
>  - lower ronding on Windows too
>  - on freebsd prefer system tsc_hz
> 
> Isaac Boukris (2):
>   timer: lower rounding of TSC estimation to 100KHz
>   timer: allow platform to override cpu TSC frequency
> 
>  lib/eal/common/eal_common_timer.c |  3 +-
>  lib/eal/common/eal_private.h      |  2 +-
>  lib/eal/freebsd/eal_timer.c       |  8 +++--
>  lib/eal/linux/eal_timer.c         | 59 +++++++++++++++++++++++++++----
>  lib/eal/windows/eal_timer.c       | 11 +++---
>  5 files changed, 68 insertions(+), 15 deletions(-)
> 
> -- 
I like this cleanup. One open on patch 2 still on my end. With that
resolved:

Series-Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v5 0/2] Improve TSC frequency accuracy

[Isaac Boukris]

Changes if V5:
 - fix absolute delta calculation per Bruce's comment.

Isaac Boukris (2):
  timer: lower rounding of TSC estimation to 100KHz
  timer: allow platform to override cpu TSC frequency

 lib/eal/common/eal_common_timer.c |  3 +-
 lib/eal/common/eal_private.h      |  2 +-
 lib/eal/freebsd/eal_timer.c       |  8 +++--
 lib/eal/linux/eal_timer.c         | 59 +++++++++++++++++++++++++++----
 lib/eal/windows/eal_timer.c       | 11 +++---
 5 files changed, 68 insertions(+), 15 deletions(-)

-- 
2.45.0

[PATCH v5 1/2] timer: lower rounding of TSC estimation to 100KHz

[Isaac Boukris]

In practice, the frequency is often not a nice round number, while
the estimation results are rather accurate, just a couple of KHz
away from the kernel's tsc_khz value, so it should suffice.

Rounding 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 +++---
 lib/eal/windows/eal_timer.c | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/eal/linux/eal_timer.c b/lib/eal/linux/eal_timer.c
index 1cb1e92193..f56a7ae15b 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_100KHZ 1E5
 
 	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 100Khz. 1E5 ~ 100Khz */
+		return RTE_ALIGN_MUL_NEAR(tsc_hz, CYC_PER_100KHZ);
 	}
 #endif
 	return 0;
diff --git a/lib/eal/windows/eal_timer.c b/lib/eal/windows/eal_timer.c
index b070cb7751..4003541b08 100644
--- a/lib/eal/windows/eal_timer.c
+++ b/lib/eal/windows/eal_timer.c
@@ -12,7 +12,7 @@
 #include "eal_private.h"
 
 #define US_PER_SEC 1E6
-#define CYC_PER_10MHZ 1E7
+#define CYC_PER_100KHZ 1E5
 
 void
 rte_delay_us_sleep(unsigned int us)
@@ -81,8 +81,8 @@ get_tsc_freq(void)
 	double secs = ((double)elapsed_us.QuadPart)/US_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 100Khz. 1E5 ~ 100Khz */
+	return RTE_ALIGN_MUL_NEAR(tsc_hz, CYC_PER_100KHZ);
 }
 
 
-- 
2.45.0

Re: [PATCH v5 1/2] timer: lower rounding of TSC estimation to 100KHz

[Bruce Richardson]

On Thu, Oct 03, 2024 at 03:26:03PM +0300, Isaac Boukris wrote:
> In practice, the frequency is often not a nice round number, while
> the estimation results are rather accurate, just a couple of KHz
> away from the kernel's tsc_khz value, so it should suffice.
> 
> Rounding 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>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [PATCH v5 1/2] timer: lower rounding of TSC estimation to 100KHz

[Stephen Hemminger]

On Thu, 3 Oct 2024 15:05:02 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Thu, Oct 03, 2024 at 03:26:03PM +0300, Isaac Boukris wrote:
> > In practice, the frequency is often not a nice round number, while
> > the estimation results are rather accurate, just a couple of KHz
> > away from the kernel's tsc_khz value, so it should suffice.
> > 
> > Rounding 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>  
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Re: [PATCH v5 1/2] timer: lower rounding of TSC estimation to 100KHz

[David Marchand]

On Thu, Oct 3, 2024 at 5:13 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Thu, 3 Oct 2024 15:05:02 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
>
> > On Thu, Oct 03, 2024 at 03:26:03PM +0300, Isaac Boukris wrote:
> > > In practice, the frequency is often not a nice round number, while
> > > the estimation results are rather accurate, just a couple of KHz
> > > away from the kernel's tsc_khz value, so it should suffice.
> > >
> > > Rounding 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>
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>

Series applied.
Thanks Issac.


-- 
David Marchand

[PATCH v5 2/2] timer: allow platform to override cpu TSC frequency

[Isaac Boukris]

The CPU provided value is often not accurate, allow overriding it
based on info from the host OS.

On Linux X86, 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.

On freebsd we have access to the kernel tsc_hz value, just use it.

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       |  8 +++--
 lib/eal/linux/eal_timer.c         | 53 +++++++++++++++++++++++++++++--
 lib/eal/windows/eal_timer.c       |  5 ++-
 5 files changed, 62 insertions(+), 9 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..4eba66eadb 100644
--- a/lib/eal/freebsd/eal_timer.c
+++ b/lib/eal/freebsd/eal_timer.c
@@ -26,7 +26,7 @@
 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;
@@ -50,9 +50,13 @@ get_tsc_freq(void)
 	sz = sizeof(tsc_hz);
 	if (sysctlbyname("machdep.tsc_freq", &tsc_hz, &sz, NULL, 0)) {
 		EAL_LOG(WARNING, "%s", strerror(errno));
-		return 0;
+		return arch_hz;
 	}
 
+	if (arch_hz && RTE_MAX(arch_hz, tsc_hz) - RTE_MIN(arch_hz, tsc_hz) > arch_hz / 100)
+		EAL_LOG(WARNING, "Host tsc_freq %"PRIu64" at odds with cpu value %"PRIu64,
+			tsc_hz, arch_hz);
+
 	return tsc_hz;
 }
 
diff --git a/lib/eal/linux/eal_timer.c b/lib/eal/linux/eal_timer.c
index f56a7ae15b..489732c116 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,41 @@ 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)
+{
+	bool ret = true; /* Assume tsc_known_freq */
+
+#if defined(RTE_ARCH_X86)
+	char line[2048];
+	FILE *stream;
+
+	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);
+#endif
+
+	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 +232,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,11 +245,22 @@ 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 (RTE_MAX(arch_hz, tsc_hz) - RTE_MIN(arch_hz, tsc_hz) > arch_hz / 100)
+				return arch_hz;
+
+			EAL_LOG(DEBUG,
+				"Refined arch frequency %"PRIu64" to measured frequency %"PRIu64,
+				arch_hz, tsc_hz);
+		}
+
 		/* Round up to 100Khz. 1E5 ~ 100Khz */
 		return RTE_ALIGN_MUL_NEAR(tsc_hz, CYC_PER_100KHZ);
 	}
 #endif
-	return 0;
+	return arch_hz;
 }
 
 int
diff --git a/lib/eal/windows/eal_timer.c b/lib/eal/windows/eal_timer.c
index 4003541b08..020035c4cc 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 v5 2/2] timer: allow platform to override cpu TSC frequency

[Bruce Richardson]

On Thu, Oct 03, 2024 at 03:26:04PM +0300, Isaac Boukris wrote:
> The CPU provided value is often not accurate, allow overriding it
> based on info from the host OS.
> 
> On Linux X86, 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.
> 
> On freebsd we have access to the kernel tsc_hz value, just use it.
> 
> Signed-off-by: Isaac Boukris <iboukris@gmail.com>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [PATCH v5 2/2] timer: allow platform to override cpu TSC frequency

[Stephen Hemminger]

On Thu, 3 Oct 2024 15:06:06 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Thu, Oct 03, 2024 at 03:26:04PM +0300, Isaac Boukris wrote:
> > The CPU provided value is often not accurate, allow overriding it
> > based on info from the host OS.
> > 
> > On Linux X86, 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.
> > 
> > On freebsd we have access to the kernel tsc_hz value, just use it.
> > 
> > Signed-off-by: Isaac Boukris <iboukris@gmail.com>
> > ---  
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Acked-by: Stephen Hemminger <stephen@networkplumber.org>