On Mon, Mar 20, 2023 at 05:35:38PM +0100, Cornelia Huck wrote:
> On Sun, Mar 12 2023, Heng Qi <[email protected]> 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 <[email protected]>
> > Reviewed-by: Xuan Zhuo <[email protected]>
> > Reviewed-by: Parav Pandit <[email protected]>
>
> [Apologies for only reviewing this right now]
>
> > @@ -1519,25 +1522,63 @@ \subsubsection{Control Virtqueue}\label{sec:Device
> > Types / Network Device / Devi
> > le32 max_usecs;
> > };
> >
> > +struct virtio_net_ctrl_coal_vq {
> > + le16 vqn;
> > + le16 reserved;
> > + struct virtio_net_ctrl_coal coal;
> > +};
> > +
> > #define VIRTIO_NET_CTRL_NOTF_COAL 6
> > #define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET 0
> > #define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET 1
> > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET 2
> > + #define VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET 3
> > \end{lstlisting}
> >
> > Coalescing parameters:
> > \begin{itemize}
> > +\item \field{vqn}: The virtqueue number of an enabled transmit or receive
> > virtqueue.
>
> As the commands obviously cannot target a control vq, I think we need a
> statement on what the device is supposed to be doing if the driver puts
> the number of the control q (or indeed an invalid number) here?
Not necessarily, we don't always require input validation. It's enough
to forbid driver from doing this (as you suggest below).
> > \item \field{max_usecs} for RX: Maximum number of microseconds to delay a
> > RX notification.
> > \item \field{max_usecs} for TX: Maximum number of microseconds to delay a
> > TX notification.
> > \item \field{max_packets} for RX: Maximum number of packets to receive
> > before a RX notification.
> > \item \field{max_packets} for TX: Maximum number of packets to send before
> > a TX notification.
> > \end{itemize}
> >
> > -The class VIRTIO_NET_CTRL_NOTF_COAL has 2 commands:
> > +\field{reserved} is reserved and it is ignored by a device.
>
> s/a/the/
>
> > +
> > +Read/Write attributes for coalescing parameters:
> > +\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.
>
> s/a/the/
>
> > +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, the structure
> > virtio_net_ctrl_coal_vq is write-only for a driver.
>
> s/a/the/
>
> > +\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.
>
> s/a/the/ (otherwise, this is kind of inconsistent)
>
> > +\end{itemize}
> > +
> > +The class VIRTIO_NET_CTRL_NOTF_COAL has 4 commands:
>
> Instead of giving the exact number of commands, maybe use "the following
> commands" instead?
>
> > \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:
>
> s/2/two/
>
> > +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 virtqueues having vqn 0 and vqn 2. Virtqueues having vqn 1
> > and vqn 3 retain their previous parameters.
> > +\item Command2: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} = 0 sets
> > coalescing parameters for virtqueue 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 vqn 0 set by command2.
> > +\item Command4: VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET with \field{vqn} = 1 sets
> > coalescing parameters for virtqueue having vqn 1. Virtqueue having vqn 3
> > retains its previous parameters.
> > +\item Command5: VIRTIO_NET_CTRL_NOTF_COAL_TX_SET sets coalescing
> > parameters for virtqueues having vqn 1 and vqn 3, and overrides the
> > parameters set by command4.
> > +\item Command6: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} = 1, the
> > device responds with coalescing parameters of vqn 1 set by command5.
> > +\end{itemize}
> > +
> > \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 +1626,33 @@ \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.
>
> s/VIRIO/VIRTIO/
>
> > +
> > +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.
>
> s/VIRIO/VIRTIO/
>
> I'm not sure whether we should be expressing this via 'before' -- it's
> not like the driver can negotiate the feature bit if it realizes later
> that it wants to issue the command. IOW, I'd prefer the 'not negotiated'
> -> 'MUST NOT issue' construct, but I'm not dead set on it.
I don't like double negation myself. Don't do A if you don't do B is
less clear than "Do B before A".
>
> > +
> > +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.
>
> Do we need to mandate here that the driver MUST NOT put anything else
> but the number of an enabled transmit or receive virtqueue into the vqn
> field?
I think it's a good idea.
> (Generally, I'd prefer "The driver" instead of "A driver" as well, but
> that might be a matter of taste.)
Check the surrounding text, and follow that example.
> >
> > \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.
>
> s/given/designated/ ?
>
> What should the device do if the virtqueue is invalid (i.e. it is the
> control vq?) I think we either need to add a statement explicitly
> forbidding that to the driver conformance section, or specify that the
> device MUST return an error here.
I prefer forbidding this from driver. Device should be free to
handle this in a variety of ways.
> > +
> > +Upon disabling and re-enabling the transmit virtqueue, the device MUST set
> > the coalescing parameters of the virtqueue
> > +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET command,
> > or, if the driver did not set any TX coalescing parameters, to 0.
>
> "a transmit virtqueue" ?
>
> > +
> > +Upon disabling and re-enabling the receive virtqueue, the device MUST set
> > the coalescing parameters of the virtqueue
> > +to those configured through the VIRTIO_NET_CTRL_NOTF_COAL_RX_SET command,
> > or, if the driver did not set any RX coalescing parameters, to 0.
>
> "a receive virtqueue" ?
>
> >
> > A device SHOULD NOT send used buffer notifications to the driver if the
> > notifications are suppressed, even if the notification conditions are met.
>
> Generally, I'd prefer "The device" here as well.
>
> >
> > +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
>
> There are a couple of typos and some style issues here which we could
> fix with an update on top, but I'd really like some clarity regarding
> invalid virtqueues first.
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
