On Fri, Feb 7, 2025 at 1:00 PM Alexander Lobakin
<aleksander.lobakin@xxxxxxxxx> wrote:
>
> From: Eric Dumazet <edumazet@xxxxxxxxxx>
> Date: Thu, 6 Feb 2025 19:35:50 +0100
>
> > On Thu, Feb 6, 2025 at 1:15 PM Alexander Lobakin
> > <aleksander.lobakin@xxxxxxxxx> wrote:
> >>
> >> From: Eric Dumazet <edumazet@xxxxxxxxxx>
> >> Date: Wed, 5 Feb 2025 18:48:50 +0100
> >>
> >>> On Wed, Feb 5, 2025 at 5:46 PM Alexander Lobakin
> >>> <aleksander.lobakin@xxxxxxxxx> 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@xxxxxxxxx>
> >>>> Acked-by: Jakub Kicinski <kuba@xxxxxxxxxx>
> >>>> Reviewed-by: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
> >>>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@xxxxxxxxx>
> >>>> ---
> >>>> 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 ?
> >>
> >> You'd need to adjust every ->napi_id access, which is a lot.
> >> Plus, as I wrote previously, napi_id doesn't really belong here, but
> >> embedding it here eases life.
> >>
> >> I'm often an old school, too, but sometimes this helps a lot.
> >> Unless you have very strong preference on this.
> >>
> >
> > Is struct_group_tagged even supported by ctags ?
> >
> > In terms of maintenance, I am sorry to say this looks bad to me.
> >
> > Even without ctags, I find git grep -n "struct xxxx {" quite good.
>
> compile_commands.json (already supported natively by Kbuild) + clangd is
> not enough?
>
> Elixir correctly tags struct_group()s.
>
> napi->napi_id is used in a lot of core files and drivers, adjusting all
> the references is not what I wanted to do in the series which does
> completely different things.
Leave napi_id in struct napi, it has nothing to do with gro.
>
> Page Pool uses tagged struct groups, as well a ton of other different
> files. Do you want to revert all this and adjust a couple thousand
> references only due to ctags and grep?
>
> (instead of just clicking on the references generated by clangd)
I obviously can not catch all netdev traffic.
