Re: [virtio-comment] [PATCH v12] virtio-net: support the virtqueue coalescing moderation

[Heng Qi <hengqi@linux.alibaba.com>]

在 2023/3/21 下午5:29, Cornelia Huck 写道:
> On Tue, Mar 21 2023, Heng Qi <hengqi@linux.alibaba.com> wrote:
>
>> 在 2023/3/21 上午12:35, Cornelia Huck 写道:
>>> On Sun, Mar 12 2023, Heng Qi <hengqi@linux.alibaba.com> wrote:
>>>>    \item \field{max_usecs} for RX: Maximum number of microseconds to delay a RX notification.
>>>>    \item \field{max_usecs} for TX: Maximum number of microseconds to delay a TX notification.
>>>>    \item \field{max_packets} for RX: Maximum number of packets to receive before a RX notification.
>>>>    \item \field{max_packets} for TX: Maximum number of packets to send before a TX notification.
>>>>    \end{itemize}
>>>>    
>>>> -The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
>>>> +\field{reserved} is reserved and it is ignored by a device.
>>> s/a/the/
>> I think "a" should be used for the first occurrence, and "the" is used
>> to refer to the one that has already appeared, am I correct :) ?
> IMHO, we are talking about a concrete device implementing this, so it
> should be "the" :) Not a native speaker, but "it is ignored by a device"
> sounds like it refers to a random device, and not the concrete one
> which will actually ignore it.

Combined with your explanation of "a/the" queue, I believe I understand! 
Thanks :)

>
>>>> +If coalescing parameters are being set, the device applies the last coalescing parameters set for a
>>>> +virtqueue, regardless of the command used to set the parameters. Use the following command sequence
>>>> +with 2 pairs of virtqueues as an example:
>>> s/2/two/
>> Yes, but I don't understand why this is important, it comes up elsewhere.
> I remember style guidance that you should spell out small numbers,
> instead of using digits. Not really important, but might be worth
> addressing if this needs a respin anyway.

Will fix.

>
>>>> +A driver MUST negotiate the VIRTIO_NET_F_VQ_NOTF_COAL feature before issuing commands VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET and VIRIO_NET_CTRL_NOTF_COAL_VQ_GET.
>>> s/VIRIO/VIRTIO/
>>>
>>> I'm not sure whether we should be expressing this via 'before' -- it's
>>> not like the driver can negotiate the feature bit if it realizes later
>>> that it wants to issue the command. IOW, I'd prefer the 'not negotiated'
>>> -> 'MUST NOT issue' construct, but I'm not dead set on it.
>> The previous version was a double negative, but I think that statement
>> seems clearer now.
>> Because we know that double negation may cause confusion:
>> when A does not happen, B must not happen, but when A happens, does it
>> mean that B can happen? Maybe B can't happen either.
> Maybe make this "MUST have negotiated (...) when issuing"?

Ok, I think it works.

>
>>>> +
>>>> +A driver MUST ignore the values of coalescing parameters received from the VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command if a device responds with VIRTIO_NET_ERR.
>>> Do we need to mandate here that the driver MUST NOT put anything else
>>> but the number of an enabled transmit or receive virtqueue into the vqn
>>> field?
>> Yes, you are right. I agree.
>>
>>> (Generally, I'd prefer "The driver" instead of "A driver" as well, but
>>> that might be a matter of taste.)
>>>
>>>>    
>>>>    \devicenormative{\subparagraph}{Notifications Coalescing}{Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing}
>>>>    
>>>> -A device SHOULD respond to the VIRTIO_NET_CTRL_NOTF_COAL commands with VIRTIO_NET_ERR if it was not able to change the parameters.
>>>> +A device MUST ignore \field{reserved}.
>>>> +
>>>> +A device SHOULD respond to VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands with VIRTIO_NET_ERR if it was not able to change the parameters.
>>>> +
>>>> +A device MUST respond to the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command with VIRTIO_NET_ERR if it was not able to change the parameters.
>>>> +
>>>> +A device MUST respond to VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET and VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET commands with VIRTIO_NET_ERR if the given virtqueue is disabled.
>>> s/given/designated/ ?
>> Ok.
>>
>>> What should the device do if the virtqueue is invalid (i.e. it is the
>>> control vq?) I think we either need to add a statement explicitly
>>> forbidding that to the driver conformance section, or specify that the
>>> device MUST return an error here.
>> Yes, I would prefer to let the driver do this.
> Ok, sounds good.
>
>>>> +
>>>> +Upon disabling and re-enabling the transmit virtqueue, the device MUST set the coalescing parameters of the virtqueue
>>>> +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command, or, if the driver did not set any TX coalescing parameters, to 0.
>>> "a transmit virtqueue" ?
>> I'm a bit confused, can you explain more? Because in the example below
>> you say "s/a driver/the driver".
>>
>>       +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} and \field{reserved} are write-only
>>       +      for a driver, and the structure virtio_net_ctrl_coal is read-only for the driver.
> It is one of the transmit virtqueues (i.e. a random one, not a concrete
> one) that is getting disabled and re-enabled, but then it is that
> concrete virtqueue that needs to be configured correctly.
>
> In the second example, we're talking about a concrete driver
> implementing this, so it should be "the".
>
> (Native or close-to-native speakers, please correct me if I'm wrong! I
> don't want to get into language lawyering, but I stumbled over this
> while reading.)
>
>>> There are a couple of typos and some style issues here which we could
>>> fix with an update on top, but I'd really like some clarity regarding
>>> invalid virtqueues first.
>> Yes, I think you are right. I see that the voting has already started,
>> do I need to wait for the voting to end and repost a new version based
>> on the results?
> If you plan to respin this, you can request to withdraw the vote, and we
> can restart voting on the updated version. If voting were to close with
> the change being approved, we'd need to include it and then vote on an
> incremental change on top. (I'd prefer to go with the first option;
> incremental changes on top are better suited to simple spelling fixes
> that don't need another round of voting.)

Ok, I got it.

May I bother you please to withdraw the vote, I will modify it according 
to your opinion and release a new version!:)

Thank you very much!



This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/