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

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

On Wed, Feb 08, 2023 at 03:04:03PM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, February 8, 2023 9:48 AM
> > 
> > On Wed, Feb 08, 2023 at 02:44:37PM +0000, Parav Pandit wrote:
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Wednesday, February 8, 2023 9:43 AM
> > > >
> > > > On Wed, Feb 08, 2023 at 02:37:55PM +0000, Parav Pandit wrote:
> > > > >
> > > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > > Sent: Wednesday, February 8, 2023 9:18 AM
> > > > > >
> > > > > > On Wed, Feb 08, 2023 at 07:30:34PM +0800, Heng Qi wrote:
> > > > > > > > I see two options.
> > > > > > > > 1. Just have per VQ params. Software has the full knowledge
> > > > > > > > of in which it is
> > > > > > operating, and state remains at software level.
> > > > > > > > This effectively achieves both the mode.
> > > > > > > >
> > > > > > > > 2. Have a mode cmd,
> > > > > > > > Mode = (a) per device or (b) per VQ (c) disable After the
> > > > > > > > mode is set, driver can set per device or per VQ.
> > > > > > >
> > > > > > > I find this more clear.
> > > > > > >
> > > > > > > Thanks.
> > > > > > >
> > > > > >
> > > > > > Rereading this I think I misunderstood the proposal.
> > > > > > Now we are burning memory on maintaining mode, and this
> > > > > > information is duplicated.
> > > > > >
> > > > > It is not maintained in the pci resident memory, so it doesn't hurt.
> > > > >
> > > > > > I'd say let's just add a new command COAL_QUEUE_SET with vqn as
> > > > parameter.
> > > > > > Existing commands are simply defined as a shortcut to running
> > > > > > COAL_QUEUE_SET on all tx/rx queues respectively.
> > > > > >
> > > > > > Latest command dictates the parameters. To disable just set
> > > > > > everything to 0 (btw we should make this explicit in the spec,
> > > > > > but it can be
> > > > guessed from:
> > > > > > Upon reset, a device MUST initialize all coalescing parameters to 0.
> > > > > > )
> > > > > >
> > > > > Switching between the modes (per q vs per device) implicitly is
> > > > > ambiguous
> > > > and it only means device may need to iterate.
> > > >
> > > > hmm i feel it's only ambiguous because i failed to explain in well.
> > > >
> > > > > This state is either better maintained in sw by always having per
> > > > > vq or have
> > > > clearly defined mode of what device should do.
> > > > >
> > > > > Per Q is very common even for several years old devices.
> > > > > Last time I counted, there were at least 15 such devices supporting it.
> > > > >
> > > > > So actual usage wise, I practically see that most implementations
> > > > > will end up
> > > > with per vq mode.
> > > > > I like to hear from Heng or Alvaro if they see any use of per device.
> > > >
> > > >
> > > > Right so given this, most devices will be in per queue mode all the
> > > > time. why do you want a mode then? just keep per queue.
> > > > existing commands are kept around for compat but internally just
> > > > translate to per-queue.
> > > Since the space is not released, do we need to keep the compat?
> > 
> > It's been accepted for half a year so we can't say for sure no one built this.
> That is likely but we should have the ability to have the Errata/ECN to correct it, specially for unrelease spec.

There's an errata process for sure.

> > The way I propose is just a bit of firmware on device that scans all queues and
> > copies same parameters everywhere. 
> This scanning loop in sw appears cheaper to me than some embedded fw.
> But is not a lot of concern.
> 
> > Seems easier than worrying about this,
> > and we get disabling coalescing for free which you wanted. With an extra mode
> > its extra logic in the device fast path. Maybe it's cheap on hardware side but in
> > software it's an extra branch, not free.
> 
> 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;



there's nothing to decide at all. No modes. TX_SET and RX_SET affect
half vqs, VQ_SET affects one vq.

-- 
MST