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

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

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.

>>>
>>> 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).

>>>>> 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.

Thanks for the reply,

Peter