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

> From: Chia-Yu Chang (Nokia) <[email protected]>
> Sent: 07 October 2025 02:12 PM
> 
> > -----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.

Can you please discuss with the netdev maintainer and clarify the usage of SKB_GSO_TCP_ECN.
i.e.
a. does SKB_GSO_TCP_ECN apply to RX side? From the name of "GSO" it does not seem so, but three drivers hns3, mlx5, virtio-net are using these on the RX.
If this applies to the RX also, can we please send comment update patch to net-next to update, include/linux/skbuff.h

Something like:

--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -671,7 +671,9 @@ enum {
        /* This indicates the skb is from an untrusted source. */
        SKB_GSO_DODGY = 1 << 1,

-       /* This indicates the tcp segment has CWR set. */
+       /* This indicates the first tcp segment has CWR set in GSO or GRO stream.
+        * and same first tcp/ip packet has IP ECN bit set accordingly.
+        */

Or if it does not have IP ECN link, above documentation to reflect accordingly.
But I do not understand how CWR bit is set but IP.ECN not. Don't they go hand-in-hand (at least for TCP)?

> 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.
> 
Instead of writing "receive TSO", it should be written as driver can receive coalesced TCP segments...

> 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.