Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[Sabrina Dubroca <sd@queasysnail.net>]

Hello,

Sorry for commenting late.

2017-12-16, 03:09:39 -0500, Michael Chan wrote:
> Introduce NETIF_F_GRO_HW feature flag and convert drivers that support
> hardware GRO to use the new flag.
> 
> v5:
> - Documentation changes requested by Alexander Duyck.
> - bnx2x changes requested by Manish Chopra to enable LRO by default, and
> disable GRO_HW if disable_tpa module parameter is set.
> 
> v4:
> - more changes requested by Alexander Duyck:
> - check GRO_HW/GRO dependency in drivers's ndo_fix_features().
> - Reverse the order of RXCSUM and GRO_HW dependency check in
> netdev_fix_features().
> - No propagation in netdev_disable_gro_hw().

IIUC, with the patches that were applied, each driver can define
whether GRO_HW depends on GRO? From a user's perspective, this
inconsistent behavior is going to be quite confusing.

Worse than inconsistent behavior, it looks like a driver deciding that
GRO_HW doesn't depend on GRO is going to introduce a change of
behavior.  Previously, when GRO was disabled, there wouldn't be any
packet over MTU handed to the network stack.  Now, even if GRO is
disabled, GRO_HW might still be enabled, so we might get over-MTU
packets because of hardware GRO.

I don't think drivers should be allowed to say "GRO_HW doesn't depend
on GRO".

I think it's reasonable to be able to disable software GRO even if
hardware GRO is enabled. Thus, I would propose:
- keep the current GRO flag
- add a GRO_HW flag, depending on GRO, enforced by the core as in
  earlier versions of these patches
- add a GRO_SW flag, also depending on GRO


Thanks,

-- 
Sabrina