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

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

[Michael S. Tsirkin]

On Thu, Mar 02, 2023 at 11:36:18AM +0000, David Edmondson wrote:
> > +                                        for an enabled transmit/receive virtqueue whose number is \field{vqn}.
> 
> Should this now be "whose index is \field{vqn}"?

Ugh.  I guess we'll have to fix the number/index mess in the spec
first. Parav you said you are looking into it?

-- 
MST


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/

[virtio-comment] RE: [PATCH v10] virtio-net: support the virtqueue coalescing moderation

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Thursday, March 2, 2023 6:35 PM
> > > +\begin{itemize}
> > > +\item For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and
> > > VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, \field{max_usecs}
> > > +      and \field{max_packets} are write-only for a driver.
> > > +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET,
> > > +\field{vqn},
> > > \field{reserved}, \field{max_usecs}
> > > +      and \field{max_packets} are write-only for a driver.
> > > +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET,
> \field{vqn}
> > > and \field{reserved} are write-only
> > > +      for a driver, and, \field{max_usecs} and \field{max_packets}
> > > + are read-only
> > > for the driver.
> > > +\end{itemize}
> > > +
> > Maybe I missed the conversation while I was sick.
> > I remember we discussed that instead of mentioning each individual field,
> better to describe the whole structure being read-only or write-only.
> 
> we can't, for GET part is RO part is WO.
> 
I am talking about the struct virtio_net_ctrl_coal.
This is RO for GET and WO for SET.
The text above can say struct virtio_net_ctrl_coal is RO or WO without dissecting its individual fields.

> > > +\begin{note}
> > > +The behavior of the device in response to these commands is best-effort:
> > > +the device may generate notifications more or less frequently than
> specified.
> > > +\end{note}
> > > +
> > Michael,
> > Why this should be a note? Shouldn't it be a normative line in the device
> requirements?
> 
> the second part with
> "may" should be upper case and used in a normative section, true.
> We can also have a note with the best-effort part, just avoid "may".
> 
Ok. In that case, just the normative line is good enough, as it avoids duplication.

Sorry for the late response, as returning back from travel and sickness.

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/

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

[Heng Qi]

在 2023/3/7 上午6:57, Michael S. Tsirkin 写道:
> On Thu, Mar 02, 2023 at 11:36:18AM +0000, David Edmondson wrote:
>>> +                                        for an enabled transmit/receive virtqueue whose number is \field{vqn}.
>> Should this now be "whose index is \field{vqn}"?
> Ugh.  I guess we'll have to fix the number/index mess in the spec
> first. Parav you said you are looking into it?



# Where virtqueue number and virtqueue index are used.
   1. In the Virtqueue Configuration Section, use the virtqueue index: 
"Write the **virtqueue index** (first queue is 0) to queue_select."
   2. Both descriptions are used separately in the Notification Section.
       2.1 Here vqn is called virtqueue index:
             "When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, 
the driver sends an available buffer notification
               to the device by writing the **16-bit virtqueue index** 
of this virtqueue to the Queue Notify address.
               ...

               le32 {
                      vqn: 16;
                      next_off : 15;
                      next_wrap: 1;
              };
              ...

              If VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the 
driver MUST use the queue_notify_data value instead of the **virtqueue 
index**."

       2.2 Here vqn is called virtqueue number:
             "When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, 
the Notification data contains the **Virtqueue number**.
             ...

             be32 {
                     vqn: 16;
                     next_off : 15;
                     next_wrap: 1;
              };
             ...

             When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, 
this notification involves sending the **virtqueue number** to the 
device (method depending on the transport).
             ...

             vqn -- **VQ number** to be notified."

# 0-based index and 0-based number are used respectively in the RSS Section:
1. "Field unclassified_queue contains the **0-based index** of the 
receive virtqueue to place unclassified packets in. Index 0 corresponds 
to receiveq1."
2. "use the result as the index in the indirection table to get 
**0-based number** of destination receiveq (value of 0 corresponds to 
receiveq1)."



\field{vqn} has been called '0-based virtqueue index' or '0-based 
virtqueue number',
I think both seem to be friendly to readers, so what are your options?

Thanks.

>


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/

RE: [virtio-comment] [PATCH v10] virtio-net: support the virtqueue coalescing moderation

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Monday, March 6, 2023 5:57 PM
> 
> On Thu, Mar 02, 2023 at 11:36:18AM +0000, David Edmondson wrote:
> > > +                                        for an enabled transmit/receive virtqueue whose
> number is \field{vqn}.
> >
> > Should this now be "whose index is \field{vqn}"?
> 
> Ugh.  I guess we'll have to fix the number/index mess in the spec first. Parav
> you said you are looking into it?

Yes, I want to send v1 for " Rename queue index to queue number" series.
V1 will include,
 
a. the vqn changes for net device rss and 
b. other ccw changes that Cornelia requested.
c. extra note that you asked to add to mmio for referring the old naming convention

I guess we agree that it should be vqn.
Therefore, vq coalescing and aq series should progress with vqn terminology.
This way all 3 series can progress in parallel (aq, vq coalescing, current spec cleanup).

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/

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

[Michael S. Tsirkin]

On Wed, Mar 08, 2023 at 04:42:25PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Monday, March 6, 2023 5:57 PM
> > 
> > On Thu, Mar 02, 2023 at 11:36:18AM +0000, David Edmondson wrote:
> > > > +                                        for an enabled transmit/receive virtqueue whose
> > number is \field{vqn}.
> > >
> > > Should this now be "whose index is \field{vqn}"?
> > 
> > Ugh.  I guess we'll have to fix the number/index mess in the spec first. Parav
> > you said you are looking into it?
> 
> Yes, I want to send v1 for " Rename queue index to queue number" series.
> V1 will include,
>  
> a. the vqn changes for net device rss and 
> b. other ccw changes that Cornelia requested.
> c. extra note that you asked to add to mmio for referring the old naming convention
> 
> I guess we agree that it should be vqn.
> Therefore, vq coalescing and aq series should progress with vqn terminology.
> This way all 3 series can progress in parallel (aq, vq coalescing, current spec cleanup).

Don't much care at this point but I think when I thought about this
deeply it seemed that yes, it is marginally better. Don't remember why
though ;)

-- 
MST


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/

[virtio-comment] RE: [virtio-dev] RE: [PATCH v10] virtio-net: support the virtqueue coalescing moderation

[Parav Pandit]

> From: virtio-dev@lists.oasis-open.org <virtio-dev@lists.oasis-open.org> On
> Behalf Of Heng Qi
> Sent: Thursday, March 2, 2023 10:27 PM
> 
> > I remember we discussed that instead of mentioning each individual field,
> better to describe the whole structure being read-only or write-only.
> 
> Consider the following scenarios:
> 1. A read-only field of the structure virtio_net_ctrl_coal is extended for
> CTRL_NOTF_COAL_RX/TX_SET to get some extra information 
A set command cannot extend the struct virtio_net_ctrl_coal, particularly for read-only and partially for write-only.
This would mean that for the tiny number of bytes, an additional dma descriptor is to be allocated with read/write-only permissions.
It would be inefficient for the driver to do so for the SET command to have vqn as write-only, reserved as read-only, rest fields as write-only dma attributes.

As I think more, I think the whole set command fields should be read-only for device. Better to describe it this way instead of splitting individual fields.
This way driver can just do a single DMA allocation with read-only attributes for set cmd.

Get command doesn’t have any choice today the way CVQ is structured to it lives with the limitation.

> > Looks good, however you have well covered in the device normative
> statements.
> > So possibly it can be removed from here.
> 
> I tend to keep this, as we have done in the past, we can have normative
> descriptions and the corresponding non-normative descriptions.
> 
Ok. but please revisit if the description can be simpler text than the normative lines.

Re: [virtio-comment] RE: [virtio-dev] RE: [PATCH v10] virtio-net: support the virtqueue coalescing moderation

[Heng Qi]

在 2023/3/9 上午6:30, Parav Pandit 写道:
>> From: virtio-dev@lists.oasis-open.org <virtio-dev@lists.oasis-open.org> On
>> Behalf Of Heng Qi
>> Sent: Thursday, March 2, 2023 10:27 PM
>>
>>> I remember we discussed that instead of mentioning each individual field,
>> better to describe the whole structure being read-only or write-only.
>>
>> Consider the following scenarios:
>> 1. A read-only field of the structure virtio_net_ctrl_coal is extended for
>> CTRL_NOTF_COAL_RX/TX_SET to get some extra information
> A set command cannot extend the struct virtio_net_ctrl_coal, particularly for read-only and partially for write-only.
> This would mean that for the tiny number of bytes, an additional dma descriptor is to be allocated with read/write-only permissions.
> It would be inefficient for the driver to do so for the SET command to have vqn as write-only, reserved as read-only, rest fields as write-only dma attributes.
>
> As I think more, I think the whole set command fields should be read-only for device. Better to describe it this way instead of splitting individual fields.
> This way driver can just do a single DMA allocation with read-only attributes for set cmd.
>
> Get command doesn’t have any choice today the way CVQ is structured to it lives with the limitation.

I think it is reasonable and will be revised in the next version.

>
>>> Looks good, however you have well covered in the device normative
>> statements.
>>> So possibly it can be removed from here.
>> I tend to keep this, as we have done in the past, we can have normative
>> descriptions and the corresponding non-normative descriptions.
>>
> Ok. but please revisit if the description can be simpler text than the normative lines.

Ok.

Thanks.



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/