Re: [RFC PATCH v2] ptp: Add vDSO-style vmclock support

[Peter Hilber <peter.hilber@opensynergy.com>]

On 01.07.24 10:57, David Woodhouse wrote:
> On Fri, 2024-06-28 at 22:27 +0100, David Woodhouse wrote:
>> On 28 June 2024 17:38:15 BST, Peter Hilber <peter.hilber@opensynergy.com> wrote:
>>> On 28.06.24 14:15, David Woodhouse wrote:
>>>> On Fri, 2024-06-28 at 13:33 +0200, Peter Hilber wrote:
>>>>> On 27.06.24 16:52, David Woodhouse wrote:
>>>>>> I already added a flags field, so this might look something like:
>>>>>>
>>>>>>         /*
>>>>>>          * Smearing flags. The UTC clock exposed through this structure
>>>>>>          * is only ever true UTC, but a guest operating system may
>>>>>>          * choose to offer a monotonic smeared clock to its users. This
>>>>>>          * merely offers a hint about what kind of smearing to perform,
>>>>>>          * for consistency with systems in the nearby environment.
>>>>>>          */
>>>>>> #define VMCLOCK_FLAGS_SMEAR_UTC_SLS (1<<5) /* draft-kuhn-leapsecond-00.txt */
>>>>>>
>>>>>> (UTC-SLS is probably a bad example but are there formal definitions for
>>>>>> anything else?)
>>>>>
>>>>> I think it could also be more generic, like flags for linear smearing,
>>>>> cosine smearing(?), and smear_start_sec and smear_end_sec fields (relative
>>>>> to the leap second start). That could also represent UTC-SLS, and
>>>>> noon-to-noon, and it would be well-defined.
>>>>>
>>>>> This should reduce the likelihood that the guest doesn't know the smearing
>>>>> variant.
>>>>
>>>> I'm wary of making it too generic. That would seem to encourage a
>>>> *proliferation* of false "UTC-like" clocks.
>>>>
>>>> It's bad enough that we do smearing at all, let alone that we don't
>>>> have a single definition of how to do it.
>>>>
>>>> I made the smearing hint a full uint8_t instead of using bits in flags,
>>>> in the end. That gives us a full 255 ways of lying to users about what
>>>> the time is, so we're unlikely to run out. And it's easy enough to add
>>>> a new VMCLOCK_SMEARING_XXX type to the 'registry' for any new methods
>>>> that get invented.
>>>>
>>>>
>>>
>>> My concern is that the registry update may come after a driver has already
>>> been implemented, so that it may be hard to ensure that the smearing which
>>> has been chosen is actually implemented.
>>
>> Well yes, but why in the name of all that is holy would anyone want
>> to invent *new* ways to lie to users about the time? If we capture
>> the existing ones as we write this, surely it's a good thing that
>> there's a barrier to entry for adding more?
> 
> Ultimately though, this isn't the hill for me to die on. I'm pushing on
> that topic because I want to avoid the proliferation of *ambiguity*. If
> we have a precision clock, we should *know* what the time is.
> 
> So how about this proposal. I line up the fields in the proposed shared
> memory structure to match your virtio-rtc proposal, using 'subtype' as
> you proposed. But, instead of the 'subtype' being valid only for
> VIRTIO_RTC_CLOCK_UTC, we define a new top-level type for *smeared* UTC.
> 
> So, you have:
> 
> +\begin{lstlisting}
> +#define VIRTIO_RTC_CLOCK_UTC 0
> +#define VIRTIO_RTC_CLOCK_TAI 1
> +#define VIRTIO_RTC_CLOCK_MONO 2
> +\end{lstlisting}
> 
> I propose that you add
> 
> #define VIRTIO_RTC_CLOCK_SMEARED_UTC 3
> 
> If my proposed memory structure is subsumed into the virtio-rtc
> proposal we'd literally use the same names, but for the time being I'll
> update mine to:

Do you intend vmclock and virtio-rtc to be ABI compatible? FYI, I see a
potential problem in that Virtio does avoid the use of signed integers so
far. I did not check carefully if there might be other problems, yet.

> 
> 	/*
> 	 * What time is exposed in the time_sec/time_frac_sec fields?
> 	 */
> 	uint8_t time_type;
> #define VMCLOCK_TIME_UTC		0	/* Since 1970-01-01 00:00:00z */
> #define VMCLOCK_TIME_TAI		1	/* Since 1970-01-01 00:00:00z */
> #define VMCLOCK_TIME_MONOTONIC		2	/* Since undefined epoch */
> #define VMCLOCK_TIME_INVALID		3	/* virtio-rtc uses this for smeared UTC */
> 
> 
> I can then use your smearing subtype values as the 'hint' field in the
> shared memory structure. You currently have:
> 
> +\begin{lstlisting}
> +#define VIRTIO_RTC_SUBTYPE_STRICT 0
> +#define VIRTIO_RTC_SUBTYPE_SMEAR 1
> +#define VIRTIO_RTC_SUBTYPE_SMEAR_NOON_LINEAR 2
> +#define VIRTIO_RTC_SUBTYPE_LEAP_UNSPECIFIED 3
> +\end{lstlisting}
> 

I agree with the above part of your proposal.

> I can certainly ensure that 'noon linear' has the same value. I don't
> think you need both 'SMEAR' and 'LEAP_UNSPECIFIED' though:
> 
> 
> +\item VIRTIO_RTC_SUBTYPE_SMEAR deviates from the UTC standard by
> +	smearing time in the vicinity of the leap second, in a not
> +	precisely defined manner. This avoids clock steps due to UTC
> +	leap seconds.
> 
> ...
> 
> +\item VIRTIO_RTC_SUBTYPE_LEAP_UNSPECIFIED may deviate from the UTC
> +	standard w.r.t.\ leap second introduction in an unspecified
> way
> +	(leap seconds may, or may not, be smeared).
> 
> To the client, both of those just mean "for a day or so around a leap
> second event, you can't trust this device to know what the time is".
> There isn't any point in separating "does lie to you" from "might lie
> to you", surely? The guest can't do anything useful with that
> distinction. Let's drop SMEAR and keep only LEAP_UNSPECIFIED?

As for VIRTIO_RTC_SUBTYPE_SMEAR, I think this could be dropped indeed
(resp., UTC_SLS may be added).

But VIRTIO_RTC_CLOCK_SMEARED_UTC is an assurance that there will be no
steps (in particular, steps backwards, which some clients might not like)
due to leap seconds, while LEAP_UNSPECIFIED provides no such guarantee.

So I think this might be better handled by adding, alongside

> #define VIRTIO_RTC_CLOCK_SMEARED_UTC 3

#define VIRTIO_RTC_CLOCK_LEAP_UNSPECIFIED_UTC 4

(or any better name, like VIRTIO_RTC_CLOCK_MAYBE_SMEARED_UTC).

> 
> And if you *really* want to parameterise it, I think that's a bad idea
> and it encourages the proliferation of different time "standards", but
> I'd probably just suck it up and do whatever you do because that's not
> strictly within the remit of my live-migration part.

I think the above proposal to have subtypes for
VIRTIO_RTC_CLOCK_SMEARED_UTC should work.