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

> 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
And without accECN negotiated, only the first segment will have ECN bits?

If so, can the header comment be updated in such a way?