Bug #13249
closed
wasteful entry allocation in dnlc
100%
Description
The namecache entry has the following layout:
> ::print -a ncache_t
0 {
0 hash_next
8 hash_prev
10 vp
18 dp
20 hash
24 namlen
25 name
}
which implies:
> ::sizeof ncache_t sizeof (ncache_t) = 0x28
but then dnlc_get:
ncp = kmem_alloc(sizeof (ncache_t) + namlen, KM_NOSLEEP);
Requests 3 bytes more than expected. Instead the code should be using offsetof(ncache_t, name) here and in dnlc_free (and perhaps size computation could be deduped).
Updated by Mateusz Guzik over 1 year ago
Something of this sort should take care of it (not even compile tested):
diff --git a/usr/src/uts/common/fs/dnlc.c b/usr/src/uts/common/fs/dnlc.c
index 102375dedd..bf044082cf 100644
--- a/usr/src/uts/common/fs/dnlc.c
+++ b/usr/src/uts/common/fs/dnlc.c
@@ -256,7 +256,7 @@ vnode_t negative_cache_vnode;
*/
#define dnlc_free(ncp) \
{ \
- kmem_free((ncp), sizeof (ncache_t) + (ncp)->namlen); \
+ kmem_free((ncp), offsetof(ncache_t, name) + (ncp)->namlen); \
atomic_dec_32(&dnlc_nentries); \
}
@@ -977,7 +977,7 @@ dnlc_get(uchar_t namlen)
dnlc_max_nentries_cnt++; /* keep a statistic */
return (NULL);
}
- ncp = kmem_alloc(sizeof (ncache_t) + namlen, KM_NOSLEEP);
+ ncp = kmem_alloc(offsetof(ncache_t, name) + namlen, KM_NOSLEEP);
if (ncp == NULL) {
return (NULL);
}
Updated by Toomas Soome over 1 year ago
- Category set to kernel
- Assignee set to Toomas Soome
- % Done changed from 0 to 80
> ::print -ath ncache_t
0 ncache_t {
0 struct ncache *hash_next
8 struct ncache *hash_prev
10 struct vnode *vp
18 struct vnode *dp
20 int hash
24 uchar_t namlen
25 char [0] name
25 unsigned <<HOLE>> :24
}
> ::print -ath dcentry_t
0 dcentry_t {
0 uint64_t de_handle
8 struct dcentry *de_next
10 int de_hash
14 uchar_t de_namelen
15 char [0] de_name
15 unsigned <<HOLE>> :24
}
Testing done: build/install/boot.
Updated by Electric Monk 5 months ago
- Status changed from New to Closed
- % Done changed from 80 to 100
git commit e7b9901ea28b51d04ea82ac040c27457112687ec
commit e7b9901ea28b51d04ea82ac040c27457112687ec
Author: Toomas Soome <tsoome@me.com>
Date: 2022-01-04T23:01:48.000Z
13249 wasteful entry allocation in dnlc
Reviewed by: Klaus Ziegler <klausz@haus-gisela.de>
Approved by: Robert Mustacchi <rm@fingolfin.org>
Updated by Marcel Telka 5 months ago
It looks like this fix removes the terminating nul from both ncache_t:name and dcentry_t:de_name. How will things like dnlcdump() (mdb) work now?
Updated by Toomas Soome 5 months ago
Marcel Telka wrote in #note-5:
It looks like this fix removes the terminating nul from both
ncache_t:nameanddcentry_t:de_name. How will things likednlcdump()(mdb) work now?
It seems to be just fine; this is because we set up dnlc data in mdb with dnlc_load(), which does allocate entry with MDB_DNLC_NCACHE_SZ() macro, which does sizeof (struct) + namelen and the name entry is properly terminated with 0. So, yep, the support code does seem to correctly take account that dnlc struct is meant to be struct + variable sized string, just the dnlc buildup itself was messing things up a bit.
Updated by Marcel Telka 5 months ago
Toomas Soome wrote in #note-6:
Marcel Telka wrote in #note-5:
It looks like this fix removes the terminating nul from both
ncache_t:nameanddcentry_t:de_name. How will things likednlcdump()(mdb) work now?It seems to be just fine; this is because we set up dnlc data in mdb with dnlc_load(), which does allocate entry with MDB_DNLC_NCACHE_SZ() macro, which does sizeof (struct) + namelen and the name entry is properly terminated with 0. So, yep, the support code does seem to correctly take account that dnlc struct is meant to be struct + variable sized string, just the dnlc buildup itself was messing things up a bit.
You are right. Thanks for explanation and confirmation. I missed the following line in dnlc_load():
307 ncp->name[ncp->namlen] = '\0';