RE: [PATCH v10 1/2] virtio-net: Fix ECN feature descriptions

> -----Original Message-----
> From: Parav Pandit <[email protected]> 
> Sent: Friday, October 3, 2025 2:47 PM
> To: Chia-Yu Chang (Nokia) <[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]; [email protected]
> Subject: RE: [PATCH v10 1/2] virtio-net: Fix ECN feature descriptions
> 
> 
> 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.
> 
> 
> 
> > From: Chia-Yu Chang (Nokia) <[email protected]>
> > Sent: 03 October 2025 05:26 PM
> >
> > > -----Original Message-----
> > > From: Parav Pandit <[email protected]>
> > > Sent: Friday, October 3, 2025 12:41 PM
> > > To: Chia-Yu Chang (Nokia) <[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]; [email protected]
> > > Subject: RE: [PATCH v10 1/2] virtio-net: Fix ECN feature 
> > > descriptions
> > >
> > >
> > > 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.
> > >
> > >
> > >
> > > > From: Chia-Yu Chang (Nokia) <[email protected]>
> > > > Sent: 01 October 2025 05:56 PM
> > > >
> > > > > -----Original Message-----
> > > > > From: Parav Pandit <[email protected]>
> > > > > Sent: Wednesday, October 1, 2025 12:51 PM
> > > > > To: Chia-Yu Chang (Nokia) <[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]; [email protected]
> > > > > Subject: RE: [PATCH v10 1/2] virtio-net: Fix ECN feature 
> > > > > descriptions
> > > > >
> > > > >
> > > > > 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.
> > > > >
> > > > >
> > > > >
> > > > > > From: Chia-Yu Chang (Nokia) 
> > > > > > <[email protected]>
> > > > > > Sent: 01 October 2025 01:04 PM
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Parav Pandit <[email protected]>
> > > > > > > Sent: Wednesday, October 1, 2025 6:24 AM
> > > > > > > To: Chia-Yu Chang (Nokia) 
> > > > > > > <[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]; 
> > > > > > > [email protected]
> > > > > > > Subject: RE: [PATCH v10 1/2] virtio-net: Fix ECN feature 
> > > > > > > descriptions
> > > > > > >
> > > > > > >
> > > > > > > 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.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > From: Chia-Yu Chang (Nokia) 
> > > > > > > > <[email protected]>
> > > > > > > >
> > > > > > > > > -----Original Message-----
> > > > > > > > > From: Parav Pandit <[email protected]>
> > > > > > > > > Sent: Tuesday, September 30, 2025 6:15 AM
> > > > > > > > > To: Chia-Yu Chang (Nokia) 
> > > > > > > > > <[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]; 
> > > > > > > > > [email protected]
> > > > > > > > > Subject: Re: [PATCH v10 1/2] virtio-net: Fix ECN feature 
> > > > > > > > > descriptions
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > 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 29-09-2025 11:14 pm, Chia-Yu Chang (Nokia) wrote:
> > > > > > > > > >> -----Original Message-----
> > > > > > > > > >> From: Parav Pandit <[email protected]>
> > > > > > > > > >> Sent: Monday, September 29, 2025 9:48 AM
> > > > > > > > > >> To: Chia-Yu Chang (Nokia) 
> > > > > > > > > >> <[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];
> > > > > > [email protected]
> > > > > > > > > >> Subject: RE: [PATCH v10 1/2] virtio-net: Fix ECN 
> > > > > > > > > >> feature descriptions
> > > > > > > > > >>
> > > > > > > > > >>
> > > > > > > > > >> 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.
> > > > > > > > > >>
> > > > > > > > > >>
> > > > > > > > > >>
> > > > [...]
> > > > > Virtio spec requirements are clearly written.
> > > > > So just by referring to RFC 3168 is as blurry as today in context of GSO.
> > > > >
> > > > > At best possibly it can be written as "device SHOULD do ECN and 
> > > > > CWR in first
> > > > pkt.. and subsequent packets SHOULD have cleared".
> > > > >
> > > > > If that is not aligning, than better to not make it more blurry.
> > > > > ACCECN feature bit can be more crisply defined in that case.
> > > >
> > > > OK, then could you let us know if we make below changes in the 
> > > > next version, are they clear enough for virtio spec?
> > > >
> > > >
> > > > 1. VIRTIO_NET_F_GUEST_ECN (9): Driver can receive TSO with the CWR 
> > > > flag set. This CWR flag is only applied to the first packet, and 
> > > > the CWR flag of subsequent packets should be cleared.
> > > >
> > > > 2. VIRTIO_NET_F_HOST_ECN (13): Device can receive TSO with the CWR 
> > > > flag set. This CWR flag is only applied to the first packet, and 
> > > > the CWR flag of subsequent packets should be cleared.
> > > >
> > > > 3. If the driver negotiated the VIRTIO_NET_F_HOST_ECN feature, the 
> > > > VIRTIO_NET_HDR_GSO_ECN bit in gso_type indicates that the CWR flag 
> > > > of the TCP packet applied only to the first packet and is cleared 
> > > > in subsequent packets.
> > > >
> > > > 4. The driver SHOULD NOT send to the device TCP packets requiring 
> > > > segmentation offload with the CWR flag set ONLY for to the first 
> > > > packet and cleared in subsequent packets, unless the 
> > > > VIRTIO_NET_F_HOST_ECN feature is negotiated, in which case the 
> > > > driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in gso_type.
> > > >
> > > > 5. The device SHOULD NOT send to the driver TCP packets requiring 
> > > > segmentation offload with the CWR flag set ONLY for to the first 
> > > > packet and cleared in subsequent packets, unless the 
> > > > VIRTIO_NET_F_GUEST_ECN feature is negotiated, in which case the 
> > > > driver MUST set the VIRTIO_NET_HDR_GSO_ECN bit in gso_type.
> > > >
> > >
> > > Also, while it is good to document existing behaviour of the spec, 
> > > devices
> > and drivers, I believe that ACCECN need not have to wait for the 
> > documentation of existing GSO_ECN.
> > >
> > > However, I am bit puzzled with the Linux kernel commit [1] of yours 
> > > that
> > hints that ACCECN may not need any special handling.
> > >
> > > You described,
> > >
> > >     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).
> > >
> > > Based on above commit log, can we say that when existing GSO_ECN is 
> > > not
> > negotiated, ACCECN can work as_is without the patch_2?
> > >
> > > [1] commit 023af5a72ab16 ("gso: AccECN support")
> >
> > Indeed, this patch description implies that NIC that currently don't 
> > support TCP_ECN will naturally support ACCECN.
> > But an extra TSO_ACCECN flag will make this into an explicit 
> > requirement, like our patch in the Linux.
> >
> If so, the 2nd patch should word saying, the device must not modify IP congestion code point bits and TCP ACE bits.
> All packets in the segment offload must have same bits.
> 
> Similarly on RX side for GRO, the device requirement to spell out that GRO stream to break when two subsequent packets have different bits etc.
> 
> If you can help to understand why RX side should set this and how one may plan to use it, it makes sense to define the GUEST bit.
> Presently it does not look useful for rx side (guest bit).
> 
> > Related to another email about existing driver, one solution would be 
> > replace SKB_GSO_TCP_ECN with SKB_GSO_TCP_ACCECN, like in 
> > 4e4f7cefb130af6aba6a393b2d13930b49390df9.
> > This can avoid CWR corruption somewhere after GRO, and we plan to 
> > submit related patches (old driver, virtio-related) into net-next 
> > after this is approved in virtio-spec.
> 
> Commit 4e4f7cefb130a looks incorrect to me, or I must be missing something.
> It looks incorrect to me is because:
> 1. SKB_GSO_TCP_ACCECN is set when cwr is set, regardless of tcp socket following rfc3168 or accecn.
> I was hoping to set this flag only when its accecn.
> 
> 2. Even though it is set, it is not read by anyone, so not sure why one would set it.
> 
> 3. It appears to me that it is TX side flag and rx GRO should not be setting it, because there isn't rx side code to handle it yet.
> May be this is part of some multi series work?
> 
> 4. The commit 4e4f7cefb130a claims to prevent field corruption.
> A corruption sounds like a bug. And if it is a bug, it the patch needs fixes tag, but it does not have it.
> Probably no fixes tag, because no users, that is strange kernel code to me.

Please let me elaborate more.

Patch 023af5a72ab16 is in tcp_gso_segment(), so it disaggregates packet on the TX side after TCP stack.
Before patch 023af5a72ab16, CWR flag from the 2nd packet will be cleaned if TSO_ECN is set by RFC3168 ECN in tcp_output.c
New TSO_ACCECN will be set by AccECN protocol in tcp_output.c, indicating that this packet does not want CWR flag cleaning from the 2nd packet.

Patch 4e4f7cefb130a is in tcp_gro_receive(), so it aggregates packets on the RX side before going to TCP stack.
And you can see before 4e4f7cefb130a patch, TSO_ECN flag will be set if cwr is 1.
This TSO_ECN flag implies that it's ok to segment this aggregated packet and clean CWR from the 2nd packets by latter operations.
But this is NOT ok for AccECN protocl, new TSO_ACCECN flag explicitly tells latter operations that it's NOT ok to clean CWR from the 2nd packets.

And the corruption in the commit message means the CWR flag cannot keep the origianl value (i.e., been reset by latter operations).