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

[Parav Pandit <parav@nvidia.com>]

> From: Peter Hilber <peter.hilber@opensynergy.com>
> Sent: Friday, October 13, 2023 9:00 PM
> 
> On 13.10.23 09:40, Parav Pandit wrote:
> >
> >> From: Peter Hilber <peter.hilber@opensynergy.com
> >
> >> Sent: Thursday, October 12, 2023 4:26 PM
> >
> >>> 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.
> 
> Sorry, I do not understand the reply. When the clock id is wrapping around, in
> the discussed scenario the device would need to know which clock had
> assigned id 0 in the past to avoid potential misinterpretation by the driver.
> 
> > 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?
> >
> 
> Current use cases do not require hotplugging, but I tried to make the spec
> future-proof.
>
If there is no use case for hotplugging a clock, we should keep things simple to a reasonable width.

 
> >>>
> >>> 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.
> >
> 
> Not caring about finding a concurrency-safe free id was the reason for 64 bits
> width.
> 
> > Is hotplug unplug the clock (instead of device) is critical that drives the clock
> id?
> >
> 
> Clock id width > 8 bit was only considered to be able to easily support
> hotplugging in the future.
> 
> >>
> >> 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.
> >
> 
> 
> >>>> 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?
> >
> 
> Of course, work on the net part could also start now.
> 
> I would prefer if the net part could be discussed as a dedicated patch, on top of
> the patch(es) introducing the RTC device (and referring to those).
>
It will be separate patch (or patches) in device-type/net/description.tex.
It should be part of same patch series as rtc and net so both devices agree and reserve necessary opcode ranges etc to utilize same infra.
 
> >>>>> 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.
> 
> 
> >>>>>> 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.
> >
> 
> OK, so I assume you still want to use the net controlq for all RTC requests.
>
Yes.