Re: [Xen-devel] [PATCH v2 6/6] x86/time: implement PVCLOCK_TSC_STABLE_BIT

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.03.16 at 15:44, <joao.m.martins@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -43,6 +43,10 @@
>  static char __initdata opt_clocksource[10];
>  string_param("clocksource", opt_clocksource);
>  
> +/* opt_nocpuhotplug: Set if CPU hotplug isn't meant to be used */
> +static bool_t __initdata opt_nocpuhotplug;
> +boolean_param("nocpuhotplug", opt_nocpuhotplug);

If we were to have such a new option, it would need to be
accompanied by an entry in the command line option doc. But
I'm opposed to this: For one, the variable being static here
means there is nothing that actually suppresses CPU hotplug
to happen. And then I think this can, for all practical purposes,
be had by suitably using existing command line options, namely
"max_cpus=", such that set_nr_cpu_ids() won't allow for any
further CPUs to get added. Albeit I admit that if someone was
to bring down some CPU and then hotplug another one, we
might still be in trouble. So maybe the better approach would
be to fail onlining of CPUs that don't meet the criteria when
"clocksource=tsc"?

> @@ -435,6 +439,7 @@ uint64_t ns_to_acpi_pm_tick(uint64_t ns)
>   * PLATFORM TIMER 4: TSC
>   */
>  static bool_t clocksource_is_tsc;
> +static bool_t use_tsc_stable_bit;

__read_mostly?

> @@ -468,6 +473,11 @@ static int __init init_tsctimer(struct 
> platform_timesource *pts)
>  
>      pts->frequency = tsc_freq;
>      clocksource_is_tsc = tsc_reliable;
> +    use_tsc_stable_bit = clocksource_is_tsc &&
> +        ((nr_cpu_ids == num_present_cpus()) || opt_nocpuhotplug);

See my remark above regarding the reliability of this.

> @@ -1431,6 +1443,22 @@ static void time_calibration_std_rendezvous(void *_r)
>      raise_softirq(TIME_CALIBRATE_SOFTIRQ);
>  }
>  
> +/*
> + * Rendezvous function used when clocksource is TSC and
> + * no CPU hotplug will be performed.
> + */
> +static void time_calibration_nop_rendezvous(void *_r)

Even if done so in existing code - no new local variable or function
parameters starting with an underscore please.

> +{
> +    struct cpu_calibration *c = &this_cpu(cpu_calibration);
> +    struct calibration_rendezvous *r = _r;

const

> +    c->local_tsc_stamp = r->master_tsc_stamp;
> +    c->stime_local_stamp = get_s_time();
> +    c->stime_master_stamp = r->master_stime;
> +
> +    raise_softirq(TIME_CALIBRATE_SOFTIRQ);
> +}

Perhaps this whole function should move up ahead of the other
two, so that they both can use this one instead of now duplicating
the same code a 3rd time? Or maybe a new helper function would
be better, to also account for the difference in what
c->local_tsc_stamp gets set from (which could then become a
parameter of that new function).

> @@ -1440,6 +1468,13 @@ static void time_calibration(void *unused)
>          .semaphore = ATOMIC_INIT(0)
>      };
>  
> +    if ( use_tsc_stable_bit )
> +    {
> +        local_irq_disable();
> +        r.master_stime = read_platform_stime(&r.master_tsc_stamp);
> +        local_irq_enable();
> +    }

So this can't be in time_calibration_nop_rendezvous() because
you want to avoid the actual rendezvousing. But isn't the then
possibly much larger gap between read_platform_stime() (which
parallels the rdtsc()-s in the other two cases) and get_s_time()
invocation going to become a problem?

Jan


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