Re: [PATCH 02/19] VFS: use global wait-queue table for d_alloc_parallel()

[NeilBrown]

On Wed, 12 Feb 2025, Al Viro wrote:
> 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@cannonball:~# 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 found SLUB_DEBUG and redzoning does that.  Disabling the debug brings
done to 192 bytes and 21 per slab which you see.  That is still only 33%
hit rate.

> 
> > 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)...

Those functions add in the next set of bits as well - effectively mixing
in more bits from the page address.  If I do that the spread is better
but there are still buckets with close to twice the median, though most
are +/- 30%.

> 
> > > > > 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...
> 

Thanks,
NeilBrown