Thread (63 messages) 63 messages, 9 authors, 2024-07-06

Re: [RFC PATCH v3 0/7] Add virtio_rtc module and related changes

From: Peter Hilber <hidden>
Date: 2024-03-13 17:51:02
Also in: linux-arm-kernel, linux-rtc, lkml, virtualization

On 13.03.24 13:45, David Woodhouse wrote:
On Wed, 2024-03-13 at 10:45 +0100, Peter Hilber wrote:
quoted
On 12.03.24 18:15, David Woodhouse wrote:
quoted
On Mon, 2024-03-11 at 19:24 +0100, Peter Hilber wrote:
quoted
On 08.03.24 13:33, David Woodhouse wrote:
quoted
On Fri, 2024-03-08 at 11:32 +0100, Peter Hilber wrote:
quoted
On 07.03.24 15:02, David Woodhouse wrote:
quoted
Hm, should we allow UTC? If you tell me the time in UTC, then
(sometimes) I still don't actually know what the time is, because some
UTC seconds occur twice. UTC only makes sense if you provide the TAI
offset, surely? Should the virtio_rtc specification make it mandatory
to provide such?

Otherwise you're just designing it to allow crappy hypervisors to
expose incomplete information.
Hi David,

(adding virtio-comment@lists.oasis-open.org for spec discussion),

thank you for your insightful comments. I think I take a broadly similar
view. The reason why the current spec and driver is like this is that I
took a pragmatic approach at first and only included features which work
out-of-the-box for the current Linux ecosystem.

The current virtio_rtc features work similar to ptp_kvm, and therefore
can work out-of-the-box with time sync daemons such as chrony.

As of RFC spec v3, UTC clock only is allowed. If mandating a TAI clock
as well, I am afraid that

- in some (embedded) scenarios, the TAI clock may not be available

- crappy hypervisors will pass off the UTC clock as the TAI clock.

For the same reasons, I am also not sure about adding a *mandatory* TAI
offset to each readout. I don't know user-space software which would
leverage this already (at least not through the PTP clock interface).
And why would such software not go straight for the TAI clock instead?

How about adding a requirement to the spec that the virtio-rtc device
SHOULD expose the TAI clock whenever it is available - would this
address your concerns?
I think that would be too easy for implementors to miss, or decide not
to obey. Or to get *wrong*, by exposing a TAI clock but actually
putting UTC in it.

I think I prefer to mandate the tai_offset field with the UTC clock.
Crappy implementations will just set it to zero, but at least that
gives a clear signal to the guests that it's *their* problem to
resolve.
To me there are some open questions regarding how this would work. Is there
a use case for this with the v3 clock reading methods, or would it be
enough to address this with the Virtio timekeeper?

Looking at clock_adjtime(2), the tai_offset could be exposed, but probably
best alongside some additional information about leap seconds. I am not
aware about any user-space user. In addition, leap second smearing should
also be addressed.
Is there even a standard yet for leap-smearing? Will it be linear over
1000 seconds like UTC-SLS? Or semi-raised-cosine over 24 hours, which I
think is what Google does? Meta does something different again, don't
they?

Exposing UTC as the only clock reference is bad enough; when leap
seconds happen there's a whole second during which you don't *know*
which second it is. It seems odd to me, for a precision clock to be
deliberately ambiguous about what the time is!
Just to be clear, the device can perfectly expose only a TAI reference
clock (or both UTC and TAI), the spec is just completely open about this,
as it tries to work for diverse use cases.
As long as the guest *knows* what it's getting, sure.
quoted
quoted
But if the virtio-rtc clock is defined as UTC and then expose something
*different* in it, that's even worse. You potentially end up providing
inaccurate time for a whole *day* leading up to the leap second.

I think you're right that leap second smearing should be addressed. At
the very least, by making it clear that the virtio-rtc clock which
advertises UTC shall be used *only* for UTC, never UTC-SLS or any other
yet-to-be-defined variant.
Agreed.
quoted
Please make it explicit that any hypervisor which wants to advertise a
smeared clock shall define a new type which specifies the precise
smearing algorithm and cannot be conflated with the one you're defining
here.
I will add a requirement that the UTC clock can never have smeared/smoothed
leap seconds.
Thanks.
quoted
I think that not every vendor would bother to first add a definition of a
smearing algorithm. Also, I think in some cases knowing the precise
smearing algorithm might not be important (when having the same time as the
hypervisor is enough and accuracy w.r.t. actual time is less important).

So maybe I should add a VIRTIO_RTC_CLOCK_UTC_SMEARED clock type, which for
now could catch every UTC-like clock which smears/smoothes leap seconds,
where the vendor cannot be bothered to add the smearing algorithm to spec
and implementations.
Please $DEITY no.

Surely the whole point of this effort is to provide guests with precise
and *unambiguous* knowledge of what the time is? 
I would say, a fundamental point of this effort is to enable such
implementations, and to detect if a device is promising to support this.

Where we might differ is as to whether the Virtio clock *for every
implementation* has to be *continuously* accurate w.r.t. a time standard,
or whether *for some implementations* it could be enough that all guests in
the local system have the same, precise local notion of time, which might
be off from the actual time standard.

Also, cf. ptp_kvm, which AFAIU doesn't address leap seconds at all...

With your described use case the UTC_SMEARED clock should of course not be
used. The UTC_SMEARED clock would get a distinct name through udev, like
/dev/ptp_virtio_utc_smeared, so the incompatibility could at least be
detected.
Using UTC is bad enough, because for a UTC timestamp in the middle of a
leap second the guest can't know know *which* occurrence of that leap
second it is, so it might be wrong by a second. To resolve that
ambiguity needs a leap indicator and/or tai_offset field.
I agree that virtio-rtc should communicate this. The question is, what
exactly, and for which clock read request?

As for PTP clocks:

- It doesn't fit into the ioctl PTP_SYS_OFFSET_PRECISE2.

- The clock_adjtime(2) tai_offset and return value could be set (if
  upstream will accept this). Would this help? As discussed, user space
  would need to interpret this (and currently no dynamic POSIX clock sets
  this).
But if you allow and encourage the use of smeared time without even a
specification of *how* it's smeared... that's even worse. You have an
unknown inaccuracy of up to a second for whole periods of time around a
leap second. That's surely the *antithesis* of what we're trying to do
here? Without an actual definition of the smearing, how is a guest
actually supposed to know what time it is?
As discussed above, I think in some use cases it is enough for the guest to
have a precise notion of time shared with the other guests.
(I suppose you could add a tai_offset_nanoseconds field? I don't know
that I want to *encourage* that thought process...)
quoted
As for UTC-SLS, this *could* also be added, although [1] says

        It is inappropriate to use Internet-Drafts as reference material or
        to cite them other than as "work in progress."

[1] https://datatracker.ietf.org/doc/html/draft-kuhn-leapsecond-00
quoted
quoted
quoted
One other thing to note is I think we're being very naïve about the TSC
on x86 hosts. Theoretically, the TSC for every vCPU might run at a
different frequency, and even if they run at the same frequency they
might be offset from each other. I'm happy to be naïve but I think we
should be *explicitly* so, and just say for example that it's defined
against vCPU0 so if other vCPUs are different then all bets are off.
ATM Virtio has no notion of vCPUs, or vCPU topology. So I wonder if you
have an opinion on how to represent this in a platform-independent way.
Well, it doesn't have a notion of TSCs either; you include that by
implicit reference don't you?
I think I can add a SHOULD requirement which vaguely refers to vCPU 0, or
boot vCPU. But the Virtio device is not necessarily hosted by a hypervisor,
so the device might not even know which vCPUs there are. E.g. there is even
interest to make virtio-rtc work as part of the virtio-net device (which
might be implemented in hardware).
Sure, but those implementations aren't going to offer the TSC pairing
at all, are they?
They could offer an Intel ART pairing (some physical PTP NICs are already
doing this, look for the convert_art_to_tsc() users).

Thanks for the comments,

Peter
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help