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]

Date:   Thu,  3 Feb 2022 17:26:19 +0100
From:   Alexander Lobakin <alexandr.lobakin@...el.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        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

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`?

> 
> It would be nice to group all struct packet_offload together in the
> same section to increase data locality.
> 
> I played in the past with a similar idea, but splitting struct
> packet_offload in two structures, one for GRO, one for GSO.
> (Note that GSO is hardly ever use with modern NIC)
> 
> But the gains were really marginal.

Thanks,
Al