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

[Willem de Bruijn]

Yan Zhai wrote:
> > > -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