RE: [virtio-comment] [RFC PATCH v2] virtio-rtc: Add device specification.

[Parav Pandit <parav@nvidia.com>]

> From: Peter Hilber <peter.hilber@opensynergy.com>
> Sent: Thursday, October 12, 2023 4:26 PM

> > If I understand correctly, inv2 we donât have a way to dynamically hotplug and
> unplug the clocks of _one_ virtio-rtc device.
> 
> Hi Parav,
> 
> correct.
> 
> > So am I missing something?
> > In an arbitrary, interface, once unplug is successful, i.e. when the
> > device exactly know that unplug is completed by the driver, It can re-use the
> id.
> 
> But this would require the device to remember previously unplugged clocks.
> 
Not always, it can do when the clock wrap arounds.
Since this interface does not have the complexity of hotplug and unplug of the clocks, we should just keep it simple to a reasonable number of clocks.
The 64-bit value is not hurting at all, I just feel it is built for unknown.

Any reason to not do hot plug/unplug of clocks now?

> >
> > This would work with 8-bit too.
> > Not that I have issue with 64-bits, but the use case for large number of clocks
> under single virtio-rtc device is unclear.
> > And if there is no use case, we should have reasonable range.
> >
> 
> Using a large id range might help in the future to avoid dealing with the whole
> class of id ABA problems, cf. the above example.
In my view the clocks are expected to give precision and accuracy, so number of clocks in a system that occupies 64-bit wide sounds distant.
If it is used to not_care about finding the free id, it is ok.

Is hotplug unplug the clock (instead of device) is critical that drives the clock id?

> 
> To me using 64 bits to avoid the potential future ABA problems looks
> reasonable, but if others disagree, I can reduce to 8 bits.
> 
It is not must, I just find the 64-bit level clocks a bit too much specially at hotplug/unplug part.

> >> This and similar scenarios can be prevented by using 64 bit ids.
> >>
> > Lets first establish a use case if it exists.
> >
> 
> The benefit would be to avoid potential future id ABA problems, as per above.
> 
> >>
> >> I understand. I am just still concerned about the dual use of the
> >> spec for the reasons which I gave below.
> >>
> > It is not the dual use of the spec.
> > It is the draft that enables common functionality for using in two use cases.
> >
> >>
> >> I don't know how spec sharing would be judged by the TC. At the
> >> moment, no other devices do this.
> >>
> > This is the first time two use cases has such a common functionality that
> brings.
> > So it is fine to introduce.
> > TC will evaluate and give their feedback that we can incorporate further.
> > Since this reduces
> > a. dual spec
> > b. dual implementation
> >
> > and improves better device scaling, it is logical to cover both use cases.
> >
> >
> >>>> - a more complex spec impacting two devices
> >>>>
> >>> Well it is the functionality, so once you look for slightly larger
> >>> use cases, overall
> >> it is still less complexity than writing two different specs, one for
> >> net and one for rtc only device.
> >
> >
> >>>
> >>
> >> Complexity-wise it would be best to have one RTC spec, and link the
> >> RTC device to other devices through UUID if needed.
> >>
> > I am looking at larger use case than just rtc device.
> > Hence, total complexity of net device and rtc is counted in spec, and also in
> implementation.
> >
> > So it looks better to do one time.
> >
> >
> >
> >>>> - strong coupling between net and RTC driver implementation (or code
> >>>>   duplication)
> >>>>
> >>> Mostly no. it is the software layer. RTC layer operates over control VQ.
> >>> So net device will provide the needed API. Same API would be
> >>> available for
> >> dedicated rtc device lower layer too.
> >>> No problem.
> >>>
> >>
> >> But Virtio API will usually be time-proven, while drivers certainly
> >> have opportunities to get the custom API wrong.
> >>
> > I do not understand this much.
> > The bottom line is common functionality exists, driver maintainers can craft
> good APIs in their OS.
> > There are more than 13 drivers in one OS that I inspected, which has been
> peer reviewed.
> > So I wouldnât claim that they all got it wrong.
> >
> >>>> - using custom interfaces (between net and RTC driver), rather than
> generic
> >>>>   interfaces
> >>>>
> >>> Multi function devices are common in nature for functionality and scale.
> >>> And OSes supports it already.
> >>>
> >>
> >> Cf. above reply.
> >>
> >>>> - RTC could not use device configuration
> >>>>
> >>> In your proposal all things are over cvq already.
> >>>
> >>
> >> But it would prevent future use of the device configuration.
> >>
> > Not really. cvq has the commands to do the device config in your patch
> already.
> > Even for net device, implementing RW registers are hard.
> > The industry in various aspects are moving away from device specific
> configurations.
> >
> >>>> - unrecognized issues, or future generic changes to the Virtio standard,
> >>>>   may inhibit future development of the RTC device or the net
> >>>> device
> >>>>
> >>> Unlikely as the reviews eliminate such issues.
> >>>
> >>>> - unclear responsibility and timeline for net device related spec and
> >>>>   proof-of-concept
> >>>>
> >>> All the responsible people of the technical committee are here.
> >>> In fact the requirements are more closely tracked by its stake
> >>> owners for net
> >> compared to other devices.
> >>> So if you need help to find net side owner, let me know. Others and
> >>> I are
> >> interested to have it in the net side.
> >>>
> >>
> >> With "unclear responsibility" I meant to refer to the responsibility
> >> for drafting a spec extension for virtio-net,
> >
> > You can count me to draft the virtio-net spec for responsibility.
> > I am sure others would join as there is common interest across multiple device
> implementors to support it.
> > We know at least 4 stake holders have interests in it in virtio-net (Alibaba,
> google, marvell, nvidia).
> >
> 
> OK, thank you for the information.
> 
> I think the arguments on how to have the RTC spec interact with the net device
> are now on the table. Maybe the discussion could continue on a draft spec
> extension for the net device?
> 
I didnât follow the comment.
My suggestion is lets draft the spec _now_ that covers rtc functionality as part of virtio-net device and as independent rtc device.
And if that requires me to work and draft the net part for you, I am here to draft it.

Would it work for you?

> >>
> >>> I would prefer to first try to upstream the RTC spec without
> >>> altering the net spec, while trying to avoid conflicts with the net
> >>> spec. Maybe most of the RTC spec could then, as a second step, also
> >>> be applied to the net device. The responsibility for this second
> >>> step and the related proof of concept work would need to be
> >>> clarified. To me it still seems better to not share the spec and
> >>> address resource problems at the
> >> transport level.
> >>
> >>>>>>>> Maybe the above use case could also be addressed by a
> >>>>>>>> standalone RTC device as follows:
> >>>>>>>>
> >>>>>>>> 1) The net device exports a UUID object to which the RTC device
> >>>>>>>> can refer to (so the drivers can link the net device to the RTC clock).
> >>>>>>>>
> >>>>>>>> 2) If needed, the RTC device aligns the lifetime of the clock
> >>>>>>>> to the net
> >>>> device.
> >>>>>>>> Then, clock lifetimes are no longer static, and can be aligned
> >>>>>>>> with the net device. Through an eventq (to be added), the RTC
> >>>>>>>> device announces clock creation/destruction, which can be
> >>>>>>>> synchronized to the net
> >>>>>> device state.
> >>>>>>>>
> >>>>>>> All of the infrastructure, cross device synchronization, its
> >>>>>>> device side
> >>>>>> management will come with equal complexity.
> >>>>>>> And we will end up with sub-optimal way to using multiple PCI devices.
> >>>>>>>
> >>>>>>
> >>>>>> As per my above reply, I think it would be better to tackle the
> >>>>>> problem at the PCI transport level...
> >>>>>>
> >>>>>>> Net already has cvq required by RTC. So cvq commands can be
> common.
> >>>>>>> Readq is additional, we need to define common between the two
> >>>>>>> and way to
> >>>>>> discover it.
> >>>>>>>
> >>>>>>
> >>>>>>  I would still try to avoid mixing net and RTC details. RTC
> >>>>>> should keep a dedicated cvq.
> >>>>> Can you please explain why a dedicate cvq is needed on net device?
> >>>>
> >>>> I think the concern about redundant queues could be addressed by
> >>>> merging the RTC readq and controlq. At the moment separating these
> >>>> queues would not yield performance benefits, so I think I will just
> >>>> merge the readq and controlq RTC queues in the next spec iteration.
> >>>>
> >>> Nice. Ok. lets have single cvq and related opcode range for RTC
> >>> device and we
> >> will place them in the net side too.
> >>>
> >>
> >> OK. In the net device, this queue should still be distinct from the
> >> net controlq, which has a less predictable usage pattern than RTC control
> requests.
> >>
> > As you describe that with cross timestamping, the predictability is not
> important.
> > Or did I misunderstand you?
> 
> Cross-timestamping is very robust to response latency, if the driver timekeeping
> is good enough. When not using cross-timestamping, the response latency is still
> important.
> 
> The expected RTC control requests pattern is that the driver issues all of them
> during device probing. For the net device, it should not be assumed that the
> driver makes all net control requests during device probing.
> 
> So in some use cases sharing the net controlq with RTC non-control requests
> could still be problematic.
> 
Net can issue when it prefers to on the ctrlq.
Rtc can issue at driver probe time on the ctrlq.

> Plus, the name "controlq" isn't really appropriate for non-control clock reading
> requests, which the driver might issue several times per second.
Net has control q for query and set.
Statistics commands are added to the ctrlq as well.
It is single queue symmetric interface for get and set.

> Existing net controlq implementations might not match such a use case well
> (the Linux driver is busy waiting on controlq responses).
> 
Oh, that can change surely change for the new use cases. Should not be a problem.
I remember there were some patches too to improve that.

> Thank you for your comments!
> 
> Best regards,
> 
> Peter