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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.03.19 at 20:20, <olaf@xxxxxxxxx> wrote:
> To reiterate the second paragraph: if a domU uses TSC as primary clock
> source, it is expected that it runs NTP to cover for the resulting
> drift. Therefore this change does no need a knob to turn it on or off.

Did you omit a 't' or a 'w' above? Judging from the patch I think you
mean "not", but I don't see how this follows, especially with your
subsequent reply validly stating that such a requirement did not
exist with the XenoLinux kernels. And please don't forget that
Linux is not the only possible guest OS. What is or is not a
requirement inside the guest depends not only on Xen's behavior,
but also on the OS'es. Hence uniformly (and even by default)
changing the behavior for everyone is imo not acceptable.

> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -43,6 +43,9 @@ static char __initdata opt_clocksource[10];
>  string_param("clocksource", opt_clocksource);
>  
>  unsigned long __read_mostly cpu_khz;  /* CPU clock frequency in kHz. */
> +#define VTSC_NTP_PPM_TOLERANCE 500UL  /* Amount of drift NTP will handle */

As per above, "will" is too strong here: I think this wants to be "can", and
you want to add "if enabled in the guest".

> +#define VTSC_JITTER_RANGE_KHZ 200UL   /* Assumed jitter in cpu_khz */

I'm struggling to understand the comment: Surely not every single
CPU surfaces a jitter of precisely 200kHz?

> @@ -1885,6 +1888,27 @@ void __init early_time_init(void)
>      printk("Detected %lu.%03lu MHz processor.\n", 
>             cpu_khz / 1000, cpu_khz % 1000);
>  
> +    /*
> +     * How many kHz (in other words: drift) is ntpd in domU expected to 
> handle?

Same here - you can't expect anything. Even the name "ntpd" is already
going too far without something like "for example" attached to it. There
shouldn't be strict dependencies on guests only possibly be Linux based.

> +     *    freq    tolerated freq difference
> +     *  ------- = -------------------------
> +     *  Million         Million + PPM      
> +     */
> +    tmp = 1000 * 1000;
> +    tmp += VTSC_NTP_PPM_TOLERANCE;
> +    tmp *= cpu_khz;
> +    tmp /= 1000 * 1000;
> +
> +    tmp -= cpu_khz;
> +
> +    /*
> +     * Reduce the theoretical upper limit by the assumed measuring 
> inaccuracy.
> +     */
> +    if ( tmp >= VTSC_JITTER_RANGE_KHZ )
> +        tmp -= VTSC_JITTER_RANGE_KHZ;

This could suggest the constant is meant to be an upper bound, but
(as said in review of prior versions) subtracting introduce a non-
linearity, _and_ it doesn't guarantee the result to be within the
assumed bounds.

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

Could you remind me again how Dom0 remains unaffected here?
And if that's indeed the case, why would that be?

Jan



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