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

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

> --
>  i.