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]

Date:   Mon, 30 Sep 2019 11:26:55 -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 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:
> > On Fri, Sep 20, 2019 at 12:49 AM Steffen Klassert
> > <steffen.klassert@...unet.com> wrote:
> > >
> > > diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> > > index b239507da2a0..34d050bb1ae6 100644
> > > --- a/include/linux/netdev_features.h
> > > +++ b/include/linux/netdev_features.h
> > > @@ -230,6 +230,9 @@ static inline int find_next_netdev_feature(u64 feature, unsigned long start)
> > >  /* changeable features with no special hardware requirements */
> > >  #define NETIF_F_SOFT_FEATURES  (NETIF_F_GSO | NETIF_F_GRO)
> > >
> > > +/* Changeable features with no special hardware requirements that defaults to off. */
> > > +#define NETIF_F_SOFT_FEATURES_OFF      NETIF_F_GRO_FRAGLIST
> > > +
> >
> > NETIF_F_GRO_FRAGLIST is not really a device feature, but a way to
> > configure which form of UDP GRO to apply.
>
> NETIF_F_GRO is also not really a device feature. It is a feature with
> no special hardware requirements, as NETIF_F_GRO_FRAGLIST is.
> Fraglist GRO is a special way to do GRO and should be configured in the
> same way we configure standard GRO.
>
> >
> > 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.

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

>
> >
> > If so, how about a ternary per-netns sysctl {off, on without gro-list,
> > on with gro-list} instead of configuring through ethtool?
>
> I'd not like to have a global knob to configure this.
> On some devices it might make sense to enable fraglist
> GRO, but on others not. Also it would be nice if we can
> configure both vatiants with the same tool (ethtool).