netdev - Re: [PATCH net-next 3/3] net: gro: register gso and gro offload on separate lists

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: <CANn89i+PnPNFpd-eap1n5VPZn__+Q-0BCP46hf+_kVJfw_phYQ@mail.gmail.com>
Date:   Thu, 3 Feb 2022 08:41:55 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>
Cc:     Paolo Abeni <pabeni@...hat.com>, netdev <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net-next 3/3] net: gro: register gso and gro offload on
 separate lists

On Thu, Feb 3, 2022 at 8:28 AM Alexander Lobakin
<alexandr.lobakin@...el.com> wrote:
>
> From: Eric Dumazet <edumazet@...gle.com>
> Date: Thu, 3 Feb 2022 08:11:43 -0800
>
> > On Thu, Feb 3, 2022 at 7:48 AM Paolo Abeni <pabeni@...hat.com> wrote:
> > >
> > > So that we know each element in gro_list has valid gro callbacks
> > > (and the same for gso). This allows dropping a bunch of conditional
> > > in fastpath.
> > >
> > > Before:
> > > objdump -t net/core/gro.o | grep " F .text"
> > > 0000000000000bb0 l     F .text  000000000000033c dev_gro_receive
> > >
> > > After:
> > > 0000000000000bb0 l     F .text  0000000000000325 dev_gro_receive
> > >
> > > Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> > > ---
> > >  include/linux/netdevice.h |  3 +-
> > >  net/core/gro.c            | 90 +++++++++++++++++++++++----------------
> > >  2 files changed, 56 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > index 3213c7227b59..406cb457d788 100644
> > > --- a/include/linux/netdevice.h
> > > +++ b/include/linux/netdevice.h
> > > @@ -2564,7 +2564,8 @@ struct packet_offload {
> > >         __be16                   type;  /* This is really htons(ether_type). */
> > >         u16                      priority;
> > >         struct offload_callbacks callbacks;
> > > -       struct list_head         list;
> > > +       struct list_head         gro_list;
> > > +       struct list_head         gso_list;
> > >  };
> > >
> >
> > On the other hand, this makes this object bigger, increasing the risk
> > of spanning cache lines.
>
> As you said, GSO callbacks are barely used with most modern NICs.
> Plus GRO and GSO callbacks are used in the completely different
> operations.
> `gro_list` occupies the same place where the `list` previously was.
> Does it make a lot of sense to care about `gso_list` being placed
> in a cacheline separate from `gro_list`?

Not sure what you are asking in this last sentence.

Putting together all struct packet_offload and struct net_offload
would reduce number of cache lines,
but making these objects bigger is conflicting.

Another approach to avoiding conditional tests would be to provide non
NULL values
for all "struct offload_callbacks"  members, just in case we missed something.

I think the test over ptype->type == type should be enough, right ?