Chia-Yu Chang (Nokia) wrote: > > From: Mirja Kuehlewind <[email protected]> > > Sent: Friday, August 14, 2026 10:57 AM > > To: Willem de Bruijn <[email protected]>; Chia-Yu Chang > > (Nokia) <[email protected]> > > Cc: Jakub Kicinski <[email protected]>; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [email protected]; [email protected]; [email protected]; > > [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]; Ingemar Johansson S > > <[email protected]>; [email protected]; [email protected]; > > [email protected]; [email protected]; Parav Pandit > > <[email protected]>; Willem de Bruijn <[email protected]> > > Subject: Re: [PATCH v5 net-next 1/2] net: update comments for > > SKB_GSO_TCP_ECN and SKB_GSO_TCP_ACCECN > > > > Hi Willem, > > > > The current function of is SKB_GSO_TCP_ECN wrong. Fixing this causes the > > regression. > > > > Mirja > > > > > > > > From: Willem de Bruijn <mailto:[email protected]> > > Subject: Re: [PATCH v5 net-next 1/2] net: update comments for > > SKB_GSO_TCP_ECN and SKB_GSO_TCP_ACCECN > > > > On Thu, Aug 13, 2026 at 6:33 AM Chia-Yu Chang (Nokia) > > <mailto:[email protected]> wrote: > > > > > On Wed, 12 Aug 2026 10:33:19 +0000 Chia-Yu Chang (Nokia) wrote: > > > > > > - /* This indicates the tcp segment has CWR set. */ > > > > > > + /* For TX, this indicates that the first TCP segment has CWR > > > > > > set, and > > > > > > + * any subsequent segment in the same skb has CWR cleared. > > > > > > This flag > > > > > > + * must not be used in RX, because the connection to which > > > > > > the segment > > > > > > + * belongs is not tracked to use RFC3168 or AccECN. Using > > > > > > RFC3168 ECN > > > > > > + * offload may clear CWR and corrupt ACE signal (CWR is part > > > > > > of it). > > > > > > + * Instead, SKB_GSO_TCP_ACCECN shall be used to avoid CWR > > > > > > corruption. > > > > > > + */ > > > > > > > > > > I still can't wrap my head around this TBH. > > > > > > > > > > SKB_GSO_TCP_ECN means RFC3168 > > > > > SKB_GSO_TCP_ACCECN means AccECN > > > > > > > > > > If the HW can correctly detect cwr on first frame and then no cwr and > > > > > report that as ECN/RFC3168 - what's the problem? TSO will produce the > > > > > exact expected segment sequence. > > > > > > > > > > Is the program that if we re-GRO that frame in SW we end up with > > > > > ECN+ACCECN on the same skb? > > > > > > > > Yes, this is the problem. > > > > The HW does not know whether the received packets belong to an RFC3168 > > > > ECN flow or an AccECN flow on the RX path. > > > > For example, HW GRO may set SKB_GSO_TCP_ECN after observing that the > > > > first packet has CWR=1: > > > > > > > > +===================+==========+=================+================+ > > > > | Packet id | CWR flag | Flag | Flushed as SKB | > > > > +===================+==========+=================+================+ > > > > | 0 | 1 | SKB_GSO_TCP_ECN | 0 | > > > > | 1 | 0 | - | 0 | > > > > | 2 | 1 | - | 0 | > > > > | 3 | 1 | - | 1 | > > > > +===================+==========+=================+================+ > > > > > > > > If the aggregated skb is forwarded through a device using GSO, e.g., > > > > HW RX (GRO) -> veth TX (GSO), the SKB_GSO_TCP_ECN applies RFC3168 > > > > semantics. This means that only the 1st segment keeps the CWR flag > > > > while all subsequent segments have CWR cleared: > > > > > > > > +===================+==========+ > > > > | Packet id | CWR flag | > > > > +===================+==========+ > > > > | 0 | 1 | > > > > | 1 | 0 | > > > > | 2 | 0 | > > > > | 3 | 0 | > > > > +===================+==========+ > > > > > > > > This behavior is ok for RFC3168, since CWR is expected to appear only > > > > once. However, for AccECN, CWR is part of the ACE signal and must be > > > > preserved across all segments. > > > > > > But this would be obviously a buggy HW-GRO implementation. > > > The rules for HW-GRO RFC3168 are -- ignore CWR on first segment (host > > > responsible for populating SKB_GSO_TCP_ECN), and CWR _must be 0_ for all > > > subsequent segments. > > > > > > > In the example above, the original CWR sequence was 1,0,1,1. > > > > But after re-segmentation it becomes: 1,0,0,0. > > > > This is why SKB_GSO_TCP_ECN should not be used in RX/GRO paths. > > > > > > We have extensive gro tests under > > > tools/testing/selftests/drivers/net/gro.py > > > > > > If you want to catch bad devices - add appropriate test cases there. > > > > > > > I added a test case in patch 6f74bc8b6e8d related to the CWR flag. > > In that case, there are 5 packets with CWR values of 0, 1, 1, 0, and 0, and > > packets are flushed after the 1st, 3rd, and 5th packets. > > The gro.py uses this case in tools/testing/selftests/net/lib/gro.c to > > verify CWR behavior. > > But indeed, that does not cover whether SKB_GSO_TCP_ECN or > > SKB_GSO_TCP_ACCECN shall be set during the GRO. > > So, a test might be added to verify the SKB_GSO_TCP_ECN or > > SKB_GSO_TCP_ACCECN flags (if there is another suggested way, please let me > > know)? > > > > > The comment as stated seems to be misleading - there's nothing wrong with > > > using the flag if the device follows the RFC3168 semantics correctly. > > > > > > And of course, adding a comment and hoping people will find it is much > > > weaker than adding tests. > > > > > > Again, maybe I'm missing what _actually_ doesn't work here. > > > > Before adding an extra test, we need to clarify the definition and usages > > of these flags. > > At the TX path, in tcp_gso_segment() of net/ipv4/tcp_offload.c, the > > SKB_GSO_TCP_ACCECN flag is used to preserve the CWR flags for AccECN flows. > > Otherwise, when without SKB_GSO_TCP_ACCECN (RFC3168 ECN or Non-ECN flows), > > cwr will be cleared from the following packets. > > > > For the RX path, unfortunately I do not find a clear rule of when > > SKB_GSO_TCP_ECN shall be set except in include/linux/skbuff.h. > > Plus, the device usually does not track packets belonging to RFC3168 ECN or > > ACCECN flows. > > So, my previous thought is to always use SKB_GSO_TCP_ACCECN in the RX path > > to avoid any potential CWR bleaching. > > > > This would be a case where AccECN support causes a regression for > > regular ECN handling, if that is no longer allowed to be coalesced. > > > > Most HW-GRO hardware out there today likely only supports ECN. In > > which case they can set SKB_GSO_TCP_ECN fine. > > > > If AccECN flows cannot be differentiated from ECN flows, on such > > devices, does the admin have to disable HW-GRO with ECN if they care > > about preserving AccECN signals? > > > > What does SW GRO do here? > > Hi Willem, > > Current SW GRO sets SKB_GSO_TCP_ACCECN when the flushed skb carries CWR in > tcp_gro_complete(): > if (th->cwr) > shinfo->gso_type |= SKB_GSO_TCP_ACCECN;
And I suppose it follows correct AccECN rules for coalescing. That is a performance regression from RFC 3168 ECN, as it allows for less effective coalescing. I have no intuition how much it will differ in practice. > > For HW GRO of a legacy device that implementing RFC3168 semantics, setting > SKB_GSO_TCP_ECN seems reasonable. > However, such a device would not be able to preserve ACCECN signaling across > the GRO/GSO. > In that case, if preserving AccECN signaling is required, disabling HW GRO > may indeed be necessary. Right. And there currently is no kernel API to disable only ECN coalescing. NETIF_F_GRO_HW enables or disables HW-GRO entirely. Or even to signal whether a HW-GRO implementation is AccECN capable. Disabling HW-GRO can be a huge efficiency regression. I suspect many users will prioritize the efficiency over preserving the AccECN signal. That said, some devices may have other ways to configure such finer details of their HW-GRO, even though not available through Ethtool. > And I still think the SKB_GSO_TCP_ECN comment could be clarified. > For example, by stating that "RX GRO implementations which need to preserve > CWR information across re-segmentation should use SKB_GSO_TCP_ACCECN."? > > Thanks! > Chia-Yu
