[PATCH v2 0/3] x86/tsc: Remove some magic constants

[PATCH v2 0/3] x86/tsc: Remove some magic constants

[Petr Tesarik]

The TSC code is full of various magic constants, making it difficult to
understand the calculations. This patch series tries to document their
actual meaning.

No functional change intended, but the code is not bit-for-bit
identical; some 32-bit multiplications are converted into 64-bit
multiplications, because HZ_PER_KHZ is an unsigned long. If anything,
this should reduce the risk of arithmetic overflow.

Changes from v1:
- Use TICK_NSEC instead of (NSEC_PER_SEC / HZ)
- Use HZ_PER_KHZ and KHZ_PER_MHZ from <linux/units.h>

Petr Tesarik (3):
  x86/tsc: Replace magic constants with xxx_PER_SEC
  x86/tsc: Use HZ_PER_KHZ and KHZ_PER_MHZ
  x86/tsc: Use TSC_CALIBRATE_MULT for the TSC multiplier

 arch/x86/kernel/tsc.c | 54 +++++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 27 deletions(-)

-- 
2.55.0

[PATCH v2 1/3] x86/tsc: Replace magic constants with xxx_PER_SEC

[Petr Tesarik]

Make it clear that these constants refer to the conversion between seconds
and milliseconds or nanoseconds.

Use TICK_NSEC instead of (NSEC_PER_SEC / HZ). Although the former rounds
differently (to the nearest integer), it makes no actual difference for any
supported value of HZ (100, 250, 300, 1000).

No functional change intended.

Signed-off-by: Petr Tesarik <ptesarik@suse.com>
---
 arch/x86/kernel/tsc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 723347e2cf7f8..7adab8b614336 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -253,7 +253,7 @@ noinstr u64 native_sched_clock(void)
 	 */
 
 	/* No locking but a rare wrong value is not a big deal: */
-	return (jiffies_64 - INITIAL_JIFFIES) * (1000000000 / HZ);
+	return (jiffies_64 - INITIAL_JIFFIES) * TICK_NSEC;
 }
 
 /*
@@ -401,11 +401,11 @@ static unsigned long calc_pmtimer_ref(u64 deltatsc, u64 pm1, u64 pm2)
 }
 
 #define CAL_MS		10
-#define CAL_LATCH	(PIT_TICK_RATE / (1000 / CAL_MS))
+#define CAL_LATCH	(PIT_TICK_RATE / (MSEC_PER_SEC / CAL_MS))
 #define CAL_PIT_LOOPS	1000
 
 #define CAL2_MS		50
-#define CAL2_LATCH	(PIT_TICK_RATE / (1000 / CAL2_MS))
+#define CAL2_LATCH	(PIT_TICK_RATE / (MSEC_PER_SEC / CAL2_MS))
 #define CAL2_PIT_LOOPS	5000
 
 
@@ -552,7 +552,7 @@ static inline int pit_expect_msb(unsigned char val, u64 *tscp, unsigned long *de
  * more than 50ms on it.
  */
 #define MAX_QUICK_PIT_MS 50
-#define MAX_QUICK_PIT_ITERATIONS (MAX_QUICK_PIT_MS * PIT_TICK_RATE / 1000 / 256)
+#define MAX_QUICK_PIT_ITERATIONS (MAX_QUICK_PIT_MS * PIT_TICK_RATE / MSEC_PER_SEC / 256)
 
 static unsigned long quick_pit_calibrate(void)
 {
-- 
2.55.0

[PATCH v2 2/3] x86/tsc: Use HZ_PER_KHZ and KHZ_PER_MHZ

[Petr Tesarik]

The KHZ macro is ambiguous. Sometimes it is used for conversion between Hz
and KHz, and sometimes for conversion between KHz and MHz. Use two other
existing macros to distinguish between them.

Convert plain 1000 constants to one of the two macros where appropriate.

No functional change intended.

Signed-off-by: Petr Tesarik <ptesarik@suse.com>
---
 arch/x86/kernel/tsc.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 7adab8b614336..7e31acad2b055 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -16,6 +16,7 @@
 #include <linux/timex.h>
 #include <linux/static_key.h>
 #include <linux/static_call.h>
+#include <linux/units.h>
 
 #include <asm/cpuid/api.h>
 #include <asm/hpet.h>
@@ -41,8 +42,6 @@ EXPORT_SYMBOL(cpu_khz);
 unsigned int __read_mostly tsc_khz;
 EXPORT_SYMBOL(tsc_khz);
 
-#define KHZ	1000
-
 /*
  * TSC can be unstable due to cpufreq or due to unsynced TSCs
  */
@@ -393,7 +392,7 @@ static unsigned long calc_pmtimer_ref(u64 deltatsc, u64 pm1, u64 pm2)
 	if (pm2 < pm1)
 		pm2 += (u64)ACPI_PM_OVRRUN;
 	pm2 -= pm1;
-	tmp = pm2 * 1000000000LL;
+	tmp = pm2 * 1000000LL * HZ_PER_KHZ;
 	do_div(tmp, PMTMR_TICKS_PER_SEC);
 	do_div(deltatsc, tmp);
 
@@ -640,7 +639,7 @@ static unsigned long quick_pit_calibrate(void)
 	 * kHz = ((t2 - t1) * PIT_TICK_RATE) / (I * 256 * 1000)
 	 */
 	delta *= PIT_TICK_RATE;
-	do_div(delta, i*256*1000);
+	do_div(delta, i*256*HZ_PER_KHZ);
 	pr_info("Fast TSC calibration using PIT\n");
 	return delta;
 }
@@ -668,7 +667,7 @@ unsigned long native_calibrate_tsc(void)
 	if (ebx_numerator == 0 || eax_denominator == 0)
 		return 0;
 
-	crystal_khz = ecx_hz / 1000;
+	crystal_khz = ecx_hz / HZ_PER_KHZ;
 
 	/*
 	 * Denverton SoCs don't report crystal clock, and also don't support
@@ -696,7 +695,7 @@ unsigned long native_calibrate_tsc(void)
 		unsigned int eax_base_mhz, ebx, ecx, edx;
 
 		cpuid(CPUID_LEAF_FREQ, &eax_base_mhz, &ebx, &ecx, &edx);
-		crystal_khz = eax_base_mhz * 1000 *
+		crystal_khz = eax_base_mhz * KHZ_PER_MHZ *
 			eax_denominator / ebx_numerator;
 	}
 
@@ -717,7 +716,7 @@ unsigned long native_calibrate_tsc(void)
 	 * lapic_timer_period here to avoid having to calibrate the APIC
 	 * timer later.
 	 */
-	lapic_timer_period = crystal_khz * 1000 / HZ;
+	lapic_timer_period = crystal_khz * HZ_PER_KHZ / HZ;
 #endif
 
 	return crystal_khz * ebx_numerator / eax_denominator;
@@ -737,7 +736,7 @@ static unsigned long cpu_khz_from_cpuid(void)
 
 	cpuid(CPUID_LEAF_FREQ, &eax_base_mhz, &ebx_max_mhz, &ecx_bus_mhz, &edx);
 
-	return eax_base_mhz * 1000;
+	return eax_base_mhz * KHZ_PER_MHZ;
 }
 
 /*
@@ -1084,7 +1083,7 @@ static void __init detect_art(void)
 	cpuid(CPUID_LEAF_TSC, &art_base_clk.denominator,
 	      &art_base_clk.numerator, &art_base_clk.freq_khz, &unused);
 
-	art_base_clk.freq_khz /= KHZ;
+	art_base_clk.freq_khz /= HZ_PER_KHZ;
 	if (art_base_clk.denominator < ART_MIN_DENOMINATOR)
 		return;
 
@@ -1348,14 +1347,14 @@ static void tsc_refine_calibration_work(struct work_struct *work)
 		if (abs(tsc_khz - freq) > (tsc_khz >> 11)) {
 			pr_warn("Warning: TSC freq calibrated by CPUID/MSR differs from what is calibrated by HW timer, please check with vendor!!\n");
 			pr_info("Previous calibrated TSC freq:\t %lu.%03lu MHz\n",
-				(unsigned long)tsc_khz / 1000,
-				(unsigned long)tsc_khz % 1000);
+				(unsigned long)tsc_khz / KHZ_PER_MHZ,
+				(unsigned long)tsc_khz % KHZ_PER_MHZ);
 		}
 
 		pr_info("TSC freq recalibrated by [%s]:\t %lu.%03lu MHz\n",
 			hpet ? "HPET" : "PM_TIMER",
-			(unsigned long)freq / 1000,
-			(unsigned long)freq % 1000);
+			(unsigned long)freq / KHZ_PER_MHZ,
+			(unsigned long)freq % KHZ_PER_MHZ);
 
 		return;
 	}
@@ -1366,8 +1365,8 @@ static void tsc_refine_calibration_work(struct work_struct *work)
 
 	tsc_khz = freq;
 	pr_info("Refined TSC clocksource calibration: %lu.%03lu MHz\n",
-		(unsigned long)tsc_khz / 1000,
-		(unsigned long)tsc_khz % 1000);
+		(unsigned long)tsc_khz / KHZ_PER_MHZ,
+		(unsigned long)tsc_khz % KHZ_PER_MHZ);
 
 	clocksource_tsc.flags |= CLOCK_SOURCE_CALIBRATED;
 
@@ -1470,20 +1469,20 @@ static bool __init determine_cpu_tsc_frequencies(bool early)
 		return false;
 
 	pr_info("Detected %lu.%03lu MHz processor\n",
-		(unsigned long)cpu_khz / KHZ,
-		(unsigned long)cpu_khz % KHZ);
+		(unsigned long)cpu_khz / KHZ_PER_MHZ,
+		(unsigned long)cpu_khz % KHZ_PER_MHZ);
 
 	if (cpu_khz != tsc_khz) {
 		pr_info("Detected %lu.%03lu MHz TSC",
-			(unsigned long)tsc_khz / KHZ,
-			(unsigned long)tsc_khz % KHZ);
+			(unsigned long)tsc_khz / KHZ_PER_MHZ,
+			(unsigned long)tsc_khz % KHZ_PER_MHZ);
 	}
 	return true;
 }
 
 static unsigned long __init get_loops_per_jiffy(void)
 {
-	u64 lpj = (u64)tsc_khz * KHZ;
+	u64 lpj = (u64)tsc_khz * HZ_PER_KHZ;
 
 	do_div(lpj, HZ);
 	return lpj;
-- 
2.55.0

[PATCH v2 3/3] x86/tsc: Use TSC_CALIBRATE_MULT for the TSC multiplier

[Petr Tesarik]

The TSC delta is multiplied by one million to improve precision. This
multiplication is later undone in calc_hpet_ref() and calc_pmtimer_ref(),
making it difficult to understand the meaning of the divisor value.

Replace the magic constants with a more descriptive macro or calculation.

No functional change intended.

Signed-off-by: Petr Tesarik <ptesarik@suse.com>
---
 arch/x86/kernel/tsc.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 7e31acad2b055..b4815fdedf1b5 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -339,6 +339,7 @@ __setup("tsc=", tsc_setup);
 
 #define MAX_RETRIES		5
 #define TSC_DEFAULT_THRESHOLD	0x20000
+#define TSC_CALIBRATE_MULT	1000000LL
 
 /*
  * Read TSC and the reference counters. Take care of any disturbances
@@ -373,7 +374,7 @@ static unsigned long calc_hpet_ref(u64 deltatsc, u64 hpet1, u64 hpet2)
 		hpet2 += 0x100000000ULL;
 	hpet2 -= hpet1;
 	tmp = ((u64)hpet2 * hpet_readl(HPET_PERIOD));
-	do_div(tmp, 1000000);
+	do_div(tmp, FSEC_PER_SEC / HZ_PER_KHZ / TSC_CALIBRATE_MULT);
 	deltatsc = div64_u64(deltatsc, tmp);
 
 	return (unsigned long) deltatsc;
@@ -392,7 +393,7 @@ static unsigned long calc_pmtimer_ref(u64 deltatsc, u64 pm1, u64 pm2)
 	if (pm2 < pm1)
 		pm2 += (u64)ACPI_PM_OVRRUN;
 	pm2 -= pm1;
-	tmp = pm2 * 1000000LL * HZ_PER_KHZ;
+	tmp = pm2 * TSC_CALIBRATE_MULT * HZ_PER_KHZ;
 	do_div(tmp, PMTMR_TICKS_PER_SEC);
 	do_div(deltatsc, tmp);
 
@@ -806,7 +807,7 @@ static unsigned long pit_hpet_ptimer_calibrate_cpu(void)
 		if (tsc1 == ULLONG_MAX || tsc2 == ULLONG_MAX)
 			continue;
 
-		tsc2 = (tsc2 - tsc1) * 1000000LL;
+		tsc2 = (tsc2 - tsc1) * TSC_CALIBRATE_MULT;
 		if (hpet)
 			tsc2 = calc_hpet_ref(tsc2, ref1, ref2);
 		else
@@ -1334,7 +1335,7 @@ static void tsc_refine_calibration_work(struct work_struct *work)
 		goto restart;
 
 	delta = tsc_stop - tsc_start;
-	delta *= 1000000LL;
+	delta *= TSC_CALIBRATE_MULT;
 	if (hpet)
 		freq = calc_hpet_ref(delta, ref_start, ref_stop);
 	else
-- 
2.55.0