On 2/7/25 12:56, Alexander Lobakin 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.
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
