Re: [virtio-dev] [PATCH v14] virtio-net: support the virtqueue coalescing moderation

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Apr 04, 2023 at 08:50:43PM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, April 4, 2023 4:14 PM
> > 
> > On Tue, Apr 04, 2023 at 03:39:21PM -0400, Parav Pandit wrote:
> > >
> > >
> > > On 4/4/2023 1:44 PM, Michael S. Tsirkin wrote:
> > > > On Tue, Apr 04, 2023 at 04:32:07PM +0000, Parav Pandit wrote:
> > > > >
> > > > > > From: Halil Pasic <pasic@linux.ibm.com>
> > > > > > Sent: Tuesday, April 4, 2023 12:29 PM
> > > > > >
> > > > > > On Thu, 23 Mar 2023 23:24:22 +0800 Heng Qi
> > > > > > <hengqi@linux.alibaba.com> wrote:
> > > > > >
> > > > > > > +struct virtio_net_ctrl_coal_vq {
> > > > > > > +    le16 vqn;
> > > > > > > +    le16 reserved;
> > > > > > > +    struct virtio_net_ctrl_coal coal; };
> > > > > > > +
> > > > > > >   #define VIRTIO_NET_CTRL_NOTF_COAL 6
> > > > > > >    #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET  0
> > > > > > >    #define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1
> > > > > > > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 2 #define
> > > > > > > + VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 3
> > > > > > >   \end{lstlisting}
> > > > > > >
> > > > > > >   Coalescing parameters:
> > > > > > >   \begin{itemize}
> > > > > > > +\item \field{vqn}: The virtqueue number of an enabled
> > > > > > > +transmit or receive
> > > > > > virtqueue.
> > > > > >
> > > > > > Just to be on the safe side: VIRTIO_F_NOTIF_CONFIG_DATA has been
> > > > > > negotiated, and queue_select != queue_notify_data, is vqn
> > > > > > supposed to contain queue_notify_data or the number/index that
> > > > > > is used for queue_select (I'm talking about the PCI transport case)?
> > > > > Vqn has zero relation to notification config data feature and featue bit.
> > > > > It is the real vqn enabled via queue_select.
> > > > >
> > > > > Once the vqn is renamed to vq_notify_id, we won't have this confusion
> > anymore.
> > > >
> > > > vqn here is the index. queue_select is also the index.
> > > >
> > > Yes to both. No plan to rename them.
> > >
> > > > Inside notifications-le.c we have:
> > > > le32 {
> > > >          vqn : 16;
> > > >          next_off : 15;
> > > >          next_wrap : 1;
> > > > };
> > > >
> > > > vqn here is queue_notify_data.
> > > >
> > > vqn in above unnamed structure is contain a. either vq index if
> > > CONFIG_DATA is not negotiated or b. it contains queue_notifiy_data if
> > > CONFIG_DATA is negotiated
> > >
> > > Therefore, instead of naming it as vqn, renaming it to vq_notify_id
> > > crisply describe what it is for.
> > >
> > > And not some vqn n stands for notification, but "d" of data is dropped
> > > somehow.
> > 
> > vqnd then? for virt queue notification data?
> > 
> "id" identifies the queue as opposed to "data" identifying a queue.
> > 
> > > A notification identifier contains depending on negotiated feature bit.
> > 
> > it's not necessarily an identifier. can be e.g. just 0 for all vqs.
> > whatever the device needs.
> For driver its just an id, content doesn't matter.

No, this value might or might not be somehow related to the vq but it
does not have to identify it. So it's some data, not an id.  Let's try
to use words for what they mean in english not try to come up with our
own language.

-- 
MST