| Message ID | 20241014205543.94787-2-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: wext: Namespacify wireless_nlevent_flush() calls. | expand |
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
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 */
+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
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 ?
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
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);
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(-)