On Fri, 2021-04-16 at 17:29 +0200, Toke Høiland-Jørgensen wrote: > Paolo Abeni <pab...@redhat.com> writes: > > > On Fri, 2021-04-09 at 16:58 +0200, Toke Høiland-Jørgensen wrote: > > > Paolo Abeni <pab...@redhat.com> writes: > > > > > > > Currently the veth device has the GRO feature bit set, even if > > > > no GRO aggregation is possible with the default configuration, > > > > as the veth device does not hook into the GRO engine. > > > > > > > > Flipping the GRO feature bit from user-space is a no-op, unless > > > > XDP is enabled. In such scenario GRO could actually take place, but > > > > TSO is forced to off on the peer device. > > > > > > > > This change allow user-space to really control the GRO feature, with > > > > no need for an XDP program. > > > > > > > > The GRO feature bit is now cleared by default - so that there are no > > > > user-visible behavior changes with the default configuration. > > > > > > > > When the GRO bit is set, the per-queue NAPI instances are initialized > > > > and registered. On xmit, when napi instances are available, we try > > > > to use them. > > > > > > Am I mistaken in thinking that this also makes XDP redirect into a veth > > > work without having to load an XDP program on the peer device? That's > > > been a long-outstanding thing we've been meaning to fix, so that would > > > be awesome! :) > > > > I have not experimented that, and I admit gross ignorance WRT this > > argument, but AFAICS the needed bits to get XDP redirect working on > > veth are the ptr_ring initialization and the napi instance available. > > > > With this patch both are in place when GRO is enabled, so I guess XPD > > redirect should work, too (modulo bugs for untested scenario). > > OK, finally got around to testing this; it doesn't quite work with just > your patch, because veth_xdp_xmit() still checks for rq->xdp_prog > instead of rq->napi. Fixing this indeed enabled veth to be an > XDP_REDIRECT target without an XDP program loaded on the peer. So yay! > I'll send a followup fixing that check.
Thank you for double checking! > So with this we seem to have some nice improvements in both > functionality and performance when GRO is turned on; so any reason why > we shouldn't just flip the default to on? Uhmmm... patch 3/4 should avoid the GRO overhead for most cases where we can't leverage the aggregation benefit, but I'm not 110% sure that enabling GRO by default will not cause performance regressions in some scenarios. It this proves to be always a win we can still change the default later, I think. Cheers, Paolo