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

> -----Original Message-----
> From: Parav Pandit <[email protected]> 
> Sent: Friday, October 17, 2025 1:28 PM
> To: Ilpo Järvinen <[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]; Vidhi Goel <[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.
> 
> 
> 
> > From: Ilpo Järvinen <[email protected]>
> > Sent: 15 October 2025 11:13 PM
> >
> > On Wed, 15 Oct 2025, Parav Pandit wrote:
> >
> > >
> > >
> > > > From: Ilpo Järvinen <[email protected]>
> > > > Sent: 15 October 2025 02:05 AM
> > > >
> > > > On Mon, 13 Oct 2025, Parav Pandit wrote:
> > > > > > 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.
> > > >
> > > > Hi,
> > > >
> > > > It feels little odd to me to make such a change to a comment now 
> > > > if the whole point with AccECN is that SKB_GSO_TCP_ECN can no 
> > > > longer be safely used on the rx side.
> > >
> > > So SKB_GSO_TCP_ECN will be overloaded where if AccECN is negotiated, 
> > > GRO stream contains IP+TCP bits as per AccECN
> >
> > No, it's not overloaded (in the sense that SKB_GSO_TCP_ECN could be 
> > interpreted in more than one way). The SKB_GSO_TCP_ECN semantics still 
> > follow RFC 3168 ECN which results in wrong behavior (corruption of 
> > AccECN signal).
> >
> > > And without accECN negotiated, only the first segment will have ECN bits?
> > > If so, can the header comment be updated in such a way?
> >
> > If some change is wanted to this, maybe something along these lines:
> >
> >  * For Tx, this indicates the first TCP segment has CWR set and any
> >  * subsequent segment in the same skb have CWR cleared.
> >  *
> >  * This cannot be used on Rx because it is not tracked whether the
> >  * connection the segment belongs to uses RFC 3168 or Accurate ECN 
> > (using
> >  * RFC 3168 ECN offloading for AccECN segment may result in corruption 
> > of
> >  * AccECN signal). Thus, on Rx any CWR flagged segment must use
> >  * SKB_GSO_TCP_ACCECN.
> >
> Sounds find to me.
> I believe virtio-net, mlx5 and hns driver to follow the fix to not set this on rx, once your commit goes through.
> Right?

Using SKB_GSO_TCP_ACCECN in RX can better explicitly clarify that any RFC3168 ECN offload shall not be taken.

So, further notes on SKB_GSO_TCP_ACCECN as below may be helpful?

--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -671,7 +671,10 @@ enum {
	SKB_GSO_DODGY = 1 << 1
-	/* This indicates the tcp segment has CWR set. */
+	/* For Tx, this indicates the first TCP segment has CWR set, and any
+	 * subsequent segment in the same skb has CWR cleared.
+	 * This cannot be used on Rx, because the connection to which the segment
+	 * belongs is not tracked to use RFC 3168 or Accurate ECN, and using
+	 * RFC 3168 ECN offload may corrupt AccECN signal of AccECN segments.
+	 */
	SKB_GSO_TCP_ECN = 1 << 2,
[...]

+	/* For TX, this indicates the TCP segment uses the CWR flag as part of
+	 * AccECN signal, and the CWR flag is not modified in the skb.
+	 * For RX, this is used to ensure the CWR flag is not cleared by any RFC
+	 * 3168 ECN offload, thus keeping AccECN signal of AccECN segments.
+	 */
	SKB_GSO_TCP_ACCECN = 1 << 19,