netdev - Re: [PATCH net-next v4 1/8] net: gro: decouple GRO from the NAPI layer

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: <CANn89iJjCOThDqwsK4v2O8LfcwAB55YohNZ8T2sR40uM2ZoX5w@mail.gmail.com>
Date: Wed, 5 Feb 2025 18:48:50 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Lorenzo Bianconi <lorenzo@...nel.org>, Daniel Xu <dxu@...uu.xyz>, Alexei Starovoitov <ast@...nel.org>, 
	Daniel Borkmann <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, 
	John Fastabend <john.fastabend@...il.com>, Toke Høiland-Jørgensen <toke@...nel.org>, 
	Jesper Dangaard Brouer <hawk@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, netdev@...r.kernel.org, 
	bpf@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Toke Høiland-Jørgensen <toke@...hat.com>
Subject: Re: [PATCH net-next v4 1/8] net: gro: decouple GRO from the NAPI layer

On Wed, Feb 5, 2025 at 5:46 PM Alexander Lobakin
<aleksander.lobakin@...el.com> wrote:
>
> In fact, these two are not tied closely to each other. The only
> requirements to GRO are to use it in the BH context and have some
> sane limits on the packet batches, e.g. NAPI has a limit of its
> budget (64/8/etc.).
> Move purely GRO fields into a new tagged group, &gro_node. Embed it
> into &napi_struct and adjust all the references. napi_id doesn't
> really belong to GRO, but:
>
> 1. struct gro_node has a 4-byte padding at the end anyway. If you
>    leave napi_id outside, struct napi_struct takes additional 8 bytes
>    (u32 napi_id + another 4-byte padding).
> 2. gro_receive_skb() uses it to mark skbs. We don't want to split it
>    into two functions or add an `if`, as this would be less efficient,
>    but we need it to be NAPI-independent. The current approach doesn't
>    change anything for NAPI-backed GROs; for standalone ones (which
>    are less important currently), the embedded napi_id will be just
>    zero => no-op.
>
> Three Ethernet drivers use napi_gro_flush() not really meant to be
> exported, so move it to <net/gro.h> and add that include there.
> napi_gro_receive() is used in more than 100 drivers, keep it
> in <linux/netdevice.h>.
> This does not make GRO ready to use outside of the NAPI context
> yet.
>
> Tested-by: Daniel Xu <dxu@...uu.xyz>
> Acked-by: Jakub Kicinski <kuba@...nel.org>
> Reviewed-by: Toke Høiland-Jørgensen <toke@...hat.com>
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@...el.com>
> ---
>  include/linux/netdevice.h                  | 26 +++++---
>  include/net/busy_poll.h                    | 11 +++-
>  include/net/gro.h                          | 35 +++++++----
>  drivers/net/ethernet/brocade/bna/bnad.c    |  1 +
>  drivers/net/ethernet/cortina/gemini.c      |  1 +
>  drivers/net/wwan/t7xx/t7xx_hif_dpmaif_rx.c |  1 +
>  net/core/dev.c                             | 60 ++++++++-----------
>  net/core/gro.c                             | 69 +++++++++++-----------
>  8 files changed, 112 insertions(+), 92 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 2a59034a5fa2..d29b6ebde73f 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -340,8 +340,8 @@ struct gro_list {
>  };
>
>  /*
> - * size of gro hash buckets, must less than bit number of
> - * napi_struct::gro_bitmask
> + * size of gro hash buckets, must be <= the number of bits in
> + * gro_node::bitmask
>   */
>  #define GRO_HASH_BUCKETS       8
>
> @@ -370,7 +370,6 @@ struct napi_struct {
>         unsigned long           state;
>         int                     weight;
>         u32                     defer_hard_irqs_count;
> -       unsigned long           gro_bitmask;
>         int                     (*poll)(struct napi_struct *, int);
>  #ifdef CONFIG_NETPOLL
>         /* CPU actively polling if netpoll is configured */
> @@ -379,11 +378,14 @@ struct napi_struct {
>         /* CPU on which NAPI has been scheduled for processing */
>         int                     list_owner;
>         struct net_device       *dev;
> -       struct gro_list         gro_hash[GRO_HASH_BUCKETS];
>         struct sk_buff          *skb;
> -       struct list_head        rx_list; /* Pending GRO_NORMAL skbs */
> -       int                     rx_count; /* length of rx_list */
> -       unsigned int            napi_id; /* protected by netdev_lock */
> +       struct_group_tagged(gro_node, gro,
> +               unsigned long           bitmask;
> +               struct gro_list         hash[GRO_HASH_BUCKETS];
> +               struct list_head        rx_list; /* Pending GRO_NORMAL skbs */
> +               int                     rx_count; /* length of rx_list */
> +               u32                     napi_id; /* protected by netdev_lock */
> +

I am old school, I would prefer a proper/standalone old C construct.

struct gro_node  {
                unsigned long           bitmask;
               struct gro_list         hash[GRO_HASH_BUCKETS];
               struct list_head        rx_list; /* Pending GRO_NORMAL skbs */
               int                     rx_count; /* length of rx_list */
               u32                     napi_id; /* protected by netdev_lock */
};

Really, what struct_group_tagged() can possibly bring here, other than
obfuscation ?

Less than 30 uses in the whole kernel tree...