xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Joao Martins <joao.m.martins@oracle.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Keir Fraser <keir@xen.org>, Jan Beulich <jbeulich@suse.com>,
	xen-devel@lists.xen.org
Subject: Re: [PATCH 2/5] x86/time: implement tsc as clocksource
Date: Tue, 22 Mar 2016 12:41:54 +0000	[thread overview]
Message-ID: <56F13D92.1030704@oracle.com> (raw)
In-Reply-To: <56EC6335.7010502@citrix.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 found out one issue in the tsc clocksource initialization path with respect to
the reliability check. This check is running when initializing platform timer,
but not all CPUS are up at that point (init_xen_time()) which means that the
check will always succeed. So for clocksource=tsc I need to defer initialization
to a later point (on verify_tsc_reliability()) and if successful switch to that
clocksource. Unless you disagree, v2 would have this and just requires one
additional preparatory change prior to this patch.

Joao

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

  parent reply	other threads:[~2016-03-22 12:41 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-17 16:12 [PATCH 0/5] x86/time: PVCLOCK_TSC_STABLE_BIT support Joao Martins
2016-03-17 16:12 ` [PATCH 1/5] public/xen.h: add flags field to vcpu_time_info Joao Martins
2016-03-18 20:12   ` Andrew Cooper
2016-03-21 11:42     ` Joao Martins
2016-03-21 11:43       ` Andrew Cooper
2016-03-21 11:51         ` Joao Martins
2016-03-21 15:10   ` Jan Beulich
2016-03-21 15:27     ` Andrew Cooper
2016-03-21 15:40       ` Joao Martins
2016-03-17 16:12 ` [PATCH 2/5] x86/time: implement tsc as clocksource Joao Martins
2016-03-18 20:21   ` Andrew Cooper
2016-03-21 11:43     ` Joao Martins
2016-03-22 12:41     ` Joao Martins [this message]
2016-03-22 12:46       ` Jan Beulich
2016-03-22 15:51         ` Joao Martins
2016-03-22 16:02           ` Jan Beulich
2016-03-22 20:40             ` Joao Martins
2016-03-23  7:28               ` Jan Beulich
2016-03-23 12:05                 ` Joao Martins
2016-03-23 14:05                   ` Jan Beulich
2016-03-17 16:12 ` [PATCH 3/5] x86/time: streamline platform time init on plt_init() Joao Martins
2016-03-18 20:32   ` Andrew Cooper
2016-03-21 11:45     ` Joao Martins
2016-03-17 16:12 ` [PATCH 4/5] x86/time: refactor read_platform_stime() Joao Martins
2016-03-18 20:34   ` Andrew Cooper
2016-03-21 11:45     ` Joao Martins
2016-03-21 13:08       ` Andrew Cooper
2016-03-17 16:12 ` [PATCH 5/5] x86/time: implement PVCLOCK_TSC_STABLE_BIT Joao Martins
2016-03-18 20:58   ` Andrew Cooper
2016-03-21 11:50     ` Joao Martins

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56F13D92.1030704@oracle.com \
    --to=joao.m.martins@oracle.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=jbeulich@suse.com \
    --cc=keir@xen.org \
    --cc=xen-devel@lists.xen.org \
    --subject='Re: [PATCH 2/5] x86/time: implement tsc as clocksource' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).