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

> > > On Fri, 14 Aug 2026 09:34:01 -0400 Willem de Bruijn wrote:
> > > > > 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.
> > >
> > > I'm still not following.. Maybe Willem can ELI5 what the problem is.
> > >
> > > _SW_ GRO follows only the AccECN rules.
> > > But if HW GRO follows RFC 3168 and we mark the aggregate as 
> > > SKB_GSO_TCP_ECN - TSO will also abide, and segmented output will be 
> > > identical to pre-GRO input.
> > 
> > +1
> > 
> > > Are we trying to ban RFC 3168 behavior in HW purely to match SW?
> > 
> > I think that's the intent here?
> Hi Willem,
> 
> I think we can still change SKB_GSO_TCP_ECN into SKB_GSO_TCP_ACCECN on the RX path, even if HW GRO and SW GRO use different aggregation rules.
> Currently, SW GRO flushes when the CWR state changes and sets SKB_GSO_TCP_ACCECN when the resulting skb carries CWR=1.
>
> 
> For HW GRO, if the device flushes immediately when a CWR=1 packet arrives, there is no need to set either SKB_GSO_TCP_ECN or SKB_GSO_TCP_ACCECN.
> In that case, each CWR=1 packet is emitted separately, and there is no need to preserve multiple CWR indications through GSO.
> So, if the HW can be confirmed to always flush CWR-marked packets without coalescing them, preserving multiple CWR indications through GSO is not required.
> In that case, using SKB_GSO_TCP_ACCECN would not provide any additional benefit.

I agree.

The difference on transmit is that SKB_GSO_TCP_ACCECN will copy the
ECN bits to every segment, whereas SKB_GSO_TCP_ECN will only set the
bits on the first segment.

From commit 023af5a72ab1 ("gso: AccECN support") that introduced
SKB_GSO_TCP_ACCECN:

    With RFC 3168 ECN aware TSO (NETIF_F_TSO_ECN) CWR flag is cleared
    starting from 2nd segment which is incompatible how AccECN handles
    the CWR flag. Such super-segments are indicated by SKB_GSO_TCP_ECN.
    With AccECN, CWR flag (or more accurately, the ACE field that also
    includes ECE & AE flags) changes only when new packet(s) with CE
    mark arrives so the flag should not be changed within a super-skb.

Makes me wonder what SKB_GS_TCP_ACCECN adds. Copying bits
from the GSO skb to all segments is the default. SKB_GSO_TCP_ECN is
an indication that the NIC knows how to diverge from this default for
these specific bits.

That commit confirms this:

    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).`

> 
> The problematic case is when a HW GRO aggregates multiple packets carrying CWR=1 and only sets SKB_GSO_TCP_ECN.

The driver of such a device could be updated to set SKB_GSO_TCP_ACCECN
or not set any such ECN GSO flag.

The driver of other devices that do follow RFC 3168 semantics will
continue to have to set SKB_GSO_TCP_ECN on the GSO skb. It is quite
plausible that few or no devices currently support ECN in their HW-GRO
coalescing.

> During GSO, only the first output segment would carry CWR=1, which loses the remaining CWR signaling information required by AccECN.
> In that case, changing from SKB_GSO_TCP_ECN to SKB_GSO_TCP_ACCECN would preserve the original signaling by ensuring that the segmented packets carry the correct CWR information.
> As I understand RFC3168, receiving additional CWR-marked packets is harmless, because once a valid CWR has been received, the receiver already stops echoing ECE.

Agreed.

> Additional CWR indications do not change the receiver state.
> This would have a performance impact since it disables HW TSO for the skb and falls back to software segmentation.

You mean if the device advertises NETIF_F_TSO_ECN, and GSO skbs may
contain AccECN signals, the host must downgrade from TSO to GSO to
avoid corrupting the signal?

The cost of that would be significant. Not something to do for
established environments. But technically seemingly correct, at least
for packets with non-zero ECN bits.

> But IMO it preserves correctness for both RFC3168 ECN and AccECN.

Side-note: this thread probably has way too many Cc: for this narrow topic.