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

[Heng Qi <hengqi@linux.alibaba.com>]

On Sun, Feb 12, 2023 at 04:35:37AM -0500, Michael S. Tsirkin wrote:
> On Sat, Feb 11, 2023 at 01:47:16PM +0000, Parav Pandit wrote:
> > 
> > 
> > > From: Alvaro Karsz <alvaro.karsz@solid-run.com>
> > > Sent: Saturday, February 11, 2023 3:45 AM
> > > 
> > > > Please add short description something like,
> > > >
> > > > When the driver prefers to use per virtqueue notifications coalescing, and if
> > > queue group (transmit or receive) level notification coalescing is enabled, driver
> > > SHOULD first disable device level notification coalescing.
> > > > Or it should be,
> > > >
> > > 
> > > I disagree here.
> > > IMO "queue group level notification coalescing" is not something to enable or
> > > disable, but a shortcut to set all TX/RX queues at once.
> > That short cut is the enable/disablement.
> > 
> > > Why should the spec force a driver to "disable device level notification
> > > coalescing" (I assume you mean send a
> > > VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET command with zeros)?
> > Yes. Because to have well defined behavior when sw configured both one after the another.
> > 
> > > What if the driver sends a VIRTIO_NET_CTRL_NOTF_COAL_[T/R]X_SET
> > > command, and then a single queue traffic increases? why should it zero the
> > > parameters to all other queues?
> > That is short transition when driver is switching over to per queue mode.
> > This is fine to have short glitch.
> > 
> > > I think that this should be discussed in the driver implementation stage, not in
> > > the spec.
> > > 
> > There should be a clear guidance on how device should behave when both per q and per device are configured.
> > 
> > > > Virtqueue level notifications coalescing, and device level notifications can be
> > > enabled together.
> > > > When both of them are enabled, per virtqueue notifications coalescing take
> > > priority over queue group level.
> > > 
> > > How do you enable  Virtqueue level notifications coalescing? Why are they
> > > different entities?
> > Using the new command that has vqn in it.
> > 
> > > I don't think that we should have priorities, but the last command should be the
> > > one that dictates the coalescing parameters.
> > > 
> > Priority is applicable when driver has issued both the commands. Per tx/rx, and per vqn.
> > 
> > > For example, let's look at vq0 (RX):
> > > Device receives VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, vq0 should change
> > > the parameters accordingly (all RX vqs should do the same).
> > > Then device receives VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with vqn = 0,
> > > vq0 changes the parameters accordingly (all RX vqs are still using the "old"
> > > parameters) Then device receives VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, vq0
> > > changes the parameters accordingly (all RX vqs should do the same).
> > In this example, per VQ were overridden with per device.
> > Yes, so the last one is applicable, so priority of last one applies.
> > 
> > We continue to refuse to add the mode, and hence need to supply these description of both the sequence on how device should behave.
> > 
> > Sequence_1:
> > 1. tx/rx group level
> > 2. per vqn level
> > When #2 is done, VQ's whose per vq level is configured, follows vqn, rest of the VQs follow #1.
> > 
> > Sequence_2:
> > 1. per vqn
> > 2. tx/rx group level
> > When #2 is done, group level overrides the per vqn parameters. 
> 
> Since there's apparently some room for misunderstanding, I think adding these examples can't hurt.

Ok, I'll handle this.

> I would also be more specific and just use specific numbers in the
> example, to avoid any ambiguity.

Agree.

Thanks.

> 
> -- 
> MST