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

[Alvaro Karsz <alvaro.karsz@solid-run.com>]

> >> -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, the driver can send the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> >> +and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands to dynamically change the coalescing parameters for all transmit
> >> +and receive virtqueues, respectively.
> >> +
> >> +If the VIRTIO_NET_F_VQ_NOTF_COAL feature is negotiated:
> >> +\begin{itemize}
> >> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command to set coalescing parameters of a given
> >> +      enabled transmit/receive virtqueue.
> >> +\item a driver can send a VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command to a device, and the device responds with
> >> +      coalescing parameters of a given enabled transmit/receive virtqueue.
> >> +\end{itemize}
> >>
> > The VQ_NOTF_COAL commands are enclosed in an itemize and the NOTF_COAL
> > commands aren't.
> > I think that we should be consistent here.
>
> The VQ commands are enclosed in an itemize because it has both GET and
> SET operations.
>
> I don't think VIRTIO_NET_CTRL_NOTF_COAL_RX_SET and
> VIRTIO_NET_CTRL_NOTF_COAL_TX_SET
> have to do this, because they are both settings, just in different
> directions, and we have described them like this elsewhere.
>
> >
> > I'm not sure that describing the commands in here is necessary, maybe
> > just mentioning which commands can be used with which feature bit is
>
> Isn't that what this sentence does?
>

Yes, but it also describes what the command does
"to dynamically change the coalescing parameters for all transmit and
receive virtqueues"

Seems a bit repetitive to me.

> > enough (this is what I meant in v8, sorry if I wasn't clear).
> >
> > I'm not saying that describing the commands in here is not good, but
> > notice that the commands are described in 3 different places.
>
> Three places describe different effects.
>
> #1 describes which commands are available for which feature.
> #2 describes which command can use which structure.
> #3 describes which parameters can be set for each command, and whether
> they can affect "global values".
> It is placed in the "Operation" section because it explains how the
> device should behave.
>

You're right, but some parts seems repetitive to me:
#1:  "commands to dynamically change the coalescing parameters for all
transmit and receive virtqueues"

#2: "commands use the virtio_net_ctrl_coal structure to set
\field{max_usecs} and \field{max_packets} for all transmit/receive
virtqueues."

#3: " set the \field{max_usecs} and \field{max_packets} parameters for
all transmit virtqueues, and values of..."

Feels like every time we re-explain the commands with more detail.
So, for example we read #2 and understand what the command does (set
usecs and packets), then we read #3 and understand that it does some
more stuff.

IMO we need to explain it once, with all the details.

> Maybe #1 and #2 descriptions can be combined and described together.
>
> > This is #1.
> >
> >>   \begin{note}
> >>   The behavior of the device in response to these commands is best-effort:
> >> @@ -1519,25 +1531,76 @@ \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}
> >>
> >> +The VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands use the
> >> +virtio_net_ctrl_coal structure to set \field{max_usecs} and \field{max_packets} for all
> >> +transmit/receive virtqueues.
> >> +
> >> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command uses the virtio_net_ctrl_coal_vq structure
> >> +to set \field{max_usecs} and \field{max_packets} for the supplied virtqueue number \field{vqn}.
> >> +
> >> +The VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET command gets the values of \field{max_usecs} and
> >> +\field{max_packets} of the specified virtqueue from the device by setting \field{vqn}
> >> +in the virtio_net_ctrl_coal_vq structure.
> >> +
> > This is #2.
> >
> >> +# 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, \field{max_usecs}
> >> +      and \field{max_packets} are write-only for a driver.
> >> +\item For the command VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET, \field{vqn}, \field{reserved}, \field{max_usecs}
> >> +      and \field{max_packets} are 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, \field{max_usecs} and \field{max_packets} are read-only for the driver.
> >> +\end{itemize}
> >> +
> >>   Coalescing parameters:
> >>   \begin{itemize}
> >> +\item \field{vqn}: The virtqueue number of an enabled transmit or receive virtqueue.
> >>   \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.
> >> +
> >> +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: set the \field{max_usecs} and \field{max_packets} parameters for all transmit virtqueues, and values of
> >> +                                        coalescing parameters are recorded as TX global values by a device.
> >> +\item VIRTIO_NET_CTRL_NOTF_COAL_RX_SET: set the \field{max_usecs} and \field{max_packets} parameters for all receive virtqueues, and values of
> >> +                                        coalescing parameters are recorded as RX global values by a device.
> >> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET: set the \field{max_usecs} and \field{max_packets} parameters for an enabled transmit/receive
> >> +                                        virtqueue whose number is \field{vqn}, and do not change the TX/RX global values.
> >> +\item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the \field{max_usecs} and \field{max_packets} parameters for an enabled
> >> +                                        transmit/receive virtqueue whose number is \field{vqn}.
> >>   \end{enumerate}
> > This is #3.
> >
> >> +If coalescing parameters are being set, the device applies the last coalescing parameters received for a
> >> +virtqueue, regardless of the command used to set the parameters. For example with 2 pairs of virtqueues:
> >> +# Command sequence
> >> +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.
> >> +\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.
> >> +\item Command3: VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET with \field{vqn} = 0, the device responds with coalescing parameters of virtqueue0 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.
> >> +\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}
> >> +
> >>   \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}.
> >> @@ -1549,6 +1612,11 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
> >>
> >>   When the device has \field{max_usecs} = 0 or \field{max_packets} = 0, the notification conditions are met after every packet received/sent.
> >>
> >> +When the device receives the VIRTIO_NET_CTRL_NOTF_COAL_TX_SET and VIRTIO_NET_CTRL_NOTF_COAL_RX_SET commands,
> >> +it saves the values of coalescing parameters as global values, and the VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET command
> >> +does not change the global values. If the device is reset, the global values will be set to 0.
> >> +When a virtqueue is enabled after virtqueue reset, its coalescing parameters are set to global values.
> >> +
> > Maybe this explanation should be put closer to the commands
> > descriptions, where the global coalescing parameters are mentioned for
> > the first time.
> > Something like:
> > ....
> > \item VIRTIO_NET_CTRL_NOTF_COAL_VQ_GET: the device returns the
> > \field{max_usecs} and \field{max_packets} parameters for an enabled
> >                                 transmit/receive virtqueue whose number
> > is \field{vqn}.
> >
> > The device maintains global coalescing parameters for TX and RX....
>
> This is the "Operation" chapter, and the description of the operation is
> more likely to be placed here, isn't it?
>

I see your point, but a reader will see the "global notifications
coalescing parameter" concept, and won't know what it is until next
paragraph.
Maybe we can have a new list with the notification coalescing concepts
(like the notification coalescing parameters)? Just throwing an idea
here.

>
> >
> > And maybe we should use "global coalescing parameters" instead of
> > "global values" (in devicenormative as well).
> >
> >>   \subparagraph{RX Example}\label{sec:Device Types / Network Device / Device Operation / Control Virtqueue / Notifications Coalescing / RX Example}
> >>
> >>   If, for example:
> >> @@ -1585,15 +1653,26 @@ \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.
> >> +If neither the VIRTIO_NET_F_NOTF_COAL nor the VIRTIO_NET_F_VQ_NOTF_COAL feature
> >> +has been negotiated, the driver MUST NOT issue VIRTIO_NET_CTRL_NOTF_COAL commands.
> > Maybe this part can be splitted to:
> > if the F1 feature has not been negotiated, the driver must not issue
> > commands C1,C2.
> > if the F2 feature has not been negotiated, the driver must not issue
> > commands C3,C4.
>
> Ok.
>
> Thanks.
>
> >> +
> >> +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 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.
> >> +
> >> +A device MUST ignore \field{reserved}.
> >>
> >>   A device SHOULD NOT send used buffer notifications to the driver if the notifications are suppressed, even if the notification conditions are met.
> >>
> >> -Upon reset, a device MUST initialize all coalescing parameters to 0.
> >> +After disabling and re-enabling a virtqueue, the device MUST revert coalescing parameters of the virtqueue to the global values.
> >> +
> >> +Upon reset, a device MUST initialize all coalescing parameters to 0 and MUST set the global values to 0.
> >>
> >>   \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> >>   Types / Network Device / Legacy Interface: Framing Requirements}
> >> --
> > Let's wait for more comments before the next version, I guess some may
> > not agree with my comments.