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

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 2/7/25 12:56, Alexander Lobakin wrote:
> From: Eric Dumazet <edumazet@google.com>
> Date: Thu, 6 Feb 2025 19:35:50 +0100
> 
>> On Thu, Feb 6, 2025 at 1:15 PM Alexander Lobakin
>> <aleksander.lobakin@intel.com> wrote:
>>>
>>> From: Eric Dumazet <edumazet@google.com>
>>> Date: Wed, 5 Feb 2025 18:48:50 +0100
>>>
>>>> On Wed, Feb 5, 2025 at 5:46 PM Alexander Lobakin
>>>> <aleksander.lobakin@intel.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@dxuuu.xyz>
>>>>> Acked-by: Jakub Kicinski <kuba@kernel.org>
>>>>> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>>>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.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 ?
>>>
>>> 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.

I get that it's not in good style to impose "new style" on the old folks
(esp. Founders, Maintainers). I agree that our drivers are just a random
sample in the pool of drivers, and it is not clearly a win-win to have 
us experiment there with "modern practices". But this particular
contribution from Olek is for benefit of all. It will be also a great
example how to use struct_group :)
I hope that we will get more struct_group usage, it's really great
mechanism to keep/pass only a part of the bigger struct (and zero/don't
care about the rest).

>>
>> Even without ctags, I find git grep -n "struct xxxx {" quite good.

I also use mostly grep to find stuff, including the ` {` "trick".
And I do add wrappers when it gets tedious, here is a one for tagged
structs:
function sgrep() {     # here is the pattern: vv
	grep -EInr "struct(_group_tagged[(]| )$*( [{]|,)" |
	awk '
		/{$/ { print; normal = 1; next }
		{ tagged[tag++] = $0 }
		END {
			if (!normal)
				for (t = 0; t < tag; t++)
					print tagged[t]
		}'; }

awk resolves the complexity of "struct something," being sometimes
a parameter of a macro

> 
> 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.
> 
> 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)
> 
> Thanks,
> Olek
>