Re: [Xen-devel] [PATCH v11] tolerate jitter in cpu_khz calculation to avoid TSC emulation

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.12.18 at 16:20, <olaf@xxxxxxxxx> wrote:
> Improve decision when vTSC emulation will be activated for a domU with
> tsc_mode=default. The current approach is to compare the cpu_khz value
> from two physical hosts. Since this value is not accurate, it can not be
> used verbatim to decide if vTSC emulation needs to be enabled. Without
> this change each TSC access from domU will be emulated after migration,
> which causes a significant perfomance drop for workloads that make use
> of rdtsc.
> 
> If a domU uses TSC as clocksoure it also must run NTP in some way to
> avoid the potential drift what will most likely happen, independent of
> any migration.

Which drift? While anyone's well advised to run NTP, a completely
isolated set of systems may have no need to, if their interactions don't
depend on exactly matching time.

> The calculation of the drift is based on the time
> returned by remote servers versus how fast the local clock advances. NTP
> can handle a drift up to 500PPM. This means the local clocksource can
> run up to 500us slower or faster. This calculation is based on the TSC
> frequency of the host where the domU was started. Once a domU is
> migrated to a host of a different class, like from "2.3GHz" to "2.4GHz",
> the TSC frequency changes, but the domU kernel may not recalibrate
> itself.

That's why we switch to emulated (or hardware scaling) mode in that
case. It's anyway not really clear to me what this entire ...

> As a result, the drift will be larger and might be outside of
> the 500 PPM range. In addition, the kernel may notice the change of
> speed in which the TSC advances and could change the clocksource. All
> this depends of course on the type of OS that is running in the domU.

... (up to here) paragraph is supposed to tell the reader.

> @@ -1885,6 +1888,16 @@ void __init early_time_init(void)
>      printk("Detected %lu.%03lu MHz processor.\n", 
>             cpu_khz / 1000, cpu_khz % 1000);
>  
> +    tmp = 1000 * 1000;
> +    tmp += VTSC_NTP_PPM_TOLERANCE;
> +    tmp *= cpu_khz;
> +    tmp /= 1000 * 1000;
> +    tmp -= cpu_khz;
> +    if (tmp >= VTSC_JITTER_RANGE_KHZ)
> +        tmp -= VTSC_JITTER_RANGE_KHZ;

Besides the style issue in the if() - how can this be correct? This
clearly introduces a discontinuity (just consider the case where
tmp is exactly VTSC_JITTER_RANGE_KHZ before the if()). And
I also can't see how it guarantees the resulting value to be
below VTSC_JITTER_RANGE_KHZ. Did you perhaps mean to
cap the value (i.e. = instead of -= )?

> +    vtsc_tolerance_khz = (unsigned int)tmp;

Stray cast.

> +    printk("Tolerating vtsc jitter for domUs: %u kHz.\n", 
> vtsc_tolerance_khz);

Please omit the full stop; the printk() in context above shouldn't
have one either.

> @@ -2223,8 +2237,25 @@ void tsc_set_info(struct domain *d,
>           * When a guest is created, gtsc_khz is passed in as zero, making
>           * d->arch.tsc_khz == cpu_khz. Thus no need to check incarnation.
>           */
> +        disable_vtsc = d->arch.tsc_khz == cpu_khz;
> +
> +        if ( tsc_mode == TSC_MODE_DEFAULT && gtsc_khz && vtsc_tolerance_khz )
> +        {
> +            long khz_diff;
> +
> +            khz_diff = ABS((long)(cpu_khz - gtsc_khz));

I think

            khz_diff = ABS((long)cpu_khz - gtsc_khz);

or some such would be less fragile, if e.g. we decided to make
cpu_khz an unsigned int (which is an option, as I don't think
we'll see above 4THz systems any time soon).

> +            disable_vtsc = khz_diff <= vtsc_tolerance_khz;
> +
> +            printk(XENLOG_G_INFO "d%d: host has %lu kHz,"
> +                   " domU expects %u kHz,"
> +                   " difference of %ld is %s tolerance of %u\n",
> +                   d->domain_id, cpu_khz, gtsc_khz, khz_diff,
> +                   disable_vtsc ? "within" : "outside",
> +                   vtsc_tolerance_khz);
> +        }
> +
>          if ( tsc_mode == TSC_MODE_DEFAULT && host_tsc_is_safe() &&
> -             (d->arch.tsc_khz == cpu_khz ||
> +             (disable_vtsc ||
>                (is_hvm_domain(d) &&
>                 hvm_get_tsc_scaling_ratio(d->arch.tsc_khz))) )
>          {

In any event I don't follow why all of the sudden this becomes
an always-on mode, with not even a boot command line override.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel