Date: Thu, 16 Apr 2026 06:06:35 +0000 From: Warner Losh <imp@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Cc: Kristofer Peterson <kris@tranception.com> Subject: git: e75b324c93a1 - main - kern_descrip.c: Clarify allocation and freeing of fd map in fdgrowtable() Message-ID: <69e07c6b.3d24b.11fc05c1@gitrepo.freebsd.org>
index | next in thread | raw e-mail
The branch main has been updated by imp: URL: https://cgit.FreeBSD.org/src/commit/?id=e75b324c93a14ab68d79d9247943ae10da184657 commit e75b324c93a14ab68d79d9247943ae10da184657 Author: Kristofer Peterson <kris@tranception.com> AuthorDate: 2026-02-18 13:56:53 +0000 Commit: Warner Losh <imp@FreeBSD.org> CommitDate: 2026-04-16 06:05:22 +0000 kern_descrip.c: Clarify allocation and freeing of fd map in fdgrowtable() When expanding a file table, the condition for allocating a new map is NDSLOTS(nnfiles) > NDSLOTS(onfiles) whereas for freeing the old map is NDSLOTS(onfiles) > NDSLOTS(NDFILE). If a previously expanded file table were to be expanded slightly again such that the map did not need to be increased, then fdgrowtable could still free the current map. This does not happen currently as nnfiles is rounded up to a multiple of NDENTRIES at the beginning of fdgrowtable() so that every enlargement after the first enlargement will always require a larger map. Though the logic is currently correct, it is unclear and should the earlier rounding up of nnfiles be relaxed or remove, the logic would be incorrect. This patch therefore adds comments and invariants checking the size of the table and map, and updates the map free condition so that it is absolutely clear that the old map will only be deallocated if a new map has been allocated. Signed-off-by: Kristofer Peterson <kris@tranception.com> Reviewed by: kib, kevans Pull Request: https://github.com/freebsd/freebsd-src/pull/2029 --- sys/kern/kern_descrip.c | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c index 2fa0621bdfca..69985c39c3c0 100644 --- a/sys/kern/kern_descrip.c +++ b/sys/kern/kern_descrip.c @@ -2006,6 +2006,21 @@ fdgrowtable(struct filedesc *fdp, int nfd) NDSLOTTYPE *nmap, *omap; KASSERT(fdp->fd_nfiles > 0, ("zero-length file table")); + KASSERT(fdp->fd_nfiles >= NDFILE, ("file table of length %d shorter " + "than NDFILE (%d)", fdp->fd_nfiles, NDFILE)); + KASSERT(fdp->fd_nfiles == NDFILE || fdp->fd_nfiles % NDENTRIES == 0, + ("file table of length %d should be multiple of NDENTRIES (%lu)", + fdp->fd_nfiles, NDENTRIES)); + KASSERT((fdp->fd_nfiles == NDFILE) == ((intptr_t)fdp->fd_files - + offsetof(struct filedesc0, fd_dfiles) == (intptr_t)fdp - + offsetof(struct filedesc0, fd_fd)), ("file table of length %d " + "should have %s table", fdp->fd_nfiles, fdp->fd_nfiles == NDFILE ? + "initial" : "dynamic")); + KASSERT((NDSLOTS(fdp->fd_nfiles) <= NDSLOTS(NDFILE)) == ((intptr_t) + fdp->fd_map - offsetof(struct filedesc0, fd_dmap) == (intptr_t)fdp - + offsetof(struct filedesc0, fd_fd)), ("file table of length %d " + "should have %s map", fdp->fd_nfiles, NDSLOTS(fdp->fd_nfiles) <= + NDSLOTS(NDFILE) ? "initial" : "dynamic")); /* save old values */ onfiles = fdp->fd_nfiles; @@ -2035,9 +2050,19 @@ fdgrowtable(struct filedesc *fdp, int nfd) onfiles * sizeof(ntable->fdt_ofiles[0])); /* - * Allocate a new map only if the old is not large enough. It will - * grow at a slower rate than the table as it can map more - * entries than the table can hold. + * Allocate a new map only if the old one is not large enough. + * + * The initial struct filedesc0 object contains a table and map sized + * for NDFILE (20) entries which means the initial map can accomodate + * up to NDENTRIES (32 or 64) before requiring reallocation. + * + * As the new table size (nnfiles) is always rounded up to a multiple + * of NDENTRIES, the map will be fully utilised following the first + * enlargement, whether it is still the initial map (which will be the + * case if nnfiles == NDENTRIES) or if a new one that has has been + * allocated (which will be the case if nnfiles == X*NDENTRIES for some + * X > 1). In either case, subsequent enlargements will always allocate + * a new map to go along with the new table. */ if (NDSLOTS(nnfiles) > NDSLOTS(onfiles)) { nmap = malloc(NDSLOTS(nnfiles) * NDSLOTSIZE, M_FILEDESC, @@ -2045,6 +2070,8 @@ fdgrowtable(struct filedesc *fdp, int nfd) /* copy over the old data and update the pointer */ memcpy(nmap, omap, NDSLOTS(onfiles) * sizeof(*omap)); fdp->fd_map = nmap; + } else { + nmap = NULL; } /* @@ -2085,9 +2112,10 @@ fdgrowtable(struct filedesc *fdp, int nfd) /* * The map does not have the same possibility of threads still * holding references to it. So always free it as long as it - * does not reference the original static allocation. + * does not reference the original static allocation and a new + * map was allocated. */ - if (NDSLOTS(onfiles) > NDSLOTS(NDFILE)) + if (nmap != NULL && NDSLOTS(onfiles) > NDSLOTS(NDFILE)) free(omap, M_FILEDESC); }home | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?69e07c6b.3d24b.11fc05c1>