|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 3/6] x86/time: implement tsc as clocksource
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@xxxxxxxxxx>
> ---
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |