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

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

On Wed, Feb 08, 2023 at 07:23:12PM +0000, Parav Pandit wrote:
> You are right. Two commands are there.
> Regardless the ambiguity remains
> âââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââââ
> From: Parav Pandit
> Sent: Wednesday, February 8, 2023 8:57 PM
> To: Michael S. Tsirkin <mst@redhat.com>
> Cc: Heng Qi <hengqi@linux.alibaba.com>; Xuan Zhuo <xuanzhuo@linux.alibaba.com>;
> virtio-comment @ lists . oasis-open . org
> <virtio-comment@lists.oasis-open.org>; virtio-dev @ lists . oasis-open . org
> <virtio-dev@lists.oasis-open.org>; Jason Wang <jasowang@redhat.com>; Alvaro
> Karsz <alvaro.karsz@solid-run.com>
> Subject: RE: [virtio-comment] [PATCH] virtio-net: support per-queue coalescing
> moderation
>  
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, February 8, 2023 10:20 AM
> 
> [..]
> 
> > >
> > > Most performant data path wouldn't implement and read the extra mode.
> > > It is always fw that is going to program same value, or per queue valued or
> > disable value in each Q regardless whichever way we craft the CVQ cmd.
> > >
> > > The sequence that bothers me is below.
> > > 1. driver set global params
> > > 2. few minutes later, now driver set param for Q=1
> > >
> > > On this command, a device need to decide:
> > > Should Q = 2 to N
> > > (a) either work with previous globals, or
> > > (b) because per Q was set for one queue, they rest of the queues implicitly
> > disable it.
> > >
> > > If it is (b),
> > > When a command on Q object =1 is issued, it affects other Q objects. <-
> This I
> > want to avoid.
> > > A cmd that modifies the object, should only modify that object.
> > >
> > > If it is (a), it is mixed mode operation, which is ambiguous definition.
> > >
> > > A better semantic is to define such change at device level and no extra
> cost in
> > the data path.
> >
> > Ugh. Looks like I didn't explain it well, yet again :(.
> > Here is my proposal in pseudo-code:
> >
> >
> > if (cmd == VQ_SET)
> >                vq[cmd.index].param = cmd.param;
> >
> > if (cmd == TX_SET)
> >        for (i = 0; ++i; i < maxvqn / 2)
> >                vq[i * 2].param = cmd.param;
> >
> > if (cmd == RX_SET)
> >        for (i = 0; ++i; i < maxvqn / 2)
> >                vq[i * 2 + 1].param = cmd.param;
> >
> >
> Currently we have cmd = GLOBAL_SET the code is:
> 
>  if (cmd == GLOBAL_SET)
>         for (i = 0; i < maxvqn; i++)
>                 vq[i].param = cmd.param;

exactly.

> >
> > there's nothing to decide at all. No modes. TX_SET and RX_SET affect half
> vqs,
> > VQ_SET affects one vq.
> >
> I explained above that, when cmd=GLOBAL_SET and after that if driver issues cmd
> =VQ_SET, at that point, the device is in partial mode.
> One VQ is running with its own param, and rest are in global mode.


Because there's no "global mode" - it is always per queue and each queue
uses a set of parameters, that is all. No ambiguity. You can quickly set
them all with a special command, or you can set them one by one.

For example consider a driver that loops over all vqs
and does RX_SET(0). Are we in global mode, per queue mode or disabled
mode? The questions is pointless is it not?

-- 
MST