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