[dpdk-dev] [PATCH] Revert "timer: get TSC frequency from /proc/cpuinfo"

[dpdk-dev] [PATCH] Revert "timer: get TSC frequency from /proc/cpuinfo"

[Bruce Richardson]

This reverts commit da6fd0759cbeb5fc14991a79e40105b9f6b99059.

The use of cpuinfo to determine the frequency of the TSC is not
advisable and leads to incorrect results when power management is
in use. This is because, while the TSC frequency does not change
in modern cpus with constant_tsc support, the frequency of the core,
and hence the frequency of the core reported by cpuinfo *does* change.

Depending on the current frequency of core 0 when an application is
started, the EAL can get a wildly incorrect value for the TSC freq.
Since frequency is scaled down for power saving, any incorrect value
is likely to be lower than the default, which means that any delay
loops inside the code which rely on the TSC will be shorter than
planned. This can cause issues (reported on the mailing list by a number
of people) where ports are not initialized correctly due to delays being
too short.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_timer.c |   34 +-----------------------------
 1 files changed, 2 insertions(+), 32 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_timer.c b/lib/librte_eal/linuxapp/eal/eal_timer.c
index 1d10457..64566d1 100644
--- a/lib/librte_eal/linuxapp/eal/eal_timer.c
+++ b/lib/librte_eal/linuxapp/eal/eal_timer.c
@@ -279,35 +279,6 @@ check_tsc_flags(void)
 }
 
 static int
-set_tsc_freq_from_cpuinfo(void)
-{
-	char line[256];
-	FILE *stream;
-	double dmhz;
-
-	stream = fopen("/proc/cpuinfo", "r");
-	if (!stream) {
-		RTE_LOG(WARNING, EAL, "WARNING: Unable to open /proc/cpuinfo\n");
-		return -1;
-	}
-
-	while (fgets(line, sizeof line, stream)) {
-		if (sscanf(line, "cpu MHz\t: %lf", &dmhz) == 1) {
-			eal_tsc_resolution_hz = (uint64_t)(dmhz * 1000000UL);
-			break;
-		}
-	}
-
-	fclose(stream);
-
-	if (!eal_tsc_resolution_hz) {
-		RTE_LOG(WARNING, EAL, "WARNING: Cannot read CPU clock from cpuinfo\n");
-		return -1;
-	}
-	return 0;
-}
-
-static int
 set_tsc_freq_from_clock(void)
 {
 #ifdef CLOCK_MONOTONIC_RAW
@@ -355,9 +326,8 @@ set_tsc_freq_fallback(void)
 static void
 set_tsc_freq(void)
 {
-	if (set_tsc_freq_from_cpuinfo() < 0 &&
-	    set_tsc_freq_from_clock() < 0)
-	    set_tsc_freq_fallback();
+	if (set_tsc_freq_from_clock() < 0)
+		set_tsc_freq_fallback();
 
 	RTE_LOG(INFO, EAL, "TSC frequency is ~%"PRIu64" KHz\n",
 			eal_tsc_resolution_hz/1000);
-- 
1.7.0.7

Re: [dpdk-dev] [PATCH] Revert "timer: get TSC frequency from /proc/cpuinfo"

[Thomas Monjalon]

2014-04-07 13:57, Bruce Richardson:
> This reverts commit da6fd0759cbeb5fc14991a79e40105b9f6b99059.
> 
> The use of cpuinfo to determine the frequency of the TSC is not
> advisable and leads to incorrect results when power management is
> in use. This is because, while the TSC frequency does not change
> in modern cpus with constant_tsc support, the frequency of the core,
> and hence the frequency of the core reported by cpuinfo *does* change.
> 
> Depending on the current frequency of core 0 when an application is
> started, the EAL can get a wildly incorrect value for the TSC freq.
> Since frequency is scaled down for power saving, any incorrect value
> is likely to be lower than the default, which means that any delay
> loops inside the code which rely on the TSC will be shorter than
> planned. This can cause issues (reported on the mailing list by a number
> of people) where ports are not initialized correctly due to delays being
> too short.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>

I agree that using CPU frequency for TSC calibration is wrong with 
constant_tsc support. The goal was to have a precise calibration but the 
result is the reverse because of constant TSC.

Acked and applied.

Thank you Bruce for catching this.
-- 
Thomas