| 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
| ||
|
Message-ID: <eba70511-b6eb-4ff2-ae1a-183fa626d60a@intel.com>
Date: Fri, 7 Feb 2025 15:21:47 +0100
From: Przemek Kitszel <przemyslaw.kitszel@...el.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>, Eric Dumazet
<edumazet@...gle.com>, Kees Cook <kees@...nel.org>
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 2/7/25 12:56, Alexander Lobakin wrote:
> From: Eric Dumazet <edumazet@...gle.com>
> Date: Thu, 6 Feb 2025 19:35:50 +0100
>
>> On Thu, Feb 6, 2025 at 1:15 PM Alexander Lobakin
>> <aleksander.lobakin@...el.com> wrote:
>>>
>>> From: Eric Dumazet <edumazet@...gle.com>
>>> Date: Wed, 5 Feb 2025 18:48:50 +0100
>>>
>>>> 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 ?
>>>
>>> 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
>
Powered by blists - more mailing lists