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

> 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.
> > > > >>
> > > > >>
> > > > >>
> > > > >> Hi,
> > > > >>
> > > > >>> From: [email protected]
> > > > >>> <chia-yu.chang@nokia-bell- labs.com>
> > > > >>>
> > > > >>> From: Chia-Yu Chang <[email protected]>
> > > > >>>
> > > > >>> Clarify that the VIRTIO_NET_HDR_GSO_ECN gso_type flag does not
> > > > >>> mean that TCP has IP-ECN set; instead, it identifies that the
> > > > >>> TCP CWR flag is set and will be cleared from the second
> > > > >>> segment of an
> > > aggregated segment.
> > > > >>>
> > > > >> Above text is seems to be clarified only in commit message, not
> > > > >> in the
> > > spec changes below.
> > > > >> Can you please add it in the actual spec wording, including
> > > requirements?
> > > > > Hi Parav,
> > > > >
> > > > > This is to fix current error in virtio spec:
> > > > > "VIRTIO_NET_HDR_GSO_ECN is a flag within the VIRTIO_NET_HDR
> > > structure that indicates the TCP header of a packet has its Explicit
> > > Congestion Notification (ECN) bit set, signifying congestion in the network"
> > > >
> > > > I am unable to find the above cited text in virtio spec.
> > > >
> > > > Do I miss to find above text in the spec?
> > > >
> > > >
> > > > What I read in spec is:
> > > >
> > > > The driver SHOULD NOT send to the device TCP packets requiring
> > > > segmentation offload which have the
> > > >
> > > > Explicit Congestion Notification bit set, 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.
> > >
> > > Yes, you are right.
> > >
> > > > > TCP header does not have ECN bit, and this flag shall indicate
> > > > > the CWR bit
> > > of TCP header is set when doing the GSO for the first packet.
> > > > > The corresponding texts in RFC3168 are "When the TCP data sender
> > > > > is
> > > ready to set the CWR bit after reducing the congestion window, it
> > > SHOULD set the CWR bit only on the first new data packet that it
> transmits."
> > > > >
> > > > > So, do you think adding above texts in the commit message is ok?
> > > >
> > > > Yes, but only commit message is not enough. We need equivalent
> > > > text in
> > > the description and in driver + device requirement section.
> > > >
> > > > Something like,
> > > >
> > > > When the device is transmitting packets of gso_type of
> > > > segmentation type
> > > A, B, or C, and if driver requested gso_type of HOST_ECN, the device
> > > must set the ECN bits in the IP header and CWR bit in the TCP header
> > > in the first data packet of the TCP or UDP segments.
> > > >
> > >
> > > I think the text changes are already proposed below.
> > >
> > > Or you can refer to my original email for better comparison:
> > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fyh
> > >
> bt%2F&data=05%7C02%7Cparav%40nvidia.com%7Cd6257712d9034f8a9e980
> 8de00
> > >
> bcf004%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C63894900867
> 87709
> > >
> 20%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjA
> uMDAwM
> > >
> CIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&
> sda
> > > ta=7zuzqOa56FsTpSJYooypn0zP3eVpaze4oDPCsFSIi5Y%3D&reserved=0
> > > .net%2Flore%2Fvirtio-comment%2F5f1f310d-8a17-414b-ac70-
> 80028832a78b%
> > > 40
> > >
> nvidia.com%2FT%2F%23mc1750dcc26d74c9b19939c4e86dae8bbade6ec78&d
> ata=0
> > > 5%
> > > 7C02%7Cchia-yu.chang%40nokia-bell-
> labs.com%7C2db0adbd1fec45d22af008d
> > > e0
> > >
> 0a252ae%7C5d4717519675428d917b70f44f9630b0%7C0%7C0%7C638948894
> 348277
> > > 75
> > >
> 4%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAu
> MDAwMC
> > > Is
> > >
> IlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sda
> ta=
> > > 9W
> > > 4P5JaFU7sBOhC7C6jqpo07Xp6TrkrcgIIIJkJtkZ4%3D&reserved=0
> > >
> > Above link indicates to follow the requirements from section 6.1.2 of RFC
> 3168.
> > Section 6.1.2 largely describes how TCP state machine to set the CWR bits.
> > It does not describe when doing GSO on tx, how first and non_first packets
> to be prepared by the device.
> > GSO of virtio net is stateless operation and segmentation does not interact
> with RTT.
> >
> > If I understand it right,
> > The requirement from the device is that if 64KB segmentation offload is
> done with mtu of 4K, out of 16 packets, only the first TCP pkt to have ECN and
> CWR bits set.
> >
> > If yes, then we need to spell that out in the virtio spec requirements section
> and description.
> >
> > Similar wording to apply in patch_2 for accecn too.
> 
> Thanks and Yes, your understand is right.
> 
> But if I remember correctly in our previous v5, one of the reviewer suggested:
> "1) It looks like the GSO requirement is not explicit described in the RFC 3168,
> so if it's need to be inferred from that RFC, it's probably not the charge of
> virtio spec.
> 2) If GSO requirement is clear in the RFC 3168, there's no need to repeat it
> again."
> 
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.

> The email thread can be found:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore-
> kernel.gnuweeb.org%2Fvirtio-
> comment%2FCACGkMEv2VQA6E6JjmC4jf%2BoHK9np_%2BFJzJ_5i4Td51FCYu
> %2BiPA%40mail.gmail.com%2FT%2F%23m2c34446bdc71be042f0cef5dd4d30
> becd323f55e&data=05%7C02%7Cparav%40nvidia.com%7Cd6257712d9034f8a
> 9e9808de00bcf004%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C6
> 38949008678793907%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOn
> RydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ
> %3D%3D%7C0%7C%7C%7C&sdata=3ULZd6n2kXr4mHo%2BpCG1QLbzaFyTeiS
> NFYTQvPym%2Fl8%3D&reserved=0
> 
> So, could you suggest which section we shall modify the virtio spec?
> 
> 
>