Paolo Abeni <pab...@redhat.com> writes: > 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.
Alright, sure, let's hold off on that and revisit once this has had some more testing :) -Toke
