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 <hen...@linux.alibaba.com>
> Reviewed-by: Xuan Zhuo <xuanz...@linux.alibaba.com>
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:
+ 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 0xFFFF inclusive,
No really because last vq is a cvq. Maybe just drop this?
> $ \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 is the same as calling
> VIRTIO_NET_CTRL_NOTF_COAL_VQ
you don't "call" commands. driver sends them, device receives them.
But here you are talking about a command abstracty so I'd just drop
a verb, or maybe "repeating".
And "same" is inexact in that it's not the same - takes more time - it
just has the same effect, or is equivalent to.
> +for virtqueues corresponding to all receiveqs.
receiveqs is confusing.
Also elsewhere we use the language receiveq1\ldots receiveqn
which seems clearer. Also you can not call VIRTIO_NET_CTRL_NOTF_COAL_VQ
for all vqs - it applies to one vq only. You mean each.
And virtqueues do not correspond to receiveqs - they
are receiveqs. If you like vqn corresponds to them.
Or better just "same as VIRTIO_NET_CTRL_NOTF_COAL_VQ repeated for
each of receiveq1\ldots receiveqn"
> +
> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command is the same as calling
> VIRTIO_NET_CTRL_NOTF_COAL_VQ
> +for virtqueues corresponding to all transmitqs.
> +
> +Virtqueue coalescing will be disabled if all parameters are set to 0.
In fact, either usecs 0 or max_packets disables coalescing, no?
> +
> +The class VIRTIO_NET_CTRL_NOTF_COAL_VQ has 2 commands:
> +\begin{enumerate}
> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_packets},
> \field{usecs} and \filed{vqn} parameters.
> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: get the \field{max_packets},
> \field{usecs} and \field{vqn} parameters.
How does VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET work then? For which vq?
I think you mean vqn is specified and you get back max_packets and
usecs. All this needs to be documented fully for each command.
I would also think it's a good idea to mention that VQ_GET does not have
to return exactly the same parameters - since it's best effort anyway,
it's ok for device to round down and store a smaller value for either
max_packets or usecs, e.g. to save space.
This kind of formalizes the best effort thing we discussed
previously.
Maybe make VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET a separate patch -
in the series,
at this point you did not make any effort to document it,
it needs more work.
> +\end{enumerate}
> +
> \subparagraph{RX Notifications}\label{sec:Device Types / Network Device /
> Device Operation / Control Virtqueue / Notifications Coalescing / RX
> Notifications}
>
> If, for example:
> @@ -4535,6 +4582,15 @@ \subsubsection{Control Virtqueue}\label{sec:Device
> Types / Network Device / Devi
> \item If the notifications are not suppressed by the driver, the device will
> send an used buffer notification, otherwise, the device will not send an used
> buffer notification as long as the notifications are suppressed.
> \end{itemize}
>
> +If, example of setting coalescing parameters for a receive virtqueue:
"If" without "then" is confusing. I'd just start with "Example".
> +\begin{itemize}
> +\item \field{max_packets} = 15.
> +\item \field{usecs} = 10.
> +\item \field{vqn} = 0;
why . above and ; here? I would just drop punctuation.
> +\end{itemize}
> +
> +The device will only operate on recieveq1 as
> VIRTIO_NET_CTRL_NOTF_COAL_RX_SET.
This really does not explain anything. Please just document exactly what
it does and does not do.
> +
> \subparagraph{TX Notifications}\label{sec:Device Types / Network Device /
> Device Operation / Control Virtqueue / Notifications Coalescing / TX
> Notifications}
>
> If, for example:
> @@ -4550,13 +4606,24 @@ \subsubsection{Control Virtqueue}\label{sec:Device
> Types / Network Device / Devi
> \item If the notifications are not suppressed by the driver, the device will
> send an used buffer notification, otherwise, the device will not send an used
> buffer notification as long as the notifications are suppressed.
> \end{itemize}
>
> +If, example of setting coalescing parameters for a transmit virtqueue:
> +\begin{itemize}
> +\item \field{max_packets} = 15.
> +\item \field{usecs} = 10.
> +\item \field{vqn} = 1;
> +\end{itemize}
> +
> +The device will only operate on transmitq1 as
> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET.
> +
I feel it's the other way around. Document
> \drivernormative{\subparagraph}{Notifications Coalescing}{Device Types /
> Network Device / Device Operation / Control Virtqueue / Notifications
> Coalescing}
>
> If the VIRTIO_NET_F_NOTF_COAL feature has not been negotiated, the driver
> MUST NOT issue VIRTIO_NET_CTRL_NOTF_COAL commands.
> +If the VIRTIO_NET_F_VQ_NOTF_COAL feature has not been negotiated, the driver
> MUST NOT issue VIRTIO_NET_CTRL_NOFT_COAL_VQ commands.
>
Don't we want to limit driver to legal values of vqn?
> \devicenormative{\subparagraph}{Notifications Coalescing}{Device Types /
> Network Device / Device Operation / Control Virtqueue / Notifications
> Coalescing}
>
> -A device SHOULD respond to the VIRTIO_NET_CTRL_NOTF_COAL commands with
> VIRTIO_NET_ERR if it was not able to change the parameters.
> +A device SHOULD respond to the VIRTIO_NET_CTRL_NOTF_COAL commands with
> VIRTIO_NET_ERR if it was not able to change the parameters or
> +was not able to find a virtqueue using the \field{vqn}.
First please create multiple statements not a single long one.
Second vqn only exists for per vq commands so create a statement
just for that.
Also more explicit please. What does this mean? I suspect that vq was not
enabled?
Also, we MUST have vqn < max_virtqueue_pairs.
>
> A device SHOULD NOT send used buffer notifications to the driver, if the
> notifications are suppressed as explained in \ref{sec:Basic Facilities of a
> Virtio Device / Virtqueues / Used Buffer Notification Suppression}, even if
> the coalescing counters expired.
>
> --
> 2.19.1.6.gb485710b
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
