[PATCH v8 0/2] Update ECN and Add AccECN feature

[PATCH v8 0/2] Update ECN and Add AccECN feature

[chia-yu.chang]

From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>

  Please find ECN and AccECN patch v8:

v8 (17-Apr-2025)
- Further typo fixing

v7 (15-Apr-2025)
- Fix typos in AccECN patch (Michael S. Tsirkin <mst@redhat.com>)

v6 (14-Apr-2025)
- Shrink descriptions and make more reference to RFC3168 ECN and AccECN

v5 (06-Mar-2025)
- Update feature bits of VIRTIO_NET_F_HOST_ACCECN(69) and VIRTIO_NET_F_GUEST_ACCECN(70)

v4 (05-Mar-2025)
- Update commit message of AccECN patch
- Add reference to RFC3168 ECN and Accurate ECN
- Simplify feature bit descriptinos to avoid duplication of text

v3 (21-Feb-2025)
- Separate the original patch into 2 different patches
- Fix several reabase issues in v2

v2 (17-Feb-2025)
- Rebase the patch on top of v1.4 virtio-spec

Best Regards,
Chia-Yu

Chia-Yu Chang (2):
  virtio-net: Fix ECN feature descriptions
  virtio-net: Define Accurate ECN feature in virtio-spec

 device-types/net/description.tex | 77 ++++++++++++++++++++++----------
 introduction.tex                 |  6 +++
 2 files changed, 60 insertions(+), 23 deletions(-)

-- 
2.34.1

[PATCH v8 1/2] virtio-net: Fix ECN feature descriptions

[chia-yu.chang]

From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>

Clarify that the VIRTIO_NET_HDR_GSO_ECN gso_type flag does not mean that
TCP has IP-ECN set; instead, it identifies that the TCP CWR flag is set
and will be cleared from the second segment of an aggregated segment.
This is used to offload the TCP CWR flag in a way that is compatible with
RFC3168 ECN but is problematic for non-RFC3168 use of the TCP CWR flag.

Foe detailed requirements, please refer to IETF RFC3168:
https://datatracker.ietf.org/doc/html/rfc3168

Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
---
 device-types/net/description.tex | 33 +++++++++++++++++---------------
 introduction.tex                 |  3 +++
 2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 1b6b54d..6b09f0a 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -54,7 +54,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
 
 \item[VIRTIO_NET_F_GUEST_TSO6 (8)] Driver can receive TSOv6.
 
-\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with ECN.
+\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with TCP CWR flag set
+    and follow the ACE bits handling approach mentioned in
+    \hyperref[intro:rfc3168]{[RFC3168]}.
 
 \item[VIRTIO_NET_F_GUEST_UFO (10)] Driver can receive UFO.
 
@@ -62,7 +64,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
 
 \item[VIRTIO_NET_F_HOST_TSO6 (12)] Device can receive TSOv6.
 
-\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with ECN.
+\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with TCP CWR flag set
+    and follow the ACE bits handling approach mentioned in
+    \hyperref[intro:rfc3168]{[RFC3168]}.
 
 \item[VIRTIO_NET_F_HOST_UFO (14)] Device can receive UFO.
 
@@ -695,8 +699,9 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De
 
   \item If the driver negotiated the VIRTIO_NET_F_HOST_ECN feature,
     the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}
-    indicates that the TCP packet has the ECN bit set\footnote{This case is not handled by some older hardware, so is called out
-specifically in the protocol.}.
+    indicates that the TCP packet has TCP CWR flag set, and the flag will be handled differently for all segments of
+    an aggregated segment, as mentioned in \hyperref[intro:rfc3168]{[RFC3168]}
+    \footnote{This case is not handled by some older hardware, so is called out specifically in the protocol.}.
    \end{itemize}
 
 \item If the driver negotiated the VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO feature and the
@@ -788,10 +793,9 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De
 \field{gso_type} to VIRTIO_NET_HDR_GSO_UDP_L4.
 
 The driver SHOULD NOT send to the device TCP packets requiring segmentation offload
-which have the Explicit Congestion Notification bit set, unless the
-VIRTIO_NET_F_HOST_ECN feature is negotiated, in which case the
-driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in
-\field{gso_type}.
+which have the TCP CWR flag set and require the flag be handled as mentioned in
+\hyperref[intro:rfc3168]{[RFC3168]}, unless the VIRTIO_NET_F_HOST_ECN feature is
+negotiated, in which case the driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
 
 If VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO is negotiated, the driver MAY set
 VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 bit or the VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 bit
@@ -1105,9 +1109,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
 \end{enumerate}
 
 Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP, UDP_TUNNEL
-and ECN features enable receive checksum, large receive offload and ECN
-support which are the input equivalents of the transmit checksum,
-transmit segmentation offloading and ECN features, as described
+and ECN features enable receive checksum, large receive offload and RFC3168
+ECN support which are the input equivalents of the transmit checksum,
+transmit segmentation offloading and RFC3168 ECN features, as described
 in \ref{sec:Device Types / Network Device / Device Operation /
 Packet Transmission}:
 \begin{enumerate}
@@ -1210,10 +1214,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
 the VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM bit in \field{flags}.
 
 The device SHOULD NOT send to the driver TCP packets requiring segmentation offload
-which have the Explicit Congestion Notification bit set, unless the
-VIRTIO_NET_F_GUEST_ECN feature is negotiated, in which case the
-device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in
-\field{gso_type}.
+which have the TCP CWR flag set and require the flag be handled as mentioned in
+\hyperref[intro:rfc3168]{[RFC3168]}, unless the VIRTIO_NET_F_GUEST_ECN feature is
+negotiated, in which case the device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
 
 If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the
 device MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in
diff --git a/introduction.tex b/introduction.tex
index e60298a..d52622e 100644
--- a/introduction.tex
+++ b/introduction.tex
@@ -168,6 +168,9 @@ \section{Normative References}\label{sec:Normative References}
     Leiba, B., "Ambiguity of Uppercase vs Lowercase in RFC 2119 Key Words", BCP
     14, RFC 8174, DOI 10.17487/RFC8174, May 2017
         \newline\url{http://www.ietf.org/rfc/rfc8174.txt}\\
+	\phantomsection\label{intro:rfc3168}\textbf{[RFC3168]} &
+    S. Floyd., ``The Addition of Explicit Congestion Notification (ECN) to IP'', September 2001.
+	\newline\url{http://www.ietf.org/rfc/rfc3168.txt}\\
 \end{longtable}
 
 \section{Non-Normative References}
-- 
2.34.1

[PATCH v8 2/2] virtio-net: Define Accurate ECN feature in virtio-spec

[chia-yu.chang]

From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>

This change implements Accurate ECN based on the AccECN specification:
  https://tools.ietf.org/id/draft-ietf-tcpm-accurate-ecn-28.txt

Unlike RFC3168 ECN, Accurate ECN uses the TCP CWR flag as part of the ACE
field to count new packets with CE marks in the IP-ECN field; however,
RFC 3168 ECN-aware TSO will clean the TCP CWR flag from the 2nd segment of
an aggregated segment. Therefore, fallback shall be applied by setting
NETIF_F_GSO_ACCECN to ensure that the CWR flag should not be changed within
the aggregated segment (e.g., super-skb in Linux).

To apply it in virtio-spec, new feature bits for host and guest are added
for feature negotiation between driver and device. And the translation of
the Accurate ECN GSO flag between virtio_net_hdr and skb header for
NETIF_F_GSO_ACCECN is also added to avoid CWR flag corruption due to RFC3168
ECN TSO.

Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
---
 device-types/net/description.tex | 50 +++++++++++++++++++++++++-------
 introduction.tex                 |  3 ++
 2 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/device-types/net/description.tex b/device-types/net/description.tex
index 6b09f0a..cd5c38f 100644
--- a/device-types/net/description.tex
+++ b/device-types/net/description.tex
@@ -140,6 +140,14 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
 
 \item[VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO_CSUM (68)] Device handles packets
   carried by a UDP tunnel with partial csum for the outer header.
+
+\item[VIRTIO_NET_F_HOST_ACCECN (69)] Device can receive TSO with TCP CWR flag set
+  and follow the ACE bits handling approach mentioned in
+  \hyperref[intro:accecn]{[AccECN]}.
+
+\item[VIRTIO_NET_F_GUEST_ACCECN (70)] Driver can receive TSO with TCP CWR flag set
+  and follow the ACE bits handling approach mentioned in
+  \hyperref[intro:accecn]{[AccECN]}.
 \end{description}
 
 \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
@@ -151,6 +159,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
 \item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM.
 \item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM.
 \item[VIRTIO_NET_F_GUEST_ECN] Requires VIRTIO_NET_F_GUEST_TSO4 or VIRTIO_NET_F_GUEST_TSO6.
+\item[VIRTIO_NET_F_GUEST_ACCECN] Requires VIRTIO_NET_F_GUEST_TSO4 or VIRTIO_NET_F_GUEST_TSO6.
 \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
 \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
 \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
@@ -161,6 +170,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
 \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
 \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
 \item[VIRTIO_NET_F_HOST_ECN] Requires VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6.
+\item[VIRTIO_NET_F_HOST_ACCECN] Requires VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6.
 \item[VIRTIO_NET_F_HOST_UFO] Requires VIRTIO_NET_F_CSUM.
 \item[VIRTIO_NET_F_HOST_USO] Requires VIRTIO_NET_F_CSUM.
 \item[VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO] Requires VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_TSO6
@@ -284,11 +294,11 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
 The device MUST NOT modify \field{mtu} once it has been set.
 
 The device MUST NOT pass received packets that exceed \field{mtu} (plus low
-level ethernet header length) size with \field{gso_type} NONE or ECN
+level ethernet header length) size with \field{gso_type} NONE, ECN or ACCECN
 after VIRTIO_NET_F_MTU has been successfully negotiated.
 
 The device MUST forward transmitted packets of up to \field{mtu} (plus low
-level ethernet header length) size with \field{gso_type} NONE or ECN, and do
+level ethernet header length) size with \field{gso_type} NONE, ECN or ACCECN, and do
 so without fragmentation, after VIRTIO_NET_F_MTU has been successfully
 negotiated.
 
@@ -338,11 +348,11 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
 
 If the driver negotiates VIRTIO_NET_F_MTU, it MUST supply enough receive
 buffers to receive at least one receive packet of size \field{mtu} (plus low
-level ethernet header length) with \field{gso_type} NONE or ECN.
+level ethernet header length) with \field{gso_type} NONE, ECN or ACCECN.
 
 If the driver negotiates VIRTIO_NET_F_MTU, it MUST NOT transmit packets of
 size exceeding the value of \field{mtu} (plus low level ethernet header length)
-with \field{gso_type} NONE or ECN.
+with \field{gso_type} NONE, ECN or ACCECN.
 
 A driver SHOULD negotiate the VIRTIO_NET_F_STANDBY feature if the device offers it.
 
@@ -433,7 +443,7 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
    The VIRTIO_NET_F_GUEST_CSUM feature indicates that partially
   checksummed packets can be received, and if it can do that then
   the VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
-  VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_USO4,
+  VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_ACCECN, VIRTIO_NET_F_GUEST_USO4,
   VIRTIO_NET_F_GUEST_USO6 VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO and
   VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM are the input equivalents of
   the features described above.
@@ -592,6 +602,7 @@ \subsection{Device Operation}\label{sec:Device Types / Network Device / Device O
 #define VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 0x20
 #define VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 0x40
 #define VIRTIO_NET_HDR_GSO_ECN      0x80
+#define VIRTIO_NET_HDR_GSO_ACCECN   0x10
         u8 gso_type;
         le16 hdr_len;
         le16 gso_size;
@@ -702,6 +713,12 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De
     indicates that the TCP packet has TCP CWR flag set, and the flag will be handled differently for all segments of
     an aggregated segment, as mentioned in \hyperref[intro:rfc3168]{[RFC3168]}
     \footnote{This case is not handled by some older hardware, so is called out specifically in the protocol.}.
+
+  \item If the driver negotiated the VIRTIO_NET_F_HOST_ACCECN feature,
+    the VIRTIO_NET_HDR_GSO_ACCECN bit in \field{gso_type}
+    indicates that the TCP packet has TCP CWR flag set, and the flag will be applied to all segments of an aggregated
+    segment, as mentioned in \hyperref[intro:accecn]{[AccECN]}
+    \footnote{This case is not handled by some older hardware, so is called out specifically in the protocol.}.
    \end{itemize}
 
 \item If the driver negotiated the VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO feature and the
@@ -797,6 +814,11 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De
 \hyperref[intro:rfc3168]{[RFC3168]}, unless the VIRTIO_NET_F_HOST_ECN feature is
 negotiated, in which case the driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
 
+The driver SHOULD NOT send to the device TCP packets requiring segmentation offload
+which have the TCP CWR flag set and require the flag be applied as mentioned in
+\hyperref[intro:accecn]{[AccECN]}, unless the VIRTIO_NET_F_HOST_ACCECN feature is
+negotiated, in which case the driver MUST set the VIRTIO_NET_HDR_GSO_ACCECN bit in \field{gso_type}.
+
 If VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO is negotiated, the driver MAY set
 VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 bit or the VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 bit
 in \field{gso_type} according to the inner network header protocol type
@@ -1108,12 +1130,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
   in case of tunnels) has been validated.
 \end{enumerate}
 
-Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP, UDP_TUNNEL
-and ECN features enable receive checksum, large receive offload and RFC3168
-ECN support which are the input equivalents of the transmit checksum,
-transmit segmentation offloading and RFC3168 ECN features, as described
-in \ref{sec:Device Types / Network Device / Device Operation /
-Packet Transmission}:
+Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP, UDP_TUNNEL, ECN and
+ACCECN features enable receive checksum, large receive offload, RFC3168 ECN
+and Accurate ECN support which are the input equivalents of the transmit
+checksum, transmit segmentation offloading, RFC3168 ECN and Accurate ECN
+features, as described in \ref{sec:Device Types / Network Device /
+Device Operation / Packet Transmission}:
 \begin{enumerate}
 \item If the VIRTIO_NET_F_GUEST_TSO4, TSO6, UFO, USO4 or USO6 options were
   negotiated, then \field{gso_type} MAY be something other than
@@ -1218,6 +1240,11 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
 \hyperref[intro:rfc3168]{[RFC3168]}, unless the VIRTIO_NET_F_GUEST_ECN feature is
 negotiated, in which case the device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
 
+The device SHOULD NOT send to the driver TCP packets requiring segmentation offload
+which have the TCP CWR flag set and require the flag be handled as mentioned in
+\hyperref[intro:accecn]{[AccECN]}, unless the VIRTIO_NET_F_GUEST_ACCECN feature is
+negotiated, in which case the device MUST set the VIRTIO_NET_HDR_GSO_ACCECN bit in \field{gso_type}.
+
 If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the
 device MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in
 \field{flags}, if so:
@@ -2193,6 +2220,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
 #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM 47
 #define VIRTIO_NET_F_GUEST_USO4       54
 #define VIRTIO_NET_F_GUEST_USO6       55
+#define VIRTIO_NET_F_GUEST_ACCECN     70
 
 #define VIRTIO_NET_CTRL_GUEST_OFFLOADS       5
  #define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET   0
diff --git a/introduction.tex b/introduction.tex
index d52622e..9320ca1 100644
--- a/introduction.tex
+++ b/introduction.tex
@@ -171,6 +171,9 @@ \section{Normative References}\label{sec:Normative References}
 	\phantomsection\label{intro:rfc3168}\textbf{[RFC3168]} &
     S. Floyd., ``The Addition of Explicit Congestion Notification (ECN) to IP'', September 2001.
 	\newline\url{http://www.ietf.org/rfc/rfc3168.txt}\\
+        \phantomsection\label{intro:accecn}\textbf{[AccECN]} &
+    B. Briscoe., ``More Accurate Explicit Congestion Notification (AccECN) Feedback in TCP'', February 2025.
+        \newline\url{https://www.ietf.org/archive/id/draft-ietf-tcpm-accurate-ecn-33.txt}\\
 \end{longtable}
 
 \section{Non-Normative References}
-- 
2.34.1

RE: [PATCH v8 0/2] Update ECN and Add AccECN feature

[Chia-Yu Chang (Nokia)]

> -----Original Message-----
> From: chia-yu.chang@nokia-bell-labs.com <chia-yu.chang@nokia-bell-labs.com> 
> Sent: Friday, April 18, 2025 12:41 AM
> To: virtio-comment@lists.linux.dev; mst@redhat.com; cohuck@redhat.com; mvaralar@redhat.com; jasowang@redhat.com; xuanzhuo@linux.alibaba.com; eperezma@redhat.com; ij@kernel.org; ncardwell@google.com; Koen De Schepper (Nokia) <koen.de_schepper@nokia-bell-labs.com>; g.white <g.white@cablelabs.com>; vidhi_goel <vidhi_goel@apple.com>; ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com
> Cc: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
> Subject: [PATCH v8 0/2] Update ECN and Add AccECN feature
> 
> From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> 
>   Please find ECN and AccECN patch v8:
> 
> v8 (17-Apr-2025)
> - Further typo fixing
> 
> v7 (15-Apr-2025)
> - Fix typos in AccECN patch (Michael S. Tsirkin <mst@redhat.com>)
> 
> v6 (14-Apr-2025)
> - Shrink descriptions and make more reference to RFC3168 ECN and AccECN
> 
> v5 (06-Mar-2025)
> - Update feature bits of VIRTIO_NET_F_HOST_ACCECN(69) and VIRTIO_NET_F_GUEST_ACCECN(70)
> 
> v4 (05-Mar-2025)
> - Update commit message of AccECN patch
> - Add reference to RFC3168 ECN and Accurate ECN
> - Simplify feature bit descriptinos to avoid duplication of text
> 
> v3 (21-Feb-2025)
> - Separate the original patch into 2 different patches
> - Fix several reabase issues in v2
> 
> v2 (17-Feb-2025)
> - Rebase the patch on top of v1.4 virtio-spec
> 
> Best Regards,
> Chia-Yu
> 
> Chia-Yu Chang (2):
>   virtio-net: Fix ECN feature descriptions
>   virtio-net: Define Accurate ECN feature in virtio-spec
> 
>  device-types/net/description.tex | 77 ++++++++++++++++++++++----------
>  introduction.tex                 |  6 +++
>  2 files changed, 60 insertions(+), 23 deletions(-)
> 
> -- 
> 2.34.1

Hi Michael,

May I ask is there any update on this patch series?
If anything is needed from my end, please let me know.

Best Regards,
Chia-Yu

Re: [PATCH v8 1/2] virtio-net: Fix ECN feature descriptions

[Michael S. Tsirkin]

On Fri, Apr 18, 2025 at 12:40:43AM +0200, chia-yu.chang@nokia-bell-labs.com wrote:
> From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> 
> Clarify that the VIRTIO_NET_HDR_GSO_ECN gso_type flag does not mean that
> TCP has IP-ECN set; instead, it identifies that the TCP CWR flag is set
> and will be cleared from the second segment of an aggregated segment.
> This is used to offload the TCP CWR flag in a way that is compatible with
> RFC3168 ECN but is problematic for non-RFC3168 use of the TCP CWR flag.
> 
> Foe detailed requirements, please refer to IETF RFC3168:
> https://datatracker.ietf.org/doc/html/rfc3168
> 
> Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> ---
>  device-types/net/description.tex | 33 +++++++++++++++++---------------
>  introduction.tex                 |  3 +++
>  2 files changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index 1b6b54d..6b09f0a 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -54,7 +54,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>  
>  \item[VIRTIO_NET_F_GUEST_TSO6 (8)] Driver can receive TSOv6.
>  
> -\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with ECN.
> +\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with TCP CWR flag set
> +    and follow the ACE bits handling approach mentioned in
> +    \hyperref[intro:rfc3168]{[RFC3168]}.
>  
>  \item[VIRTIO_NET_F_GUEST_UFO (10)] Driver can receive UFO.
>  
> @@ -62,7 +64,9 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>  
>  \item[VIRTIO_NET_F_HOST_TSO6 (12)] Device can receive TSOv6.
>  
> -\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with ECN.
> +\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with TCP CWR flag set
> +    and follow the ACE bits handling approach mentioned in
> +    \hyperref[intro:rfc3168]{[RFC3168]}.

What is "the ACE bits handling approach"? It's agrammatical to use
a plural while reducing the relative in english.

I looked at RFC3168 and I see no mention of ACE anywhere.

Also "mentioned in" is weirdly informal.
described maybe?
which part of the
spec do you refer to?

you should also expand acronims at first use.

>  
>  \item[VIRTIO_NET_F_HOST_UFO (14)] Device can receive UFO.
>  
> @@ -695,8 +699,9 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De
>  
>    \item If the driver negotiated the VIRTIO_NET_F_HOST_ECN feature,
>      the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}
> -    indicates that the TCP packet has the ECN bit set\footnote{This case is not handled by some older hardware, so is called out
> -specifically in the protocol.}.
> +    indicates that the TCP packet has TCP CWR flag set, and the flag will be handled differently for all segments of

the CWR flag.

> +    an aggregated segment, as mentioned in \hyperref[intro:rfc3168]{[RFC3168]}

i could not find this in the spec either.
"segment" is mentioned once, when talking about MSS.

> +    \footnote{This case is not handled by some older hardware, so is called out specifically in the protocol.}.

what does this refer to? which protocol? hardware older than what?

>     \end{itemize}
>  
>  \item If the driver negotiated the VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO feature and the
> @@ -788,10 +793,9 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De
>  \field{gso_type} to VIRTIO_NET_HDR_GSO_UDP_L4.
>  
>  The driver SHOULD NOT send to the device TCP packets requiring segmentation offload
> -which have the Explicit Congestion Notification bit set, unless the
> -VIRTIO_NET_F_HOST_ECN feature is negotiated, in which case the
> -driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in
> -\field{gso_type}.
> +which have the TCP CWR flag set and require the flag be handled as mentioned in
> +\hyperref[intro:rfc3168]{[RFC3168]},

still confusing.
should not send packets that require the flag? or should not require the
flag?
if the former, "and which would require" ... if the later "and should not
require".
or better, make sentences shorter.


>c unless the VIRTIO_NET_F_HOST_ECN feature is
> +negotiated, in which case the driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.

in which case? when it is or when it is not negotiated? or when it sends
packets maybe?

>  
>  If VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO is negotiated, the driver MAY set
>  VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 bit or the VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 bit
> @@ -1105,9 +1109,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>  \end{enumerate}
>  
>  Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP, UDP_TUNNEL
> -and ECN features enable receive checksum, large receive offload and ECN
> -support which are the input equivalents of the transmit checksum,
> -transmit segmentation offloading and ECN features, as described
> +and ECN features enable receive checksum, large receive offload and RFC3168
> +ECN support which are the input equivalents of the transmit checksum,
> +transmit segmentation offloading and RFC3168 ECN features, as described
>  in \ref{sec:Device Types / Network Device / Device Operation /
>  Packet Transmission}:
>  \begin{enumerate}
> @@ -1210,10 +1214,9 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>  the VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM bit in \field{flags}.
>  
>  The device SHOULD NOT send to the driver TCP packets requiring segmentation offload
> -which have the Explicit Congestion Notification bit set, unless the
> -VIRTIO_NET_F_GUEST_ECN feature is negotiated, in which case the
> -device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in
> -\field{gso_type}.
> +which have the TCP CWR flag set and require the flag be handled as mentioned in
> +\hyperref[intro:rfc3168]{[RFC3168]}, unless the VIRTIO_NET_F_GUEST_ECN feature is
> +negotiated, in which case the device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
>  
>  If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the
>  device MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in
> diff --git a/introduction.tex b/introduction.tex
> index e60298a..d52622e 100644
> --- a/introduction.tex
> +++ b/introduction.tex
> @@ -168,6 +168,9 @@ \section{Normative References}\label{sec:Normative References}
>      Leiba, B., "Ambiguity of Uppercase vs Lowercase in RFC 2119 Key Words", BCP
>      14, RFC 8174, DOI 10.17487/RFC8174, May 2017
>          \newline\url{http://www.ietf.org/rfc/rfc8174.txt}\\
> +	\phantomsection\label{intro:rfc3168}\textbf{[RFC3168]} &
> +    S. Floyd., ``The Addition of Explicit Congestion Notification (ECN) to IP'', September 2001.
> +	\newline\url{http://www.ietf.org/rfc/rfc3168.txt}\\
>  \end{longtable}
>  
>  \section{Non-Normative References}
> -- 
> 2.34.1

Re: [PATCH v8 2/2] virtio-net: Define Accurate ECN feature in virtio-spec

[Michael S. Tsirkin]

On Fri, Apr 18, 2025 at 12:40:44AM +0200, chia-yu.chang@nokia-bell-labs.com wrote:
> From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> 
> This change implements Accurate ECN based on the AccECN specification:
>   https://tools.ietf.org/id/draft-ietf-tcpm-accurate-ecn-28.txt
> 
> Unlike RFC3168 ECN, Accurate ECN uses the TCP CWR flag as part of the ACE
> field to count new packets with CE marks in the IP-ECN field; however,
> RFC 3168 ECN-aware TSO will clean the TCP CWR flag from the 2nd segment of
> an aggregated segment. Therefore, fallback shall be applied by setting
> NETIF_F_GSO_ACCECN to ensure that the CWR flag should not be changed within
> the aggregated segment (e.g., super-skb in Linux).
> 
> To apply it in virtio-spec, new feature bits for host and guest are added
> for feature negotiation between driver and device. And the translation of
> the Accurate ECN GSO flag between virtio_net_hdr and skb header for
> NETIF_F_GSO_ACCECN is also added to avoid CWR flag corruption due to RFC3168
> ECN TSO.
> 
> Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> ---
>  device-types/net/description.tex | 50 +++++++++++++++++++++++++-------
>  introduction.tex                 |  3 ++
>  2 files changed, 42 insertions(+), 11 deletions(-)
> 
> diff --git a/device-types/net/description.tex b/device-types/net/description.tex
> index 6b09f0a..cd5c38f 100644
> --- a/device-types/net/description.tex
> +++ b/device-types/net/description.tex
> @@ -140,6 +140,14 @@ \subsection{Feature bits}\label{sec:Device Types / Network Device / Feature bits
>  
>  \item[VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO_CSUM (68)] Device handles packets
>    carried by a UDP tunnel with partial csum for the outer header.
> +
> +\item[VIRTIO_NET_F_HOST_ACCECN (69)] Device can receive TSO with TCP CWR flag set
> +  and follow the ACE bits handling approach mentioned in
> +  \hyperref[intro:accecn]{[AccECN]}.
> +
> +\item[VIRTIO_NET_F_GUEST_ACCECN (70)] Driver can receive TSO with TCP CWR flag set
> +  and follow the ACE bits handling approach mentioned in
> +  \hyperref[intro:accecn]{[AccECN]}.
>  \end{description}


same comments as patch 1

>  \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device / Feature bits / Feature bit requirements}
> @@ -151,6 +159,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
>  \item[VIRTIO_NET_F_GUEST_TSO4] Requires VIRTIO_NET_F_GUEST_CSUM.
>  \item[VIRTIO_NET_F_GUEST_TSO6] Requires VIRTIO_NET_F_GUEST_CSUM.
>  \item[VIRTIO_NET_F_GUEST_ECN] Requires VIRTIO_NET_F_GUEST_TSO4 or VIRTIO_NET_F_GUEST_TSO6.
> +\item[VIRTIO_NET_F_GUEST_ACCECN] Requires VIRTIO_NET_F_GUEST_TSO4 or VIRTIO_NET_F_GUEST_TSO6.
>  \item[VIRTIO_NET_F_GUEST_UFO] Requires VIRTIO_NET_F_GUEST_CSUM.
>  \item[VIRTIO_NET_F_GUEST_USO4] Requires VIRTIO_NET_F_GUEST_CSUM.
>  \item[VIRTIO_NET_F_GUEST_USO6] Requires VIRTIO_NET_F_GUEST_CSUM.
> @@ -161,6 +170,7 @@ \subsubsection{Feature bit requirements}\label{sec:Device Types / Network Device
>  \item[VIRTIO_NET_F_HOST_TSO4] Requires VIRTIO_NET_F_CSUM.
>  \item[VIRTIO_NET_F_HOST_TSO6] Requires VIRTIO_NET_F_CSUM.
>  \item[VIRTIO_NET_F_HOST_ECN] Requires VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6.
> +\item[VIRTIO_NET_F_HOST_ACCECN] Requires VIRTIO_NET_F_HOST_TSO4 or VIRTIO_NET_F_HOST_TSO6.
>  \item[VIRTIO_NET_F_HOST_UFO] Requires VIRTIO_NET_F_CSUM.
>  \item[VIRTIO_NET_F_HOST_USO] Requires VIRTIO_NET_F_CSUM.
>  \item[VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO] Requires VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_TSO6
> @@ -284,11 +294,11 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
>  The device MUST NOT modify \field{mtu} once it has been set.
>  
>  The device MUST NOT pass received packets that exceed \field{mtu} (plus low
> -level ethernet header length) size with \field{gso_type} NONE or ECN
> +level ethernet header length) size with \field{gso_type} NONE, ECN or ACCECN
>  after VIRTIO_NET_F_MTU has been successfully negotiated.
>  
>  The device MUST forward transmitted packets of up to \field{mtu} (plus low
> -level ethernet header length) size with \field{gso_type} NONE or ECN, and do
> +level ethernet header length) size with \field{gso_type} NONE, ECN or ACCECN, and do
>  so without fragmentation, after VIRTIO_NET_F_MTU has been successfully
>  negotiated.
>  
> @@ -338,11 +348,11 @@ \subsection{Device configuration layout}\label{sec:Device Types / Network Device
>  
>  If the driver negotiates VIRTIO_NET_F_MTU, it MUST supply enough receive
>  buffers to receive at least one receive packet of size \field{mtu} (plus low
> -level ethernet header length) with \field{gso_type} NONE or ECN.
> +level ethernet header length) with \field{gso_type} NONE, ECN or ACCECN.
>  
>  If the driver negotiates VIRTIO_NET_F_MTU, it MUST NOT transmit packets of
>  size exceeding the value of \field{mtu} (plus low level ethernet header length)
> -with \field{gso_type} NONE or ECN.
> +with \field{gso_type} NONE, ECN or ACCECN.
>  
>  A driver SHOULD negotiate the VIRTIO_NET_F_STANDBY feature if the device offers it.
>  
> @@ -433,7 +443,7 @@ \subsection{Device Initialization}\label{sec:Device Types / Network Device / Dev
>     The VIRTIO_NET_F_GUEST_CSUM feature indicates that partially
>    checksummed packets can be received, and if it can do that then
>    the VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
> -  VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_USO4,
> +  VIRTIO_NET_F_GUEST_UFO, VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_ACCECN, VIRTIO_NET_F_GUEST_USO4,
>    VIRTIO_NET_F_GUEST_USO6 VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO and
>    VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM are the input equivalents of
>    the features described above.
> @@ -592,6 +602,7 @@ \subsection{Device Operation}\label{sec:Device Types / Network Device / Device O
>  #define VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 0x20
>  #define VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 0x40
>  #define VIRTIO_NET_HDR_GSO_ECN      0x80
> +#define VIRTIO_NET_HDR_GSO_ACCECN   0x10
>          u8 gso_type;
>          le16 hdr_len;
>          le16 gso_size;
> @@ -702,6 +713,12 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De
>      indicates that the TCP packet has TCP CWR flag set, and the flag will be handled differently for all segments of
>      an aggregated segment, as mentioned in \hyperref[intro:rfc3168]{[RFC3168]}
>      \footnote{This case is not handled by some older hardware, so is called out specifically in the protocol.}.
> +
> +  \item If the driver negotiated the VIRTIO_NET_F_HOST_ACCECN feature,
> +    the VIRTIO_NET_HDR_GSO_ACCECN bit in \field{gso_type}
> +    indicates that the TCP packet has TCP CWR flag set, and the flag will be applied to all segments of an aggregated

the CWR

> +    segment, as mentioned in \hyperref[intro:accecn]{[AccECN]}
> +    \footnote{This case is not handled by some older hardware, so is called out specifically in the protocol.}.
>     \end{itemize}
>  
>  \item If the driver negotiated the VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO feature and the
> @@ -797,6 +814,11 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De
>  \hyperref[intro:rfc3168]{[RFC3168]}, unless the VIRTIO_NET_F_HOST_ECN feature is
>  negotiated, in which case the driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
>  
> +The driver SHOULD NOT send to the device TCP packets requiring segmentation offload
> +which have the TCP CWR flag set and require the flag be applied as mentioned in
> +\hyperref[intro:accecn]{[AccECN]}, unless the VIRTIO_NET_F_HOST_ACCECN feature is
> +negotiated, in which case the driver MUST set the VIRTIO_NET_HDR_GSO_ACCECN bit in \field{gso_type}.
> +

same comments as patch 1

>  If VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO is negotiated, the driver MAY set
>  VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 bit or the VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 bit
>  in \field{gso_type} according to the inner network header protocol type
> @@ -1108,12 +1130,12 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>    in case of tunnels) has been validated.
>  \end{enumerate}
>  
> -Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP, UDP_TUNNEL
> -and ECN features enable receive checksum, large receive offload and RFC3168
> -ECN support which are the input equivalents of the transmit checksum,
> -transmit segmentation offloading and RFC3168 ECN features, as described
> -in \ref{sec:Device Types / Network Device / Device Operation /
> -Packet Transmission}:
> +Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP, UDP_TUNNEL, ECN and
> +ACCECN features enable receive checksum, large receive offload, RFC3168 ECN
> +and Accurate ECN support which are the input equivalents of the transmit
> +checksum, transmit segmentation offloading, RFC3168 ECN and Accurate ECN
> +features, as described in \ref{sec:Device Types / Network Device /
> +Device Operation / Packet Transmission}:
>  \begin{enumerate}
>  \item If the VIRTIO_NET_F_GUEST_TSO4, TSO6, UFO, USO4 or USO6 options were
>    negotiated, then \field{gso_type} MAY be something other than
> @@ -1218,6 +1240,11 @@ \subsubsection{Processing of Incoming Packets}\label{sec:Device Types / Network
>  \hyperref[intro:rfc3168]{[RFC3168]}, unless the VIRTIO_NET_F_GUEST_ECN feature is
>  negotiated, in which case the device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
>  
> +The device SHOULD NOT send to the driver TCP packets requiring segmentation offload
> +which have the TCP CWR flag set and require the flag be handled as mentioned in
> +\hyperref[intro:accecn]{[AccECN]}, unless the VIRTIO_NET_F_GUEST_ACCECN feature is
> +negotiated, in which case the device MUST set the VIRTIO_NET_HDR_GSO_ACCECN bit in \field{gso_type}.
> +

same comments as patch 1

>  If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the
>  device MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in
>  \field{flags}, if so:
> @@ -2193,6 +2220,7 @@ \subsubsection{Control Virtqueue}\label{sec:Device Types / Network Device / Devi
>  #define VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO_CSUM 47
>  #define VIRTIO_NET_F_GUEST_USO4       54
>  #define VIRTIO_NET_F_GUEST_USO6       55
> +#define VIRTIO_NET_F_GUEST_ACCECN     70
>  
>  #define VIRTIO_NET_CTRL_GUEST_OFFLOADS       5
>   #define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET   0
> diff --git a/introduction.tex b/introduction.tex
> index d52622e..9320ca1 100644
> --- a/introduction.tex
> +++ b/introduction.tex
> @@ -171,6 +171,9 @@ \section{Normative References}\label{sec:Normative References}
>  	\phantomsection\label{intro:rfc3168}\textbf{[RFC3168]} &
>      S. Floyd., ``The Addition of Explicit Congestion Notification (ECN) to IP'', September 2001.
>  	\newline\url{http://www.ietf.org/rfc/rfc3168.txt}\\
> +        \phantomsection\label{intro:accecn}\textbf{[AccECN]} &
> +    B. Briscoe., ``More Accurate Explicit Congestion Notification (AccECN) Feedback in TCP'', February 2025.
> +        \newline\url{https://www.ietf.org/archive/id/draft-ietf-tcpm-accurate-ecn-33.txt}\\
>  \end{longtable}
>  
>  \section{Non-Normative References}
> -- 
> 2.34.1

RE: [PATCH v8 1/2] virtio-net: Fix ECN feature descriptions

[Chia-Yu Chang (Nokia)]

> -----Original Message-----
> From: Michael S. Tsirkin <mst@redhat.com> 
> Sent: Monday, April 28, 2025 10:00 AM
> To: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
> Cc: virtio-comment@lists.linux.dev; cohuck@redhat.com; mvaralar@redhat.com; jasowang@redhat.com; xuanzhuo@linux.alibaba.com; eperezma@redhat.com; ij@kernel.org; ncardwell@google.com; Koen De Schepper (Nokia) <koen.de_schepper@nokia-bell-labs.com>; g.white <g.white@cablelabs.com>; vidhi_goel <vidhi_goel@apple.com>; ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com
> Subject: Re: [PATCH v8 1/2] virtio-net: Fix ECN feature descriptions
> 
> 
> CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
> 
> 
> 
> On Fri, Apr 18, 2025 at 12:40:43AM +0200, chia-yu.chang@nokia-bell-labs.com wrote:
> > From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> >
> > Clarify that the VIRTIO_NET_HDR_GSO_ECN gso_type flag does not mean 
> > that TCP has IP-ECN set; instead, it identifies that the TCP CWR flag 
> > is set and will be cleared from the second segment of an aggregated segment.
> > This is used to offload the TCP CWR flag in a way that is compatible 
> > with
> > RFC3168 ECN but is problematic for non-RFC3168 use of the TCP CWR flag.
> >
> > Foe detailed requirements, please refer to IETF RFC3168:
> > https://datatracker.ietf.org/doc/html/rfc3168
> >
> > Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > ---
> >  device-types/net/description.tex | 33 +++++++++++++++++---------------
> >  introduction.tex                 |  3 +++
> >  2 files changed, 21 insertions(+), 15 deletions(-)
> >
> > diff --git a/device-types/net/description.tex 
> > b/device-types/net/description.tex
> > index 1b6b54d..6b09f0a 100644
> > --- a/device-types/net/description.tex
> > +++ b/device-types/net/description.tex
> > @@ -54,7 +54,9 @@ \subsection{Feature bits}\label{sec:Device Types / 
> > Network Device / Feature bits
> >
> >  \item[VIRTIO_NET_F_GUEST_TSO6 (8)] Driver can receive TSOv6.
> >
> > -\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with ECN.
> > +\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with TCP CWR flag set
> > +    and follow the ACE bits handling approach mentioned in
> > +    \hyperref[intro:rfc3168]{[RFC3168]}.
> >
> >  \item[VIRTIO_NET_F_GUEST_UFO (10)] Driver can receive UFO.
> >
> > @@ -62,7 +64,9 @@ \subsection{Feature bits}\label{sec:Device Types / 
> > Network Device / Feature bits
> >
> >  \item[VIRTIO_NET_F_HOST_TSO6 (12)] Device can receive TSOv6.
> >
> > -\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with ECN.
> > +\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with TCP CWR flag set
> > +    and follow the ACE bits handling approach mentioned in
> > +    \hyperref[intro:rfc3168]{[RFC3168]}.
> 
> What is "the ACE bits handling approach"? It's agrammatical to use a plural while reducing the relative in english.
> 
> I looked at RFC3168 and I see no mention of ACE anywhere.
> 
> Also "mentioned in" is weirdly informal.
> described maybe?
> which part of the
> spec do you refer to?
> 
> you should also expand acronims at first use.

Hi Michael,

Thanks for the comments.

RFC3168 does not explicitly discuss offloading, and one relevant statement is "When the TCP data sender is ready to set the CWR bit after reducing the congestion window, it SHOULD set the CWR bit only on the first new data packet that it transmits."
Therefore, IMO, RFC3168 does not provide a clear specification of what occurs when offloading is applied.
While the ACE field is a new terminology introduced in the AccECN draft, it incorporates the AE, CWR, and ECE flags.

To make the virtio spec clear (i.e., without expanding on the implied information that should be in the RFC) and simple to grasp, I would propose the following changes.
"Device can receive TSO with the CWR flag set and follow the SHOULD requirements of the CWR bit described in Section 6.1 of RFC3168."

> >
> >  \item[VIRTIO_NET_F_HOST_UFO (14)] Device can receive UFO.
> >
> > @@ -695,8 +699,9 @@ \subsubsection{Packet 
> > Transmission}\label{sec:Device Types / Network Device / De
> >
> >    \item If the driver negotiated the VIRTIO_NET_F_HOST_ECN feature,
> >      the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}
> > -    indicates that the TCP packet has the ECN bit set\footnote{This case is not handled by some older hardware, so is called out
> > -specifically in the protocol.}.
> > +    indicates that the TCP packet has TCP CWR flag set, and the flag 
> > + will be handled differently for all segments of
> 
> the CWR flag.

Will be changed.

> 
> > +    an aggregated segment, as mentioned in 
> > + \hyperref[intro:rfc3168]{[RFC3168]}
> 
> i could not find this in the spec either.
> "segment" is mentioned once, when talking about MSS.

Here refers to an aggregated segment (e.g., one big super skb in Linux) that has more than one MSS block.
And the CWR flag will be handled differently for all segments inside, like I mentioned above, as implicitly described in RFC3168.
Is there any other suggested terminology that is common across OSs?

> 
> > +    \footnote{This case is not handled by some older hardware, so is called out specifically in the protocol.}.
> 
> what does this refer to? which protocol? hardware older than what?
>
> >     \end{itemize}
> >
> >  \item If the driver negotiated the VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO 
> > feature and the @@ -788,10 +793,9 @@ \subsubsection{Packet 
> > Transmission}\label{sec:Device Types / Network Device / De  \field{gso_type} to VIRTIO_NET_HDR_GSO_UDP_L4.
> >
> >  The driver SHOULD NOT send to the device TCP packets requiring 
> > segmentation offload 
> > -which have the Explicit Congestion Notification bit set, unless the
> > -VIRTIO_NET_F_HOST_ECN feature is negotiated, in which case the
> > -driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in 
> > -\field{gso_type}.
> > +which have the TCP CWR flag set and require the flag be handled as 
> > +mentioned in \hyperref[intro:rfc3168]{[RFC3168]},
> 
> still confusing.
> should not send packets that require the flag? or should not require the flag?
> if the former, "and which would require" ... if the later "and should not require".
> or better, make sentences shorter.

Should not send packets that (1) require segment offload, (2) have the CWR flag set, and (3) need to follow the SHOULD requirements in Section 6.1 of RFC3168.

Is it better like this?
"The driver should not send TCP packets to the device that require segmentation offload, and the set CWR flag needs to follow the "should requirement" in RFC 3168 section 6.1, unless the VIRTIO_NET_F_HOST_ECN capability is negotiated, in which case the driver must set VI_IO_NET_SO in GSO_type."

BRs,
Chia-Yu
> 
> > +unless the VIRTIO_NET_F_HOST_ECN feature is
> > +negotiated, in which case the driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
> 
> in which case? when it is or when it is not negotiated? or when it sends packets maybe?
> 
> >
> >  If VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO is negotiated, the driver MAY set
> >  VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 bit or the 
> > VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 bit @@ -1105,9 +1109,9 @@ 
> > \subsubsection{Processing of Incoming Packets}\label{sec:Device Types 
> > / Network  \end{enumerate}
> >
> >  Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP, UDP_TUNNEL 
> > -and ECN features enable receive checksum, large receive offload and 
> > ECN -support which are the input equivalents of the transmit checksum, 
> > -transmit segmentation offloading and ECN features, as described
> > +and ECN features enable receive checksum, large receive offload and 
> > +RFC3168 ECN support which are the input equivalents of the transmit 
> > +checksum, transmit segmentation offloading and RFC3168 ECN features, 
> > +as described
> >  in \ref{sec:Device Types / Network Device / Device Operation /  
> > Packet Transmission}:
> >  \begin{enumerate}
> > @@ -1210,10 +1214,9 @@ \subsubsection{Processing of Incoming 
> > Packets}\label{sec:Device Types / Network  the VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM bit in \field{flags}.
> >
> >  The device SHOULD NOT send to the driver TCP packets requiring 
> > segmentation offload -which have the Explicit Congestion Notification 
> > bit set, unless the -VIRTIO_NET_F_GUEST_ECN feature is negotiated, in 
> > which case the -device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in 
> > -\field{gso_type}.
> > +which have the TCP CWR flag set and require the flag be handled as 
> > +mentioned in \hyperref[intro:rfc3168]{[RFC3168]}, unless the 
> > +VIRTIO_NET_F_GUEST_ECN feature is negotiated, in which case the device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
> >
> >  If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the  
> > device MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in diff --git 
> > a/introduction.tex b/introduction.tex index e60298a..d52622e 100644
> > --- a/introduction.tex
> > +++ b/introduction.tex
> > @@ -168,6 +168,9 @@ \section{Normative References}\label{sec:Normative References}
> >      Leiba, B., "Ambiguity of Uppercase vs Lowercase in RFC 2119 Key Words", BCP
> >      14, RFC 8174, DOI 10.17487/RFC8174, May 2017
> >          \newline\url{http://www.ietf.org/rfc/rfc8174.txt}\\
> > +     \phantomsection\label{intro:rfc3168}\textbf{[RFC3168]} &
> > +    S. Floyd., ``The Addition of Explicit Congestion Notification (ECN) to IP'', September 2001.
> > +     \newline\url{http://www.ietf.org/rfc/rfc3168.txt}\\
> >  \end{longtable}
> >
> >  \section{Non-Normative References}
> > --
> > 2.34.1

Re: [PATCH v8 1/2] virtio-net: Fix ECN feature descriptions

[Michael S. Tsirkin]

On Mon, Apr 28, 2025 at 11:28:59AM +0000, Chia-Yu Chang (Nokia) wrote:
> > -----Original Message-----
> > From: Michael S. Tsirkin <mst@redhat.com> 
> > Sent: Monday, April 28, 2025 10:00 AM
> > To: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
> > Cc: virtio-comment@lists.linux.dev; cohuck@redhat.com; mvaralar@redhat.com; jasowang@redhat.com; xuanzhuo@linux.alibaba.com; eperezma@redhat.com; ij@kernel.org; ncardwell@google.com; Koen De Schepper (Nokia) <koen.de_schepper@nokia-bell-labs.com>; g.white <g.white@cablelabs.com>; vidhi_goel <vidhi_goel@apple.com>; ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com
> > Subject: Re: [PATCH v8 1/2] virtio-net: Fix ECN feature descriptions
> > 
> > 
> > CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
> > 
> > 
> > 
> > On Fri, Apr 18, 2025 at 12:40:43AM +0200, chia-yu.chang@nokia-bell-labs.com wrote:
> > > From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > >
> > > Clarify that the VIRTIO_NET_HDR_GSO_ECN gso_type flag does not mean 
> > > that TCP has IP-ECN set; instead, it identifies that the TCP CWR flag 
> > > is set and will be cleared from the second segment of an aggregated segment.
> > > This is used to offload the TCP CWR flag in a way that is compatible 
> > > with
> > > RFC3168 ECN but is problematic for non-RFC3168 use of the TCP CWR flag.
> > >
> > > Foe detailed requirements, please refer to IETF RFC3168:
> > > https://datatracker.ietf.org/doc/html/rfc3168
> > >
> > > Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > > ---
> > >  device-types/net/description.tex | 33 +++++++++++++++++---------------
> > >  introduction.tex                 |  3 +++
> > >  2 files changed, 21 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/device-types/net/description.tex 
> > > b/device-types/net/description.tex
> > > index 1b6b54d..6b09f0a 100644
> > > --- a/device-types/net/description.tex
> > > +++ b/device-types/net/description.tex
> > > @@ -54,7 +54,9 @@ \subsection{Feature bits}\label{sec:Device Types / 
> > > Network Device / Feature bits
> > >
> > >  \item[VIRTIO_NET_F_GUEST_TSO6 (8)] Driver can receive TSOv6.
> > >
> > > -\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with ECN.
> > > +\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with TCP CWR flag set
> > > +    and follow the ACE bits handling approach mentioned in
> > > +    \hyperref[intro:rfc3168]{[RFC3168]}.
> > >
> > >  \item[VIRTIO_NET_F_GUEST_UFO (10)] Driver can receive UFO.
> > >
> > > @@ -62,7 +64,9 @@ \subsection{Feature bits}\label{sec:Device Types / 
> > > Network Device / Feature bits
> > >
> > >  \item[VIRTIO_NET_F_HOST_TSO6 (12)] Device can receive TSOv6.
> > >
> > > -\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with ECN.
> > > +\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with TCP CWR flag set
> > > +    and follow the ACE bits handling approach mentioned in
> > > +    \hyperref[intro:rfc3168]{[RFC3168]}.
> > 
> > What is "the ACE bits handling approach"? It's agrammatical to use a plural while reducing the relative in english.
> > 
> > I looked at RFC3168 and I see no mention of ACE anywhere.
> > 
> > Also "mentioned in" is weirdly informal.
> > described maybe?
> > which part of the
> > spec do you refer to?
> > 
> > you should also expand acronims at first use.
> 
> Hi Michael,
> 
> Thanks for the comments.
> 
> RFC3168 does not explicitly discuss offloading, and one relevant statement is "When the TCP data sender is ready to set the CWR bit after reducing the congestion window, it SHOULD set the CWR bit only on the first new data packet that it transmits."
> Therefore, IMO, RFC3168 does not provide a clear specification of what occurs when offloading is applied.
> While the ACE field is a new terminology introduced in the AccECN draft, it incorporates the AE, CWR, and ECE flags.
> 
> To make the virtio spec clear (i.e., without expanding on the implied information that should be in the RFC) and simple to grasp, I would propose the following changes.
> "Device can receive TSO with the CWR flag set and follow the SHOULD requirements of the CWR bit described in Section 6.1 of RFC3168."

This looks better.
receive from where? The driver?
Add a section name, too. You can drop SHOULD just say
requirements, too.

> > >
> > >  \item[VIRTIO_NET_F_HOST_UFO (14)] Device can receive UFO.
> > >
> > > @@ -695,8 +699,9 @@ \subsubsection{Packet 
> > > Transmission}\label{sec:Device Types / Network Device / De
> > >
> > >    \item If the driver negotiated the VIRTIO_NET_F_HOST_ECN feature,
> > >      the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}
> > > -    indicates that the TCP packet has the ECN bit set\footnote{This case is not handled by some older hardware, so is called out
> > > -specifically in the protocol.}.
> > > +    indicates that the TCP packet has TCP CWR flag set, and the flag 
> > > + will be handled differently for all segments of
> > 
> > the CWR flag.
> 
> Will be changed.
> 
> > 
> > > +    an aggregated segment, as mentioned in 
> > > + \hyperref[intro:rfc3168]{[RFC3168]}
> > 
> > i could not find this in the spec either.
> > "segment" is mentioned once, when talking about MSS.
> 
> Here refers to an aggregated segment (e.g., one big super skb in Linux) that has more than one MSS block.
> And the CWR flag will be handled differently for all segments inside, like I mentioned above, as implicitly described in RFC3168.
> Is there any other suggested terminology that is common across OSs?

I don't really know what you refer to.
Given it's according to RFC, maybe no need to talk about segments?

> > 
> > > +    \footnote{This case is not handled by some older hardware, so is called out specifically in the protocol.}.
> > 
> > what does this refer to? which protocol? hardware older than what?
> >
> > >     \end{itemize}
> > >
> > >  \item If the driver negotiated the VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO 
> > > feature and the @@ -788,10 +793,9 @@ \subsubsection{Packet 
> > > Transmission}\label{sec:Device Types / Network Device / De  \field{gso_type} to VIRTIO_NET_HDR_GSO_UDP_L4.
> > >
> > >  The driver SHOULD NOT send to the device TCP packets requiring 
> > > segmentation offload 
> > > -which have the Explicit Congestion Notification bit set, unless the
> > > -VIRTIO_NET_F_HOST_ECN feature is negotiated, in which case the
> > > -driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in 
> > > -\field{gso_type}.
> > > +which have the TCP CWR flag set and require the flag be handled as 
> > > +mentioned in \hyperref[intro:rfc3168]{[RFC3168]},
> > 
> > still confusing.
> > should not send packets that require the flag? or should not require the flag?
> > if the former, "and which would require" ... if the later "and should not require".
> > or better, make sentences shorter.
> 
> Should not send packets that (1) require segment offload, (2) have the CWR flag set, and (3) need to follow the SHOULD requirements in Section 6.1 of RFC3168.

Any of these? All of these? Any two of these?

> Is it better like this?
> "The driver should not send TCP packets to the device that require
> segmentation offload, and the set CWR flag needs to follow the "should
> requirement" in RFC 3168 section 6.1,

this part is still confusing. what is "and" here? do not send packets
that both require segmentation and have the CWR flag?
do not send packets that require segmentation and also do not
send packets that follow requirements?


> unless the VIRTIO_NET_F_HOST_ECN
> capability is negotiated, in which case the driver must set
> VI_IO_NET_SO in GSO_type."
> 
> BRs,
> Chia-Yu
> > 
> > > +unless the VIRTIO_NET_F_HOST_ECN feature is
> > > +negotiated, in which case the driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
> > 
> > in which case? when it is or when it is not negotiated? or when it sends packets maybe?
> > 
> > >
> > >  If VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO is negotiated, the driver MAY set
> > >  VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 bit or the 
> > > VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 bit @@ -1105,9 +1109,9 @@ 
> > > \subsubsection{Processing of Incoming Packets}\label{sec:Device Types 
> > > / Network  \end{enumerate}
> > >
> > >  Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP, UDP_TUNNEL 
> > > -and ECN features enable receive checksum, large receive offload and 
> > > ECN -support which are the input equivalents of the transmit checksum, 
> > > -transmit segmentation offloading and ECN features, as described
> > > +and ECN features enable receive checksum, large receive offload and 
> > > +RFC3168 ECN support which are the input equivalents of the transmit 
> > > +checksum, transmit segmentation offloading and RFC3168 ECN features, 
> > > +as described
> > >  in \ref{sec:Device Types / Network Device / Device Operation /  
> > > Packet Transmission}:
> > >  \begin{enumerate}
> > > @@ -1210,10 +1214,9 @@ \subsubsection{Processing of Incoming 
> > > Packets}\label{sec:Device Types / Network  the VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM bit in \field{flags}.
> > >
> > >  The device SHOULD NOT send to the driver TCP packets requiring 
> > > segmentation offload -which have the Explicit Congestion Notification 
> > > bit set, unless the -VIRTIO_NET_F_GUEST_ECN feature is negotiated, in 
> > > which case the -device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in 
> > > -\field{gso_type}.
> > > +which have the TCP CWR flag set and require the flag be handled as 
> > > +mentioned in \hyperref[intro:rfc3168]{[RFC3168]}, unless the 
> > > +VIRTIO_NET_F_GUEST_ECN feature is negotiated, in which case the device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
> > >
> > >  If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the  
> > > device MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in diff --git 
> > > a/introduction.tex b/introduction.tex index e60298a..d52622e 100644
> > > --- a/introduction.tex
> > > +++ b/introduction.tex
> > > @@ -168,6 +168,9 @@ \section{Normative References}\label{sec:Normative References}
> > >      Leiba, B., "Ambiguity of Uppercase vs Lowercase in RFC 2119 Key Words", BCP
> > >      14, RFC 8174, DOI 10.17487/RFC8174, May 2017
> > >          \newline\url{http://www.ietf.org/rfc/rfc8174.txt}\\
> > > +     \phantomsection\label{intro:rfc3168}\textbf{[RFC3168]} &
> > > +    S. Floyd., ``The Addition of Explicit Congestion Notification (ECN) to IP'', September 2001.
> > > +     \newline\url{http://www.ietf.org/rfc/rfc3168.txt}\\
> > >  \end{longtable}
> > >
> > >  \section{Non-Normative References}
> > > --
> > > 2.34.1

RE: [PATCH v8 1/2] virtio-net: Fix ECN feature descriptions

[Chia-Yu Chang (Nokia)]

> -----Original Message-----
> From: Michael S. Tsirkin <mst@redhat.com> 
> Sent: Monday, April 28, 2025 2:32 PM
> To: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
> Cc: virtio-comment@lists.linux.dev; cohuck@redhat.com; mvaralar@redhat.com; jasowang@redhat.com; xuanzhuo@linux.alibaba.com; eperezma@redhat.com; ij@kernel.org; ncardwell@google.com; Koen De Schepper (Nokia) <koen.de_schepper@nokia-bell-labs.com>; g.white <g.white@cablelabs.com>; vidhi_goel <vidhi_goel@apple.com>; ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com
> Subject: Re: [PATCH v8 1/2] virtio-net: Fix ECN feature descriptions
> 
> 
> CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
> 
> 
> 
> On Mon, Apr 28, 2025 at 11:28:59AM +0000, Chia-Yu Chang (Nokia) wrote:
> > > -----Original Message-----
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Monday, April 28, 2025 10:00 AM
> > > To: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
> > > Cc: virtio-comment@lists.linux.dev; cohuck@redhat.com; 
> > > mvaralar@redhat.com; jasowang@redhat.com; 
> > > xuanzhuo@linux.alibaba.com; eperezma@redhat.com; ij@kernel.org; 
> > > ncardwell@google.com; Koen De Schepper (Nokia) 
> > > <koen.de_schepper@nokia-bell-labs.com>; g.white 
> > > <g.white@cablelabs.com>; vidhi_goel <vidhi_goel@apple.com>; 
> > > ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com
> > > Subject: Re: [PATCH v8 1/2] virtio-net: Fix ECN feature descriptions
> > >
> > >
> > > CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
> > >
> > >
> > >
> > > On Fri, Apr 18, 2025 at 12:40:43AM +0200, chia-yu.chang@nokia-bell-labs.com wrote:
> > > > From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > > >
> > > > Clarify that the VIRTIO_NET_HDR_GSO_ECN gso_type flag does not 
> > > > mean that TCP has IP-ECN set; instead, it identifies that the TCP 
> > > > CWR flag is set and will be cleared from the second segment of an aggregated segment.
> > > > This is used to offload the TCP CWR flag in a way that is 
> > > > compatible with
> > > > RFC3168 ECN but is problematic for non-RFC3168 use of the TCP CWR flag.
> > > >
> > > > Foe detailed requirements, please refer to IETF RFC3168:
> > > > https://datatracker.ietf.org/doc/html/rfc3168
> > > >
> > > > Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > > > ---
> > > >  device-types/net/description.tex | 33 +++++++++++++++++---------------
> > > >  introduction.tex                 |  3 +++
> > > >  2 files changed, 21 insertions(+), 15 deletions(-)
> > > >
> > > > diff --git a/device-types/net/description.tex
> > > > b/device-types/net/description.tex
> > > > index 1b6b54d..6b09f0a 100644
> > > > --- a/device-types/net/description.tex
> > > > +++ b/device-types/net/description.tex
> > > > @@ -54,7 +54,9 @@ \subsection{Feature bits}\label{sec:Device Types 
> > > > / Network Device / Feature bits
> > > >
> > > >  \item[VIRTIO_NET_F_GUEST_TSO6 (8)] Driver can receive TSOv6.
> > > >
> > > > -\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with ECN.
> > > > +\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with TCP CWR flag set
> > > > +    and follow the ACE bits handling approach mentioned in
> > > > +    \hyperref[intro:rfc3168]{[RFC3168]}.
> > > >
> > > >  \item[VIRTIO_NET_F_GUEST_UFO (10)] Driver can receive UFO.
> > > >
> > > > @@ -62,7 +64,9 @@ \subsection{Feature bits}\label{sec:Device Types 
> > > > / Network Device / Feature bits
> > > >
> > > >  \item[VIRTIO_NET_F_HOST_TSO6 (12)] Device can receive TSOv6.
> > > >
> > > > -\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with ECN.
> > > > +\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with TCP CWR flag set
> > > > +    and follow the ACE bits handling approach mentioned in
> > > > +    \hyperref[intro:rfc3168]{[RFC3168]}.
> > >
> > > What is "the ACE bits handling approach"? It's agrammatical to use a plural while reducing the relative in english.
> > >
> > > I looked at RFC3168 and I see no mention of ACE anywhere.
> > >
> > > Also "mentioned in" is weirdly informal.
> > > described maybe?
> > > which part of the
> > > spec do you refer to?
> > >
> > > you should also expand acronims at first use.
> >
> > Hi Michael,
> >
> > Thanks for the comments.
> >
> > RFC3168 does not explicitly discuss offloading, and one relevant statement is "When the TCP data sender is ready to set the CWR bit after reducing the congestion window, it SHOULD set the CWR bit only on the first new data packet that it transmits."
> > Therefore, IMO, RFC3168 does not provide a clear specification of what occurs when offloading is applied.
> > While the ACE field is a new terminology introduced in the AccECN draft, it incorporates the AE, CWR, and ECE flags.
> >
> > To make the virtio spec clear (i.e., without expanding on the implied information that should be in the RFC) and simple to grasp, I would propose the following changes.
> > "Device can receive TSO with the CWR flag set and follow the SHOULD requirements of the CWR bit described in Section 6.1 of RFC3168."
> 
> This looks better.
> receive from where? The driver?
> Add a section name, too. You can drop SHOULD just say requirements, too.

OK, then I will put
"Device can receive TSO with the CWR flag set and follow the requirements of the CWR bit described in "Section 6.1.2. The TCP Sender" of RFC3168."

> 
> > > >
> > > >  \item[VIRTIO_NET_F_HOST_UFO (14)] Device can receive UFO.
> > > >
> > > > @@ -695,8 +699,9 @@ \subsubsection{Packet 
> > > > Transmission}\label{sec:Device Types / Network Device / De
> > > >
> > > >    \item If the driver negotiated the VIRTIO_NET_F_HOST_ECN feature,
> > > >      the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}
> > > > -    indicates that the TCP packet has the ECN bit set\footnote{This case is not handled by some older hardware, so is called out
> > > > -specifically in the protocol.}.
> > > > +    indicates that the TCP packet has TCP CWR flag set, and the 
> > > > + flag will be handled differently for all segments of
> > >
> > > the CWR flag.
> >
> > Will be changed.
> >
> > >
> > > > +    an aggregated segment, as mentioned in 
> > > > + \hyperref[intro:rfc3168]{[RFC3168]}
> > >
> > > i could not find this in the spec either.
> > > "segment" is mentioned once, when talking about MSS.
> >
> > Here refers to an aggregated segment (e.g., one big super skb in Linux) that has more than one MSS block.
> > And the CWR flag will be handled differently for all segments inside, like I mentioned above, as implicitly described in RFC3168.
> > Is there any other suggested terminology that is common across OSs?
> 
> I don't really know what you refer to.
> Given it's according to RFC, maybe no need to talk about segments?

OK, so I suggest the following without going into the details:
"If the driver negotiated the VIRTIO_NET_F_HOST_ECN feature, the VIRTIO_NET_HDR_GSO_ECN bit in gso_type indicates that the TCP packet 
has the TCP CWR flag set and follows the requirements of the CWR bit described in "Section 6.1.2. The TCP Sender" of RFC3168."

> > >
> > > > +    \footnote{This case is not handled by some older hardware, so is called out specifically in the protocol.}.
> > >
> > > what does this refer to? which protocol? hardware older than what?
> > >
> > > >     \end{itemize}
> > > >
> > > >  \item If the driver negotiated the 
> > > > VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO feature and the @@ -788,10 +793,9 
> > > > @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De  \field{gso_type} to VIRTIO_NET_HDR_GSO_UDP_L4.
> > > >
> > > >  The driver SHOULD NOT send to the device TCP packets requiring 
> > > > segmentation offload -which have the Explicit Congestion 
> > > > Notification bit set, unless the -VIRTIO_NET_F_HOST_ECN feature is 
> > > > negotiated, in which case the -driver MUST set the 
> > > > VIRTIO_NET_HDR_GSO_ECN bit in -\field{gso_type}.
> > > > +which have the TCP CWR flag set and require the flag be handled 
> > > > +as mentioned in \hyperref[intro:rfc3168]{[RFC3168]},
> > >
> > > still confusing.
> > > should not send packets that require the flag? or should not require the flag?
> > > if the former, "and which would require" ... if the later "and should not require".
> > > or better, make sentences shorter.
> >
> > Should not send packets that (1) require segment offload, (2) have the CWR flag set, and (3) need to follow the SHOULD requirements in Section 6.1 of RFC3168.
> 
> Any of these? All of these? Any two of these?
> 
> > Is it better like this?
> > "The driver should not send TCP packets to the device that require 
> > segmentation offload, and the set CWR flag needs to follow the "should 
> > requirement" in RFC 3168 section 6.1,
> 
> this part is still confusing. what is "and" here? do not send packets that both require segmentation and have the CWR flag?
> do not send packets that require segmentation and also do not send packets that follow requirements?

OK, it shall be the latter case you describe.
Because, if the TCP packet requires segment offload but the RFC3168 requirement is not required (notice that it is a SHOULD requirement rather than a MUST), the packet can still be transmitted.
In this case, the CWR flag will be delivered not just in the first packet, but this is as intended.

As such, I would like to propose a shorten text:
"The driver should not send to the device TCP packets that require not only segmentation offload but also following the CWR flag requirements described in "Section 6.1.2 The TCP Sender" of RFC3168, unless..."

> 
> > unless the VIRTIO_NET_F_HOST_ECN
> > capability is negotiated, in which case the driver must set 
> > VI_IO_NET_SO in GSO_type."
> >
> > BRs,
> > Chia-Yu
> > >
> > > > +unless the VIRTIO_NET_F_HOST_ECN feature is negotiated, in which 
> > > > +case the driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
> > >
> > > in which case? when it is or when it is not negotiated? or when it sends packets maybe?
> > >
> > > >
> > > >  If VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO is negotiated, the driver MAY 
> > > > set
> > > >  VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 bit or the
> > > > VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 bit @@ -1105,9 +1109,9 @@ 
> > > > \subsubsection{Processing of Incoming Packets}\label{sec:Device 
> > > > Types / Network  \end{enumerate}
> > > >
> > > >  Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP, 
> > > > UDP_TUNNEL -and ECN features enable receive checksum, large 
> > > > receive offload and ECN -support which are the input equivalents 
> > > > of the transmit checksum, -transmit segmentation offloading and 
> > > > ECN features, as described
> > > > +and ECN features enable receive checksum, large receive offload 
> > > > +and
> > > > +RFC3168 ECN support which are the input equivalents of the 
> > > > +transmit checksum, transmit segmentation offloading and RFC3168 
> > > > +ECN features, as described
> > > >  in \ref{sec:Device Types / Network Device / Device Operation / 
> > > > Packet Transmission}:
> > > >  \begin{enumerate}
> > > > @@ -1210,10 +1214,9 @@ \subsubsection{Processing of Incoming 
> > > > Packets}\label{sec:Device Types / Network  the VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM bit in \field{flags}.
> > > >
> > > >  The device SHOULD NOT send to the driver TCP packets requiring 
> > > > segmentation offload -which have the Explicit Congestion 
> > > > Notification bit set, unless the -VIRTIO_NET_F_GUEST_ECN feature 
> > > > is negotiated, in which case the -device MUST set the 
> > > > VIRTIO_NET_HDR_GSO_ECN bit in -\field{gso_type}.
> > > > +which have the TCP CWR flag set and require the flag be handled 
> > > > +as mentioned in \hyperref[intro:rfc3168]{[RFC3168]}, unless the 
> > > > +VIRTIO_NET_F_GUEST_ECN feature is negotiated, in which case the device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
> > > >
> > > >  If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the 
> > > > device MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in diff --git 
> > > > a/introduction.tex b/introduction.tex index e60298a..d52622e 
> > > > 100644
> > > > --- a/introduction.tex
> > > > +++ b/introduction.tex
> > > > @@ -168,6 +168,9 @@ \section{Normative References}\label{sec:Normative References}
> > > >      Leiba, B., "Ambiguity of Uppercase vs Lowercase in RFC 2119 Key Words", BCP
> > > >      14, RFC 8174, DOI 10.17487/RFC8174, May 2017
> > > >          \newline\url{http://www.ietf.org/rfc/rfc8174.txt}\\
> > > > +     \phantomsection\label{intro:rfc3168}\textbf{[RFC3168]} &
> > > > +    S. Floyd., ``The Addition of Explicit Congestion Notification (ECN) to IP'', September 2001.
> > > > +     \newline\url{http://www.ietf.org/rfc/rfc3168.txt}\\
> > > >  \end{longtable}
> > > >
> > > >  \section{Non-Normative References}
> > > > --
> > > > 2.34.1

Re: [PATCH v8 1/2] virtio-net: Fix ECN feature descriptions

[Michael S. Tsirkin]

On Mon, Apr 28, 2025 at 03:11:25PM +0000, Chia-Yu Chang (Nokia) wrote:
> > -----Original Message-----
> > From: Michael S. Tsirkin <mst@redhat.com> 
> > Sent: Monday, April 28, 2025 2:32 PM
> > To: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
> > Cc: virtio-comment@lists.linux.dev; cohuck@redhat.com; mvaralar@redhat.com; jasowang@redhat.com; xuanzhuo@linux.alibaba.com; eperezma@redhat.com; ij@kernel.org; ncardwell@google.com; Koen De Schepper (Nokia) <koen.de_schepper@nokia-bell-labs.com>; g.white <g.white@cablelabs.com>; vidhi_goel <vidhi_goel@apple.com>; ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com
> > Subject: Re: [PATCH v8 1/2] virtio-net: Fix ECN feature descriptions
> > 
> > 
> > CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
> > 
> > 
> > 
> > On Mon, Apr 28, 2025 at 11:28:59AM +0000, Chia-Yu Chang (Nokia) wrote:
> > > > -----Original Message-----
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Monday, April 28, 2025 10:00 AM
> > > > To: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
> > > > Cc: virtio-comment@lists.linux.dev; cohuck@redhat.com; 
> > > > mvaralar@redhat.com; jasowang@redhat.com; 
> > > > xuanzhuo@linux.alibaba.com; eperezma@redhat.com; ij@kernel.org; 
> > > > ncardwell@google.com; Koen De Schepper (Nokia) 
> > > > <koen.de_schepper@nokia-bell-labs.com>; g.white 
> > > > <g.white@cablelabs.com>; vidhi_goel <vidhi_goel@apple.com>; 
> > > > ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com
> > > > Subject: Re: [PATCH v8 1/2] virtio-net: Fix ECN feature descriptions
> > > >
> > > >
> > > > CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
> > > >
> > > >
> > > >
> > > > On Fri, Apr 18, 2025 at 12:40:43AM +0200, chia-yu.chang@nokia-bell-labs.com wrote:
> > > > > From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > > > >
> > > > > Clarify that the VIRTIO_NET_HDR_GSO_ECN gso_type flag does not 
> > > > > mean that TCP has IP-ECN set; instead, it identifies that the TCP 
> > > > > CWR flag is set and will be cleared from the second segment of an aggregated segment.
> > > > > This is used to offload the TCP CWR flag in a way that is 
> > > > > compatible with
> > > > > RFC3168 ECN but is problematic for non-RFC3168 use of the TCP CWR flag.
> > > > >
> > > > > Foe detailed requirements, please refer to IETF RFC3168:
> > > > > https://datatracker.ietf.org/doc/html/rfc3168
> > > > >
> > > > > Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > > > > ---
> > > > >  device-types/net/description.tex | 33 +++++++++++++++++---------------
> > > > >  introduction.tex                 |  3 +++
> > > > >  2 files changed, 21 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/device-types/net/description.tex
> > > > > b/device-types/net/description.tex
> > > > > index 1b6b54d..6b09f0a 100644
> > > > > --- a/device-types/net/description.tex
> > > > > +++ b/device-types/net/description.tex
> > > > > @@ -54,7 +54,9 @@ \subsection{Feature bits}\label{sec:Device Types 
> > > > > / Network Device / Feature bits
> > > > >
> > > > >  \item[VIRTIO_NET_F_GUEST_TSO6 (8)] Driver can receive TSOv6.
> > > > >
> > > > > -\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with ECN.
> > > > > +\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with TCP CWR flag set
> > > > > +    and follow the ACE bits handling approach mentioned in
> > > > > +    \hyperref[intro:rfc3168]{[RFC3168]}.
> > > > >
> > > > >  \item[VIRTIO_NET_F_GUEST_UFO (10)] Driver can receive UFO.
> > > > >
> > > > > @@ -62,7 +64,9 @@ \subsection{Feature bits}\label{sec:Device Types 
> > > > > / Network Device / Feature bits
> > > > >
> > > > >  \item[VIRTIO_NET_F_HOST_TSO6 (12)] Device can receive TSOv6.
> > > > >
> > > > > -\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with ECN.
> > > > > +\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with TCP CWR flag set
> > > > > +    and follow the ACE bits handling approach mentioned in
> > > > > +    \hyperref[intro:rfc3168]{[RFC3168]}.
> > > >
> > > > What is "the ACE bits handling approach"? It's agrammatical to use a plural while reducing the relative in english.
> > > >
> > > > I looked at RFC3168 and I see no mention of ACE anywhere.
> > > >
> > > > Also "mentioned in" is weirdly informal.
> > > > described maybe?
> > > > which part of the
> > > > spec do you refer to?
> > > >
> > > > you should also expand acronims at first use.
> > >
> > > Hi Michael,
> > >
> > > Thanks for the comments.
> > >
> > > RFC3168 does not explicitly discuss offloading, and one relevant statement is "When the TCP data sender is ready to set the CWR bit after reducing the congestion window, it SHOULD set the CWR bit only on the first new data packet that it transmits."
> > > Therefore, IMO, RFC3168 does not provide a clear specification of what occurs when offloading is applied.
> > > While the ACE field is a new terminology introduced in the AccECN draft, it incorporates the AE, CWR, and ECE flags.
> > >
> > > To make the virtio spec clear (i.e., without expanding on the implied information that should be in the RFC) and simple to grasp, I would propose the following changes.
> > > "Device can receive TSO with the CWR flag set and follow the SHOULD requirements of the CWR bit described in Section 6.1 of RFC3168."
> > 
> > This looks better.
> > receive from where? The driver?
> > Add a section name, too. You can drop SHOULD just say requirements, too.
> 
> OK, then I will put
> "Device can receive TSO with the CWR flag set and follow the requirements of the CWR bit described in "Section 6.1.2. The TCP Sender" of RFC3168."
> 
> > 
> > > > >
> > > > >  \item[VIRTIO_NET_F_HOST_UFO (14)] Device can receive UFO.
> > > > >
> > > > > @@ -695,8 +699,9 @@ \subsubsection{Packet 
> > > > > Transmission}\label{sec:Device Types / Network Device / De
> > > > >
> > > > >    \item If the driver negotiated the VIRTIO_NET_F_HOST_ECN feature,
> > > > >      the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}
> > > > > -    indicates that the TCP packet has the ECN bit set\footnote{This case is not handled by some older hardware, so is called out
> > > > > -specifically in the protocol.}.
> > > > > +    indicates that the TCP packet has TCP CWR flag set, and the 
> > > > > + flag will be handled differently for all segments of
> > > >
> > > > the CWR flag.
> > >
> > > Will be changed.
> > >
> > > >
> > > > > +    an aggregated segment, as mentioned in 
> > > > > + \hyperref[intro:rfc3168]{[RFC3168]}
> > > >
> > > > i could not find this in the spec either.
> > > > "segment" is mentioned once, when talking about MSS.
> > >
> > > Here refers to an aggregated segment (e.g., one big super skb in Linux) that has more than one MSS block.
> > > And the CWR flag will be handled differently for all segments inside, like I mentioned above, as implicitly described in RFC3168.
> > > Is there any other suggested terminology that is common across OSs?
> > 
> > I don't really know what you refer to.
> > Given it's according to RFC, maybe no need to talk about segments?
> 
> OK, so I suggest the following without going into the details:
> "If the driver negotiated the VIRTIO_NET_F_HOST_ECN feature, the VIRTIO_NET_HDR_GSO_ECN bit in gso_type indicates that the TCP packet 
> has the TCP CWR flag set and follows the requirements of the CWR bit described in "Section 6.1.2. The TCP Sender" of RFC3168."
> 
> > > >
> > > > > +    \footnote{This case is not handled by some older hardware, so is called out specifically in the protocol.}.
> > > >
> > > > what does this refer to? which protocol? hardware older than what?
> > > >
> > > > >     \end{itemize}
> > > > >
> > > > >  \item If the driver negotiated the 
> > > > > VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO feature and the @@ -788,10 +793,9 
> > > > > @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De  \field{gso_type} to VIRTIO_NET_HDR_GSO_UDP_L4.
> > > > >
> > > > >  The driver SHOULD NOT send to the device TCP packets requiring 
> > > > > segmentation offload -which have the Explicit Congestion 
> > > > > Notification bit set, unless the -VIRTIO_NET_F_HOST_ECN feature is 
> > > > > negotiated, in which case the -driver MUST set the 
> > > > > VIRTIO_NET_HDR_GSO_ECN bit in -\field{gso_type}.
> > > > > +which have the TCP CWR flag set and require the flag be handled 
> > > > > +as mentioned in \hyperref[intro:rfc3168]{[RFC3168]},
> > > >
> > > > still confusing.
> > > > should not send packets that require the flag? or should not require the flag?
> > > > if the former, "and which would require" ... if the later "and should not require".
> > > > or better, make sentences shorter.
> > >
> > > Should not send packets that (1) require segment offload, (2) have the CWR flag set, and (3) need to follow the SHOULD requirements in Section 6.1 of RFC3168.
> > 
> > Any of these? All of these? Any two of these?
> > 
> > > Is it better like this?
> > > "The driver should not send TCP packets to the device that require 
> > > segmentation offload, and the set CWR flag needs to follow the "should 
> > > requirement" in RFC 3168 section 6.1,
> > 
> > this part is still confusing. what is "and" here? do not send packets that both require segmentation and have the CWR flag?
> > do not send packets that require segmentation and also do not send packets that follow requirements?
> 
> OK, it shall be the latter case you describe.
> Because, if the TCP packet requires segment offload but the RFC3168 requirement is not required (notice that it is a SHOULD requirement rather than a MUST), the packet can still be transmitted.
> In this case, the CWR flag will be delivered not just in the first packet, but this is as intended.
> 
> As such, I would like to propose a shorten text:
> "The driver should not send to the device TCP packets that require not only segmentation offload but also following the CWR flag requirements described in "Section 6.1.2 The TCP Sender" of RFC3168, unless..."

same issue, still confusing.
do you just mean VIRTIO_NET_HDR_GSO_ECN set?


> > 
> > > unless the VIRTIO_NET_F_HOST_ECN
> > > capability is negotiated, in which case the driver must set 
> > > VI_IO_NET_SO in GSO_type."
> > >
> > > BRs,
> > > Chia-Yu
> > > >
> > > > > +unless the VIRTIO_NET_F_HOST_ECN feature is negotiated, in which 
> > > > > +case the driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
> > > >
> > > > in which case? when it is or when it is not negotiated? or when it sends packets maybe?
> > > >
> > > > >
> > > > >  If VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO is negotiated, the driver MAY 
> > > > > set
> > > > >  VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 bit or the
> > > > > VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 bit @@ -1105,9 +1109,9 @@ 
> > > > > \subsubsection{Processing of Incoming Packets}\label{sec:Device 
> > > > > Types / Network  \end{enumerate}
> > > > >
> > > > >  Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP, 
> > > > > UDP_TUNNEL -and ECN features enable receive checksum, large 
> > > > > receive offload and ECN -support which are the input equivalents 
> > > > > of the transmit checksum, -transmit segmentation offloading and 
> > > > > ECN features, as described
> > > > > +and ECN features enable receive checksum, large receive offload 
> > > > > +and
> > > > > +RFC3168 ECN support which are the input equivalents of the 
> > > > > +transmit checksum, transmit segmentation offloading and RFC3168 
> > > > > +ECN features, as described
> > > > >  in \ref{sec:Device Types / Network Device / Device Operation / 
> > > > > Packet Transmission}:
> > > > >  \begin{enumerate}
> > > > > @@ -1210,10 +1214,9 @@ \subsubsection{Processing of Incoming 
> > > > > Packets}\label{sec:Device Types / Network  the VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM bit in \field{flags}.
> > > > >
> > > > >  The device SHOULD NOT send to the driver TCP packets requiring 
> > > > > segmentation offload -which have the Explicit Congestion 
> > > > > Notification bit set, unless the -VIRTIO_NET_F_GUEST_ECN feature 
> > > > > is negotiated, in which case the -device MUST set the 
> > > > > VIRTIO_NET_HDR_GSO_ECN bit in -\field{gso_type}.
> > > > > +which have the TCP CWR flag set and require the flag be handled 
> > > > > +as mentioned in \hyperref[intro:rfc3168]{[RFC3168]}, unless the 
> > > > > +VIRTIO_NET_F_GUEST_ECN feature is negotiated, in which case the device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
> > > > >
> > > > >  If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, the 
> > > > > device MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in diff --git 
> > > > > a/introduction.tex b/introduction.tex index e60298a..d52622e 
> > > > > 100644
> > > > > --- a/introduction.tex
> > > > > +++ b/introduction.tex
> > > > > @@ -168,6 +168,9 @@ \section{Normative References}\label{sec:Normative References}
> > > > >      Leiba, B., "Ambiguity of Uppercase vs Lowercase in RFC 2119 Key Words", BCP
> > > > >      14, RFC 8174, DOI 10.17487/RFC8174, May 2017
> > > > >          \newline\url{http://www.ietf.org/rfc/rfc8174.txt}\\
> > > > > +     \phantomsection\label{intro:rfc3168}\textbf{[RFC3168]} &
> > > > > +    S. Floyd., ``The Addition of Explicit Congestion Notification (ECN) to IP'', September 2001.
> > > > > +     \newline\url{http://www.ietf.org/rfc/rfc3168.txt}\\
> > > > >  \end{longtable}
> > > > >
> > > > >  \section{Non-Normative References}
> > > > > --
> > > > > 2.34.1

RE: [PATCH v8 1/2] virtio-net: Fix ECN feature descriptions

[Chia-Yu Chang (Nokia)]

> -----Original Message-----
> From: Michael S. Tsirkin <mst@redhat.com> 
> Sent: Monday, April 28, 2025 7:55 PM
> To: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
> Cc: virtio-comment@lists.linux.dev; cohuck@redhat.com; mvaralar@redhat.com; jasowang@redhat.com; xuanzhuo@linux.alibaba.com; eperezma@redhat.com; ij@kernel.org; ncardwell@google.com; Koen De Schepper (Nokia) <koen.de_schepper@nokia-bell-labs.com>; g.white <g.white@cablelabs.com>; vidhi_goel <vidhi_goel@apple.com>; ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com
> Subject: Re: [PATCH v8 1/2] virtio-net: Fix ECN feature descriptions
> 
> 
> CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
> 
> 
> 
> On Mon, Apr 28, 2025 at 03:11:25PM +0000, Chia-Yu Chang (Nokia) wrote:
> > > -----Original Message-----
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Monday, April 28, 2025 2:32 PM
> > > To: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
> > > Cc: virtio-comment@lists.linux.dev; cohuck@redhat.com; 
> > > mvaralar@redhat.com; jasowang@redhat.com; 
> > > xuanzhuo@linux.alibaba.com; eperezma@redhat.com; ij@kernel.org; 
> > > ncardwell@google.com; Koen De Schepper (Nokia) 
> > > <koen.de_schepper@nokia-bell-labs.com>; g.white 
> > > <g.white@cablelabs.com>; vidhi_goel <vidhi_goel@apple.com>; 
> > > ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com
> > > Subject: Re: [PATCH v8 1/2] virtio-net: Fix ECN feature descriptions
> > >
> > >
> > > CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
> > >
> > >
> > >
> > > On Mon, Apr 28, 2025 at 11:28:59AM +0000, Chia-Yu Chang (Nokia) wrote:
> > > > > -----Original Message-----
> > > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > > Sent: Monday, April 28, 2025 10:00 AM
> > > > > To: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
> > > > > Cc: virtio-comment@lists.linux.dev; cohuck@redhat.com; 
> > > > > mvaralar@redhat.com; jasowang@redhat.com; 
> > > > > xuanzhuo@linux.alibaba.com; eperezma@redhat.com; ij@kernel.org; 
> > > > > ncardwell@google.com; Koen De Schepper (Nokia) 
> > > > > <koen.de_schepper@nokia-bell-labs.com>; g.white 
> > > > > <g.white@cablelabs.com>; vidhi_goel <vidhi_goel@apple.com>; 
> > > > > ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com
> > > > > Subject: Re: [PATCH v8 1/2] virtio-net: Fix ECN feature 
> > > > > descriptions
> > > > >
> > > > >
> > > > > CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
> > > > >
> > > > >
> > > > >
> > > > > On Fri, Apr 18, 2025 at 12:40:43AM +0200, chia-yu.chang@nokia-bell-labs.com wrote:
> > > > > > From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > > > > >
> > > > > > Clarify that the VIRTIO_NET_HDR_GSO_ECN gso_type flag does not 
> > > > > > mean that TCP has IP-ECN set; instead, it identifies that the 
> > > > > > TCP CWR flag is set and will be cleared from the second segment of an aggregated segment.
> > > > > > This is used to offload the TCP CWR flag in a way that is 
> > > > > > compatible with
> > > > > > RFC3168 ECN but is problematic for non-RFC3168 use of the TCP CWR flag.
> > > > > >
> > > > > > Foe detailed requirements, please refer to IETF RFC3168:
> > > > > > https://datatracker.ietf.org/doc/html/rfc3168
> > > > > >
> > > > > > Signed-off-by: Chia-Yu Chang 
> > > > > > <chia-yu.chang@nokia-bell-labs.com>
> > > > > > ---
> > > > > >  device-types/net/description.tex | 33 +++++++++++++++++---------------
> > > > > >  introduction.tex                 |  3 +++
> > > > > >  2 files changed, 21 insertions(+), 15 deletions(-)
> > > > > >
> > > > > > diff --git a/device-types/net/description.tex
> > > > > > b/device-types/net/description.tex
> > > > > > index 1b6b54d..6b09f0a 100644
> > > > > > --- a/device-types/net/description.tex
> > > > > > +++ b/device-types/net/description.tex
> > > > > > @@ -54,7 +54,9 @@ \subsection{Feature bits}\label{sec:Device 
> > > > > > Types / Network Device / Feature bits
> > > > > >
> > > > > >  \item[VIRTIO_NET_F_GUEST_TSO6 (8)] Driver can receive TSOv6.
> > > > > >
> > > > > > -\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with ECN.
> > > > > > +\item[VIRTIO_NET_F_GUEST_ECN (9)] Driver can receive TSO with TCP CWR flag set
> > > > > > +    and follow the ACE bits handling approach mentioned in
> > > > > > +    \hyperref[intro:rfc3168]{[RFC3168]}.
> > > > > >
> > > > > >  \item[VIRTIO_NET_F_GUEST_UFO (10)] Driver can receive UFO.
> > > > > >
> > > > > > @@ -62,7 +64,9 @@ \subsection{Feature bits}\label{sec:Device 
> > > > > > Types / Network Device / Feature bits
> > > > > >
> > > > > >  \item[VIRTIO_NET_F_HOST_TSO6 (12)] Device can receive TSOv6.
> > > > > >
> > > > > > -\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with ECN.
> > > > > > +\item[VIRTIO_NET_F_HOST_ECN (13)] Device can receive TSO with TCP CWR flag set
> > > > > > +    and follow the ACE bits handling approach mentioned in
> > > > > > +    \hyperref[intro:rfc3168]{[RFC3168]}.
> > > > >
> > > > > What is "the ACE bits handling approach"? It's agrammatical to use a plural while reducing the relative in english.
> > > > >
> > > > > I looked at RFC3168 and I see no mention of ACE anywhere.
> > > > >
> > > > > Also "mentioned in" is weirdly informal.
> > > > > described maybe?
> > > > > which part of the
> > > > > spec do you refer to?
> > > > >
> > > > > you should also expand acronims at first use.
> > > >
> > > > Hi Michael,
> > > >
> > > > Thanks for the comments.
> > > >
> > > > RFC3168 does not explicitly discuss offloading, and one relevant statement is "When the TCP data sender is ready to set the CWR bit after reducing the congestion window, it SHOULD set the CWR bit only on the first new data packet that it transmits."
> > > > Therefore, IMO, RFC3168 does not provide a clear specification of what occurs when offloading is applied.
> > > > While the ACE field is a new terminology introduced in the AccECN draft, it incorporates the AE, CWR, and ECE flags.
> > > >
> > > > To make the virtio spec clear (i.e., without expanding on the implied information that should be in the RFC) and simple to grasp, I would propose the following changes.
> > > > "Device can receive TSO with the CWR flag set and follow the SHOULD requirements of the CWR bit described in Section 6.1 of RFC3168."
> > >
> > > This looks better.
> > > receive from where? The driver?

Hi Michael,

Indeed, it's indeed received from the driver. But it is skipped in the proposed texts, like the original text.

> > > Add a section name, too. You can drop SHOULD just say requirements, too.
> >
> > OK, then I will put
> > "Device can receive TSO with the CWR flag set and follow the requirements of the CWR bit described in "Section 6.1.2. The TCP Sender" of RFC3168."
> >
> > >
> > > > > >
> > > > > >  \item[VIRTIO_NET_F_HOST_UFO (14)] Device can receive UFO.
> > > > > >
> > > > > > @@ -695,8 +699,9 @@ \subsubsection{Packet 
> > > > > > Transmission}\label{sec:Device Types / Network Device / De
> > > > > >
> > > > > >    \item If the driver negotiated the VIRTIO_NET_F_HOST_ECN feature,
> > > > > >      the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}
> > > > > > -    indicates that the TCP packet has the ECN bit set\footnote{This case is not handled by some older hardware, so is called out
> > > > > > -specifically in the protocol.}.
> > > > > > +    indicates that the TCP packet has TCP CWR flag set, and 
> > > > > > + the flag will be handled differently for all segments of
> > > > >
> > > > > the CWR flag.
> > > >
> > > > Will be changed.
> > > >
> > > > >
> > > > > > +    an aggregated segment, as mentioned in 
> > > > > > + \hyperref[intro:rfc3168]{[RFC3168]}
> > > > >
> > > > > i could not find this in the spec either.
> > > > > "segment" is mentioned once, when talking about MSS.
> > > >
> > > > Here refers to an aggregated segment (e.g., one big super skb in Linux) that has more than one MSS block.
> > > > And the CWR flag will be handled differently for all segments inside, like I mentioned above, as implicitly described in RFC3168.
> > > > Is there any other suggested terminology that is common across OSs?
> > >
> > > I don't really know what you refer to.
> > > Given it's according to RFC, maybe no need to talk about segments?
> >
> > OK, so I suggest the following without going into the details:
> > "If the driver negotiated the VIRTIO_NET_F_HOST_ECN feature, the 
> > VIRTIO_NET_HDR_GSO_ECN bit in gso_type indicates that the TCP packet has the TCP CWR flag set and follows the requirements of the CWR bit described in "Section 6.1.2. The TCP Sender" of RFC3168."
> >
> > > > >
> > > > > > +    \footnote{This case is not handled by some older hardware, so is called out specifically in the protocol.}.
> > > > >
> > > > > what does this refer to? which protocol? hardware older than what?
> > > > >
> > > > > >     \end{itemize}
> > > > > >
> > > > > >  \item If the driver negotiated the 
> > > > > > VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO feature and the @@ -788,10 
> > > > > > +793,9 @@ \subsubsection{Packet Transmission}\label{sec:Device Types / Network Device / De  \field{gso_type} to VIRTIO_NET_HDR_GSO_UDP_L4.
> > > > > >
> > > > > >  The driver SHOULD NOT send to the device TCP packets 
> > > > > > requiring segmentation offload -which have the Explicit 
> > > > > > Congestion Notification bit set, unless the 
> > > > > > -VIRTIO_NET_F_HOST_ECN feature is negotiated, in which case 
> > > > > > the -driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in -\field{gso_type}.
> > > > > > +which have the TCP CWR flag set and require the flag be 
> > > > > > +handled as mentioned in \hyperref[intro:rfc3168]{[RFC3168]},
> > > > >
> > > > > still confusing.
> > > > > should not send packets that require the flag? or should not require the flag?
> > > > > if the former, "and which would require" ... if the later "and should not require".
> > > > > or better, make sentences shorter.
> > > >
> > > > Should not send packets that (1) require segment offload, (2) have the CWR flag set, and (3) need to follow the SHOULD requirements in Section 6.1 of RFC3168.
> > >
> > > Any of these? All of these? Any two of these?
> > >
> > > > Is it better like this?
> > > > "The driver should not send TCP packets to the device that require 
> > > > segmentation offload, and the set CWR flag needs to follow the 
> > > > "should requirement" in RFC 3168 section 6.1,
> > >
> > > this part is still confusing. what is "and" here? do not send packets that both require segmentation and have the CWR flag?
> > > do not send packets that require segmentation and also do not send packets that follow requirements?
> >
> > OK, it shall be the latter case you describe.
> > Because, if the TCP packet requires segment offload but the RFC3168 requirement is not required (notice that it is a SHOULD requirement rather than a MUST), the packet can still be transmitted.
> > In this case, the CWR flag will be delivered not just in the first packet, but this is as intended.
> >
> > As such, I would like to propose a shorten text:
> > "The driver should not send to the device TCP packets that require not only segmentation offload but also following the CWR flag requirements described in "Section 6.1.2 The TCP Sender" of RFC3168, unless..."
> 
> same issue, still confusing.
> do you just mean VIRTIO_NET_HDR_GSO_ECN set?

Yes, I mean VIRTIO_NET_HDR_GSO_ECN, and below is another proposal:
"The driver should not send TCP packets that require both segmentation offload and CWR flag handling (described in Section 6.1.2 The TCP Sender of RFC3168), unless the VIRTIO_NET_F_HOST_ECN capability is negotiated, in which case the driver must set the VIRTIO_NET_HDR_GSO_ECN bit in gso_type."

BRs,
Chia-Yu 
> > >
> > > > unless the VIRTIO_NET_F_HOST_ECN
> > > > capability is negotiated, in which case the driver must set 
> > > > VI_IO_NET_SO in GSO_type."
> > > >
> > > > BRs,
> > > > Chia-Yu
> > > > >
> > > > > > +unless the VIRTIO_NET_F_HOST_ECN feature is negotiated, in 
> > > > > > +which case the driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
> > > > >
> > > > > in which case? when it is or when it is not negotiated? or when it sends packets maybe?
> > > > >
> > > > > >
> > > > > >  If VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO is negotiated, the driver 
> > > > > > MAY set
> > > > > >  VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 bit or the
> > > > > > VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 bit @@ -1105,9 +1109,9 @@ 
> > > > > > \subsubsection{Processing of Incoming 
> > > > > > Packets}\label{sec:Device Types / Network  \end{enumerate}
> > > > > >
> > > > > >  Additionally, VIRTIO_NET_F_GUEST_CSUM, TSO4, TSO6, UDP, 
> > > > > > UDP_TUNNEL -and ECN features enable receive checksum, large 
> > > > > > receive offload and ECN -support which are the input 
> > > > > > equivalents of the transmit checksum, -transmit segmentation 
> > > > > > offloading and ECN features, as described
> > > > > > +and ECN features enable receive checksum, large receive 
> > > > > > +offload and
> > > > > > +RFC3168 ECN support which are the input equivalents of the 
> > > > > > +transmit checksum, transmit segmentation offloading and 
> > > > > > +RFC3168 ECN features, as described
> > > > > >  in \ref{sec:Device Types / Network Device / Device Operation 
> > > > > > / Packet Transmission}:
> > > > > >  \begin{enumerate}
> > > > > > @@ -1210,10 +1214,9 @@ \subsubsection{Processing of Incoming 
> > > > > > Packets}\label{sec:Device Types / Network  the VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM bit in \field{flags}.
> > > > > >
> > > > > >  The device SHOULD NOT send to the driver TCP packets 
> > > > > > requiring segmentation offload -which have the Explicit 
> > > > > > Congestion Notification bit set, unless the 
> > > > > > -VIRTIO_NET_F_GUEST_ECN feature is negotiated, in which case 
> > > > > > the -device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in -\field{gso_type}.
> > > > > > +which have the TCP CWR flag set and require the flag be 
> > > > > > +handled as mentioned in \hyperref[intro:rfc3168]{[RFC3168]}, 
> > > > > > +unless the VIRTIO_NET_F_GUEST_ECN feature is negotiated, in which case the device MUST set the VIRTIO_NET_HDR_GSO_ECN bit in \field{gso_type}.
> > > > > >
> > > > > >  If the VIRTIO_NET_F_GUEST_CSUM feature has been negotiated, 
> > > > > > the device MAY set the VIRTIO_NET_HDR_F_NEEDS_CSUM bit in diff 
> > > > > > --git a/introduction.tex b/introduction.tex index 
> > > > > > e60298a..d52622e
> > > > > > 100644
> > > > > > --- a/introduction.tex
> > > > > > +++ b/introduction.tex
> > > > > > @@ -168,6 +168,9 @@ \section{Normative References}\label{sec:Normative References}
> > > > > >      Leiba, B., "Ambiguity of Uppercase vs Lowercase in RFC 2119 Key Words", BCP
> > > > > >      14, RFC 8174, DOI 10.17487/RFC8174, May 2017
> > > > > >          \newline\url{http://www.ietf.org/rfc/rfc8174.txt}\\
> > > > > > +     \phantomsection\label{intro:rfc3168}\textbf{[RFC3168]} &
> > > > > > +    S. Floyd., ``The Addition of Explicit Congestion Notification (ECN) to IP'', September 2001.
> > > > > > +     \newline\url{http://www.ietf.org/rfc/rfc3168.txt}\\
> > > > > >  \end{longtable}
> > > > > >
> > > > > >  \section{Non-Normative References}
> > > > > > --
> > > > > > 2.34.1