Re: [PATCH v5 net-next 2/2] net: hns3: fix GSO_ECN flag setting in the RX path

[Jijie Shao <shaojijie@huawei.com>]

on 2026/8/11 21:31, Jijie Shao wrote:
>
> on 2026/8/5 5:35, chia-yu.chang@nokia-bell-labs.com wrote:
>> From: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
>>
>> Currently, the hns3 Rx path sets the SKB_GSO_TCP_ECN flag when a TCP
>> segment with the CWR flag set is received. This is incorrect because
>> the hns3 hardware GRO resets the IP ToS byte to 0 during packet
>> aggregation. Consequently, no valid ECN signals are carreid, meaning
>> that it cannot support RFC3168 ECN or Accurate ECN protocols. Setting
>> SKB_GSO_TCP_ECN or SKB_GSO_TCP_ACCECN under these conditions misleads
>> the upper network stack.
>>
>> Fix this by removing the incorrect SKB_GSO_TCP_ECN flag assignment
>> in hns3_gro_complete().
>
>
> Tested on hns3 HW (2x 100G, direct cable, openEuler 24.03, kernel 
> 7.2.0-rc6).
>
> tcpdump confirms HW GRO zeroes IP ToS: aggregated (>MTU) packets carry
> tos 0x0, while non-aggregated packets keep their ToS/ECN marks — so the
> zeroing is done by HW GRO, not the sender. Before patch, CWR packets had
> SKB_GSO_TCP_ECN set (0x5 = SKB_GSO_TCPV4 | SKB_GSO_TCP_ECN). After patch,
> all GRO events gso_type=0x1 (SKB_GSO_TCPV4 only), no 0x5 observed.
>
> Tested-by: Jijie Shao <shaojijie@huawei.com>


I need to correct my earlier test report where I stated:

   "tcpdump confirms HW GRO zeroes IP ToS: aggregated (>MTU) packets
    carry tos 0x0, while non-aggregated packets keep their ToS/ECN
    marks — so the zeroing is done by HW GRO, not the sender"

This conclusion was wrong. I am withdrawing that Tested-by tag.

Root cause of the error
========================

I mixed two test tools with different sender-side paths and incorrectly
compared their results:

1. The observation that "non-aggregated packets keep their ToS/ECN
    marks" came from a Python script (scapy). Scapy constructs L2/L3
    packets directly, bypassing the kernel TCP stack, so the ToS value
    on the wire is exactly what the script sets — this says nothing
    about whether a kernel-stack sender would clear ToS or not.

2. The observation that "aggregated packets carry tos 0x0" came from
    iperf3 with --tos. iperf3 goes through the kernel TCP stack, and I
    did not set net.ipv4.tcp_ecn=1, so the kernel cleared the ECN bits
    (low 2 bits of ToS) before the packets hit the wire. The tos=0x0 on
    aggregated packets was already tos=0x0 on the wire at the sender
    side — it was a TX-side kernel behavior, not HW GRO zeroing.

I mistakenly used the scapy observation ("non-aggregated packets keep
ToS") as evidence that "the sender didn't clear ToS", then concluded
that iperf3's tos=0x0 must be caused by HW GRO. But these two
observations came from different sender paths and cannot be compared
this way. I only captured at the receiver side with no sender-side
wire capture to compare against, so I wrongly attributed the TX-side
clearing to HW GRO.

New test with tesgine (direct packet construction)
===================================================

To eliminate the kernel TCP stack interference, I retested using
tesgine, a packet generator that constructs TCP segments directly at
L2/L3 with full control over the IP ToS byte. This bypasses the kernel
TCP ECN state machine entirely, so the ToS value on the wire is exactly
what was configured.

Test setup:
   - hns3 NIC, HW GRO on, software GRO off (ethtool -K <dev> gro off)
   - tesgine sends 1460-byte TCP segments with controlled ToS values
   - Capture at receiver post-GRO

Four test cases were run, each verifying a different ToS boundary:

Test 1: Non-ECT <-> ECT(0)
   Sent:  4x tos=0x00 + 1x tos=0x02 + 4x tos=0x00 + 2x tos=0x02 + 1x tos=0x00
   Got:   4 aggregated (tos=0x00) | 1 single (tos=0x02)
        | 4 aggregated (tos=0x00) | 2 aggregated (tos=0x02)
        | 1 single (tos=0x00)
   => GRO segments at every ToS change, each group keeps its ToS.

Test 2: ECT(0) <-> CE
   Sent:  4x tos=0x02 + 1x tos=0x03 (CE) + 4x tos=0x02
   Got:   4 aggregated (tos=0x02) | 1 single (tos=0x03 CE)
        | 4 aggregated (tos=0x02)
   => CE packet is delivered independently, CE mark preserved.

Test 3: Non-ECT <-> CE
   Sent:  4x tos=0x00 + 1x tos=0x03 (CE)
   Got:   4 aggregated (tos=0x00) | 1 single (tos=0x03 CE)
   => Same behavior: CE is segmented out, ToS preserved.

Test 4: Pure DSCP change (ECN bits identical)
   Sent:  4x tos=0x10 (DSCP4) + 4x tos=0xB8 (DSCP46)
   Got:   4 aggregated (tos=0x10) | 4 aggregated (tos=0xB8)
   => Even pure DSCP change (no ECN bit change) triggers segmentation.

Conclusions from tesgine testing
================================

   1. HW GRO does NOT zero IP ToS. Each aggregate group retains the
      head packet's ToS value (verified with 0x00/0x02/0x03/0x10/0xB8).

   2. HW GRO segments flows by ToS boundaries. Any ToS change — whether
      DSCP (bits 0-5) or ECN (bits 6-7) — terminates the current
      aggregate group and starts a new one.

   3. ECN signals are preserved: CE packets (tos=0x03) are delivered
      independently with the CE mark intact, not merged into non-CE
      aggregates.

Impact on this patch
====================

The commit message states:

   "the hns3 hardware GRO resets the IP ToS byte to 0 during packet
    aggregation. Consequently, no valid ECN signals are carried"

This factual claim is incorrect per the tesgine test results above.
HW GRO preserves ToS and segments by ToS boundaries, so ECN signals
are not lost during aggregation.

The code change itself (removing the SKB_GSO_TCP_ECN assignment in
hns3_gro_complete) may still be correct, but the justification should
be based on the RX-path semantic argument from patch 1/2 (SKB_GSO_TCP_ECN
is a TX-path flag; using it in RX can corrupt the ACE signal in AccECN
forwarding scenarios), not on the ToS-zeroing claim. Or change to AccECN.

I will re-verify and provide a new Tested-by once the commit message is
updated. Sorry for the confusion caused by my earlier report.

Test captures and methodology details available on request.

Regards,
Jijie Shao