Re: [RFC net-next 1/9] skb: introduce gro_disabled bit

On Fri, Jun 21, 2024 at 4:49 AM Paolo Abeni [off-list ref] wrote:

On Thu, 2024-06-20 at 15:19 -0700, Yan Zhai wrote:

quoted

Software GRO is currently controlled by a single switch, i.e.

  ethtool -K dev gro on|off

However, this is not always desired. When GRO is enabled, even if the
kernel cannot GRO certain traffic, it has to run through the GRO receive
handlers with no benefit.

There are also scenarios that turning off GRO is a requirement. For
example, our production environment has a scenario that a TC egress hook
may add multiple encapsulation headers to forwarded skbs for load
balancing and isolation purpose. The encapsulation is implemented via
BPF. But the problem arises then: there is no way to properly offload a
double-encapsulated packet, since skb only has network_header and
inner_network_header to track one layer of encapsulation, but not two.
On the other hand, not all the traffic through this device needs double
encapsulation. But we have to turn off GRO completely for any ingress
device as a result.

Introduce a bit on skb so that GRO engine can be notified to skip GRO on
this skb, rather than having to be 0-or-1 for all traffic.

Signed-off-by: Yan Zhai <redacted>
---
 include/linux/netdevice.h |  9 +++++++--
 include/linux/skbuff.h    | 10 ++++++++++
 net/Kconfig               | 10 ++++++++++
 net/core/gro.c            |  2 +-
 net/core/gro_cells.c      |  2 +-
 net/core/skbuff.c         |  4 ++++
 6 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index c83b390191d4..2ca0870b1221 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h

@@ -2415,11 +2415,16 @@ struct net_device {
      ((dev)->devlink_port = (port));                         \
 })

-static inline bool netif_elide_gro(const struct net_device *dev)
+static inline bool netif_elide_gro(const struct sk_buff *skb)
 {
-     if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
+     if (!(skb->dev->features & NETIF_F_GRO) || skb->dev->xdp_prog)
              return true;
+
+#ifdef CONFIG_SKB_GRO_CONTROL
+     return skb->gro_disabled;
+#else
      return false;
+#endif

This will generate OoO if the gro_disabled is flipped in the middle of
a stream.

Assuming the above is fine for your use case (I think it's _not_ in
general), you could get the same result without an additional costly
bit in sk_buff.

Calling it per-packet control seems inaccurate here, the motivation is
to give users the ability to control per-flow behaviors. OoO is indeed
a consequence if users don't do it correctly.

Let xdp_frame_fixup_skb_offloading() return a bool - e.g. 'true' when
gro should be avoided - and let the NIC driver call netif_receive_skb()
instead of the gro rx hook for such packet.

For rx on a single device, directly calling netif_receive_skb is
reasonable. For tunnel receivers it is kinda inconsistent IMHO. For
example, we terminate GRE tunnels in a netns, and it is necessary to
disable GRO on both the entering veth device and also the GRE tunnel
to shutdown GRO. That's why I'd hope to use a bit of skb, to be
consistent within the same netns. Let me add a bit more context to
clarify why we think this is necessary in another thread.

best,
Yan

All in all the approach implemented in this series does not look worthy
to me.

Thanks,

Paolo