netdev - Re: [PATCH v1 wl-next 1/3] wifi: wext: Move wext

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241016235850.29495-1-kuniyu@amazon.com>
Date: Wed, 16 Oct 2024 16:58:50 -0700
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <johannes@...solutions.net>
CC: <alexandre.ferrieux@...il.com>, <kuni1840@...il.com>, <kuniyu@...zon.com>,
	<linux-wireless@...r.kernel.org>, <netdev@...r.kernel.org>
Subject: Re: [PATCH v1 wl-next 1/3] wifi: wext: Move wext_nlevents to net->gen[].

From: Johannes Berg <johannes@...solutions.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@...solutions.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 ?