Re: [PATCH] virtio-net: Add an optional device control over the receive buffers length

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Dec 30, 2019 at 01:59:17PM +0000, Vitaly Mireyno wrote:
> This patch gives devices some level of control over the receive buffers length.
> The driver declares the minimum receive buffer length, and the device requests max/min buffer length ratio.
> This is a follow-up to the "[PATCH] virtio-net: Add equal-sized receive buffers feature" discussion: https://lists.oasis-open.org/archives/virtio-comment/201912/msg00007.html
> 
> 
> Signed-off-by: Vitaly Mireyno <vmireyno@marvell.com>
> ---
>  content.tex | 41 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/content.tex b/content.tex
> index d68cfaf..0a4cfba 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -2815,6 +2815,13 @@ \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_RXBUF_LEN_RATIO(57)] Device requests to limit the
> +    maximum/minimum receive buffers length ratio.
> +

So VIRTIO_NET_F_RXBUF_LEN_RATIO only makes sense with
VIRTIO_NET_F_RXBUF_MIN_LEN, right?  I think we can just use a single
feature bit then, and just set ratio to 0 to mean "infinity".

> +\item[VIRTIO_NET_F_RXBUF_MIN_LEN(58)] Device requests to know the minimum
> +    receive buffers length. Driver declares the minimum receive buffers
> +    length.
> +
>  \item[VIRTIO_NET_F_GUEST_HDRLEN(59)] Driver can provide the exact \field{hdr_len}
>      value. Device benefits from knowing the exact header length.
>  
> @@ -2861,8 +2868,8 @@ \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types / Network
>  \subsection{Device configuration layout}\label{sec:Device Types / Network Device / Device configuration layout}
>  \label{sec:Device Types / Block Device / Feature bits / Device configuration layout}
>  
> -Three driver-read-only configuration fields are currently defined. The \field{mac} address field
> -always exists (though is only valid if VIRTIO_NET_F_MAC is set), and
> +The driver-read-only \field{mac} address field always exists
> +(though is only valid if VIRTIO_NET_F_MAC is set), and
>  \field{status} only exists if VIRTIO_NET_F_STATUS is set. Two
>  read-only bits (for the driver) are currently defined for the status field:
>  VIRTIO_NET_S_LINK_UP and VIRTIO_NET_S_ANNOUNCE.
> @@ -2882,12 +2889,22 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
>  VIRTIO_NET_F_MTU is set. This field specifies the maximum MTU for the driver to
>  use.
>  
> +The device-read-only field \field{min_rx_buf_len} only exists if
> +VIRTIO_NET_F_RXBUF_MIN_LEN is set. This field specifies the minimum receive
> +buffers length.
> +
> +The driver-read-only field \field{rx_buf_len_ratio} only exists if
> +VIRTIO_NET_F_RXBUF_LEN_RATIO is set. This field specifies the maximum/minimum receive
> +buffers length ratio.

Actually shouldn't all of these always exist and only be valid
if a flag is set? I think existing text should be fixed too.

> +
>  \begin{lstlisting}
>  struct virtio_net_config {
>          u8 mac[6];
>          le16 status;
>          le16 max_virtqueue_pairs;
>          le16 mtu;
> +        le32 min_rx_buf_len;
> +        le16 rx_buf_len_ratio;
>  };
>  \end{lstlisting}
>  
> @@ -2916,6 +2933,15 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
>  If the driver negotiates the VIRTIO_NET_F_STANDBY feature, the device MAY act
>  as a standby device for a primary device with the same MAC address.
>  
> +A driver SHOULD accept the VIRTIO_NET_F_RXBUF_MIN_LEN feature if offered.
> +
> +If VIRTIO_NET_F_RXBUF_MIN_LEN feature has been negotiated, the driver MUST
> +set \field{min_rx_buf_len}.
> +
> +A driver MUST NOT modify \field{min_rx_buf_len} once it has been set.
> +
> +A driver SHOULD accept the VIRTIO_NET_F_RXBUF_LEN_RATIO feature if offered.
> +
>  \drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
>  
>  A driver SHOULD negotiate VIRTIO_NET_F_MAC if the device offers it.
> @@ -3281,6 +3307,17 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi
>  If VIRTIO_NET_F_MQ is negotiated, each of receiveq1\ldots receiveqN
>  that will be used SHOULD be populated with receive buffers.
>  
> +If VIRTIO_NET_F_RXBUF_MIN_LEN feature has been negotiated, the driver
> +MUST initialize all receive virtqueue descriptors \field{len} field with
> +the value greater than or equal to the value configured in the
> +\field{min_rx_buf_len} device configuration field, and allocate receive
> +buffers accordingly.

I think above should talk about buffer length, discussion about
descriptors doesn't belong in devices, it's a way to describe buffers.

If we want to limit descriptors per rx buffer, I think it's a
separate feature similar to max_sg for blk/scsi.


> +If VIRTIO_NET_F_RXBUF_LEN_RATIO feature has been negotiated, the driver
> +MUST initialize all receive virtqueue descriptors \field{len} field with
> +the value less than or equal to (\field{min_rx_buf_len} * \field{rx_buf_len_ratio}),
> +and allocate receive buffers accordingly.
> +
>  \devicenormative{\paragraph}{Setting Up Receive Buffers}{Device Types / Network Device / Device Operation / Setting Up Receive Buffers}
>  
>  The device MUST set \field{num_buffers} to the number of descriptors used to
> --