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

[Alexander Lobakin]

From: Eric Dumazet <edumazet@xxxxxxxxxx>
Date: Fri, 7 Feb 2025 15:55:35 +0100

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

Do you read commit messages or reply just to reply?
Have you read previous revisions' threads (links are in the cover
letter, as always)?
I recommend doing that, and then proposing a solution that will be as
optimized as this one in terms of both performance and napi_struct layout.

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

I didn't say you should. But I'm 100% sure it's not the first time you
see struct_group() usage, so why complain only now? Esp. given that the
whole series is reviewed in v1/v2/v3 and there were discussions
regarding all those things you mentioned and I explained everything in
details both there and in the commitmsgs / cover letters?

Thanks,
Olek