netdev - Re: [PATCH RFC 3/5] net: Add a netdev software feature set that defaults to off.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+FuTSdTDAG95XCc8qcb7pJJn_6HuxCrCJnta+sJZa7Bi9x6tw@mail.gmail.com>
Date:   Tue, 1 Oct 2019 08:43:05 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Steffen Klassert <steffen.klassert@...unet.com>
Cc:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        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 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:
> > On Mon, Sep 30, 2019 at 2:24 AM Steffen Klassert
> > <steffen.klassert@...unet.com> wrote:
> > >
> > > On Mon, Sep 23, 2019 at 08:38:56AM -0400, Willem de Bruijn wrote:
> > > >
> > > > The UDP GRO benchmarks were largely positive, but not a strict win if
> > > > I read Paolo's previous results correctly. Even if enabling to by
> > > > default, it probably should come with a sysctl to disable for specific
> > > > workloads.
> > >
> > > Maybe we can just keep the default for the local input path
> > > as is and enable GRO as this:
> > >
> > > For standard UDP GRO on local input, do GRO only if a GRO enabled
> > > socket is found.
> > >
> > > If there is no local socket found and forwarding is enabled,
> > > assume forwarding and do standard GRO.
> > >
> > > If fraglist GRO is enabled, do it as default on local input and
> > > forwarding because it is explicitly configured.
> > >
> > > Would such a policy make semse?
> >
> > Making the choice between fraglist or non-fraglist GRO explicitly
> > configurable sounds great. Per device through ethtool over global
> > sysctl, too.
> >
> > My main concern is not this patch, but 1/5 that enables UDP GRO by
> > default. There should be a way to disable it, at least.
> >
> > I guess your suggestion is to only enable it with forwarding, which is
> > unlikely to see a cycle regression. And if there is a latency
> > regression, disable all GRO to disable UDP GRO.
>
> Yes, do GRO only for forwarding or if there is a GRO capable socket.
>
> In this case it can be disabled only by disable all GRO.
> It might be a disadvantage, but that's how it is with other
> protocols too.
>
> >
> > 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. 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.

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