Re: [PATCH] x86/tsc: Fix diagnostics for TSC frequency

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

Thanks,

~Andrew