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

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

On Fri, Feb 17, 2023 at 04:12:34PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Friday, February 17, 2023 6:35 AM
> 
> > > 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".
> > >
> Not really.
> When the device resets, VQ objects are destroyed in the device.
> So VQ's notifications parameters doesn't exists on device reset.
> 
> And so the same case with VQ reset.
> When a VQ is reset (disabled), VQ's notifications configuration is removed in the device too.
> Just like its desc ring and other addresses are invalid.

Yes, but there seems to be such a situation: when the device is reactivated, as the specification says,
all parameters are set to 0 (use parameters as the default configuration on the device).
When CTRL_COAL_SET and CTRL_COAL_VQ_SET are sent, the configuration is updated (the parameters of each vq may be different,
but the global parameter configuration may be recorded), at this time, if vq is reset,
should the parameters be 0 or a recorded global parameters after it is re-enabled?

> 
> > > 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.
> 
> True default is not zero when global are configured.
> It is ok to report VQ parameters on GET command when globals are configured.