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

[Cornelia Huck <cohuck@redhat.com>]

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.

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

>>> +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"?

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