Re: [virtio-dev] Re: [PATCH v3] virtio-net: support the virtqueue coalescing moderation
> > 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."
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.
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [PATCH v3] virtio-net: support the virtqueue coalescing moderation
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
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [PATCH v3] virtio-net: support the virtqueue coalescing moderation
> 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. I like the idea, maintaining default coalescing parameters is compatible with vq reset. "After a queue has been reset by the driver, the device MUST NOT execute any requests from that virtqueue, or notify the driver for it. The device MUST reset any state of a virtqueue to the *default state*, including the available state and the used state." I wonder if some of that should be included in my patch. Most of that is not relevant before introducing the per vq command. So maybe I could just mention in the device conformance that: "Coalescing parameters MUST/SHOULD persist a virtqueue reset." And then Heng can edit/add on top of that. Now that the existing commands do more than just iterating through all the virtqueues, maybe we should be able to modify all virtqueues with VIRTIO_NET_F_VQ_NOTF_COAL. I think that using a special vqn is messy, we can split "le32 reserved" to "le16 flags" and "le16 reserved". Then we can have: - A flag to apply parameters to all receive VQs, vqn is ignored in this case. (same as VIRTIO_NET_CTRL_NOTF_COAL_RX_SET but without changing the default parameters). - A flag to apply parameters to all transmit VQs, vqn is ignored in this case. (same as VIRTIO_NET_CTRL_NOTF_COAL_TX_SET but without changing the default parameters). And maybe a flag to apply to all vqs (transmit + receive), I actually am not sure how useful this is.. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
RE: [virtio-dev] Re: [PATCH v3] virtio-net: support the virtqueue coalescing moderation
> From: Alvaro Karsz > Sent: Friday, February 17, 2023 7:18 AM > I wonder if some of that should be included in my patch. > Most of that is not relevant before introducing the per vq command. > So maybe I could just mention in the device conformance that: > "Coalescing parameters MUST/SHOULD persist a virtqueue reset." > For sure coalescing parameters must not persist on a vq reset in the device. VQ reset operation destroys or disables the VQ. Device should not have any obligation to maintain some parameter on the disabled/destroyed object. User likely disabled the VQ because user may want to have a fresh start for this VQ for same or different use. And hence, reconfiguring the new addresses, new vq size, new vq notifications parameters..
RE: [virtio-dev] Re: [PATCH v3] virtio-net: support the virtqueue coalescing moderation
> From: Michael S. Tsirkin > 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. > > 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. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] Re: [PATCH v3] virtio-net: support the virtqueue coalescing moderation
On Fri, Feb 17, 2023 at 04:12:34PM +, Parav Pandit wrote: > > > From: Michael S. Tsirkin > > 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. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
RE: [virtio-dev] Re: [PATCH v3] virtio-net: support the virtqueue coalescing moderation
> From: Heng Qi > Sent: Friday, February 17, 2023 12:17 PM > 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? Recorded global parameters should be fine to report on an enabled vq even if the COAL_VQ_SET was not done. This is because the global value reflects on the VQ. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
