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

> -----Original Message-----
> From: Ilpo Järvinen <[email protected]> 
> Sent: Saturday, October 4, 2025 12:05 AM
> To: Parav Pandit <[email protected]>
> Cc: Chia-Yu Chang (Nokia) <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Koen De Schepper (Nokia) <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]
> Subject: RE: [PATCH v10 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, 3 Oct 2025, Parav Pandit wrote:
> 
> > > > > > You described,
> > > > > >
> > > > > >     If NIC is completely unaware of RFC3168 ECN (doesn't support
> > > > > >     NETIF_F_TSO_ECN) or its TSO engine can be set to not touch CWR flag
> > > > > >     despite supporting also NETIF_F_TSO_ECN, TSO could be safely used
> > > > > >     with AccECN on such NIC. This should be evaluated per NIC basis
> > > > > >     (not done in this patch series for any NICs).
> > > > > >
> > > > > > Based on above commit log, can we say that when existing 
> > > > > > GSO_ECN is not
> > > > > negotiated, ACCECN can work as_is without the patch_2?
> > > > > >
> > > > > > [1] commit 023af5a72ab16 ("gso: AccECN support")
> > > > >
> > > > > Indeed, this patch description implies that NIC that currently 
> > > > > don't support TCP_ECN will naturally support ACCECN.
> > > > > But an extra TSO_ACCECN flag will make this into an explicit 
> > > > > requirement, like our patch in the Linux.
> > > > >
> > > > If so, the 2nd patch should word saying, the device must not 
> > > > modify IP
> > > congestion code point bits and TCP ACE bits.
> > > > All packets in the segment offload must have same bits.
> > > >
> > > > Similarly on RX side for GRO, the device requirement to spell out 
> > > > that GRO
> > > stream to break when two subsequent packets have different bits etc.
> > > >
> > > > If you can help to understand why RX side should set this and how 
> > > > one may
> > > plan to use it, it makes sense to define the GUEST bit.
> > > > Presently it does not look useful for rx side (guest bit).
> > > >
> > > > > Related to another email about existing driver, one solution 
> > > > > would be replace SKB_GSO_TCP_ECN with SKB_GSO_TCP_ACCECN, like 
> > > > > in 4e4f7cefb130af6aba6a393b2d13930b49390df9.
> > > > > This can avoid CWR corruption somewhere after GRO, and we plan 
> > > > > to submit related patches (old driver, virtio-related) into 
> > > > > net-next after this is approved in virtio-spec.
> > > >
> > > > Commit 4e4f7cefb130a looks incorrect to me, or I must be missing
> > > something.
> > > > It looks incorrect to me is because:
> > > > 1. SKB_GSO_TCP_ACCECN is set when cwr is set, regardless of tcp 
> > > > socket
> > > following rfc3168 or accecn.
> > > > I was hoping to set this flag only when its accecn.
> > > >
> > > > 2. Even though it is set, it is not read by anyone, so not sure 
> > > > why one would
> > > set it.
> > > >
> > > > 3. It appears to me that it is TX side flag and rx GRO should not 
> > > > be setting it,
> > > because there isn't rx side code to handle it yet.
> > > > May be this is part of some multi series work?
> > > >
> > > > 4. The commit 4e4f7cefb130a claims to prevent field corruption.
> > > > A corruption sounds like a bug. And if it is a bug, it the patch 
> > > > needs fixes tag,
> > > but it does not have it.
> > > > Probably no fixes tag, because no users, that is strange kernel code to me.
> > >
> > > Please let me elaborate more.
> > >
> > > Patch 023af5a72ab16 is in tcp_gso_segment(), so it disaggregates 
> > > packet on the TX side after TCP stack.
> > > Before patch 023af5a72ab16, CWR flag from the 2nd packet will be 
> > > cleaned if TSO_ECN is set by RFC3168 ECN in tcp_output.c New 
> > > TSO_ACCECN will be set by AccECN protocol in tcp_output.c, 
> > > indicating that this packet does not want CWR flag cleaning from the 2nd packet.
> > >
> > I understood the commit 023af5a72ab16 and commit looks good to me.
> >
> > > Patch 4e4f7cefb130a is in tcp_gro_receive(), so it aggregates 
> > > packets on the RX side before going to TCP stack.
> > > And you can see before 4e4f7cefb130a patch, TSO_ECN flag will be set 
> > > if cwr is 1.
> > SKB_GSO_TCP_ACCECN and SKB_GSO_TCP_ECN are mutually exclusive.
> > So not sure how gro receive commit 4e4f7cefb130a replaces TCP_ECN with ACCECN.
> >
> > But regardless of that, you explain that rx side can set skb field 
> > based on how device has coalesced the packet, And tx side can use the same skb when using tcp/ip stack as router/forwarding unit.
> > (instead of terminating/originating tcp connection from the socket).
> >
> > > This TSO_ECN flag implies that it's ok to segment this aggregated 
> > > packet and clean CWR from the 2nd packets by latter operations.
> > > But this is NOT ok for AccECN protocl, new TSO_ACCECN flag 
> > > explicitly tells latter operations that it's NOT ok to clean CWR from the 2nd packets.
> > >
> > Right.
> >
> > > And the corruption in the commit message means the CWR flag cannot 
> > > keep the origianl value (i.e., been reset by latter operations).
> >
> > If the code reset the value when it should not, it means it was bug 
> > and needs fixes tag.
> 
> It was intentional for RFC 3168 ECN offloading to the reset the value.
> In the traditional sense it isn't "bug" because it was done intentionally.
> "When it should not" too, should considered with the background this offloading feature has (from era when AccECN didn't exists at all). But it's fine to end up disagreeing on this detail about Fixes tag, in any case that commit is immutable now so this discussion is pretty moot :-).
> 
> > Anyway, I don't want to diverge on the discussion.
> >
> > Seems like setting SKB_GSO_TCP_ECN seems fine even on rx skb too.
> 
> SKB_GSO_TCP_ECN will not replicate the same TCP header flags in a forwarding scenario:
> 
> Segment 1 CWR set
> Segment 2 CWR set
> 
> GRO rx and GSO tx with SKB_GSO_TCP_ECN, after forwarding outputs these
> segments:
> 
> Segment 1 CWR set
> Segment 2 CWR cleared
> 
> Thus, the ACE field in Segment 2 no longer contains the same value as it was sent with.
> 
> As rx side, generally, does not know if the sender is using AccECN or RFC
> 3168 (or some entirely different semantics for these flags as these TCP features are negotiated). I said "generally" because it would required connection tracking to know it which is expensive and has its own challenges.
> 
> 
> If you, on the other hand, imply that SKB_GSO_TCP_ECN could just become SKB_GSO_TCP_ACCECN just by declaring this corruption a "bug", that too is not possible.
> 
> Or to be more precise, that would mean removing RFC 3168 ECN offloading support from the tx side (where it may have some, albeit IMO quite limited [*], performance benefits). When we're TX'ing segment the kernel knows originate from RFC 3168 sender as indicated by SKB_GSO_TCP_ECN flag, it can safely perform the RFC 3168 offloading without fear of corruption.
> 
> 
> [*] RFC 3168 CWR is only sent once per RTT so those segments should not appear very frequently. What RFC 3168 offloading does is effectively allow CWR to be send within the same super-skb with non-CWR segments as the RFC 3168 offloading with SKB_GSO_TCP_ECN ensures CWR is only sent out in one segment. Without SKB_GSO_TCP_ECN, the sender would have to send CWR segment separately (this potentially comes with a minor performance cost).
> 
> > So you should just refine the wordings for patch 1 and 2 to match 
> > exactly what it means for virtio spec requiirements and description 
> > section.

Thanks Ilpo for example above, and I just realized that the mail server miss-resend an old email on Sunday, sorry for spamming.
And I hope above explanations related to the CWR flag are clear.

One more thing I would like to raise is about the ECN flag mentioned in current virtio-spec.
As SKB_GSO_TCP_ECN is not related to IP.ECN in RX path even before 4e4f7cefb130a patch, and neither in TX path before 023af5a72ab16.
So, in below texts, I still only keep CWR flag but do updates based on feedback from Parav:

1. VIRTIO_NET_F_GUEST_ECN (9): Driver can receive TSO with the CWR flag set.
   This CWR flag applies to only the first packet; this flag is cleared in
   subsequent packets in the offloaded segment.

2. VIRTIO_NET_F_HOST_ECN (13): Device can receive TSO with the CWR flag set.
   This CWR flag applis to only the first packet; this flag is cleared in
   subsequent packets in the offloaded segment.

3. If the driver negotiated the VIRTIO_NET_F_HOST_ECN feature,
   VIRTIO_NET_HDR_GSO_ECN bit in gso_type indicates that the CWR flag of the
   TCP packet applied to only the first packet; this flag is cleared in
   subsequent packets in the offloaded segment.

4. The driver SHOULD NOT send to the device TCP packets requiring segmentation
   offload with the CWR flag set ONLY for to the first packet and cleared in
   subsequent packets, unless the VIRTIO_NET_F_HOST_ECN feature is negotiated,
   in which case the driver MUST set VIRTIO_NET_HDR_GSO_ECN bit in gso_type.

5. The device SHOULD NOT coalesce TCP packets under single GSO stream if the
   first packet has the CWR flag set and from the second onwards packets do not
   have the CWR flag set, unless the VIRTIO_NET_F_GUEST_ECN feature is negotiated,
   in which case the device MUST set VIRTIO_NET_HDR_GSO_ECN bit in gso_type.