Date: Thu, 21 May 2009 19:15:35 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: Scott Long <scottl@samsco.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, attilio@freebsd.org, rwatson@freebsd.org, svn-src-head@freebsd.org, "M. Warner Losh" <imp@bsdimp.com> Subject: Re: svn commit: r192535 - head/sys/kern Message-ID: <20090521161535.GQ1927@deviant.kiev.zoral.com.ua> In-Reply-To: <200905211211.00168.jhb@freebsd.org> References: <3bbf2fe10905210629p46c7a204v6863aaba77354462@mail.gmail.com> <20090521.094100.70797067.imp@bsdimp.com> <4A157919.7040103@samsco.org> <200905211211.00168.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--0oEQci8Wf3TZ4Qnh Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, May 21, 2009 at 12:10:59PM -0400, John Baldwin wrote: > On Thursday 21 May 2009 11:54:01 am Scott Long wrote: > > M. Warner Losh wrote: > > > In message: <alpine.BSF.2.00.0905211610140.18790@fledge.watson.org> > > > Robert Watson <rwatson@FreeBSD.org> writes: > > > : On Thu, 21 May 2009, John Baldwin wrote: > > > :=20 > > > : >>>> Move the M_WAITOK flag in notify() into an M_NOWAIT one in o= rder=20 > to > > > : > match > > > : >>>> the behaviour alredy present with the further malloc() call = in > > > : >>>> devctl_notify(). > > > : >>>> This fixes a bug in the CAM layer where the camisr handler= =20 > finished to > > > : >>>> call camperiphfree() (and subsequently destroy_dev() resulti= ng in=20 > a new > > > : >>>> dev notify) while the xpt lock is held. > > > : >>> This is wrong. You cannot call destroy_dev() while holding any= =20 > mutex.=20 > > > : >>> Taking this into account, it makes no sense to use M_NOWAIT in= =20 > notify(). > > > : >> > > > : >> As long as devctl_notify() also calls M_NOWAIT and if not availa= ble=20 > skips=20 > > > : >> "silently" it just does the same thing, I think this approach is= more=20 > > > : >> consistent. > > > : >> > > > : >> It remains, though, the fact to fix CAM when calling destroy_dev= ().=20 > Maybe=20 > > > : >> we should add a witness_warn() there? > > > : > > > > : > I agree with kib, this should be reverted and CAM fixed instead. = I=20 > also=20 > > > : > agree that M_NOWAIT use should be limited where possible. > > > :=20 > > > : devctl_notify() probably needs to grow a sleepable flag, or perhaps= we=20 > need=20 > > > : two variations, one that can sleep. > > >=20 > > > devctl_notify() has expanded well beyond its original needs. Having > > > an extra case for sleeping is the wrong way to solve this problem. > > > Really. We're adding hacks on hacks on hacks here and we need to step > > > back and think. > > >=20 > > > I specifically didn't put in CDEV notifications into devd when I > > > originally did it because one can get the same notification via > > > kevents on /dev. Maybe the right answer is to remove this stuff > > > entirely and update devd to do that instead? It isn't a lot of code, > > > and should provide equivalent functionality without needing to change > > > the rules of the game when it comes to destroy_dev(). Especially this > > > close to the code slush... > > >=20 > > > Comments? > > >=20 > > > Warner > >=20 > > Very much in agreement here. I would also love to have destroy_dev()= =20 > > and make_dev() be locking-neutral. Having sleepable locks in leaf APIs > > is unpleasant for consumers of those APIs. >=20 > destroy_dev() does not use a sleepable lock, the problem is it drains so = it=20 > can provide sane semantics to a caller who wants to ensure that all outsi= de=20 > references to a cdev are gone when it returns. You can't provide that=20 > without doing some sort of synchronization with the other threads trying = to=20 > call d_open(), etc. And you most certainly can't do it if you call=20 > destroy_dev() while holding your driver's mutex as you then have the prob= lem=20 > that some other thread could be blocked on that mutex already in your=20 > d_open() routine when you call destroy_dev(). These sane semantics are= =20 > needed so drivers can do things like safely free softcs and destroy locks= ,=20 > etc. Another thing done inside destroy_dev is the call to the destructors of the cdevpriv data, that never had any restrictions on the sleepable context. We do have the KPI for the callers that cannot drop the locks and need to do destroy_dev, destroy_dev_sched(9). --0oEQci8Wf3TZ4Qnh Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (FreeBSD) iEYEARECAAYFAkoVficACgkQC3+MBN1Mb4hyVQCaAiEfReu++iOf9z9sIld9D55I rqsAn2Jbhj37bADDDheoIcH7uREDQrzQ =3Zxs -----END PGP SIGNATURE----- --0oEQci8Wf3TZ4Qnh--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090521161535.GQ1927>