Re: [PATCH v2 3/6] x86/time: implement tsc as clocksource

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Mar 29, 2016 at 02:44:08PM +0100, 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 only set when adminstrator
> changes "clocksource" boot option to "tsc". Initializing TSC
> clocksource requires all CPUs to have the tsc reliability checks
> performed. init_xen_time is called before all CPUs are up, and so we
> start with HPET at boot time, and switch later to TSC. The switch then
> happens on verify_tsc_reliability initcall that is invoked when all
> CPUs are up. When attempting to initializing TSC 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 keep the clocksource that was previously initialized on
> init_xen_time.
> 
> 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 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>
> ---
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Changes since v1:
>  - s/printk/printk(XENLOG_INFO
>  - Remove extra space on inner brackets
>  - Add missing space around brackets
>  - Defer clocksource TSC initialization when all CPUs are up.
> 
> 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 | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 95 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index ed4ed24..2602dda 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;

No need to set it to zero.
> +
> +    tsc_check_reliability();

This has been already called by verify_tsc_reliability which calls this
function. Should we set tsc_max_warp to zero before calling it?

> +
> +    if ( tsc_max_warp > 0 )
> +    {
> +        tsc_reliable = 0;

Ditto. It is by default zero.

> +        printk(XENLOG_INFO "TSC: didn't passed warp test\n");

So the earlier test by verify_tsc_reliability did already this check and
printed this out - and also cleared the X86_FEATURE_TSC_RELIABLE.

So can this check above be removed then?

Or are you thinking to ditch what verify_tsc_reliability does?

> +    }
> +    else if ( boot_cpu_has(X86_FEATURE_TSC_RELIABLE) ||
> +              (boot_cpu_has(X86_FEATURE_CONSTANT_TSC) &&
> +               boot_cpu_has(X86_FEATURE_NONSTOP_TSC)) )
> +    {
> +        tsc_reliable = 1;
> +    }
> +    else if ( boot_cpu_has(X86_FEATURE_CONSTANT_TSC) )
> +    {
> +        tsc_reliable = (max_cstate <= 2);
> +
> +        if ( tsc_reliable )
> +            printk(XENLOG_INFO "TSC: no deep Cstates, deemed reliable\n");
> +        else
> +            printk(XENLOG_INFO "TSC: deep Cstates possible, so not reliable\n");
> +    }
> +
> +    pts->frequency = tsc_freq;
> +    return tsc_reliable;
> +}
> +
> +static u64 read_tsc(void)
> +{
> +    return rdtsc();
> +}
> +
> +static void resume_tsctimer(struct platform_timesource *pts)
> +{
> +}
> +
> +static struct platform_timesource __initdata plt_tsc =
> +{
> +    .id = "tsc",
> +    .name = "TSC",
> +    .read_counter = read_tsc,
> +    .counter_bits = 64,
> +    .init = init_tsctimer,

Could you add a note in here:

/* Not called by init_platform_timer as it is not on the plt_timers array. */

> +    .resume = resume_tsctimer,
> +};
> +
> +/************************************************************
>   * GENERIC PLATFORM TIMER INFRASTRUCTURE
>   */
>  
> @@ -533,6 +590,21 @@ static void resume_platform_timer(void)
>      plt_stamp = plt_src.read_counter();
>  }
>  
> +static void __init reset_platform_timer(void)
> +{
> +    /* Deactivate any timers running */
> +    kill_timer(&plt_overflow_timer);
> +    kill_timer(&calibration_timer);
> +
> +    /* Reset counters and stamps */
> +    spin_lock_irq(&platform_timer_lock);
> +    plt_stamp = 0;
> +    plt_stamp64 = 0;
> +    platform_timer_stamp = 0;
> +    stime_platform_stamp = 0;
> +    spin_unlock_irq(&platform_timer_lock);
> +}
> +
>  static int __init try_platform_timer(struct platform_timesource *pts)
>  {
>      int rc = -1;
> @@ -541,6 +613,10 @@ static int __init try_platform_timer(struct platform_timesource *pts)
>      if ( rc <= 0 )
>          return rc;
>  
> +    /* We have a platform timesource already so reset it */
> +    if ( plt_src.counter_bits != 0 )
> +        reset_platform_timer();
> +
>      plt_mask = (u64)~0ull >> (64 - pts->counter_bits);
>  
>      set_time_scale(&plt_scale, pts->frequency);
> @@ -566,7 +642,9 @@ static void __init init_platform_timer(void)
>      struct platform_timesource *pts = NULL;
>      int i, rc = -1;
>  
> -    if ( opt_clocksource[0] != '\0' )
> +    /* clocksource=tsc is initialized later when all CPUS are up */

Perhaps:
/* clocksource=tsc is initialized via __initcalls (when CPUs are up). */ ?

> +    if ( (opt_clocksource[0] != '\0') &&
> +         (strcmp(opt_clocksource, "tsc") != 0) )
>      {
>          for ( i = 0; i < ARRAY_SIZE(plt_timers); i++ )
>          {
> @@ -1192,7 +1270,7 @@ static void check_tsc_warp(unsigned long tsc_khz, unsigned long *max_warp)
>      }
>  }
>  
> -static unsigned long tsc_max_warp, tsc_check_count;
> +static unsigned long tsc_check_count;
>  static cpumask_t tsc_check_cpumask;
>  
>  static void tsc_check_slave(void *unused)
> @@ -1437,6 +1515,20 @@ static int __init verify_tsc_reliability(void)
>          }
>      }
>  
> +    if ( !strcmp(opt_clocksource, "tsc") )
> +    {
> +        if ( try_platform_timer(&plt_tsc) > 0 )
> +        {
> +            printk(XENLOG_INFO "Switched to Platform timer %s TSC\n",
> +                   freq_string(plt_src.frequency));
> +
> +            init_percpu_time();
> +
> +            init_timer(&calibration_timer, time_calibration, NULL, 0);
> +            set_timer(&calibration_timer, NOW() + EPOCH);
> +        }
> +    }
> +
>      return 0;
>  }
>  __initcall(verify_tsc_reliability);
> @@ -1476,6 +1568,7 @@ void __init early_time_init(void)
>      struct cpu_time *t = &this_cpu(cpu_time);
>      u64 tmp = init_pit_and_calibrate_tsc();
>  
> +    tsc_freq = tmp;
>      set_time_scale(&t->tsc_scale, tmp);
>      t->local_tsc_stamp = boot_tsc_stamp;
>  
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

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