RE: [PATCH v5 net-next 1/2] net: update comments for SKB_GSO_TCP_ECN and SKB_GSO_TCP_ACCECN

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Chia-Yu Chang (Nokia) wrote:
> > From: Mirja Kuehlewind <mirja.kuehlewind@ericsson.com> 
> > Sent: Friday, August 14, 2026 10:57 AM
> > To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>; Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
> > Cc: Jakub Kicinski <kuba@kernel.org>; shaojijie@huawei.com; shenjian15@huawei.com; linux-rdma@vger.kernel.org; eperezma@redhat.com; jasowang@redhat.com; virtualization@lists.linux.dev; mst@redhat.com; xuanzhuo@linux.alibaba.com; pabeni@redhat.com; edumazet@google.com; linux-doc@vger.kernel.org; corbet@lwn.net; horms@kernel.org; dsahern@kernel.org; kuniyu@google.com; bpf@vger.kernel.org; netdev@vger.kernel.org; dave.taht@gmail.com; jhs@mojatatu.com; stephen@networkplumber.org; xiyou.wangcong@gmail.com; jiri@resnulli.us; davem@davemloft.net; andrew+netdev@lunn.ch; donald.hunter@gmail.com; ast@fiberby.net; liuhangbin@gmail.com; shuah@kernel.org; linux-kselftest@vger.kernel.org; ij@kernel.org; ncardwell@google.com; Koen De Schepper (Nokia) <koen.de_schepper@nokia-bell-labs.com>; g.white@cablelabs.com; Ingemar Johansson S <ingemar.s.johansson@ericsson.com>; cheshire@apple.com; rs.ietf@gmx.at; Jason_Livingood@comcast.com; vidhi_goel@apple.com; Parav Pandit <parav@nvidia.com>; Willem de Bruijn <willemb@google.com>
> > 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:willemdebruijn.kernel@gmail.com>
> > 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:chia-yu.chang@nokia-bell-labs.com> 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