svn commit: r192535 - head/sys/kern

[John Baldwin]

On Thursday 21 May 2009 11:54:01 am Scott Long wrote:
> M. Warner Losh wrote:
> > In message: <alpine.BSF.2.00.0905211610140.18790 at fledge.watson.org>
> >             Robert Watson <rwatson at FreeBSD.org> 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?
> > 
> > Warner
> 
> Very much in agreement here.  I would also love to have destroy_dev() 
> and make_dev() be locking-neutral.  Having sleepable locks in leaf APIs
> is unpleasant for consumers of those APIs.

destroy_dev() does not use a sleepable lock, the problem is it drains so it 
can provide sane semantics to a caller who wants to ensure that all outside 
references to a cdev are gone when it returns.  You can't provide that 
without doing some sort of synchronization with the other threads trying to 
call d_open(), etc.  And you most certainly can't do it if you call 
destroy_dev() while holding your driver's mutex as you then have the problem 
that some other thread could be blocked on that mutex already in your 
d_open() routine when you call destroy_dev().  These sane semantics are 
needed so drivers can do things like safely free softcs and destroy locks, 
etc.

-- 
John Baldwin