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

Yan Zhai wrote:

quoted

quoted

-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

Yet more branches in the hot path.

Compile time configurability does not help, as that will be
enabled by distros.

For a fairly niche use case. Where functionality of GRO already
works. So just a performance for a very rare case at the cost of a
regression in the common case. A small regression perhaps, but death
by a thousand cuts.

I share your concern on operating on this hotpath. Will a
static_branch + sysctl make it less aggressive?

That is always a possibility. But we have to use it judiciously,
cannot add a sysctl for every branch.

I'm still of the opinion that Paolo shared that this seems a lot of
complexity for a fairly minor performance optimization for a rare
case.

Speaking of
performance, I'd hope this can give us more control so we can achieve
the best of two worlds: for TCP and some UDP traffic, we can enable
GRO, while for some other classes that we know GRO does no good or
even harm, let's disable GRO to save more cycles. The key observation
is that developers may already know which traffic is blessed by GRO,
but lack a way to realize it.

Following up also on Daniel's point on using BPF as GRO engine. Even
earlier I tried to add an option to selectively enable GRO protocols
without BPF. Definitely worthwhile to be able to disable GRO handlers
to reduce attack surface to bad input.

best
Yan