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

-----Original Message-----
From: Willem de Bruijn <[email protected]> 
Sent: Tuesday, August 25, 2026 6:12 PM
To: Chia-Yu Chang (Nokia) <[email protected]>; Willem de Bruijn <[email protected]>; Jakub Kicinski <[email protected]>
Cc: Mirja Kuehlewind <[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]>; [email protected]
Subject: RE: [PATCH v5 net-next 1/2] net: update comments for SKB_GSO_TCP_ECN and SKB_GSO_TCP_ACCECN


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, 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.
> 

Yes, I agree.

In another email I was asking Jijie how their hns3 HW-GRO handles CWR-marked packets.
If their device flushes immediately when a CWR=1 packet arrives, then the SKB_GSO_TCP_ECN handling in hns3_enet.c may not be necessary.
In that case, each CWR-marked packet is emitted separately and there may be no need to use either SKB_GSO_TCP_ACCECN or SKB_GSO_TCP_ECN.

If their HW-GRO can coalesce multiple CWR=1 packets, flagging the resulting skb with SKB_GSO_TCP_ECN would lose AccECN information during segmentation.
In that case, the skb should either carry SKB_GSO_TCP_ACCECN, or no ECN GSO flag at all if the segmentation engine preserves the CWR signaling unchanged.

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

Yes, this is what I mean. And I agree the cost would not be small.
So, I am thinking of first checking the existing drivers that still use SKB_GSO_TCP_ECN. So far I have only found hns3_enet.
I also plan to update the comments for SKB_GSO_TCP_ECN and SKB_GSO_TCP_ACCECN in include/linux/skbuff.h to capture the conclusions from this discussion.

Then we can at least clarify the expected semantics and review whether the existing HW-GRO implementations and drivers are consistent with them.
Also, patch 6f74bc8b6e8d0 already added a flags_cwr GRO test, which may be useful when reviewing the expected behavior of future HW-GRO implementations.

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