Re: [PATCH 2/5] x86/time: implement tsc as clocksource

[Joao Martins <joao.m.martins@oracle.com>]

On 03/18/2016 08:21 PM, Andrew Cooper wrote:
> On 17/03/16 16:12, Joao Martins wrote:
>> Introduce support for using TSC as platform time which is the highest
>> resolution time and most performant to get (~20 nsecs).  Though there
>> are also several problems associated with its usage, and there isn't a
>> complete (and architecturally defined) guarantee that all machines
>> will provide reliable and monotonic TSC across all CPUs, on different
>> sockets and different P/C states.  I believe Intel to be the only that
>> can guarantee that. For this reason it's set with less priority when
>> compared to HPET unless adminstrator changes "clocksource" boot option
>> to "tsc". Upon initializing it, we also check for time warps and
>> appropriate CPU features i.e.  TSC_RELIABLE, CONSTANT_TSC and
>> NONSTOP_TSC. And in case none of these conditions are met, we fail in
>> order to fallback to the next available clocksource.
>>
>> It is also worth noting that with clocksource=tsc there isn't any
>> need to synchronise with another clocksource, and I could verify that
>> great portion the time skew was eliminated and seeing much less time
>> warps happening. With HPET I observed ~500 warps in the period
>> of 1h of around 27 us, and with TSC down to 50 warps in the same
>> period having each warp < 100 ns. The warps still exist though but
>> are only related to cross CPU calibration (being how much it takes to
>> rendezvous with master), in which a later patch in this series aims to
>> solve.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>> Cc: Keir Fraser <keir@xen.org>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Some style corrections, but no functional problems.
> 
> Reviewed-by Andrew Cooper <andrew.cooper3@citrix.com>
> 
I've fixed all your comments for v2, Thanks!

>>
>> Changes since RFC:
>>  - Spelling fixes in the commit message.
>>  - Remove unused clocksource_is_tsc variable and introduce it instead
>>  on the patch that uses it.
>>  - Move plt_tsc from second to last in the available clocksources.
>> ---
>>  xen/arch/x86/time.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 60 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
>> index 687e39b..1311c58 100644
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -432,6 +432,63 @@ uint64_t ns_to_acpi_pm_tick(uint64_t ns)
>>  }
>>  
>>  /************************************************************
>> + * PLATFORM TIMER 4: TSC
>> + */
>> +static u64 tsc_freq;
>> +static unsigned long tsc_max_warp;
>> +static void tsc_check_reliability(void);
>> +
>> +static int __init init_tsctimer(struct platform_timesource *pts)
>> +{
>> +    bool_t tsc_reliable = 0;
>> +
>> +    tsc_check_reliability();
>> +
>> +    if ( tsc_max_warp > 0 )
>> +    {
>> +        tsc_reliable = 0;
>> +        printk("TSC: didn't passed warp test\n");
> 
> printk(XENLOG_INFO "TSC ...
> 
>> +    }
>> +    else if ( boot_cpu_has(X86_FEATURE_TSC_RELIABLE) ||
>> +              ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
>> +                boot_cpu_has(X86_FEATURE_NONSTOP_TSC) ) )
> 
> You don't need extra spaces on inner brackets, so
> 
> boot_cpu_has(X86_FEATURE_TSC_RELIABLE) ||
> (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
>  boot_cpu_has(X86_FEATURE_NONSTOP_TSC)) )
> 
> is fine.
> 
>> +    {
>> +        tsc_reliable = 1;
>> +    }
>> +    else if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) )
>> +    {
>> +        tsc_reliable = (max_cstate <= 2);
>> +
>> +        if (tsc_reliable)
> 
> Please add some extra ones here though.
> 
>> +            printk("TSC: no deep Cstates, deemed reliable\n");
>> +        else
>> +            printk("TSC: deep Cstates possible, so not reliable\n");
> 
> XENLOG_INFO as well please.
> 
> ~Andrew
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel