Re: [virtio-comment] Re: [PATCH] virtio-net: support per-queue coalescing moderation

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

On Wed, Feb 08, 2023 at 10:20:08AM +0800, Heng Qi wrote:
> 
> 
> å 2023/2/7 äå10:29, Michael S. Tsirkin åé:
> > On Tue, Feb 07, 2023 at 12:51:26PM +0000, Parav Pandit wrote:
> > > > From: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > Sent: Tuesday, February 7, 2023 6:50 AM
> > > > 
> > > > On Tue, 7 Feb 2023 13:25:13 +0200, Alvaro Karsz <alvaro.karsz@solid-run.com>
> > > > wrote:
> > > > > Hi Heng,
> > > > > 
> > > > > > Currently, the coalescing profile is directly applied to all queues.
> > > > > > This patch supports configuring the parameters for a specified queue.
> > > > > > 
> > > > > > When the traffic between queues is unbalanced, for example, one
> > > > > > queue is busy and another queue is idle, then it will be very useful
> > > > > > to control coalescing parameters at the queue granularity.
> > > > > > 
> > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > > ---
> > > > > >   content.tex | 49 ++++++++++++++++++++++++++++++++++++++++++-------
> > > > > >   1 file changed, 42 insertions(+), 7 deletions(-)
> > > > > > 
> > > > > > diff --git a/content.tex b/content.tex index e863709..049c0e4 100644
> > > > > > --- a/content.tex
> > > > > > +++ b/content.tex
> > > > > > @@ -3084,6 +3084,9 @@ \subsection{Feature bits}\label{sec:Device
> > > > > > Types / Network Device / Feature bits
> > > > \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
> > > > > >       channel.
> > > > > > 
> > > > > > +\item[VIRTIO_NET_F_PERQUEUE_NOTF_COAL(52)] Device supports per-
> > > > queue
> > > > > > +       notifications coalescing.
> > > > > > +
> > > > > Since this feature allows us to change the coalescing parameters for
> > > > > all the queues when rx/tx_qid = 0xFFFF, and since version 1.3 wasn't
> > > > > released yet, maybe the "per-vq" functionality can be added to
> > > > > VIRTIO_NET_F_NOTF_COAL instead of adding a new feature?
> > > > 
> > > > According to my understanding, all the features of voting are formal. It can be
> > > > used by the manufacturer.
> > > > 
> > > > Of course, as far as I know, no manufacturer has used this feature for the time
> > > > being. But I think we should add a new feature.
> > > > 
> > > > Or other people have other ideas.
> > > I believe we should treat it as fix and avoid a new feature bit as spec is not released, and it is very recent change.
> > Arguably it's true.
> > It will all be up to the committee vote of course ...
> > To keep things a bit safer how about we just allow commands
> > without qid instead of a special qid value?
> 
> Good idea, the new command seems to make compatibility easier to achieve.
> An error can be returned when the old device does not recognize the new
> command.

Not that is a bad way to figure out what is supported. E.g. driver
can't reasonably probe a ton of commands just to check there's
compatibility for migration. If it's optional pls gate by feature
bit or some such.

> > Also if we are going to change things how about adding a query command too?
> 
> Yes, we should.
> 
> Thanks.
> 
> > 
> > Also Alvaro what is your take on whether the gloabal version counts
> > packets and time for all queues together or per queue? The spec
> > does not make it clear ATM.
> >