Re: [PATCH v4 net-next 02/13] gro: flushing when CWR is set negatively affects AccECN

["Ilpo Järvinen" <ij@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3453 bytes --]

On Thu, 16 Oct 2025, Paolo Abeni wrote:
> On 10/13/25 7:03 PM, chia-yu.chang@nokia-bell-labs.com wrote:
> > From: Ilpo Järvinen <ij@kernel.org>
> > 
> > As AccECN may keep CWR bit asserted due to different
> > interpretation of the bit, flushing with GRO because of
> > CWR may effectively disable GRO until AccECN counter
> > field changes such that CWR-bit becomes 0.
> > 
> > There is no harm done from not immediately forwarding the
> > CWR'ed segment with RFC3168 ECN.
> 
> I guess this change could introduce additional latency for RFC3168
> notification, which sounds not good.
> 
> @Eric: WDYT?

I'm not Eric but I want to add I foresaw somebody making this argument 
and thus wanted to not hide this change into some other patch so it can be 
properly discussed and rejected if so preferred, either way it's not a 
correctness issue.

I agree it's possible for some delay be added but the question is why 
would that matter? "CWR" tells sender did already reduce its sending rate 
which is where congestion control aims to. So the reaction to congestion 
is already done when GRO sees CWR (some might have a misconception that
delivering CWR causes sender to reduce sending rate but that's not the 
case). With RFC 3168 ECN, CWR only tells the receiving end to stop sending 
ECE. Why does it matter if that information arrives a bit later?

If there are other segments, they normally don't have CWR with RFC 3168 
ECN which normally set CWR once per RTT. A non-CWR'ed segment results in 
flush after an inter-packet delay due to flags difference. That delay is 
nothing compared to GRO aggregating non-CWR segments en masse which is 
in n times the inter-packet delay (simplification, ignores burstiness, 
etc.).

If there are no other segments, the receiver won't be sending any ECEs 
either, so the extra delay does not seem that impactful.

Some might argue that with this "special delivery" for CWR the segment 
could trigger an ACK "sooner", but GRO shouldn't hold the segment forever 
either (though I don't recall the details anymore). But if we make that 
argument (which is no longer ECN signalling related at all, BTW), why use 
GRO at all as it add delay for other segments too delaying other ACKs, why 
is this CWR'ed segment so special that it in particular must elicit ACK 
ASAP? It's hard to justify that distinction/CWR speciality, unless one has 
that misconception CWR must arrive ASAP to expedite congestion reaction 
which is based on misunderstanding how RFC 3168 ECN works.

Thus, what I wrote to the changelog about the delay not being harmful 
seems well justified.

> On the flip side adding too much
> AccECN logic to GRO (i.e. to allow aggregation only for AccECN enabled
> flows) looks overkill.

The usual aggregation works on header bits remaining identical which 
just happens to also suit AccECN better here. The RFC 3168 CWR trickery is 
what is an expection to the rule, and as explained above, it does not seem 
even that useful.

This CWR special delivery rule, on the other hand, is clearly harmful for 
aggregating AccECN segments which may have long row of CWR flagged 
segments if ACE field remains unchanging. None of them can be aggregated 
by GRO if this particular change is not accepted. Not an end of the world 
but if we weight the pros and cons, it seems to clearly favor not keeping 
this special delivery rule.


-- 
 i.