Date:   Wed, 2 Oct 2019 08:32:11 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Steffen Klassert <steffen.klassert@...unet.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 Wed, Oct 2, 2019 at 4:27 AM Steffen Klassert
<steffen.klassert@...unet.com> wrote:
>
> 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.

That is what your patch 1/5 does. My concern was that that is a bold
change without an admin opt-out.

> 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.

I was using ethtool -K just yesterday to debug a peculiar mix of
tunneling protocols. And yes, used grep on it ;) But I don't have much
of a problem with this.

But agreed that if default on works in all cases, then it's unnecessary.

> >
> > > 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.

This early in the cycle, that may work. Yes, it's definitely good to
have the plan B at the ready.

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

Agreed.

Powered by blists - more mailing lists