Re: [PATCH 1/4] net: hsr: fix packet drops caused by GRO superpackets

[Sebastian Andrzej Siewior]

On 2026-09-19 01:46:34 [+0200], Xin Xie wrote:
> On 14/08/2026 03:21, Jakub Kicinski wrote:
> > 1) could you explain in more detail why GRO is breaking things?
> > GRO shouldn't merge packets with different L2/L2.5 and packets
> > with trailers?
> The reproducer is plain SAN traffic entering a PRP RedBox's
> interlink: matching Ethernet headers, no RCT yet, so GRO can merge it.
> PRP then appends one RCT to the aggregate, using its length for
> LSDU_size and assigning one sequence number. If the aggregate LSDU
> exceeds 4095, the 12-bit field truncates it. Later TCP segmentation
> cannot generate per-segment RCTs. An aggregate left unsegmented may
> instead be dropped for exceeding the lower device's frame-size limit.
> Our wire captures showed segments without RCTs and duplicate delivery
> through the two LANs.Disabling interlink GRO raised single-stream TCP 
> throughput from 27-37 Mbit/s to about 94 Mbit/s on the 100 Mbit/s rig.
> 
> > 2) the code seems inspired by LRO but LRO also has
> > NETIF_F_UPPER_DISABLES. This change looks rather incomplete.
> > Calling it "best effort" seems like an excuse.
> NETIF_F_UPPER_DISABLES prevents a lower's ethtool request from
> overriding an upper's LRO-off policy. GRO is outside that mask and
> remains changeable; extending the global mask would affect other
> stacked devices too. Patch 1 sets the GRO-off default, while patch 3
> segments plain, trailer-free aggregates before tagging, including
> when GRO is re-enabled or GSO arrives directly. GRO_HW can also be
> fixed on, so feature disabling alone cannot exclude all aggregates;
> device-specific hardware-coalesced formats remain outside v6's
> segmentation guarantee. I should have explained this distinction
> from LRO explicitly.

Would adding a NETIF_F_HSR_SLAVE flag to slave device(s) and then filter
invalid flags in netdev_fix_features() fix this? This should help
against enabling and add a warning if a flag can not be removed.

A netdev_change_features() might do the job then.

Sebastian