Date:   Wed, 2 Oct 2019 10:27:33 +0200
From:   Steffen Klassert <steffen.klassert@...unet.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
CC:     Network Development <netdev@...r.kernel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Subject: Re: [PATCH RFC 3/5] net: Add a netdev software feature set that
 defaults to off.

On Tue, Oct 01, 2019 at 08:43:05AM -0400, Willem de Bruijn wrote:
> On Tue, Oct 1, 2019 at 2:18 AM Steffen Klassert
> <steffen.klassert@...unet.com> wrote:
> >
> > On Mon, Sep 30, 2019 at 11:26:55AM -0400, Willem de Bruijn wrote:
> > >
> > > Instead, how about adding a UDP GRO ethtool feature independent of
> > > forwarding, analogous to fraglist GRO? Then both are explicitly under
> > > admin control. And can be enabled by default (either now, or after
> > > getting more data).
> >
> > We could add a protocol specific feature, but what would it mean
> > if UDP GRO is enabled?
> >
> > Would it be enabled for forwarding, and for local input only if there
> > is a GRO capable socket? Or would it be enabled even if there
> > is no GRO capable socket? Same question when UDP GRO is disabled.
> 
> Enable UDP GRO for all traffic if GRO and UDP GRO are set, and only
> then. 

But this means that we would need to enable UDP GRO by default then.
Currently, if an application uses a UDP GRO capable socket, it
can expect that it gets GROed packets without doing any additional
configuration. This would change if we disable it by default.
Unfortunately, enabling UDP GRO by default has the biggest
risk because most applications don't use UDP GRO capable sockets.

The most condervative way would be to leave standard GRO as it is.
But on some workloads standard GRO might be preferable, in
particular on forwarding to a NIC that can do UDP segmentation
in hardware.

> That seems like the easiest to understand behavior to me, and
> gives administrators an opt-out for workloads where UDP GRO causes a
> regression. We cannot realistically turn off all GRO on a mixed
> TCP/UDP workload (like, say, hosting TCP and QUIC).
> 
> > Also, what means enabling GRO then? Enable GRO for all protocols
> > but UDP? Either UDP becomes something special then,
> 
> Yes and true. But it is something special. We don't know whether UDP
> GRO is safe to deploy everywhere.
> 
> Only enabling it for the forwarding case is more conservative, but
> gives no path to enabling it systemwide, is arguably confusing and
> still lacks the admin control to turn off in case of unexpected
> regressions. I do think that for a time this needs to be configurable
> unless you're confident that the forwarding path is such a win that
> no plan B is needed. But especially without fraglist, I'm not sure.

On my tests it was a win on forwarding, but there might be
usecases where it is not. I guess the only way to find this out
is to enable is and wait what happens.

I'm a bit hesitating on adding a feature flag that might be only
temporary usefull. In particular on the background of the talk
that Jesse Brandeburg gave on the LPC last year. Maybe you
remember the slide where he showed the output of
ethtool --show-offloads, it filled the whole page.

> 
> > or we need
> > to create protocol specific features for the other protocols
> > too. Same would apply for fraglist GRO.
> 
> We don't need it for other protocols after the fact, but it's a good
> question: I don't know how it was enabled for them. Perhaps confidence
> was gained based on testing. Or it was enabled for -rc1, no one
> complained and stayed turned on. In which case you could do the same.

Maybe we should go that way to enable it and wait whether somebody
complains. A patch to add the feature flag could be prepared
beforehand for that case.

It is easy to make a suboptimal design decision here, so
some more opinions would be helpfull.

Powered by blists - more mailing lists