From owner-svn-src-head@FreeBSD.ORG Thu May 21 15:52:55 2009 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 3F7DB1065670; Thu, 21 May 2009 15:52:55 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id F0AF78FC17; Thu, 21 May 2009 15:52:54 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id A1F4F46B2C; Thu, 21 May 2009 11:52:54 -0400 (EDT) Received: from jhbbsd.hudson-trading.com (unknown [209.249.190.8]) by bigwig.baldwin.cx (Postfix) with ESMTPA id 583138A028; Thu, 21 May 2009 11:52:53 -0400 (EDT) From: John Baldwin To: "M. Warner Losh" Date: Thu, 21 May 2009 11:51:16 -0400 User-Agent: KMail/1.9.7 References: <3bbf2fe10905210629p46c7a204v6863aaba77354462@mail.gmail.com> <20090521.094100.70797067.imp@bsdimp.com> In-Reply-To: <20090521.094100.70797067.imp@bsdimp.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200905211151.17427.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.0.1 (bigwig.baldwin.cx); Thu, 21 May 2009 11:52:53 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.95 at bigwig.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-2.5 required=4.2 tests=AWL,BAYES_00,RDNS_NONE autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on bigwig.baldwin.cx Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, attilio@freebsd.org, svn-src-head@freebsd.org, rwatson@freebsd.org, kostikbel@gmail.com Subject: Re: svn commit: r192535 - head/sys/kern X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 May 2009 15:52:55 -0000 On Thursday 21 May 2009 11:41:00 am M. Warner Losh wrote: > In message: > Robert Watson writes: > : On Thu, 21 May 2009, John Baldwin wrote: > : > : >>>> Move the M_WAITOK flag in notify() into an M_NOWAIT one in order 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 finished to > : >>>> call camperiphfree() (and subsequently destroy_dev() resulting in a new > : >>>> dev notify) while the xpt lock is held. > : >>> This is wrong. You cannot call destroy_dev() while holding any mutex. > : >>> Taking this into account, it makes no sense to use M_NOWAIT in notify(). > : >> > : >> As long as devctl_notify() also calls M_NOWAIT and if not available skips > : >> "silently" it just does the same thing, I think this approach is more > : >> consistent. > : >> > : >> It remains, though, the fact to fix CAM when calling destroy_dev(). Maybe > : >> we should add a witness_warn() there? > : > > : > I agree with kib, this should be reverted and CAM fixed instead. I also > : > agree that M_NOWAIT use should be limited where possible. > : > : devctl_notify() probably needs to grow a sleepable flag, or perhaps we need > : two variations, one that can sleep. > > 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. > > 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... > > Comments? destroy_dev() is not a good idea to call with a mutex held period. devctl_notify() is the least of one's worries in that case. In general the code holding a mutex over destroy_dev() should be fixed and I think devctl_notify() can be left unchanged. destroy_dev() is a draining operation similar to bus_teardown_intr(), callout_drain(), taskqueue_drain(), if_detach() (which doesn't drain yet, but needs to), etc. One simply cannot hold locks across those operations. -- John Baldwin