Re: [virtio-comment] [PATCH 1/2] virtio-net: Fix receive buffer size cal

Subject: Re: [virtio-comment] [PATCH 1/2] virtio-net: Fix receive buffer size calculation text

On Wed, Dec 13, 2023 at 8:44âAM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:

On Fri, 10 Nov 2023 14:40:39 +0200, Parav Pandit <parav@nvidia.com> wrote:
> Receive buffer size calculation is based on the following
> negotiated features.
>
> The text has wrong calculation for IPv6 and also it has missed
> VIRTIO_NET_F_HASH_REPORT.
>
> The problem of igorance of VIRTIO_NET_F_HASH_REPORT is reported
> in [1], however fix for ipv6 payload length must also be
> considered.
>
> Since for the both the fixes touching same requirements, a
> new issue is created as [2].
>
> This patch brings following fixes.
>
> 1. Fix annotating struct virtio_net_hdr as field
> 2. Fix receive buffer calculation for guest GSO cases to consider
>Â Â ipv6 payload length
> 3. small grammar corrections for article
> 4. reword the requirement to consider the virtio_ndr_hdr which is
>Â Â depends on the negotiated feature, hence first clarify the
>Â Â struct virtio_net_hdr size
>
> [1] https://github.com/oasis-tcs/virtio-spec/issues/170
> [2] https://github.com/oasis-tcs/virtio-spec/issues/183
>
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/170
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/183
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> ---
>Â device-types/net/description.tex | 25 ++++++++++++++++++-------
>Â 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index 96f7dc5..f5647d4 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -660,24 +660,35 @@ \subsubsection{Setting Up Receive Buffers}\label{sec:Device Types / Network Devi
>Â If the VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
>Â VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_GUEST_USO4 or VIRTIO_NET_F_GUEST_USO6
>Â features are used, the maximum incoming packet
> -will be to 65550 bytes long (the maximum size of a
> -TCP or UDP packet, plus the 14 byte ethernet header), otherwise
> -1514 bytes.Â The 12-byte struct virtio_net_hdr is prepended to this,
> -making for 65562 or 1526 bytes.
> +will of size 65589 bytes long (14 bytes of Ethernet header, plus 40 bytes of
> +the IPv6 header, plus 65535 bytes of maximum IPv6 payload including any
> +extension header) otherwise 1514 bytes.
> +When VIRTIO_NET_F_HASH_REPORT is not negotiated, the required receive buffer
> +size is either 65601 or 1526 bytes.
> +When VIRTIO_NET_F_HASH_REPORT is negotiated, the required receive buffer
> +size is either 65609 or 1534 bytes.
>
>Â \drivernormative{\paragraph}{Setting Up Receive Buffers}{Device Types / Network Device / Device Operation / Setting Up Receive Buffers}
>
> +If VIRTIO_NET_F_HASH_REPORT is not negotiated, the size of the field
> +\field{struct virtio_net_hdr} is 12 bytes.
> +
> +If VIRTIO_NET_F_HASH_REPORT is negotiated, the size of the field
> +\field{struct virtio_net_hdr} is 20 bytes.

For the receive side, is these needed?

Other is ok to me.

Thanks

At the moment the virtio_net header size is equal for RX and TX

So, if theÂ VIRTIO_NET_F_HASH_REPORT is negotiated the header size will be 20 bytes for both directions..ÂÂ

In TX direction the extra space is the header is currently not used (if I'm not mistaken).

> +
>Â \begin{itemize}
>Â \item If VIRTIO_NET_F_MRG_RXBUF is not negotiated:
>Â Â \begin{itemize}
>Â Â Â \item If VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6, VIRTIO_NET_F_GUEST_UFO,
>Â Â Â ÂVIRTIO_NET_F_GUEST_USO4 or VIRTIO_NET_F_GUEST_USO6 are negotiated, the driver SHOULD populate
> -Â Â Â the receive queue(s) with buffers of at least 65562 bytes.
> +Â Â Â the receive queue(s) with buffers of at least 65589 bytes plus the size
> +Â Â Â of the \field{struct virtio_net_hdr}.
>Â Â Â \item Otherwise, the driver SHOULD populate the receive queue(s)
> -Â Â Â with buffers of at least 1526 bytes.
> +Â Â Â with buffers of at least 1514 bytes plus the size of the
> +Â Â Â \field{struct virtio_net_hdr}.
>Â Â \end{itemize}
>Â \item If VIRTIO_NET_F_MRG_RXBUF is negotiated, each buffer MUST be at
> -least the size of the struct virtio_net_hdr.
> +least the size of the \field{struct virtio_net_hdr}.
>Â \end{itemize}
>
>Â \begin{note}
> --
> 2.34.1
>
>
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
>
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
>
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/
>

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

virtio-comment message