From nobody Tue Nov 30 22:06:17 2021 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 30FDC182306B; Tue, 30 Nov 2021 22:06:18 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4J3bs55t42z3ttV; Tue, 30 Nov 2021 22:06:17 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id AA83F1E115; Tue, 30 Nov 2021 22:06:17 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 1AUM6HMF096129; Tue, 30 Nov 2021 22:06:17 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 1AUM6HhQ096128; Tue, 30 Nov 2021 22:06:17 GMT (envelope-from git) Date: Tue, 30 Nov 2021 22:06:17 GMT Message-Id: <202111302206.1AUM6HhQ096128@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Warner Losh Subject: git: 08e781915363 - main - Make device_busy/unbusy work w/o Giant held List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: imp X-Git-Repository: src X-Git-Refname: refs/heads/main X-Git-Reftype: branch X-Git-Commit: 08e781915363f98f4318a864b3b5a52bd99424c6 Auto-Submitted: auto-generated ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1638309977; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=YCFroQ5J0+9tiyHVoZa9af84Xue6+MLhc0rZQ6NhbDs=; b=kXF1+/JYpUIxzJUvMDO5/hN7yZa1+t0AkRCh45bmyWHfe9D/WUbd3dF7FYsmscP6tMN2bs lzQYRy/aK3V3b7Lf7h/VCissi04XTmMPXG1CjSv8Q9q/9X8O6XSpDrJS2sBwP9obDVhRTu UgrNWSMsP353Q3XAtXvE1pYgOycc+OGS52ZE0COlgI/LqnCEPOAh2jScIA2ZpYOKYoc4yf hKyJl6ueQJQ7OuaOxMl+e8YTPmtSUbH9j6wxaL6NPYTuvUXyAX0E6isSQR2ECl7wG29pF/ CQPdovrP9r/tjdgnbiSnY03DqtLOlbtkj/Qj/LcSCDLC8YHjU4awbSG9nv0iYw== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1638309977; a=rsa-sha256; cv=none; b=eFYbaKJUpmkzXiAn0RsWjPzd0F+/2NfJxxoqEbpRYXCmmoutuov/qTxq2vADJaCMz1HMuN M3MFCSsoFX2MJnEACixT4518HFMFIW0UjBguNIjqpueaqfqiNjOCelbW3K21niSErLKKLL DTzRGMCB1wJQqsDrOaAOFybQWk0wz12OPNWpUqZsZNy4kNGs54IizmOIGZ+9GHOzKO6Xxk oN4lWt0DlF7p+ajyN439/4H8xCeXxFIA/7qINbrw1/kbd7Hm39Y5uCy4MiOVuZSualiy/D dMoAOjIEMTkTXTfcDFsHd0FA/ras6hzawn3jW4f4iBHhBh7g1IIG/uUa9ae0pg== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N The branch main has been updated by imp: URL: https://cgit.FreeBSD.org/src/commit/?id=08e781915363f98f4318a864b3b5a52bd99424c6 commit 08e781915363f98f4318a864b3b5a52bd99424c6 Author: Warner Losh AuthorDate: 2021-11-30 22:03:26 +0000 Commit: Warner Losh CommitDate: 2021-11-30 22:03:26 +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. Create a new device_busy_locked and device_unbusy_locked that are the current implemntations of device_busy and device_unbusy. Make device_busy and unbusy acquire Giant before calling the _locked versrions. Since we never sleep in the busy/unbusy path, Giant's single threaded semantics suffice to keep this safe. 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_STRUCTPROC *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 thread *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 call - * 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 #include #include +#include #include #include #include @@ -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_flags() */ 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_ATTACH); 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 */ } device_state_t; /**