From nobody Thu Dec 9 22:15:56 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 4EA2318DCFA5; Thu, 9 Dec 2021 22:16:01 +0000 (UTC) (envelope-from cy.schubert@cschubert.com) Received: from omta001.cacentral1.a.cloudfilter.net (omta001.cacentral1.a.cloudfilter.net [3.97.99.32]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "Client", Issuer "CA" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 4J97f81hzHz3l4Z; Thu, 9 Dec 2021 22:15:59 +0000 (UTC) (envelope-from cy.schubert@cschubert.com) Received: from shw-obgw-4003a.ext.cloudfilter.net ([10.228.9.183]) by cmsmtp with ESMTP id vQvGmUnQ8lW5qvRhnmAwz8; Thu, 09 Dec 2021 22:15:59 +0000 Received: from spqr.komquats.com ([70.66.148.124]) by cmsmtp with ESMTPA id vRhlmfq2smX1kvRhmmG8ed; Thu, 09 Dec 2021 22:15:59 +0000 X-Authority-Analysis: v=2.4 cv=Fe4keby6 c=1 sm=1 tr=0 ts=61b2801f a=Cwc3rblV8FOMdVN/wOAqyQ==:117 a=Cwc3rblV8FOMdVN/wOAqyQ==:17 a=kj9zAlcOel0A:10 a=IOMw9HtfNCkA:10 a=6I5d2MoRAAAA:8 a=YxBL1-UpAAAA:8 a=EkcXrb_YAAAA:8 a=V-21NY8WNMxSXY6NNdoA:9 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 a=Ia-lj3WSrqcvXOmTRaiG:22 a=LK5xJRSDVpKd5WXXoEvA:22 Received: from slippy.cwsent.com (slippy [10.1.1.91]) by spqr.komquats.com (Postfix) with ESMTPS id 8D4241552; Thu, 9 Dec 2021 14:15:56 -0800 (PST) Received: from slippy (localhost [127.0.0.1]) by slippy.cwsent.com (8.16.1/8.16.1) with ESMTP id 1B9MFu2h093286; Thu, 9 Dec 2021 14:15:56 -0800 (PST) (envelope-from Cy.Schubert@cschubert.com) Message-Id: <202112092215.1B9MFu2h093286@slippy.cwsent.com> X-Mailer: exmh version 2.9.0 11/07/2018 with nmh-1.7.1 Reply-to: Cy Schubert From: Cy Schubert X-os: FreeBSD X-Sender: cy@cwsent.com X-URL: http://www.cschubert.com/ To: Warner Losh 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 In-reply-to: <202111302221.1AUML4IA017860@gitrepo.freebsd.org> References: <202111302221.1AUML4IA017860@gitrepo.freebsd.org> Comments: In-reply-to Warner Losh message dated "Tue, 30 Nov 2021 22:21:04 +0000." 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=us-ascii Date: Thu, 09 Dec 2021 14:15:56 -0800 X-CMAE-Envelope: MS4xfKb8E7Xwv+wWuxP9eTVW/TqTNamTmaRC7G0yRDJb2vKD/XsbF55nqCd+bLzys11EXfsVlSqGVRZqIcpUWJZPDo3bEsiMeIf9QsbrzklCnWu/ENuxxL4s H6MwADtgi06xh6nHASIvYE7gsQCL7OaioWbC00HCgiz1W//4B7YeHoNWzFWw7DdW9usgDd3i/WxW6S2MsLAZX9KDkLpYspCqxFQNbMLtJZoeVXF5jxK2BJvF b/MdttlpemUQREK3mcKuVHpRMJ4fTWvaVxvLQpq8RH/tOK0aU8EFvjvVQqdjXFvPp9zYExtfEAMPWhBpkILLgHWv1xInG/f4vsQekxYVeuA= X-Rspamd-Queue-Id: 4J97f81hzHz3l4Z X-Spamd-Bar: ++ Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=none; spf=none (mx1.freebsd.org: domain of cy.schubert@cschubert.com has no SPF policy when checking 3.97.99.32) smtp.mailfrom=cy.schubert@cschubert.com X-Spamd-Result: default: False [2.27 / 15.00]; HAS_REPLYTO(0.00)[Cy.Schubert@cschubert.com]; RCVD_VIA_SMTP_AUTH(0.00)[]; RCVD_COUNT_FIVE(0.00)[5]; REPLYTO_EQ_FROM(0.00)[]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; MV_CASE(0.50)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; MIME_GOOD(-0.10)[text/plain]; DMARC_NA(0.00)[cschubert.com: no valid DMARC record]; ARC_NA(0.00)[]; TO_DN_SOME(0.00)[]; NEURAL_SPAM_SHORT(1.00)[0.997]; AUTH_NA(1.00)[]; RECEIVED_SPAMHAUS_PBL(0.00)[70.66.148.124:received]; NEURAL_HAM_MEDIUM(-0.95)[-0.950]; NEURAL_SPAM_LONG(0.93)[0.927]; R_SPF_NA(0.00)[no SPF record]; FROM_EQ_ENVFROM(0.00)[]; R_DKIM_NA(0.00)[]; MIME_TRACE(0.00)[0:+]; ASN(0.00)[asn:16509, ipnet:3.96.0.0/15, country:US]; RCVD_TLS_LAST(0.00)[]; RWL_MAILSPIKE_VERYGOOD(0.00)[3.97.99.32:from]; RCVD_IN_DNSWL_LOW(-0.10)[3.97.99.32:from] X-ThisMailContainsUnwantedMimeParts: N 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 > AuthorDate: 2021-11-30 22:03:26 +0000 > Commit: Warner Losh > 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 > #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_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 FreeBSD UNIX: Web: https://FreeBSD.org NTP: Web: https://nwtime.org The need of the many outweighs the greed of the few.