Re: [PATCH net-next 2/4] veth: allow enabling NAPI even without XDP

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Paolo Abeni <pabeni@redhat.com> writes:

> On Fri, 2021-04-16 at 17:29 +0200, Toke Høiland-Jørgensen wrote:
>> Paolo Abeni <pabeni@redhat.com> writes:
>> 
>> > On Fri, 2021-04-09 at 16:58 +0200, Toke Høiland-Jørgensen wrote:
>> > > Paolo Abeni <pabeni@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