public inbox for virtio-comment@lists.linux.dev
 help / color / mirror / Atom feed
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Jason Wang <jasowang@redhat.com>
Cc: virtio-comment@lists.linux.dev, lulu@redhat.com,
	nguyenlienviet@google.com
Subject: Re: [PATCH] virtio-net: introduce TSO limit feature
Date: Tue, 14 Oct 2025 04:59:14 -0400	[thread overview]
Message-ID: <20251014044005-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <20251014042243.22087-1-jasowang@redhat.com>

thanks for the patch! yet something to improve.
I note issues only when encountered 1st time but please
do go and check all patch for each issue.


On Tue, Oct 14, 2025 at 12:22:43PM +0800, Jason Wang wrote:
> This patch introduces TSO limit feature which allows the device to
> advertise:
> 
> - Maximum TCP length of a TSO packet or inner TSO packet when UDP
>   tunnel is support
> - Maximum number of segment that can be produced by the device after
>   segmentation of TSO or inner TSO packet of a UDP tunnel
> 
> This is a must to implement TCP jumbogram, as networking stack needs
> to know the limitation of the device in order to produce TSO packet as
> large as possible.

I am not sure I understand the use-case. So how is it used,
exactly?

Why is TSO singled out as compared to USO?

> 
> And it would also help for the case where host has a different TSO
> limitation than the assumption (for example, Linux assumes 64K to be
> the maximum number of segs and payload length).

how does it assume it? 64K segments?

> Signed-off-by: Jason Wang <jasowang@redhat.com>




> ---
>  device-types/net/description.tex | 46 ++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index 415c7fd..e56df75 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -146,6 +146,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>   when VIRTIO_NET_F_IPSEC is negotiated. When a device offers IPsec feature, it SHOULD
>   also offer the VIRTIO_NET_F_OUT_NET_HEADER feature.
>  
> +\item[VIRTIO_NET_F_HOST_TSO_LIMIT(71)] Device limits the maximum TCP
> +  length and the number of segments when performing TCP segmentation.
> +
>  \end{description}
>  
>  \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
> @@ -184,6 +187,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
>  \item[VIRTIO_NET_F_VQ_NOTF_COAL] Requires VIRTIO_NET_F_CTRL_VQ.
>  \item[VIRTIO_NET_F_HASH_TUNNEL] Requires VIRTIO_NET_F_CTRL_VQ along with VIRTIO_NET_F_RSS or VIRTIO_NET_F_HASH_REPORT.
>  \item[VIRTIO_NET_F_RSS_CONTEXT] Requires VIRTIO_NET_F_CTRL_VQ and VIRTIO_NET_F_RSS.
> +\item[VIRTIO_NET_F_HOST_TSO_LIMIT] Requires VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6
>  \end{description}
>  
>  \begin{note}
> @@ -220,6 +224,8 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
>          le16 rss_max_indirection_table_length;
>          le32 supported_hash_types;
>          le32 supported_tunnel_types;
> +        le32 tso_max_size;
> +        le32 tso_max_segs;
>  };
>  \end{lstlisting}
>  
> @@ -276,6 +282,19 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
>  Encapsulation types are defined in \ref{sec:Device Types / Network Device / Device Operation / Processing of Incoming Packets /
>  Hash calculation for incoming packets / Encapsulation types supported/enabled for inner header hash}.
>  
> +The following field, \field{tso_max_size} only exists if
> +VIRTIO_NET_F_HOST_TSO_LIMIT is set.
> +It specifies the maximum TCP length

what is TCP length?

> of a TSO packet

what is a TSO packet? is length likely to be same for TCP6 and TCP4?

> that the
> +device can process.

process in which direction? you mean device can receive?

> When VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO is set,
> +it specifies the maximum inner TCP length of a UDP tunnel TSO packet
> +that the device can process.

Rest of spec talks of " GSO over UDP tunnels packets" is this the same?

even if it's actually unused?

this, on the assumption that the length for tunnel is smaller?

I think this kind of things should be explicit.


> +
> +The following field, \field{tso_max_segs} only exists if
> +VIRTIO_NET_F_HOST_TSO_LIMIT is set.
> +It specifies the maximum number of segments that can be produced by
> +the device after performing segmentation on TSO packet or a UDP tunnel
> +TSO packet (when VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO is set).

I don't get this field at all. the assumption is that all segments
are the same size, right? Then it is just based on length?



> +
>  \devicenormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
>  
>  The device MUST set \field{max_virtqueue_pairs} to between 1 and 0x8000 inclusive,
> @@ -326,6 +345,17 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
>  The device SHOULD NOT offer VIRTIO_NET_F_CTRL_RX_EXTRA if it
>  does not offer VIRTIO_NET_F_CTRL_VQ.
>  
> +If VIRTIO_NET_F_HOST_TSO_LIMIT and VIRTIO_NET_F_MTU have been
> +negotiated, the device SHOULD set \field{tso_max_size} so that a TCP
> +segment that fully utilizes the configured MTU can be processed by TSO
> +(e.g., for IPv4 without options: at least \field{mtu} - 20; for IPv6
> +without extension headers: at least \field{mtu} - 40). This
> +recommendation does not account for IPv4 options or IPv6 extension
> +headers, which reduce the effective segment size.
> +
> +If VIRTIO_NET_F_HOST_TSO_LIMIT has been negotiated, the device MUST
> +set \field{tso_max_segs} to at least 64.

where does this 64 come from? pls document.

> +
>  \drivernormative{\subsubsection}{Device configuration layout}{Device Types / Network Device / Device configuration layout}
>  
>  The driver MUST NOT write to any of the device configuration fields.
> @@ -379,6 +409,22 @@ \subsubsection{Legacy Interface: Device configuration layout}\label{sec:Device T
>  which provided a way for drivers to update the MAC without
>  negotiating VIRTIO_NET_F_CTRL_MAC_ADDR.
>  
> +If the driver negotiates VIRTIO_NET_F_HOST_TSO_LIMIT, it MUST NOT
> +transmit TSO packets with TCP length exceeding \field{tso_max_size}.
> +
> +If the driver negotiates both VIRTIO_NET_F_HOST_TSO_LIMIT and
> +VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO, it MUST NOT transmit UDP tunnel TSO
> +packets with inner TCP length exceeding \field{tso_max_size}.
> +
> +If the driver negotiates VIRTIO_NET_F_HOST_TSO_LIMIT, it MUST NOT
> +transmit TSO packets with \field{gso_size} that would cause the device
> +to generate more than \field{tso_max_segs} segments.

and how does it know?


> +
> +If the driver negotiates both VIRTIO_NET_F_HOST_TSO_LIMIT and
> +VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO, it MUST NOT transmit UDP tunnel TSO
> +packets with \field{gso_size} that would cause the device to generate
> +more than \field{tso_max_segs} segments.
> +
>  \subsection{Device Initialization}\label{sec:Device Types / Network Device / Device Initialization}
>  
>  A driver would perform a typical initialization routine like so:
> -- 
> 2.42.0


  reply	other threads:[~2025-10-14  8:59 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-10-14  4:22 [PATCH] virtio-net: introduce TSO limit feature Jason Wang
2025-10-14  8:59 ` Michael S. Tsirkin [this message]
2025-10-15  4:29   ` Jason Wang
2025-10-15  7:01     ` Michael S. Tsirkin
2025-10-16  5:46       ` Jason Wang
2025-10-16  6:17         ` Michael S. Tsirkin
2025-10-16  6:19         ` Michael S. Tsirkin
2025-10-16  6:22           ` Jason Wang
2025-10-16 10:16             ` Michael S. Tsirkin
2025-10-15  7:27     ` Michael S. Tsirkin
2025-10-16  5:57       ` Jason Wang
2025-10-16  6:17         ` Michael S. Tsirkin
2025-10-16  6:31           ` Jason Wang
2025-10-16 10:02             ` Michael S. Tsirkin
2025-10-20  6:25               ` Jason Wang
2025-10-20  8:19                 ` Michael S. Tsirkin
2025-10-21  3:01                   ` Jason Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20251014044005-mutt-send-email-mst@kernel.org \
    --to=mst@redhat.com \
    --cc=jasowang@redhat.com \
    --cc=lulu@redhat.com \
    --cc=nguyenlienviet@google.com \
    --cc=virtio-comment@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox