From b056467210e9bc0ad677543adced5f7f6f2befa7 Mon Sep 17 00:00:00 2001 From: Matthew Dillon Date: Thu, 22 Mar 2018 09:38:09 -0700 Subject: [PATCH] kernel - devfs cleanup * staticize two functions, remove *_fff() from manual pages (now static). * Adjust documentation. * Rename internal function from devfs_clone_bitmap_resize() to devfs_clone_bitmap_extend(). Document the slight weirdness in the extent of the passed-in 'newchunks' parameter, but do not change the API. Submitted-by: Aaron LI, with added code comments by Matt --- Makefile_upgrade.inc | 1 + share/man/man9/Makefile | 1 - share/man/man9/make_autoclone_dev.9 | 23 +++++---------- sys/sys/devfs.h | 4 +-- sys/vfs/devfs/devfs_helper.c | 45 ++++++++++++++++++++--------- 5 files changed, 41 insertions(+), 33 deletions(-) diff --git a/Makefile_upgrade.inc b/Makefile_upgrade.inc index fa0fb0bdac..881f6dbb8c 100644 --- a/Makefile_upgrade.inc +++ b/Makefile_upgrade.inc @@ -3378,6 +3378,7 @@ TO_REMOVE+=/etc/periodic/weekly/120.clean-kvmdb TO_REMOVE+=/etc/periodic/daily/470.status-named TO_REMOVE+=/etc/periodic/daily/430.status-rwho TO_REMOVE+=/etc/mtree/BSD.local.dist +TO_REMOVE+=/usr/share/man/man9/devfs_clone_bitmap_fff.9.gz .if !defined(WANT_INSTALLER) TO_REMOVE+=/usr/sbin/dfuibe_installer diff --git a/share/man/man9/Makefile b/share/man/man9/Makefile index 30edd18445..66e90f8487 100644 --- a/share/man/man9/Makefile +++ b/share/man/man9/Makefile @@ -635,7 +635,6 @@ MLINKS+=lwbuf.9 lwbuf_alloc.9 \ MLINKS+=make_autoclone_dev.9 destroy_autoclone_dev.9 \ make_autoclone_dev.9 DEVFS_CLONE_BITMAP.9 \ make_autoclone_dev.9 devfs_clone_bitmap_chk.9 \ - make_autoclone_dev.9 devfs_clone_bitmap_fff.9 \ make_autoclone_dev.9 devfs_clone_bitmap_get.9 \ make_autoclone_dev.9 devfs_clone_bitmap_init.9 \ make_autoclone_dev.9 devfs_clone_bitmap_put.9 \ diff --git a/share/man/man9/make_autoclone_dev.9 b/share/man/man9/make_autoclone_dev.9 index acb8ab0839..122ded7b76 100644 --- a/share/man/man9/make_autoclone_dev.9 +++ b/share/man/man9/make_autoclone_dev.9 @@ -37,7 +37,6 @@ .Nm destroy_autoclone_dev , .Nm devfs_clone_bitmap_init , .Nm devfs_clone_bitmap_uninit , -.Nm devfs_clone_bitmap_fff , .Nm devfs_clone_bitmap_chk , .Nm devfs_clone_bitmap_set , .Nm devfs_clone_bitmap_get , @@ -59,8 +58,6 @@ .Ft void .Fn devfs_clone_bitmap_uninit "struct devfs_bitmap *bitmap" .Ft int -.Fn devfs_clone_bitmap_fff "struct devfs_bitmap *bitmap" -.Ft int .Fn devfs_clone_bitmap_chk "struct devfs_bitmap *bitmap" "int unit" .Ft void .Fn devfs_clone_bitmap_set "struct devfs_bitmap *bitmap" "int unit" @@ -176,12 +173,6 @@ frees the memory associated with the specified clone .Fa bitmap and leaves it unusable. .Pp -.Fn devfs_clone_bitmap_fff -returns the first unused unit in -.Fa bitmap . -To use this unit, it has to be acquired by a call to -.Fn devfs_clone_bitmap_set . -.Pp .Fn devfs_clone_bitmap_chk checks if .Fa unit @@ -193,8 +184,6 @@ marks in .Fa bitmap as used, so further calls to -.Fn devfs_clone_bitmap_fff -or .Fn devfs_clone_bitmap_get cannot retrieve it. If one intends to use a clone handler along with preallocated devices, it @@ -213,13 +202,15 @@ as unused so further calls to or .Fn devfs_clone_bitmap_get can retrieve it again. +It is recommended that the associated device be destroyed prior to +making the +.Fa unit +available in the bitmap again, to avoid racing against a new clone. .Pp .Fn devfs_clone_bitmap_get -is a shortcut to -.Fn devfs_clone_bitmap_fff -and -.Fn devfs_clone_bitmap_set . -It will return the first unused unit number and also mark it as used. +will return the first unused unit number and also mark it as used +in the +.Fa bitmap . .Pp The .Fn DEVFS_DEFINE_CLONE_BITMAP diff --git a/sys/sys/devfs.h b/sys/sys/devfs.h index 0e8b89826b..857fafd220 100644 --- a/sys/sys/devfs.h +++ b/sys/sys/devfs.h @@ -336,8 +336,6 @@ struct devfs_unit_hash { void devfs_clone_bitmap_init(struct devfs_bitmap *); void devfs_clone_bitmap_uninit(struct devfs_bitmap *); -void devfs_clone_bitmap_resize(struct devfs_bitmap *, int); -int devfs_clone_bitmap_fff(struct devfs_bitmap *); void devfs_clone_bitmap_set(struct devfs_bitmap *, int); int devfs_clone_bitmap_get(struct devfs_bitmap *, int); int devfs_clone_bitmap_chk(struct devfs_bitmap *, int); @@ -442,7 +440,7 @@ void devfs_clear_cdevpriv(struct file *file); int devfs_WildCmp(const char *w, const char *s); int devfs_WildCaseCmp(const char *w, const char *s); -#endif /* KERNEL */ +#endif /* KERNEL || _KERNEL_STRUCTURES */ #define DEVFS_MNT_RULESET 0x01 #define DEVFS_MNT_JAIL 0x02 diff --git a/sys/vfs/devfs/devfs_helper.c b/sys/vfs/devfs/devfs_helper.c index 5f8b0842ce..4044439e33 100644 --- a/sys/vfs/devfs/devfs_helper.c +++ b/sys/vfs/devfs/devfs_helper.c @@ -59,9 +59,13 @@ devfs_clone_bitmap_uninit(struct devfs_bitmap *bitmap) kfree(bitmap->bitmap, M_DEVFS); } - -void -devfs_clone_bitmap_resize(struct devfs_bitmap *bitmap, int newchunks) +/* + * Extend the bitmap past (not just up to) 'newchunks' chunks. While + * probably not necessary, we go a little further and give ourselves + * one extra chunk's worth of bitmap beyond what is required. + */ +static void +devfs_clone_bitmap_extend(struct devfs_bitmap *bitmap, int newchunks) { int oldchunks = bitmap->chunks; @@ -73,8 +77,11 @@ devfs_clone_bitmap_resize(struct devfs_bitmap *bitmap, int newchunks) sizeof(u_long) * (bitmap->chunks - oldchunks)); } - -int +/* + * This helper function determines the next available free unit in the + * bitmap, extending the bitmap if necessary. It cannot fail. + */ +static int devfs_clone_bitmap_fff(struct devfs_bitmap *bitmap) { u_long curbitmap; @@ -85,7 +92,7 @@ devfs_clone_bitmap_fff(struct devfs_bitmap *bitmap) for (i = 0; i < chunks + 1; i++) { if (i == chunks) - devfs_clone_bitmap_resize(bitmap, i); + devfs_clone_bitmap_extend(bitmap, i); curbitmap = bitmap->bitmap[i]; if (curbitmap) { @@ -108,6 +115,8 @@ devfs_clone_bitmap_fff(struct devfs_bitmap *bitmap) * Caller wants to know if the specified unit has been allocated * or not. Return 0, indicating that it has not been allocated. * + * Caller must hold a lock to prevent chk-to-set races. + * * (the bitmap implements 0=allocated, 1=not-allocated but the * return value is inverted). */ @@ -125,7 +134,14 @@ devfs_clone_bitmap_chk(struct devfs_bitmap *bitmap, int unit) return !((bitmap->bitmap[chunk]) & (1L << unit)); } - +/* + * Unconditionally mark the specified unit as allocated in the bitmap. + * + * Caller must hold a lock to prevent chk-to-set races. If the unit had + * never been allocated in the past, a token lock might not be sufficient + * to avoid races (if this function must extend the bitmap it could block + * temporary in kmalloc()). + */ void devfs_clone_bitmap_set(struct devfs_bitmap *bitmap, int unit) { @@ -135,14 +151,14 @@ devfs_clone_bitmap_set(struct devfs_bitmap *bitmap, int unit) unit -= chunk * (sizeof(u_long) * 8); if (chunk >= bitmap->chunks) - devfs_clone_bitmap_resize(bitmap, chunk); + devfs_clone_bitmap_extend(bitmap, chunk); bitmap->bitmap[chunk] &= ~(1L << unit); } /* - * Deallocate a unit number. We must synchronize any destroy_dev()'s - * the device called before we clear the bitmap bit to avoid races - * against a new clone. + * Unconditionally deallocate a unit number. Caller must synchronize any + * destroy_dev()'s the device called before clearing the bitmap bit to + * avoid races against a new clone. */ void devfs_clone_bitmap_put(struct devfs_bitmap *bitmap, int unit) @@ -160,7 +176,11 @@ devfs_clone_bitmap_put(struct devfs_bitmap *bitmap, int unit) } /* - * Allocate a unit number for a device + * Conditionally allocate a unit from the bitmap. Returns -1 if no + * more units are available. + * + * Caller must hold a lock to avoid bitmap races. Since *_fff() below + * pre-extends the bitmap as necessary, a token lock is sufficient. */ int devfs_clone_bitmap_get(struct devfs_bitmap *bitmap, int limit) @@ -174,4 +194,3 @@ devfs_clone_bitmap_get(struct devfs_bitmap *bitmap, int limit) return unit; } - -- 2.41.0