Re: [PATCH] lock assertion macros for 2.5.30

[Joshua MacDonald <jmacd@namesys.com>]

MUST_NOT_HOLD_LOCK means exactly I_AM_NOT_HOLDING_LOCK, otherwise the
assertion is obviously meaningless because another processor could be holding
the lock.  But since there is no reason to assert NO_ONE_IS_HOLDING_LOCK
(since it means the lock is unnecessary), the obvious meaning of
MUST_NOT_HOLD_LOCK is the correct one, that the current thread/CPU does not
hold the lock.

In order to implement MOST_NOT_HOLD_LOCK the spinlock must contain some record
of who holds the lock, and since the SCSI-layer apparently does not have such
a mechanism, it appears that something is broken in there.

-josh



On Wed, Aug 07, 2002 at 04:37:40PM -0500, Oliver Xymoron wrote:
> On Wed, 7 Aug 2002, Jesse Barnes wrote:
> 
> > On Wed, Aug 07, 2002 at 06:02:19PM -0300, Rik van Riel wrote:
> > > On Wed, 7 Aug 2002, Jesse Barnes wrote:
> > >
> > > > +++ linux-2.5.30-lockassert/drivers/scsi/scsi.c Wed Aug  7 11:35:32 2002
> > > > @@ -262,7 +262,7 @@
> > >
> > > > +        MUST_NOT_HOLD(q->queue_lock);
> > >
> > > ...
> > >
> > > > +#if defined(CONFIG_DEBUG_SPINLOCK) && defined(CONFIG_SMP)
> > > > +#define MUST_HOLD(lock)			BUG_ON(!spin_is_locked(lock))
> > > > +#define MUST_NOT_HOLD(lock)		BUG_ON(spin_is_locked(lock))
> > >
> > > Please tell me the MUST_NOT_HOLD thing is a joke.
> > >
> > > What is to prevent another CPU in another code path
> > > from holding this spinlock when the code you've
> > > inserted the MUST_NOT_HOLD in is on its merry way
> > > not holding the lock ?
> >
> > Nothing at all, but isn't that how the scsi ASSERT_LOCK(&lock, 0)
> > macro worked before?  I could just remove all those checks in the scsi
> > code I guess.
> 
> Who's to say that they actually worked? They look like crap to me.
> 
> > After I posted the last patch, a few people asked for MUST_NOT_HOLD so
> > I added it back in.  Do you think it's a bad idea?  See the last
> > thread if you're curious (Joshua's comments in particular):
> > http://marc.theaimsgroup.com/?t=102764009400001&r=1&w=2
> 
> Interesting. I'm still going to say that MUST_NOT_HOLD is wrong, at least
> in its current form/name.
> 
> What MUST_HOLD is saying is "the current thread is holding this lock, go
> ahead, double check if you want". What MUST_NOT_HOLD says is "the current
> thread is not holding this lock, feel free to check". Right now the kernel
> doesn't record who grabbed a lock and the best it can do is check whether
> _anyone_ is holding the lock. In the first case, it can prove a negative
> if no one is holding the lock, in the second case it can't because it
> can't distiguish between the current task holding a lock and any other
> task holding a lock.
> 
> If we want a MUST_NOT_RECURSE, we can do that, but it means adding cpu or
> current into the debugging version of spinlocks. I'd also add eip, so we
> can see where the lock was acquired last and dump that when we hit a
> conflict/deadlock.
> 
> And if you interpret MUST_NOT_HOLD_LOCK to mean "no one is holding this
> lock" then you run into Rik's problem. Anyone who actually means this
> ought to be simply taking the lock, otherwise why would they care?
> 
> -- 
>  "Love the dolphins," she advised him. "Write by W.A.S.T.E.."
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 
>