> -----Original Message----- > From: Eric Dumazet <[email protected]> > Sent: Monday, October 20, 2025 5:32 PM > To: Chia-Yu Chang (Nokia) <[email protected]> > Cc: Ilpo Järvinen <[email protected]>; Paolo Abeni <[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 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.
Thanks for the feedback, and I will look into this tool (Anything I could refer to use/update this tool?). Also, maybe we could move this patch to a latter series and make other patches being reviewed? Chia-Yu
