On Mon, Oct 20, 2025 at 8:26 AM Chia-Yu Chang (Nokia) <[email protected]> wrote: > > > -----Original Message----- > > From: Ilpo Järvinen <[email protected]> > > Sent: Thursday, October 16, 2025 10:27 PM > > To: Paolo Abeni <[email protected]> > > Cc: Chia-Yu Chang (Nokia) <[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]; > > [email protected]; [email protected]; cheshire > > <[email protected]>; [email protected]; [email protected]; Vidhi > > Goel <[email protected]> > > Subject: Re: [PATCH v4 net-next 02/13] gro: flushing when CWR is set > > negatively affects 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 Thu, 16 Oct 2025, Paolo Abeni wrote: > > > On 10/13/25 7:03 PM, [email protected] wrote: > > > > From: Ilpo Järvinen <[email protected]> > > > > > > > > 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. > > Hi Paolo, > > I agree with what was mentioned by Ilpo above. > > But if Eric can share extra comments or some particular cases would be > helpful. > > Shall we submit all patches with changes (and keep this patch unchanged)? Or > please suggest other ways to move forward.
Hmm... maybe now is a good time to amend tools/testing/selftests/net/gro.c In general, the lack of tests in your series is not really appealing to me.
