linux-kernel - Re: [PATCH 02/19] VFS: use global wait-queue table for d_alloc

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: <20250212002543.GK1977892@ZenIV>
Date: Wed, 12 Feb 2025 00:25:43 +0000
From: Al Viro <viro@...iv.linux.org.uk>
To: NeilBrown <neilb@...e.de>
Cc: Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Jeff Layton <jlayton@...nel.org>,
	Dave Chinner <david@...morbit.com>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 02/19] VFS: use global wait-queue table for
 d_alloc_parallel()

On Wed, Feb 12, 2025 at 10:35:41AM +1100, NeilBrown wrote:

> Without lockdep making the dentry extra large, struct dentry is 192
> bytes, exactly 3 cache lines.  There are 16 entries per 4K slab.
> Now exactly 1/4 of possible indices are used.
> For every group of 16 possible indices, only 0, 4, 8, 12 are used.
> slabinfo says the object size is 256 which explains some of the spread. 

Interesting...

root@...nonball:~# grep -w dentry /proc/slabinfo
dentry            1370665 1410864    192   21    1 : tunables    0    0    0 : slabdata  67184  67184      0

Where does that 256 come from?  The above is on amd64, with 6.1-based debian
kernel and I see the same object size on other boxen (with local configs).

> I don't think there is a good case here for selecting bits from the
> middle of the dentry address.
> 
> If I use hash_ptr(dentry, 8) I get a more uniform distribution.  64000
> entries would hope for 250 per bucket.  Median is 248.  Range is 186 to
> 324 so +/- 25%.
> 
> Maybe that is the better choice.

That's really interesting, considering the implications for m_hash() and mp_hash()
(see fs/namespace.c)...

> > > > 3) the dance with conditional __wake_up() is worth a helper, IMO.
> > 
> > I mean an inlined helper function.
> 
> Yes.. Of course...
> 
> Maybe we should put
> 
> static inline void wake_up_key(struct wait_queue_head *wq, void *key)
> {
> 	__wake_up(wq, TASK_NORMAL, 0, key);
> }
> 
> in include/linux/wait.h to avoid the __wake_up() "internal" name, and
> then use
> 	wake_up_key(d_wait, dentry);
> in the two places in dcache.c, or did you want something
> dcache-specific?

More like
	if (wq)
		__wake_up(wq, TASK_NORMAL, 0, key);
probably...