| Message ID | 20200805141804.2585-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/tsc: Fix diagnostics for TSC frequency | expand |
On 05.08.2020 16:18, Andrew Cooper wrote: > A Gemini Lake platform prints: > > (XEN) CPU0: TSC: 19200000MHz * 279 / 3 = 1785600000MHz > (XEN) CPU0: 800..1800 MHz > > during boot. The units on the first line are Hz, not MHz, so correct that and > add a space for clarity. > > Also, for the min/max line, use three dots instead of two and add more spaces > so that the line can't be mistaken for being a double decimal point typo. > > Boot now reads: > > (XEN) CPU0: TSC: 19200000 Hz * 279 / 3 = 1785600000 Hz > (XEN) CPU0: 800 ... 1800 MHz > > Extend these changes to the other TSC diagnostics. I'm happy to see the unit mistake fixed, but the choice of formatting was pretty deliberate when the code was introduced: As dense as possible without making things unreadable or ambiguous. (Considering "a double decimal point typo" looks like a joke to me, really.) > --- a/xen/arch/x86/cpu/intel.c > +++ b/xen/arch/x86/cpu/intel.c > @@ -396,14 +396,14 @@ static void intel_log_freq(const struct cpuinfo_x86 *c) > > val *= ebx; > do_div(val, eax); > - printk("CPU%u: TSC: %uMHz * %u / %u = %LuMHz\n", > + printk("CPU%u: TSC: %u Hz * %u / %u = %Lu Hz\n", > smp_processor_id(), ecx, ebx, eax, val); For this one I wonder whether ecx wouldn't better be scaled down to kHz, and val down to MHz. > } > else if ( ecx | eax | ebx ) > { > printk("CPU%u: TSC:", smp_processor_id()); > if ( ecx ) > - printk(" core: %uMHz", ecx); > + printk(" core: %u MHz", ecx); This one now clearly wants to say Hz too, or (as above) scaling down to kHz. With at least this last issue addressed Reviewed-by: Jan Beulich <jbeulich@suse.com> albeit I'd much prefer if the formatting adjustments were dropped. Jan
On 05/08/2020 15:54, Jan Beulich wrote: > On 05.08.2020 16:18, Andrew Cooper wrote: >> A Gemini Lake platform prints: >> >> (XEN) CPU0: TSC: 19200000MHz * 279 / 3 = 1785600000MHz >> (XEN) CPU0: 800..1800 MHz >> >> during boot. The units on the first line are Hz, not MHz, so correct that and >> add a space for clarity. >> >> Also, for the min/max line, use three dots instead of two and add more spaces >> so that the line can't be mistaken for being a double decimal point typo. >> >> Boot now reads: >> >> (XEN) CPU0: TSC: 19200000 Hz * 279 / 3 = 1785600000 Hz >> (XEN) CPU0: 800 ... 1800 MHz >> >> Extend these changes to the other TSC diagnostics. > I'm happy to see the unit mistake fixed, but the choice of > formatting was pretty deliberate when the code was introduced: > As dense as possible without making things unreadable or > ambiguous. (Considering "a double decimal point typo" looks > like a joke to me, really.) I literally thought it was a typo until I read the code. So no - I'm very much not joking. Decimal points are extremely commonly seen with frequencies, and nothing else in the log line gives any hint that it is range. Despite being deliberate, it is overly dense and ambiguous as a consequence. >> --- a/xen/arch/x86/cpu/intel.c >> +++ b/xen/arch/x86/cpu/intel.c >> @@ -396,14 +396,14 @@ static void intel_log_freq(const struct cpuinfo_x86 *c) >> >> val *= ebx; >> do_div(val, eax); >> - printk("CPU%u: TSC: %uMHz * %u / %u = %LuMHz\n", >> + printk("CPU%u: TSC: %u Hz * %u / %u = %Lu Hz\n", >> smp_processor_id(), ecx, ebx, eax, val); > For this one I wonder whether ecx wouldn't better be scaled down to > kHz, and val down to MHz. That depends on whether we will lose precision in the process. In principle we can, given ecx's unit of Hz, so I'd be tempted to leave it as is. > >> } >> else if ( ecx | eax | ebx ) >> { >> printk("CPU%u: TSC:", smp_processor_id()); >> if ( ecx ) >> - printk(" core: %uMHz", ecx); >> + printk(" core: %u MHz", ecx); > This one now clearly wants to say Hz too, or (as above) scaling > down to kHz. Oops. Will fix. > With at least this last issue addressed > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks, ~Andrew
On 05.08.2020 18:25, Andrew Cooper wrote: > On 05/08/2020 15:54, Jan Beulich wrote: >> On 05.08.2020 16:18, Andrew Cooper wrote: >>> --- a/xen/arch/x86/cpu/intel.c >>> +++ b/xen/arch/x86/cpu/intel.c >>> @@ -396,14 +396,14 @@ static void intel_log_freq(const struct cpuinfo_x86 *c) >>> >>> val *= ebx; >>> do_div(val, eax); >>> - printk("CPU%u: TSC: %uMHz * %u / %u = %LuMHz\n", >>> + printk("CPU%u: TSC: %u Hz * %u / %u = %Lu Hz\n", >>> smp_processor_id(), ecx, ebx, eax, val); >> For this one I wonder whether ecx wouldn't better be scaled down to >> kHz, and val down to MHz. > > That depends on whether we will lose precision in the process. I don't think losing the last three digits for the base clock and the last six ones of the calculated value would do any harm at all. All it would do (imo) is to make the numbers better readable (due less counting, and hence less possible counting mistakes). > In principle we can, given ecx's unit of Hz, so I'd be tempted to leave > it as is. Well, okay. Jan
diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c index 0cc6853c42..8bc51bec10 100644 --- a/xen/arch/x86/cpu/amd.c +++ b/xen/arch/x86/cpu/amd.c @@ -624,10 +624,10 @@ void amd_log_freq(const struct cpuinfo_x86 *c) if (idx && idx < h && !rdmsr_safe(0xC0010064 + idx, val) && (val >> 63) && !rdmsr_safe(0xC0010064, hi) && (hi >> 63)) - printk("CPU%u: %lu (%lu..%lu) MHz\n", + printk("CPU%u: %lu (%lu ... %lu) MHz\n", smp_processor_id(), FREQ(val), FREQ(lo), FREQ(hi)); else if (h && !rdmsr_safe(0xC0010064, hi) && (hi >> 63)) - printk("CPU%u: %lu..%lu MHz\n", + printk("CPU%u: %lu ... %lu MHz\n", smp_processor_id(), FREQ(lo), FREQ(hi)); else printk("CPU%u: %lu MHz\n", smp_processor_id(), FREQ(lo)); diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c index 69e99bb358..ed70b43942 100644 --- a/xen/arch/x86/cpu/intel.c +++ b/xen/arch/x86/cpu/intel.c @@ -396,14 +396,14 @@ static void intel_log_freq(const struct cpuinfo_x86 *c) val *= ebx; do_div(val, eax); - printk("CPU%u: TSC: %uMHz * %u / %u = %LuMHz\n", + printk("CPU%u: TSC: %u Hz * %u / %u = %Lu Hz\n", smp_processor_id(), ecx, ebx, eax, val); } else if ( ecx | eax | ebx ) { printk("CPU%u: TSC:", smp_processor_id()); if ( ecx ) - printk(" core: %uMHz", ecx); + printk(" core: %u MHz", ecx); if ( ebx && eax ) printk(" ratio: %u / %u", ebx, eax); printk("\n"); @@ -417,11 +417,11 @@ static void intel_log_freq(const struct cpuinfo_x86 *c) { printk("CPU%u:", smp_processor_id()); if ( ecx ) - printk(" bus: %uMHz", ecx); + printk(" bus: %u MHz", ecx); if ( eax ) - printk(" base: %uMHz", eax); + printk(" base: %u MHz", eax); if ( ebx ) - printk(" max: %uMHz", ebx); + printk(" max: %u MHz", ebx); printk("\n"); } } @@ -446,7 +446,7 @@ static void intel_log_freq(const struct cpuinfo_x86 *c) printk("CPU%u: ", smp_processor_id()); if ( min_ratio ) - printk("%u..", (factor * min_ratio + 50) / 100); + printk("%u ... ", (factor * min_ratio + 50) / 100); printk("%u MHz\n", (factor * max_ratio + 50) / 100); } }
A Gemini Lake platform prints: (XEN) CPU0: TSC: 19200000MHz * 279 / 3 = 1785600000MHz (XEN) CPU0: 800..1800 MHz during boot. The units on the first line are Hz, not MHz, so correct that and add a space for clarity. Also, for the min/max line, use three dots instead of two and add more spaces so that the line can't be mistaken for being a double decimal point typo. Boot now reads: (XEN) CPU0: TSC: 19200000 Hz * 279 / 3 = 1785600000 Hz (XEN) CPU0: 800 ... 1800 MHz Extend these changes to the other TSC diagnostics. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/cpu/amd.c | 4 ++-- xen/arch/x86/cpu/intel.c | 12 ++++++------ 2 files changed, 8 insertions(+), 8 deletions(-)