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

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

å 2023/3/10 äå2:53, Parav Pandit åé:
> On 3/9/2023 8:05 AM, Heng Qi wrote:
> > Currently, coalescing parameters are grouped for all transmit and 
> > receive
> > virtqueues. This patch supports setting or getting the parameters for a
> > specified virtqueue, and a typical application of this function is 
> > netdim[1].
> >
> > When the traffic between virtqueues is unbalanced, for example, one 
> > virtqueue
> > is busy and another virtqueue is idle, then it will be very useful to
> > control coalescing parameters at the virtqueue granularity.
> >
> > [1] https://docs.kernel.org/networking/net_dim.html
> >
> > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> [..]
> > Â device-types/net/description.tex | 86 ++++++++++++++++++++++++++++----
> > Â 1 file changed, 75 insertions(+), 11 deletions(-)
> >
> > diff --git a/device-types/net/description.tex 
> > b/device-types/net/description.tex
> > index e71e33b..c896485 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -83,6 +83,8 @@ \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 virtqueue 
> > notification coalescing.
> > +
> > Â \item[VIRTIO_NET_F_NOTF_COAL(53)] Device supports notifications 
> > coalescing.
> > Â Â \item[VIRTIO_NET_F_GUEST_USO4 (54)] Driver can receive USOv4 
> > packets.
> > @@ -139,6 +141,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.
> > Â \end{description}
> > Â Â \subsubsection{Legacy Interface: Feature bits}\label{sec:Device 
> > Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> > @@ -1505,13 +1508,13 @@ \subsubsection{Control 
> > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > Â Â \paragraph{Notifications Coalescing}\label{sec:Device Types / 
> > Network Device / Device Operation / Control Virtqueue / Notifications 
> > Coalescing}
> > Â -If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, the driver can
> > -send control commands for dynamically changing the coalescing 
> > parameters.
> > +If the VIRTIO_NET_F_NOTF_COAL feature is negotiated, a driver can send
> Above text had "the driver".
> Better to not change it.

Ok.

> dynamically changing got dropped, better to keep it.

The motivation for this modification comes from Alvaro. We only need to 
say here that
feature A can send B, instead of repeatedly mentioning "action: 
dynamically changing the coalescing parameters."
and "object: for all tx/rx virtqueues or for the virtqueue with vqn" 
every time a command is mentioned.

> > +commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and 
> > VIRTIO_NET_CTRL_NOTF_COAL_RX_SET
> > +for notification coalescing.
>
>
> [..]
>
> > +\begin{itemize}
> > +\item For commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and 
> > VIRTIO_NET_CTRL_NOTF_COAL_RX_SET, the structure virtio_net_ctrl_coal 
> > is write-only for a driver.
> > +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, the 
> > structure virtio_net_ctrl_coal_vq is write-only for a driver.
> > +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET, \field{vqn} 
> > and \field{reserved} are write-only
> > +ÂÂÂÂÂ for a driver, and, the structure virtio_net_ctrl_coal is 
> > read-only for the driver.
> for a driver and the structure ..
> drop extra ",".

I'll remove it.

> > +\end{itemize}
> > +
> > +The class VIRTIO_NET_CTRL_NOTF_COAL has 4 commands:
> > Â \begin{enumerate}
> > -\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: set the \field{max_usecs} 
> > and \field{max_packets} parameters for all transmit virtqueues.
> > -\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} 
> > and \field{max_packets} parameters for all receive virtqueues.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_TX_SET: use the structure 
> > virtio_net_ctrl_coal to set the \field{max_usecs} and 
> > \field{max_packets} parameters for all transmit virtqueues.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: use the structure 
> > virtio_net_ctrl_coal to set the \field{max_usecs} and 
> > \field{max_packets} parameters for all receive virtqueues.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: use the structure 
> > virtio_net_ctrl_coal_vq to set the \field{max_usecs} and 
> > \field{max_packets} parameters
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ for an enabled 
> > transmit/receive virtqueue whose number is \field{vqn}.
> > +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: use the structure 
> > virtio_net_ctrl_coal_vq to get the \field{max_usecs} and 
> > \field{max_packets} parameters
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ for an enabled 
> > transmit/receive virtqueue whose number is \field{vqn}.
> > Â \end{enumerate}
> > Â +The device may generate notifications more or less frequently than 
> > specified by set commands of the VIRTIO_NET_CTRL_NOTF_COAL class.
> > +
> > +If coalescing parameters are being set, the device applies the last 
> > coalescing parameters set for a
> > +virtqueue, regardless of the command used to set the parameters. Use 
> > the following command sequence
> > +with 2 pairs of virtqueues as an example:
> > +Each of the following commands sets \field{max_usecs} and 
> > \field{max_packets} parameters for virtqueues.
> > +\begin{itemize}
> > +\item Command1: VIRTIO_NET_CTRL_NOTF_COAL_RX_SET sets coalescing 
> > parameters for virtqueue0 and virtqueue2, and, virtqueue1 and 
> > virtqueue3 retain their previous parameter values.
> above "and" is confusing. It implies set on vq0,vq2 and vq1, vq3.
> Same for below Command2 as well.
> It also introduces new terms as virtqueue0 and virtqueueN.
> It is better to avoid.

Indeed, I agree.

> Rewriting it as below avoids creating new terms and avoids confusion 
> around "and".
>
> Command1: VIRTIO_NET_CTRL_NOTF_COAL_RX_SET sets coalescing parameters 
> for virtqueues having vqn 0 and vqn 2. Virtqueues having vqn 1 and 3 
> retain their previous parameters.

I think it works, thanks for the example.

> > +\item Command2: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} = 
> > 0 sets coalescing parameters for virtqueue0, and virtqueue2 retains 
> > the values from command1.
> Command2: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} sets 
> coalescing parameters for the virtuqueue having vqn 0. Virtqueue 
> having vqn 2 retains the parameters from command1.
>
> > +\item Command3: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} = 
> > 0, the device responds with coalescing parameters of virtqueue0 set 
> > by command2.
> parameters of vqn 0 set by command2.
>
> > +\item Command4: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} = 
> > 1 sets coalescing parameters for virtqueue1, and virtqueue3 retains 
> > its previous values.for vqn 1. Virtqueue having vqn 3 retains its 
> > previous parameters.
> > +\item Command5: VIRTIO_NET_CTRL_NOTF_COAL_TX_SET sets coalescing 
> > parameters for virtqueue1 and virtqueue3, and overrides the values 
> > set by command4.
> > +\item Command6: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} = 
> > 1, the device responds with coalescing parameters of virtqueue1 set 
> > by command5.
> > +\end{itemize}
> > +
> > +Upon disabling and re-enabling the transmit virtqueue, the device 
> > will set the coalescing parameters of the virtqueue
> > +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET 
> > command, or to 0 if the command did not set any TX coalescing 
> > parameters.
> > +
> s/if the command/if the driver/
>
> > +Upon disabling and re-enabling the receive virtqueue, the device 
> > will set the coalescing parameters of the virtqueue
> > +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 
> > command, or to 0 if the command did not set any RX coalescing 
> > parameters.
> > +
> s/if the command/if the driver/
> > \subparagraph{Operation}\label{sec:Device Types / Network Device / 
> > Device Operation / Control Virtqueue / Notifications Coalescing / 
> > Operation}
> > Â Â The device sends a used buffer notification once the notification 
> > conditions are met and if the notifications are not suppressed as 
> > explained in \ref{sec:Basic Facilities of a Virtio Device / 
> > Virtqueues / Used Buffer Notification Suppression}.
> > @@ -1585,14 +1632,31 @@ \subsubsection{Control 
> > Virtqueue}\label{sec:Device Types / Network Device / Devi
> > Â Â \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.
> > +A driver MUST negotiate the VIRTIO_NET_F_NOTF_COAL feature before 
> > issuing commands VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and 
> > VIRIO_NET_CTRL_NOTF_COAL_RX_SET.
> > +
> > +A driver MUST negotiate the VIRTIO_NET_F_VQ_NOTF_COAL feature before 
> > issuing commands VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET and 
> > VIRIO_NET_CTRL_NOTF_COAL_VQ_GET.
> > +
> > +A driver MUST ignore the values of coalescing parameters received 
> > from the VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command if a device 
> > responds with VIRTIO_NET_ERR.
> > Â Â \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 MUST ignore \field{reserved}.
> > +
> > +A device SHOULD respond to VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and 
> > VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands with VIRTIO_NET_ERR if it 
> > was not able to change the parameters.
> > +
> > +A device MUST respond to the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 
> > command with VIRTIO_NET_ERR if it was not able to change the parameters.
> > +
> > +A device MUST respond to VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET and 
> > VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET commands with VIRTIO_NET_ERR if the 
> > given virtqueue is disabled.
> > +
> > +After disabling and re-enabling a transmit/receive virtqueue, the 
> > device MUST set coalescing parameters of the virtqueue
> > +to those configured using the 
> > VIRTIO_NET_CTRL_NOTF_COAL_TX_SET/VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 
> > command, or, if the command
> s/if the command/if the driver/
> > +did not configure TX/RX coalescing parameters, to 0.
>
> Also the sentence structure should be:
> set params to value A configured using method1, else to params B if 
> not configured.
>
> Instead currently it is,
> set params to value A using method1, else not configured to param B.
>
> So to keep if-else pattern:
>
> the device MUST set coalescing parameters of the virtqueue to those 
> configured using the 
> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET/VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 
> command, or to 0 if the driver did not configure TX/RX coalescing 
> parameters.

Ok. Then I think we can move the description of the non-normative part 
here, and keep only one description to avoid repetition. As Michael 
suggested, we keep the 'if then' syntax for readability.

> Just like how you wrote it in the non normative places. :)
>
> I still think that disable,re-enable doesnt need to be documented twice.
>
> Keeping it in the conformance section is good enough, specially when 
> the text is exact same.

Sure.

> > Â Â A device SHOULD NOT send used buffer notifications to the driver 
> > if the notifications are suppressed, even if the notification 
> > conditions are met.
> > Â +The behavior of the device in response to set commands of the 
> > VIRTIO_NET_CTRL_NOTF_COAL class is best-effort:
> > +the device MAY generate notifications more or less frequently than 
> > specified.
> > +
> > Â Upon reset, a device MUST initialize all coalescing parameters to 0.
> > Â Â \subsubsection{Legacy Interface: Framing 
> > Requirements}\label{sec:Device
>
> Other than above small wordings change, it looks good to me.
> Reviewed-by: Parav Pandit <parav@nvidia.com>

Thank you very much! :)