[PATCH v1 wl-next 0/3] wifi: wext: Namespacify wireless_nlevent_flush() calls.

[PATCH v1 wl-next 0/3] wifi: wext: Namespacify wireless_nlevent_flush() calls.

[Kuniyuki Iwashima]

Currently, wext's netdev notifier calls wireless_nlevent_flush()
for any event of any device and iterates all netns.

It happens even on a host without wext devices and is problematic
if the host has thousands of netns & devices as reported in the
thread below. [0]

This series will address the issue by removing the netns iteration
in wireless_nlevent_flush().

[0]: https://lore.kernel.org/netdev/CAKYWH0Ti3=4GeeuVyWKJ9LyTuRnf3Wy9GKg4Jb7tdeaT39qADA@mail.gmail.com/


Kuniyuki Iwashima (3):
  wifi: wext: Move wext_nlevents to net->gen[].
  wifi: wext: Convert wireless_nlevent_work to per-netns work.
  wifi: wext: Don't iterate all netns in wireless_nlevent_flush().

 include/net/iw_handler.h    |  4 +--
 include/net/net_namespace.h |  3 --
 net/wireless/core.c         |  2 +-
 net/wireless/wext-core.c    | 69 +++++++++++++++++++++++++------------
 4 files changed, 50 insertions(+), 28 deletions(-)

-- 
2.39.5 (Apple Git-154)

[PATCH v1 wl-next 1/3] wifi: wext: Move wext_nlevents to net->gen[].

[Kuniyuki Iwashima]

CONFIG_WEXT_CORE cannot be built as a module and is enabled in a
generic kernel in major distros.

We will namespacify wireless_nlevent_work that would require yet
another wext-specific field in struct net.

Given wext has already registered its pernet_operations, we can
use struct net_generic for wext-specific storage.

Let's move wext_nlevents to net->gen[].

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/net_namespace.h |  3 ---
 net/wireless/wext-core.c    | 31 +++++++++++++++++++++++++++----
 2 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/include/net/net_namespace.h b/include/net/net_namespace.h
index 873c0f9fdac6..f720225f7f6d 100644
--- a/include/net/net_namespace.h
+++ b/include/net/net_namespace.h
@@ -150,9 +150,6 @@ struct net {
 #if IS_ENABLED(CONFIG_NF_FLOW_TABLE)
 	struct netns_ft ft;
 #endif
-#endif
-#ifdef CONFIG_WEXT_CORE
-	struct sk_buff_head	wext_nlevents;
 #endif
 	struct net_generic __rcu	*gen;
 
diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
index 3bb04b05c5ce..bfc13e0d9883 100644
--- a/net/wireless/wext-core.c
+++ b/net/wireless/wext-core.c
@@ -20,6 +20,7 @@
 #include <net/netlink.h>
 #include <net/wext.h>
 #include <net/net_namespace.h>
+#include <net/netns/generic.h>
 
 typedef int (*wext_ioctl_func)(struct net_device *, struct iwreq *,
 			       unsigned int, struct iw_request_info *,
@@ -343,6 +344,17 @@ static const int compat_event_type_size[] = {
 
 /* IW event code */
 
+struct wext_net {
+	struct sk_buff_head nlevents;
+};
+
+static int wext_net_id;
+
+static struct wext_net *wext_net(struct net *net)
+{
+	return net_generic(net, wext_net_id);
+}
+
 void wireless_nlevent_flush(void)
 {
 	struct sk_buff *skb;
@@ -350,7 +362,9 @@ void wireless_nlevent_flush(void)
 
 	down_read(&net_rwsem);
 	for_each_net(net) {
-		while ((skb = skb_dequeue(&net->wext_nlevents)))
+		struct wext_net *wnet = wext_net(net);
+
+		while ((skb = skb_dequeue(&wnet->nlevents)))
 			rtnl_notify(skb, net, 0, RTNLGRP_LINK, NULL,
 				    GFP_KERNEL);
 	}
@@ -379,18 +393,25 @@ static struct notifier_block wext_netdev_notifier = {
 
 static int __net_init wext_pernet_init(struct net *net)
 {
-	skb_queue_head_init(&net->wext_nlevents);
+	struct wext_net *wnet = wext_net(net);
+
+	skb_queue_head_init(&wnet->nlevents);
+
 	return 0;
 }
 
 static void __net_exit wext_pernet_exit(struct net *net)
 {
-	skb_queue_purge(&net->wext_nlevents);
+	struct wext_net *wnet = wext_net(net);
+
+	skb_queue_purge(&wnet->nlevents);
 }
 
 static struct pernet_operations wext_pernet_ops = {
 	.init = wext_pernet_init,
 	.exit = wext_pernet_exit,
+	.id = &wext_net_id,
+	.size = sizeof(struct wext_net),
 };
 
 static int __init wireless_nlevent_init(void)
@@ -462,6 +483,7 @@ void wireless_send_event(struct net_device *	dev,
 	int wrqu_off = 0;			/* Offset in wrqu */
 	/* Don't "optimise" the following variable, it will crash */
 	unsigned int	cmd_index;		/* *MUST* be unsigned */
+	struct wext_net *wnet;
 	struct sk_buff *skb;
 	struct nlmsghdr *nlh;
 	struct nlattr *nla;
@@ -632,7 +654,8 @@ void wireless_send_event(struct net_device *	dev,
 
 	skb_shinfo(skb)->frag_list = compskb;
 #endif
-	skb_queue_tail(&dev_net(dev)->wext_nlevents, skb);
+	wnet = wext_net(dev_net(dev));
+	skb_queue_tail(&wnet->nlevents, skb);
 	schedule_work(&wireless_nlevent_work);
 }
 EXPORT_SYMBOL(wireless_send_event);
-- 
2.39.5 (Apple Git-154)

Re: [PATCH v1 wl-next 1/3] wifi: wext: Move wext_nlevents to net->gen[].

[Johannes Berg]

On Mon, 2024-10-14 at 13:55 -0700, Kuniyuki Iwashima wrote:
> CONFIG_WEXT_CORE cannot be built as a module

Isn't that precisely an argument for _not_ using net->gen[] with all the
additional dynamic allocations that implies? I'm not really against
doing this, but it does make the third patch more complex, requiring the
new wext_net->net pointer, and given allocations (rounded up) will take
more space - for something always present - than just going with the
existing scheme?

What's the reason to use net->gen[]?

johannes

Re: [PATCH v1 wl-next 1/3] wifi: wext: Move wext_nlevents to net->gen[].

[Kuniyuki Iwashima]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Tue, 15 Oct 2024 08:36:24 +0200
> On Mon, 2024-10-14 at 13:55 -0700, Kuniyuki Iwashima wrote:
> > CONFIG_WEXT_CORE cannot be built as a module
> 
> Isn't that precisely an argument for _not_ using net->gen[] with all the
> additional dynamic allocations that implies?

Exactly...

Recently I was thinking most of the structs in struct net (except for
first-class citizens like ipv4/ipv6) should use net->gen[] given the
distro kernel enables most configs.

But yes, WEXT is always built-in.


> I'm not really against
> doing this, but it does make the third patch more complex, requiring the
> new wext_net->net pointer,

Right, FWIW, before posting this patch, I checked 5 structs have
a similar pointer.

rdma_dev_net : possible_net_t net
pktgen_net : struct net
netns_ipvs : struct net
bond_net : struct net
afs_net : struct net


> and given allocations (rounded up) will take
> more space - for something always present - than just going with the
> existing scheme?
> 
> What's the reason to use net->gen[]?

Probably because wext_nlevents was just before a cacheline
on my setup ?

$ pahole -EC net vmlinux | grep net_generic -C 30
...
	} wext_nlevents; /*  2536    24 */
	/* --- cacheline 40 boundary (2560 bytes) --- */
	struct net_generic *       gen;                                                  /*  2560     8 */

Re: [PATCH v1 wl-next 1/3] wifi: wext: Move wext_nlevents to net->gen[].

[Johannes Berg]

+netdev, I think we're starting to discuss more general things :)

On Tue, 2024-10-15 at 17:49 -0700, Kuniyuki Iwashima wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Tue, 15 Oct 2024 08:36:24 +0200
> > On Mon, 2024-10-14 at 13:55 -0700, Kuniyuki Iwashima wrote:
> > > CONFIG_WEXT_CORE cannot be built as a module
> > 
> > Isn't that precisely an argument for _not_ using net->gen[] with all the
> > additional dynamic allocations that implies?
> 
> Exactly...
> 
> Recently I was thinking most of the structs in struct net (except for
> first-class citizens like ipv4/ipv6) should use net->gen[] given the
> distro kernel enables most configs.

Wait I'm confused, to me it seems you're contradicting yourself? :)

If we agree that making it use net->gen[] is more overhead since it
requires additional allocations (which necessarily require more memory
due to alignment etc., but even without that because now you needed
wext_net->net too) ...

Then why do you think more should use net->gen[] if it's built-in?

> But yes, WEXT is always built-in.

I can see an argument for things that aren't always present, obviously,
like bonding and pktgen, but I don't see much of an argument for things
like wext that are either present or not?

> Probably because wext_nlevents was just before a cacheline
> on my setup ?
> 
> $ pahole -EC net vmlinux | grep net_generic -C 30
> ...
> 	} wext_nlevents; /*  2536    24 */
> 	/* --- cacheline 40 boundary (2560 bytes) --- */
> 	struct net_generic *       gen;                                                  /*  2560     8 */

I'd argue that doesn't really mean it makes sense to pull it into
net->gen (where it gets accessed via two indirect pointers)?

That's an argument for reordering things there perhaps, but in struct
net that's probably not too much of an issue unless it shares a
cacheline with something that's used all the time?

johannes

Re: [PATCH v1 wl-next 1/3] wifi: wext: Move wext_nlevents to net->gen[].

[Kuniyuki Iwashima]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Wed, 16 Oct 2024 10:56:44 +0200
> +netdev, I think we're starting to discuss more general things :)
> 
> On Tue, 2024-10-15 at 17:49 -0700, Kuniyuki Iwashima wrote:
> > From: Johannes Berg <johannes@sipsolutions.net>
> > Date: Tue, 15 Oct 2024 08:36:24 +0200
> > > On Mon, 2024-10-14 at 13:55 -0700, Kuniyuki Iwashima wrote:
> > > > CONFIG_WEXT_CORE cannot be built as a module
> > > 
> > > Isn't that precisely an argument for _not_ using net->gen[] with all the
> > > additional dynamic allocations that implies?
> > 
> > Exactly...
> > 
> > Recently I was thinking most of the structs in struct net (except for
> > first-class citizens like ipv4/ipv6) should use net->gen[] given the
> > distro kernel enables most configs.
> 
> Wait I'm confused, to me it seems you're contradicting yourself? :)

Sorry, I meant the above is for module :)

> 
> If we agree that making it use net->gen[] is more overhead since it
> requires additional allocations (which necessarily require more memory
> due to alignment etc., but even without that because now you needed
> wext_net->net too) ...
> 
> Then why do you think more should use net->gen[] if it's built-in?
> 
> > But yes, WEXT is always built-in.
> 
> I can see an argument for things that aren't always present, obviously,
> like bonding and pktgen, but I don't see much of an argument for things
> like wext that are either present or not?
> 
> > Probably because wext_nlevents was just before a cacheline
> > on my setup ?
> > 
> > $ pahole -EC net vmlinux | grep net_generic -C 30
> > ...
> > 	} wext_nlevents; /*  2536    24 */
> > 	/* --- cacheline 40 boundary (2560 bytes) --- */
> > 	struct net_generic *       gen;                                                  /*  2560     8 */
> 
> I'd argue that doesn't really mean it makes sense to pull it into
> net->gen (where it gets accessed via two indirect pointers)?
> 
> That's an argument for reordering things there perhaps, but in struct
> net that's probably not too much of an issue unless it shares a
> cacheline with something that's used all the time?

Yes, avoiding false shareing would be the only reason to use ->gen[]
for builtin.

I'll drop the patch 1 in v2.

Btw, why WEXT cannot be module ?

Re: [PATCH v1 wl-next 1/3] wifi: wext: Move wext_nlevents to net->gen[].

[Johannes Berg]

On Wed, 2024-10-16 at 16:58 -0700, Kuniyuki Iwashima wrote:
> 
> Btw, why WEXT cannot be module ?

TBH, I don't remember well. I feel like I may have tried ~20 years ago,
but hit issues, and just made the built-in parts minimal. Might've been
we didn't have net->gen yet (did we? I don't recall), but I wouldn't be
surprised if there are other issues with it as well with ioctl linkage
and /proc and whatever else it does.

Not sure it's worth trying, WEXT really ought to be on the way out now,
and with WiFi7 (and higher) devices it's completely disabled.



Btw, if you really wanted to, I suspect you _could_ use net->gen[], make
the .size only a pointer size and then allocate the real data only if a
wireless capable device shows up in the namespace? Then that'd actually
be a win (vs. the other discussion we just had above) since wireless
devices are probably almost never in a netns. Not sure you'd be able to
easily free it when the last wifi capable devices leaves a netns, but
that probably also doesn't matter.

I don't know though how much the size of the netns matters for the
scalability issue you have in mind, seems the O(N) time behaviour here
is more problematic than a handful of bytes.

johannes

[PATCH v1 wl-next 2/3] wifi: wext: Convert wireless_nlevent_work to per-netns work.

[Kuniyuki Iwashima]

The next patch will namespacify wireless_nlevent_flush(), which is called
by wireless_nlevent_process() and two netdev notifiers.

As a preparation, let's convert wireless_nlevent_work to per-netns work.

Suggested-by: Johannes Berg <johannes.berg@intel.com>
Link: https://lore.kernel.org/netdev/cd8045c03573a012f71a1afdcfb5d9c108b6fefa.camel@sipsolutions.net/
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 net/wireless/wext-core.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
index bfc13e0d9883..4d7699135f46 100644
--- a/net/wireless/wext-core.c
+++ b/net/wireless/wext-core.c
@@ -346,6 +346,7 @@ static const int compat_event_type_size[] = {
 
 struct wext_net {
 	struct sk_buff_head nlevents;
+	struct work_struct nlevent_work;
 };
 
 static int wext_net_id;
@@ -391,11 +392,18 @@ static struct notifier_block wext_netdev_notifier = {
 	.notifier_call = wext_netdev_notifier_call,
 };
 
+/* Process events generated by the wireless layer or the driver. */
+static void wireless_nlevent_process(struct work_struct *work)
+{
+	wireless_nlevent_flush();
+}
+
 static int __net_init wext_pernet_init(struct net *net)
 {
 	struct wext_net *wnet = wext_net(net);
 
 	skb_queue_head_init(&wnet->nlevents);
+	INIT_WORK(&wnet->nlevent_work, wireless_nlevent_process);
 
 	return 0;
 }
@@ -405,6 +413,7 @@ static void __net_exit wext_pernet_exit(struct net *net)
 	struct wext_net *wnet = wext_net(net);
 
 	skb_queue_purge(&wnet->nlevents);
+	cancel_work_sync(&wnet->nlevent_work);
 }
 
 static struct pernet_operations wext_pernet_ops = {
@@ -429,14 +438,6 @@ static int __init wireless_nlevent_init(void)
 
 subsys_initcall(wireless_nlevent_init);
 
-/* Process events generated by the wireless layer or the driver. */
-static void wireless_nlevent_process(struct work_struct *work)
-{
-	wireless_nlevent_flush();
-}
-
-static DECLARE_WORK(wireless_nlevent_work, wireless_nlevent_process);
-
 static struct nlmsghdr *rtnetlink_ifinfo_prep(struct net_device *dev,
 					      struct sk_buff *skb)
 {
@@ -656,7 +657,7 @@ void wireless_send_event(struct net_device *	dev,
 #endif
 	wnet = wext_net(dev_net(dev));
 	skb_queue_tail(&wnet->nlevents, skb);
-	schedule_work(&wireless_nlevent_work);
+	schedule_work(&wnet->nlevent_work);
 }
 EXPORT_SYMBOL(wireless_send_event);
 
-- 
2.39.5 (Apple Git-154)

[PATCH v1 wl-next 3/3] wifi: wext: Don't iterate all netns in wireless_nlevent_flush().

[Kuniyuki Iwashima]

Commit 8bf862739a77 ("wext: fix message delay/ordering") introduced
wext_netdev_notifier_call() to avoid message delay and out-of-order.

The notifier calls wireless_nlevent_flush(), which iterates all netns.

The problem is that this happens for any event and netdev, even if
the host does not have a wext device.

This is too noisy, especially on a host with thousands of netns as
reported by Alexandre Ferrieux.

Given that the wext event queue was implemented in struct net,
wireless_nlevent_flush() does not need to iterate all netns.

Let's avoid unnecessary netns iteration in wireless_nlevent_flush().

Reported-by: Alexandre Ferrieux <alexandre.ferrieux@gmail.com>
Closes: https://lore.kernel.org/netdev/CAKYWH0Ti3=4GeeuVyWKJ9LyTuRnf3Wy9GKg4Jb7tdeaT39qADA@mail.gmail.com/
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 include/net/iw_handler.h |  4 ++--
 net/wireless/core.c      |  2 +-
 net/wireless/wext-core.c | 25 +++++++++++++------------
 3 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/include/net/iw_handler.h b/include/net/iw_handler.h
index b80e474cb0aa..e6acff086d39 100644
--- a/include/net/iw_handler.h
+++ b/include/net/iw_handler.h
@@ -412,9 +412,9 @@ void wireless_send_event(struct net_device *dev, unsigned int cmd,
 			 union iwreq_data *wrqu, const char *extra);
 #ifdef CONFIG_WEXT_CORE
 /* flush all previous wext events - if work is done from netdev notifiers */
-void wireless_nlevent_flush(void);
+void wireless_nlevent_flush(struct net *net);
 #else
-static inline void wireless_nlevent_flush(void) {}
+static inline void wireless_nlevent_flush(struct net *net) {}
 #endif
 
 /* We may need a function to send a stream of events to user space.
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 4c8d8f167409..a864ec889de3 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -1629,7 +1629,7 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 		return NOTIFY_DONE;
 	}
 
-	wireless_nlevent_flush();
+	wireless_nlevent_flush(dev_net(dev));
 
 	return NOTIFY_OK;
 }
diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
index 4d7699135f46..28033211c9e3 100644
--- a/net/wireless/wext-core.c
+++ b/net/wireless/wext-core.c
@@ -345,6 +345,7 @@ static const int compat_event_type_size[] = {
 /* IW event code */
 
 struct wext_net {
+	struct net *net;
 	struct sk_buff_head nlevents;
 	struct work_struct nlevent_work;
 };
@@ -356,26 +357,22 @@ static struct wext_net *wext_net(struct net *net)
 	return net_generic(net, wext_net_id);
 }
 
-void wireless_nlevent_flush(void)
+void wireless_nlevent_flush(struct net *net)
 {
+	struct wext_net *wnet = wext_net(net);
 	struct sk_buff *skb;
-	struct net *net;
 
-	down_read(&net_rwsem);
-	for_each_net(net) {
-		struct wext_net *wnet = wext_net(net);
 
-		while ((skb = skb_dequeue(&wnet->nlevents)))
-			rtnl_notify(skb, net, 0, RTNLGRP_LINK, NULL,
-				    GFP_KERNEL);
-	}
-	up_read(&net_rwsem);
+	while ((skb = skb_dequeue(&wnet->nlevents)))
+		rtnl_notify(skb, net, 0, RTNLGRP_LINK, NULL, GFP_KERNEL);
 }
 EXPORT_SYMBOL_GPL(wireless_nlevent_flush);
 
 static int wext_netdev_notifier_call(struct notifier_block *nb,
 				     unsigned long state, void *ptr)
 {
+	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
+
 	/*
 	 * When a netdev changes state in any way, flush all pending messages
 	 * to avoid them going out in a strange order, e.g. RTM_NEWLINK after
@@ -383,7 +380,7 @@ static int wext_netdev_notifier_call(struct notifier_block *nb,
 	 * or similar - all of which could otherwise happen due to delays from
 	 * schedule_work().
 	 */
-	wireless_nlevent_flush();
+	wireless_nlevent_flush(dev_net(dev));
 
 	return NOTIFY_OK;
 }
@@ -395,13 +392,17 @@ static struct notifier_block wext_netdev_notifier = {
 /* Process events generated by the wireless layer or the driver. */
 static void wireless_nlevent_process(struct work_struct *work)
 {
-	wireless_nlevent_flush();
+	struct wext_net *wnet;
+
+	wnet = container_of(work, struct wext_net, nlevent_work);
+	wireless_nlevent_flush(wnet->net);
 }
 
 static int __net_init wext_pernet_init(struct net *net)
 {
 	struct wext_net *wnet = wext_net(net);
 
+	wnet->net = net;
 	skb_queue_head_init(&wnet->nlevents);
 	INIT_WORK(&wnet->nlevent_work, wireless_nlevent_process);
 
-- 
2.39.5 (Apple Git-154)