[virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation
On Fri, Feb 10, 2023 at 05:53:40PM +0800, Heng Qi wrote:
>
>
> 在 2023/2/10 下午4:38, Michael S. Tsirkin 写道:
> > On Fri, Feb 10, 2023 at 03:01:30PM +0800, Heng Qi wrote:
> > > Currently, the coalescing profile is directly applied to all queues.
> > > This patch supports setting or getting the parameters for a specified
> > > queue,
> > > and a typical application of this function is NetDIM.
> > >
> > > When the traffic between queues is unbalanced, for example, one queue
> > > is busy and another queue is idle, then it will be very useful to
> > > control coalescing parameters at the queue granularity.
> > >
> > > Signed-off-by: Heng Qi
> > > Reviewed-by: Xuan Zhuo
> >
> > I like this generally, thanks! Language needs to be tightened a bit.
> > > ---
> > > v1->v2:
> > > 1. Rename VIRTIO_NET_F_PERQUEUE_NOTF_COAL to
> > > VIRTIO_NET_F_VQ_NOTF_COAL. @Michael S. Tsirkin
> > > 2. Use the \field{vqn} instead of the qid. @Michael S. Tsirkin
> > > 3. Unify tx and rx control structres into one structure
> > > virtio_net_ctrl_coal_vq. @Michael S. Tsirkin
> > > 4. Add a new control command VIRTIO_NET_CTRL_NOTF_COAL_VQ. @Michael
> > > S. Tsirkin, @Parav Pandit, @Alvaro Karsz
> > > 5. The special value 0xFFF is removed because
> > > VIRTIO_NET_CTRL_NOTF_COAL can be used. @Alvaro Karsz
> > > 6. Clarify some special scenarios. @Michael S. Tsirkin, @Parav
> > > Pandit, @Alvaro Karsz
> > >
> > > content.tex | 69 -
> > > 1 file changed, 68 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/content.tex b/content.tex
> > > index e863709..2c497e1 100644
> > > --- a/content.tex
> > > +++ b/content.tex
> > > @@ -3084,6 +3084,9 @@ \subsection{Feature bits}\label{sec:Device Types /
> > > Network Device / Feature bits
> > > \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through control
> > > channel.
> > > +\item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports the virtqueue
> > > +notifications coalescing.
> > > +
> > > \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications
> > > coalescing.
> > > \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 packets.
> > > @@ -3140,6 +3143,7 @@ \subsubsection{Feature bit
> > > requirements}\label{sec:Device Types / Network Device
> > > \item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
> > > \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or
> > > VIRTIO_NET_F_HOST_TSO6.
> > > \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
> > > +\item[VIRTIO_NET_F_VQ_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ and
> > > VIRTIO_NET_F_NOTF_COAL.
> > > \end{description}
> > > \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types /
> > > Network Device / Feature bits / Legacy Interface: Feature bits}
> > > @@ -4520,6 +4524,49 @@ \subsubsection{Control Virtqueue}\label{sec:Device
> > > Types / Network Device / Devi
> > > \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs} and
> > > \field{rx_max_packets} parameters.
> > > \end{enumerate}
> > > +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver can
> > > send
> > > +control commands to set or get the coalescing parameters of a specified
> > > +virtqueue (excluding the control virtqueue).
> > > +
> > > +\begin{lstlisting}
> > > +struct virtio_net_ctrl_coal_vq {
> > > +le32 max_packets;
> > > +le32 usecs;
> > > +le16 vqn;
> > Add le16 reserved here, so keep things aligned naturally.
> > In fact if you want to support GET you need to re-order things
> > since write buffers come before read buffers:
>
> I see, thanks for pointing it out.
>
> > +le16 vqn;
> > +le16 reserved;
> >
> > +le32 max_packets;
> > +le32 usecs;
> >
> > see below for more explanation.
> >
> >
> >
> > > +};
> > > +
> > > +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
> > > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
> > > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
> > > +\end{lstlisting}
> > > +
> > > +Virtqueue coalescing parameters:
> > > +\begin{itemize}
> > > +\item \field{max_packets}: The maximum number of packets sent/received
> > > by the
> > > +specified virtqueue before a TX/RX notification.
> > > +\item \field{usecs}: The maximum number of TX/RX usecs that the specified
> > > +virtqueue delays a TX/RX notification.
> > > +\item \field{vqn}: The virtqueue number of the specified virtqueue.
> > > +\end{itemize}
> > > +
> > > +The range of \filed{vqn} is between 0 and 0x inclusive,
> >
> > No really because last vq is a cvq. Maybe just drop this?
>
> In fact I have ruled out the control virtqueue.
>
> Its virtqueue number should be 0x1.
Nope, vq numberes are 16 bit.
> >
> > > $ \lfloor vqn / 2 \rfloor $
> > > +is the index of the corresponding receiveq, and $\lfloor (vqn / 2) + 1
> > > \rfloor $ is
> > > +the corresponding tranmitq.
> > > +
> > > +The VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command i
[virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation
在 2023/2/11 下午4:45, Alvaro Karsz 写道: 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. 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)? 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? Hi, Alvaro! Thanks for your reply! I think Parav refers more to the scene where ethool sets parameters at the queue group level and the scene where netdim sets parameters for a single queue. In this scenario, netdim should really determine the coalescing parameters of the device, and the parameters at the queue group level set by ethtool should be ignored (many drivers are designed this way, such as mlx), that is, we need to give netdim the right, because it It has the ability to dynamically adjust parameters. (However, I think this friendly constraint is also possible in driver implementation.) Of course, if we consider setting coalescing parameters at the queue group level and single queue level separately through ethtool, then as you said, we should not set any priority for them. Back to reality, I think the function of ethtool to set single queue parameters may come later, which is thankless for users because of netdim. Therefore, if our specification tends to be practical, we can add Parav's proposal, and if our specification tends to be more general, then hand over the constraints to the driver implementation. what do you think? I think that this should be discussed in the driver implementation stage, not in the spec. 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? I don't think that we should have priorities, but the last command should be the one that dictates the coalescing parameters. 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). Yes I see what you mean, thanks for the clear example. This should be the second scenario I described above, let's discuss how to solve it in the above reply. Thanks! :) This publicly archived list offers a means to provide input to the OASIS Virtual I/O Device (VIRTIO) TC. In order to verify user consent to the Feedback License terms and to minimize spam in the list archive, subscription is required before posting. Subscribe: virtio-comment-subscr...@lists.oasis-open.org Unsubscribe: virtio-comment-unsubscr...@lists.oasis-open.org List help: virtio-comment-h...@lists.oasis-open.org List archive: https://lists.oasis-open.org/archives/virtio-comment/ Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists Committee: https://www.oasis-open.org/committees/virtio/ Join OASIS: https://www.oasis-open.org/join/ - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation
> >> 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. > > 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)? > > 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? > > Hi, Alvaro! Thanks for your reply! > > I think Parav refers more to the scene where ethool sets parameters at > the queue group level and > the scene where netdim sets parameters for a single queue. In this > scenario, netdim should really > determine the coalescing parameters of the device, and the parameters at > the queue group level set > by ethtool should be ignored (many drivers are designed this way, such > as mlx), that is, we need to give netdim the right, > because it It has the ability to dynamically adjust parameters. > (However, I think this friendly constraint is also possible in driver > implementation.) > > Of course, if we consider setting coalescing parameters at the queue > group level and single queue level separately through ethtool, > then as you said, we should not set any priority for them. > > Back to reality, I think the function of ethtool to set single queue > parameters may come later, which is thankless for users because of netdim. > > Therefore, if our specification tends to be practical, we can add > Parav's proposal, and if our specification tends to be more general, > then hand over > the constraints to the driver implementation. what do you think? Hi, I understand your and Parav's point, and that this is needed for netdim. I just think that the spec should not dictate this, and this should be implementation specific. What if a OS with a different adaptive functionality wants to support these features? This is just my opinion though. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation
> From: Alvaro Karsz > Sent: Saturday, February 11, 2023 5:19 AM > > Therefore, if our specification tends to be practical, we can add > > Parav's proposal, and if our specification tends to be more general, > > then hand over the constraints to the driver implementation. what do > > you think? > > Hi, > I understand your and Parav's point, and that this is needed for netdim. > I just think that the spec should not dictate this, and this should be > implementation specific. > What if a OS with a different adaptive functionality wants to support these > features? > > This is just my opinion though. We don’t dictate it in the spec. in the spec just defines a. the behavior for device should do when both are configured One way to say, it doesn’t try to configure both. Driver should disable one and switch to other. Simple for driver, device implementation and spec as this is more common scenario. Other way to say, spec wants the door to remain open for mixed mode here. b. In this case the priority is defined as (last configured for a given object). So device is clear how to implement mixed mode. #a is well known case. #b is more flexible. Both spec definitions look good to me, we just got to describe it.
[virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation
You write "both are configured", "try to configure both", "disable one and switch to other" "mixed mode" etc.. But no modes/levels are mentioned in the patch, not once. I totally agree that *if* we define 2 modes, we should have strict rules on how to switch between modes and how to operate with both modes. But if we decide to have modes, your suggestion is not enough IMO, we should explicitly define the modes. In my POV, we should have no modes at all, everything is per queue, and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET is exactly the same as iterating through the RX queues and issuing a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command. Every queue will use the last coalescing parameters it received from the driver, regardless to the command used to deliver the parameters. No modes at all. I really don't see what we gain from having different operation modes. I thought that we agreed not to have operation modes in v1. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation
> From: Alvaro Karsz > Sent: Saturday, February 11, 2023 10:47 AM > > You write "both are configured", "try to configure both", "disable one and > switch to other" "mixed mode" etc.. > > But no modes/levels are mentioned in the patch, not once. using RX_SET and VQ_SET simultaneously implies "both, mixed mode, configured both". Spec cannot be silent about it. Did I miss the requirements section that describes the behavior of above two commands in combination? > > I totally agree that *if* we define 2 modes, we should have strict rules on > how > to switch between modes and how to operate with both modes. > Even without any explicit mode, we need to define behavior of two commands invoked one after the other. > But if we decide to have modes, your suggestion is not enough IMO, we should > explicitly define the modes. > > In my POV, we should have no modes at all, everything is per queue, and > VIRTIO_NET_CTRL_NOTF_COAL_RX_SET is exactly the same as iterating > through the RX queues and issuing a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET > command. > > Every queue will use the last coalescing parameters it received from the > driver, > regardless to the command used to deliver the parameters. > This to be part of the spec. I don’t see it or I missed it? > No modes at all. > > I really don't see what we gain from having different operation modes. > > I thought that we agreed not to have operation modes in v1. We agreed to not have explicit mode, at condition to define the clear behavior on what should happen when both commands are used one after other.
[virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation
I think that I wasn't clear enough. I'm not saying that we should not define in the spec how to handle a situation when a device receives both RX_SET and VQ_SET (or a driver sends both). I'm saying that I don't think that the driver should handle the situation the way you described it: > 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, > > 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. This implies that we have 2 modes and have priorities. I think that if we want to refer to this situation in the spec, it should be something like: "A Device should use the last coalescing parameters received for a virtqueue, regardless of the command used to deliver the parameters." (just an example to make the point). - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation
> From: Alvaro Karsz > Sent: Saturday, February 11, 2023 11:14 AM > > I think that I wasn't clear enough. > > I'm not saying that we should not define in the spec how to handle a situation > when a device receives both RX_SET and VQ_SET (or a driver sends both). > I'm saying that I don't think that the driver should handle the situation the > way > you described it: > > > 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, > > > > 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. > These are the two options I proposed. In second option missed the case of per vq followed by group level. Which I further acked in [1] to have it as last configured. > I think that if we want to refer to this situation in the spec, it should be > something like: > "A Device should use the last coalescing parameters received for a virtqueue, > regardless of the command used to deliver the parameters." > (just an example to make the point). Yes, as acked in [1]. :) We both agree on #b which you described in above example. So things looks good. Lets wait for Heng to generate v3 covering above case. [1] https://lists.oasis-open.org/archives/virtio-dev/202302/msg00227.html
[virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation
On Sat, Feb 11, 2023 at 12:18:45PM +0200, Alvaro Karsz wrote: > > >> 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. > > > 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)? > > > 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? > > > > Hi, Alvaro! Thanks for your reply! > > > > I think Parav refers more to the scene where ethool sets parameters at > > the queue group level and > > the scene where netdim sets parameters for a single queue. In this > > scenario, netdim should really > > determine the coalescing parameters of the device, and the parameters at > > the queue group level set > > by ethtool should be ignored (many drivers are designed this way, such > > as mlx), that is, we need to give netdim the right, > > because it It has the ability to dynamically adjust parameters. > > (However, I think this friendly constraint is also possible in driver > > implementation.) > > > > Of course, if we consider setting coalescing parameters at the queue > > group level and single queue level separately through ethtool, > > then as you said, we should not set any priority for them. > > > > Back to reality, I think the function of ethtool to set single queue > > parameters may come later, which is thankless for users because of netdim. > > > > Therefore, if our specification tends to be practical, we can add > > Parav's proposal, and if our specification tends to be more general, > > then hand over > > the constraints to the driver implementation. what do you think? > > Hi, > I understand your and Parav's point, and that this is needed for netdim. > I just think that the spec should not dictate this, and this should be > implementation specific. > What if a OS with a different adaptive functionality wants to support > these features? > > This is just my opinion though. I'm not sure the specific property is even a feature and not a bug. One can argue e.g. that ethtool should fail with EBUSY rather than being silently overwritten. Thats why I feel this kind of policy is best set by driver. -- MST - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation
> I'm not sure the specific property is even a feature and not a bug. > One can argue e.g. that ethtool should fail with EBUSY rather > than being silently overwritten. > Thats why I feel this kind of policy is best set by driver. When you write "set by driver", you mean that [X] the spec needs to define the driver behavior in this case, or that [Y] every driver should decide what's best? I'll answer to [X], if you meant [Y] the following part is not relevant. If we talk implementation here, I would probably implement it like that: net_dim_start_tx: set_ethtool_blocking_bool save_current_coalescing_params # Maybe? clear_all_tx_coalescing_params # NOTF_COAL_TX_SET 0 do_net_dim net_dim_stop_tx: restore_prev_ethtool_coalesing_params clear_ethtool_blocking_bool It's reasonable to assume that sending a ethtool set coalesce when netdim is operating will affect the performance. But, windows' virtio_net driver may want to use _CTRL_NOTF_COAL_TX_SET in its adaptive algorithm, and may have no userspace tool to configure these parameters. If we force the driver to clear/block some commands while using others, we may do more harm than good in windows' case. This is just my personal perspective. I think that this is an important feature either way. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation
On Sun, Feb 12, 2023 at 10:53:50PM +0200, Alvaro Karsz wrote: > > I'm not sure the specific property is even a feature and not a bug. > > One can argue e.g. that ethtool should fail with EBUSY rather > > than being silently overwritten. > > Thats why I feel this kind of policy is best set by driver. > > When you write "set by driver", you mean that [X] the spec needs to > define the driver behavior in this case, or that [Y] every driver > should decide what's best? > I'll answer to [X], if you meant [Y] the following part is not relevant. Y. > If we talk implementation here, I would probably implement it like that: > > net_dim_start_tx: > set_ethtool_blocking_bool > save_current_coalescing_params # Maybe? > clear_all_tx_coalescing_params # NOTF_COAL_TX_SET 0 > do_net_dim > > net_dim_stop_tx: > restore_prev_ethtool_coalesing_params > clear_ethtool_blocking_bool > > It's reasonable to assume that sending a ethtool set coalesce when > netdim is operating will affect the performance. > > But, windows' virtio_net driver may want to use _CTRL_NOTF_COAL_TX_SET > in its adaptive algorithm, and may have no userspace tool to configure > these parameters. > If we force the driver to clear/block some commands while using > others, we may do more harm than good in windows' case. > > This is just my personal perspective. > > I think that this is an important feature either way. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation
On Sat, Feb 11, 2023 at 06:13:55PM +0200, Alvaro Karsz wrote: > I think that I wasn't clear enough. > > I'm not saying that we should not define in the spec how to handle a > situation when a device receives both RX_SET and VQ_SET (or a driver > sends both). > I'm saying that I don't think that the driver should handle the > situation the way you described it: > > > 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, > > > > 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. > > This implies that we have 2 modes and have priorities. > > I think that if we want to refer to this situation in the spec, it > should be something like: > "A Device should use the last coalescing parameters received for a > virtqueue, regardless of the command used to deliver the parameters." Your suggestion is good, the per-device command and the per-queue command need some examples and behavior definitions, I will add them to avoid some misunderstandings. Thanks. > (just an example to make the point). - 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: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation
On Fri, Feb 10, 2023 at 07:19:49PM +0800, Heng Qi wrote:
>
>
> 在 2023/2/10 下午6:29, Michael S. Tsirkin 写道:
> > On Fri, Feb 10, 2023 at 05:53:40PM +0800, Heng Qi wrote:
> > >
> > > 在 2023/2/10 下午4:38, Michael S. Tsirkin 写道:
> > > > On Fri, Feb 10, 2023 at 03:01:30PM +0800, Heng Qi wrote:
> > > > > Currently, the coalescing profile is directly applied to all queues.
> > > > > This patch supports setting or getting the parameters for a specified
> > > > > queue,
> > > > > and a typical application of this function is NetDIM.
> > > > >
> > > > > When the traffic between queues is unbalanced, for example, one queue
> > > > > is busy and another queue is idle, then it will be very useful to
> > > > > control coalescing parameters at the queue granularity.
> > > > >
> > > > > Signed-off-by: Heng Qi
> > > > > Reviewed-by: Xuan Zhuo
> > > > I like this generally, thanks! Language needs to be tightened a bit.
> > > > > ---
> > > > > v1->v2:
> > > > > 1. Rename VIRTIO_NET_F_PERQUEUE_NOTF_COAL to
> > > > > VIRTIO_NET_F_VQ_NOTF_COAL. @Michael S. Tsirkin
> > > > > 2. Use the \field{vqn} instead of the qid. @Michael S. Tsirkin
> > > > > 3. Unify tx and rx control structres into one structure
> > > > > virtio_net_ctrl_coal_vq. @Michael S. Tsirkin
> > > > > 4. Add a new control command VIRTIO_NET_CTRL_NOTF_COAL_VQ.
> > > > > @Michael S. Tsirkin, @Parav Pandit, @Alvaro Karsz
> > > > > 5. The special value 0xFFF is removed because
> > > > > VIRTIO_NET_CTRL_NOTF_COAL can be used. @Alvaro Karsz
> > > > > 6. Clarify some special scenarios. @Michael S. Tsirkin, @Parav
> > > > > Pandit, @Alvaro Karsz
> > > > >
> > > > >content.tex | 69
> > > > > -
> > > > >1 file changed, 68 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/content.tex b/content.tex
> > > > > index e863709..2c497e1 100644
> > > > > --- a/content.tex
> > > > > +++ b/content.tex
> > > > > @@ -3084,6 +3084,9 @@ \subsection{Feature bits}\label{sec:Device
> > > > > Types / Network Device / Feature bits
> > > > >\item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through
> > > > > control
> > > > >channel.
> > > > > +\item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports the virtqueue
> > > > > +notifications coalescing.
> > > > > +
> > > > >\item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications
> > > > > coalescing.
> > > > >\item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4
> > > > > packets.
> > > > > @@ -3140,6 +3143,7 @@ \subsubsection{Feature bit
> > > > > requirements}\label{sec:Device Types / Network Device
> > > > >\item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
> > > > >\item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4 or
> > > > > VIRTIO_NET_F_HOST_TSO6.
> > > > >\item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
> > > > > +\item[VIRTIO_NET_F_VQ_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ and
> > > > > VIRTIO_NET_F_NOTF_COAL.
> > > > >\end{description}
> > > > >\subsubsection{Legacy Interface: Feature bits}\label{sec:Device
> > > > > Types / Network Device / Feature bits / Legacy Interface: Feature
> > > > > bits}
> > > > > @@ -4520,6 +4524,49 @@ \subsubsection{Control
> > > > > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > > >\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{rx_usecs}
> > > > > and \field{rx_max_packets} parameters.
> > > > >\end{enumerate}
> > > > > +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the driver
> > > > > can send
> > > > > +control commands to set or get the coalescing parameters of a
> > > > > specified
> > > > > +virtqueue (excluding the control virtqueue).
> > > > > +
> > > > > +\begin{lstlisting}
> > > > > +struct virtio_net_ctrl_coal_vq {
> > > > > +le32 max_packets;
> > > > > +le32 usecs;
> > > > > +le16 vqn;
> > > > Add le16 reserved here, so keep things aligned naturally.
> > > > In fact if you want to support GET you need to re-order things
> > > > since write buffers come before read buffers:
> > > I see, thanks for pointing it out.
> > >
> > > >+le16 vqn;
> > > >+le16 reserved;
> > > >
> > > >+le32 max_packets;
> > > >+le32 usecs;
> > > >
> > > > see below for more explanation.
> > > >
> > > >
> > > >
> > > > > +};
> > > > > +
> > > > > +#define VIRTIO_NET_CTRL_NOTF_COAL_VQ 7
> > > > > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 0
> > > > > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 1
> > > > > +\end{lstlisting}
> > > > > +
> > > > > +Virtqueue coalescing parameters:
> > > > > +\begin{itemize}
> > > > > +\item \field{max_packets}: The maximum number of packets
> > > > > sent/received by the
> > > > > +specified virtqueue before a TX/RX notification.
> > > > > +\item \field{usecs}: The maximum number of TX/RX usecs that the
> > > > > specified
> > > > > +virtqueue delays a TX/RX notification.
> > > >
Re: [virtio-dev] Re: [virtio-comment] Re: [PATCH v2] virtio-net: support the virtqueue coalescing moderation
On Mon, Feb 13, 2023 at 10:36:13AM +0800, Heng Qi wrote:
>
>
> 在 2023/2/13 上午4:47, Michael S. Tsirkin 写道:
> > On Fri, Feb 10, 2023 at 07:19:49PM +0800, Heng Qi wrote:
> > >
> > > 在 2023/2/10 下午6:29, Michael S. Tsirkin 写道:
> > > > On Fri, Feb 10, 2023 at 05:53:40PM +0800, Heng Qi wrote:
> > > > > 在 2023/2/10 下午4:38, Michael S. Tsirkin 写道:
> > > > > > On Fri, Feb 10, 2023 at 03:01:30PM +0800, Heng Qi wrote:
> > > > > > > Currently, the coalescing profile is directly applied to all
> > > > > > > queues.
> > > > > > > This patch supports setting or getting the parameters for a
> > > > > > > specified queue,
> > > > > > > and a typical application of this function is NetDIM.
> > > > > > >
> > > > > > > When the traffic between queues is unbalanced, for example, one
> > > > > > > queue
> > > > > > > is busy and another queue is idle, then it will be very useful to
> > > > > > > control coalescing parameters at the queue granularity.
> > > > > > >
> > > > > > > Signed-off-by: Heng Qi
> > > > > > > Reviewed-by: Xuan Zhuo
> > > > > > I like this generally, thanks! Language needs to be tightened a bit.
> > > > > > > ---
> > > > > > > v1->v2:
> > > > > > >1. Rename VIRTIO_NET_F_PERQUEUE_NOTF_COAL to
> > > > > > > VIRTIO_NET_F_VQ_NOTF_COAL. @Michael S. Tsirkin
> > > > > > >2. Use the \field{vqn} instead of the qid. @Michael S.
> > > > > > > Tsirkin
> > > > > > >3. Unify tx and rx control structres into one structure
> > > > > > > virtio_net_ctrl_coal_vq. @Michael S. Tsirkin
> > > > > > >4. Add a new control command VIRTIO_NET_CTRL_NOTF_COAL_VQ.
> > > > > > > @Michael S. Tsirkin, @Parav Pandit, @Alvaro Karsz
> > > > > > >5. The special value 0xFFF is removed because
> > > > > > > VIRTIO_NET_CTRL_NOTF_COAL can be used. @Alvaro Karsz
> > > > > > >6. Clarify some special scenarios. @Michael S. Tsirkin,
> > > > > > > @Parav Pandit, @Alvaro Karsz
> > > > > > >
> > > > > > > content.tex | 69
> > > > > > > -
> > > > > > > 1 file changed, 68 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/content.tex b/content.tex
> > > > > > > index e863709..2c497e1 100644
> > > > > > > --- a/content.tex
> > > > > > > +++ b/content.tex
> > > > > > > @@ -3084,6 +3084,9 @@ \subsection{Feature bits}\label{sec:Device
> > > > > > > Types / Network Device / Feature bits
> > > > > > > \item[VIRTIO_NET_F_CTRL_MAC_ADDR(23)] Set MAC address through
> > > > > > > control
> > > > > > > channel.
> > > > > > > +\item[VIRTIO_NET_F_VQ_NOTF_COAL(52)] Device supports the
> > > > > > > virtqueue
> > > > > > > +notifications coalescing.
> > > > > > > +
> > > > > > > \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports
> > > > > > > notifications coalescing.
> > > > > > > \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4
> > > > > > > packets.
> > > > > > > @@ -3140,6 +3143,7 @@ \subsubsection{Feature bit
> > > > > > > requirements}\label{sec:Device Types / Network Device
> > > > > > > \item[VIRTIO_NET_F_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
> > > > > > > \item[VIRTIO_NET_F_RSC_EXT] Requires VIRTIO_NET_F_HOST_TSO4
> > > > > > > or VIRTIO_NET_F_HOST_TSO6.
> > > > > > > \item[VIRTIO_NET_F_RSS] Requires VIRTIO_NET_F_CTRL_VQ.
> > > > > > > +\item[VIRTIO_NET_F_VQ_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ
> > > > > > > and VIRTIO_NET_F_NOTF_COAL.
> > > > > > > \end{description}
> > > > > > > \subsubsection{Legacy Interface: Feature
> > > > > > > bits}\label{sec:Device Types / Network Device / Feature bits /
> > > > > > > Legacy Interface: Feature bits}
> > > > > > > @@ -4520,6 +4524,49 @@ \subsubsection{Control
> > > > > > > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > > > > > > \item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the
> > > > > > > \field{rx_usecs} and \field{rx_max_packets} parameters.
> > > > > > > \end{enumerate}
> > > > > > > +If additionally VIRTIO_NET_F_VQ_NOTF_COAL is negotiated, the
> > > > > > > driver can send
> > > > > > > +control commands to set or get the coalescing parameters of a
> > > > > > > specified
> > > > > > > +virtqueue (excluding the control virtqueue).
> > > > > > > +
> > > > > > > +\begin{lstlisting}
> > > > > > > +struct virtio_net_ctrl_coal_vq {
> > > > > > > +le32 max_packets;
> > > > > > > +le32 usecs;
> > > > > > > +le16 vqn;
> > > > > > Add le16 reserved here, so keep things aligned naturally.
> > > > > > In fact if you want to support GET you need to re-order things
> > > > > > since write buffers come before read buffers:
> > > > > I see, thanks for pointing it out.
> > > > >
> > > > > > +le16 vqn;
> > > > > > +le16 reserved;
> > > > > >
> > > > > > +le32 max_packets;
> > > > > > +le32 usecs;
> > > > > >
> > > > > > see below for more explanation.
> > > > > >
> > > > > >
> > > > > >
> > > > > > > +};
> > > > > > > +
> > > > > > > +#define
