Date: Thu, 09 Dec 2021 14:15:56 -0800 From: Cy Schubert <Cy.Schubert@cschubert.com> To: Warner Losh <imp@FreeBSD.org> Cc: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: 1c7d15b03071 - main - Make device_busy/unbusy work w/o Giant held Message-ID: <202112092215.1B9MFu2h093286@slippy.cwsent.com> In-Reply-To: <202111302221.1AUML4IA017860@gitrepo.freebsd.org> References: <202111302221.1AUML4IA017860@gitrepo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
In message <202111302221.1AUML4IA017860@gitrepo.freebsd.org>, Warner Losh write s: > The branch main has been updated by imp: > > URL: https://cgit.FreeBSD.org/src/commit/?id=1c7d15b030718d9d8cc70916fe3216a1 > 9f30896b > > commit 1c7d15b030718d9d8cc70916fe3216a19f30896b > Author: Warner Losh <imp@FreeBSD.org> > AuthorDate: 2021-11-30 22:03:26 +0000 > Commit: Warner Losh <imp@FreeBSD.org> > CommitDate: 2021-11-30 22:18:01 +0000 > > Make device_busy/unbusy work w/o Giant held > > The vast majority of the busy/unbusy users in the tree don't acquire > Giant before calling device_busy/unbusy. However, if multiple threads > are opening a file, say, that causes the device to busy/unbusy, then we > can race to the root marking things busy. Move to using a reference > count to keep track of how many times a device_t has been made busy. Use > that count to make the same decisions that we'd make with the old device > state. > > Note: gpiopps.c uses D_TRACKCLOSE. Others do as well. However, there's a > known race with closes that will be corrected for all the drivers that > do this in a future commit. > > Sponsored by: Netflix > Reviewed by: hselasky, jhb > Differential Revision: https://reviews.freebsd.org/D26284 > --- > sys/dev/drm2/drm_fops.c | 6 ------ > sys/dev/gpio/gpiopps.c | 9 ++------- > sys/dev/pccard/pccard.c | 2 +- > sys/kern/subr_bus.c | 41 +++++++++++++++++------------------------ > sys/sys/bus.h | 1 - > 5 files changed, 20 insertions(+), 39 deletions(-) > > diff --git a/sys/dev/drm2/drm_fops.c b/sys/dev/drm2/drm_fops.c > index 3b3be06345e5..0c8b71759292 100644 > --- a/sys/dev/drm2/drm_fops.c > +++ b/sys/dev/drm2/drm_fops.c > @@ -157,9 +157,7 @@ int drm_open(struct cdev *kdev, int flags, int fmt, DRM_S > TRUCTPROC *p) > return 0; > > err_undo: > - mtx_lock(&Giant); /* FIXME: Giant required? */ > device_unbusy(dev->dev); > - mtx_unlock(&Giant); > dev->open_count--; > sx_xunlock(&drm_global_mutex); > return -retcode; > @@ -273,9 +271,7 @@ static int drm_open_helper(struct cdev *kdev, int flags, > int fmt, > list_add(&priv->lhead, &dev->filelist); > DRM_UNLOCK(dev); > > - mtx_lock(&Giant); /* FIXME: Giant required? */ > device_busy(dev->dev); > - mtx_unlock(&Giant); > > ret = devfs_set_cdevpriv(priv, drm_release); > if (ret != 0) > @@ -453,9 +449,7 @@ void drm_release(void *data) > */ > > atomic_inc(&dev->counts[_DRM_STAT_CLOSES]); > - mtx_lock(&Giant); > device_unbusy(dev->dev); > - mtx_unlock(&Giant); > if (!--dev->open_count) { > if (atomic_read(&dev->ioctl_count)) { > DRM_ERROR("Device busy: %d\n", > diff --git a/sys/dev/gpio/gpiopps.c b/sys/dev/gpio/gpiopps.c > index 4700acf19bcd..741bfa4498a6 100644 > --- a/sys/dev/gpio/gpiopps.c > +++ b/sys/dev/gpio/gpiopps.c > @@ -73,9 +73,7 @@ gpiopps_open(struct cdev *dev, int flags, int fmt, struct t > hread *td) > > /* We can't be unloaded while open, so mark ourselves BUSY. */ > mtx_lock(&sc->pps_mtx); > - if (device_get_state(sc->dev) < DS_BUSY) { > - device_busy(sc->dev); > - } > + device_busy(sc->dev); > mtx_unlock(&sc->pps_mtx); > > return 0; > @@ -86,10 +84,6 @@ gpiopps_close(struct cdev *dev, int flags, int fmt, struct > thread *td) > { > struct pps_softc *sc = dev->si_drv1; > > - /* > - * Un-busy on last close. We rely on the vfs counting stuff to only cal > l > - * this routine on last-close, so we don't need any open-count logic. > - */ > mtx_lock(&sc->pps_mtx); > device_unbusy(sc->dev); > mtx_unlock(&sc->pps_mtx); > @@ -113,6 +107,7 @@ gpiopps_ioctl(struct cdev *dev, u_long cmd, caddr_t data, > int flags, struct thre > > static struct cdevsw pps_cdevsw = { > .d_version = D_VERSION, > + .d_flags = D_TRACKCLOSE, > .d_open = gpiopps_open, > .d_close = gpiopps_close, > .d_ioctl = gpiopps_ioctl, > diff --git a/sys/dev/pccard/pccard.c b/sys/dev/pccard/pccard.c > index 8f19eb84725c..48d2e6973a38 100644 > --- a/sys/dev/pccard/pccard.c > +++ b/sys/dev/pccard/pccard.c > @@ -342,7 +342,7 @@ pccard_detach_card(device_t dev) > if (pf->dev == NULL) > continue; > state = device_get_state(pf->dev); > - if (state == DS_ATTACHED || state == DS_BUSY) > + if (state == DS_ATTACHED) > device_detach(pf->dev); > if (pf->cfe != NULL) > pccard_function_disable(pf); > diff --git a/sys/kern/subr_bus.c b/sys/kern/subr_bus.c > index 09072e1a23de..ab7de881d57d 100644 > --- a/sys/kern/subr_bus.c > +++ b/sys/kern/subr_bus.c > @@ -51,6 +51,7 @@ __FBSDID("$FreeBSD$"); > #include <sys/queue.h> > #include <machine/bus.h> > #include <sys/random.h> > +#include <sys/refcount.h> > #include <sys/rman.h> > #include <sys/sbuf.h> > #include <sys/selinfo.h> > @@ -140,7 +141,7 @@ struct _device { > int unit; /**< current unit number */ > char* nameunit; /**< name+unit e.g. foodev0 */ > char* desc; /**< driver specific description */ > - int busy; /**< count of calls to device_busy() */ > + u_int busy; /**< count of calls to device_busy() */ > device_state_t state; /**< current device state */ > uint32_t devflags; /**< api level flags for device_get_fla > gs() */ > u_int flags; /**< internal device flags */ > @@ -2634,13 +2635,13 @@ device_disable(device_t dev) > void > device_busy(device_t dev) > { > - if (dev->state < DS_ATTACHING) > - panic("device_busy: called for unattached device"); > - if (dev->busy == 0 && dev->parent) > + > + /* > + * Mark the device as busy, recursively up the tree if this busy count > + * goes 0->1. > + */ > + if (refcount_acquire(&dev->busy) == 0 && dev->parent != NULL) > device_busy(dev->parent); > - dev->busy++; > - if (dev->state == DS_ATTACHED) > - dev->state = DS_BUSY; > } > > /** > @@ -2649,17 +2650,12 @@ device_busy(device_t dev) > void > device_unbusy(device_t dev) > { > - if (dev->busy != 0 && dev->state != DS_BUSY && > - dev->state != DS_ATTACHING) > - panic("device_unbusy: called for non-busy device %s", > - device_get_nameunit(dev)); > - dev->busy--; > - if (dev->busy == 0) { > - if (dev->parent) > - device_unbusy(dev->parent); > - if (dev->state == DS_BUSY) > - dev->state = DS_ATTACHED; > - } > + > + /* > + * Mark the device as unbsy, recursively if this is the last busy count > . > + */ > + if (refcount_release(&dev->busy) && dev->parent != NULL) > + device_unbusy(dev->parent); > } > > /** > @@ -3004,10 +3000,7 @@ device_attach(device_t dev) > attachentropy = (uint16_t)(get_cyclecount() - attachtime); > random_harvest_direct(&attachentropy, sizeof(attachentropy), RANDOM_ATT > ACH); > device_sysctl_update(dev); > - if (dev->busy) > - dev->state = DS_BUSY; > - else > - dev->state = DS_ATTACHED; > + dev->state = DS_ATTACHED; > dev->flags &= ~DF_DONENOMATCH; > EVENTHANDLER_DIRECT_INVOKE(device_attach, dev); > devadded(dev); > @@ -3038,7 +3031,7 @@ device_detach(device_t dev) > GIANT_REQUIRED; > > PDEBUG(("%s", DEVICENAME(dev))); > - if (dev->state == DS_BUSY) > + if (dev->busy > 0) > return (EBUSY); > if (dev->state == DS_ATTACHING) { > device_printf(dev, "device in attaching state! Deferring detach > .\n"); > @@ -3090,7 +3083,7 @@ int > device_quiesce(device_t dev) > { > PDEBUG(("%s", DEVICENAME(dev))); > - if (dev->state == DS_BUSY) > + if (dev->busy > 0) > return (EBUSY); > if (dev->state != DS_ATTACHED) > return (0); > diff --git a/sys/sys/bus.h b/sys/sys/bus.h > index 0942b7246ae4..4a1afa864c2f 100644 > --- a/sys/sys/bus.h > +++ b/sys/sys/bus.h > @@ -58,7 +58,6 @@ typedef enum device_state { > DS_ALIVE = 20, /**< @brief probe succeeded */ > DS_ATTACHING = 25, /**< @brief currently attaching */ > DS_ATTACHED = 30, /**< @brief attach method called */ > - DS_BUSY = 40 /**< @brief device is open */ Hi Warner, This part of this patch has caused a ports build failure, in audio/oss. In file included from lynxone.c:20: In file included from ./module.inc:77: ./bsdpci.inc:95:33: error: use of undeclared identifier 'DS_BUSY' if (device_get_state(dev) == DS_BUSY) ^ 1 error generated. *** [lynxone.o] Error code 1 make[2]: stopped in /wrkdirs/usr/ports/audio/oss/work/.build/prototype/usr/l ocal/lib/oss/build 1 error -- Cheers, Cy Schubert <Cy.Schubert@cschubert.com> FreeBSD UNIX: <cy@FreeBSD.org> Web: https://FreeBSD.org NTP: <cy@nwtime.org> Web: https://nwtime.org The need of the many outweighs the greed of the few.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202112092215.1B9MFu2h093286>