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

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

On 12/29/2015 03:11 PM, Andrew Cooper wrote:
> On 28/12/2015 16:59, 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 nothing that with clocksource=tsc there isn't no
> 
> I presume you mean "worth noting" and either "is no" or "isn't any"?
> 
Yeap.

> However, looking at the sentence below, do you mean "isn't any need to 
> synchronise with another clocksource" ?
That's correct.

Will fix the commit message with these on the next version.

> 
>> synchronization 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 used to observe ~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>
>> ---
>>   xen/arch/x86/time.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 61 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
>> index 30d52c4..c9e5c14 100644
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -433,6 +433,64 @@ uint64_t ns_to_acpi_pm_tick(uint64_t ns)
>>   }
>>   
>>   /************************************************************
>> + * PLATFORM TIMER 4: TSC
>> + */
>> +static bool_t clocksource_is_tsc = 0;
> 
> What does clocksource_is_tsc signify? It looks to be unused in this patch.
> 
Ah right, it's unused here which is wrong. It should only be added on patch 5 of
this series, which is where it gets used.

> ~Andrew
>