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

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

On Fri, Feb 17, 2023 at 01:17:50PM +0200, Alvaro Karsz wrote:
> > > Yes, max_packets and max_usecs SHOULD be reset to 0.
> > > When the virtqueue is re-enabled, it means that notification conditions are met after each packet is sent/received.
> > >
> > > As described by Alvaro in "[PATCH v5] virtio-net: Fix and update VIRTIO_NET_F_NOTF_COAL feature":
> > > "+When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the notification conditions are met after every packet received/sent."
> >
> > Oh. Hmm.
> >
> > What Alvaro's patch does not describe is what happens when VQ is reset.
> >
> > Alvaro you said you have hardware implementing this right?
> > How does the command interact with vq reset?
> >
> 
> The device doesn't offer VIRTIO_F_RING_RESET at the moment.
> 
> >
> > > > What about VIRTIO_NET_CTRL_NOTF_COAL?
> > >
> > > I think it should be handled in the same way as the above VQ_SET, that is, reset the corresponding virtqeuue parameters to 0.
> >
> > sounds consistent. but the problem is, I don't think this is
> > how we previously behaved. and RING_RESET is out in 1.2.
> > So we need something compatible. I am sorry.
> >
> > I suspect that instead we can say that existing hardware has a default set of
> > parameters for tx and rx. And global commands change that
> > besides changing all enabled VQs.
> >
> > That is a side effect beyond just changing all VQs.
> >
> > Hmm.
> > Alvaro?
> >
> 
> This is indeed a good point.
> We mention the device reset case, but nothing about VQ reset.
> 
> I feel that no matter how we handle this, we break something.
> 
> Having default coalescing values may collide with "Upon reset, a
> device MUST initialize all coalescing parameters to 0."

No this is after device reset.

> We can say that VQ reset doesn't affect the "global parameters" and a
> device reset does, but this collides with "Device Requirements:
> Virtqueue Reset".
> 
> In fact, resetting coalescing values after vq reset may be derived
> from "Upon reset, a device MUST initialize all coalescing parameters
> to 0".
> This is consistent with "Device Requirements: Virtqueue Reset".
> 
> I can add in my patch some clarifications.
> 
> This will break the linux virtio_net ethtool implementation a little,
> we'll need to fix it.

Not good. I feel we must come up with spec that is backwards compatible.
Hmm, maybe this is why Parav kept talking about modes.
I did not realize at the time, sorry Parav.

I still feel modes are not the best way to describe things so I propose this:
- in addition to per vq parameters, device that supports global TX/RX
  commands and ring reset maintains two sets of default parameters: for RX and TX
- existing commands change default and change all enabled vqs
  of the correct type (RX/TX) to the same value
- vq reset changes a vq to the default
- device reset changes defaults to 0 and changes all vqs to  0

note how defaults are only used for ring reset.  is "vq reset parameter"
a better name? I feel we will repeat "reset" too many times in a
sentence if we call it that though.

So fundamentally the only change is with RING_RESET, then
default is not always 0, it can be set by the global command.

-- 
MST