| Message ID | 20241014205543.94787-1-kuniyu@amazon.com |
|---|---|
| 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