Re: [PATCH] lock assertion macros for 2.5.28

[Joshua MacDonald]

On Fri, Jul 26, 2002 at 10:42:58AM -0700, Jesse Barnes wrote:
> On Fri, Jul 26, 2002 at 04:09:18PM +0400, Joshua MacDonald wrote:
> > In reiser4 we are looking forward to having a MUST_NOT_HOLD (i.e.,
> > spin_is_not_locked) assertion for kernel spinlocks.  Do you know if any
> > progress has been made in that direction?
> 
> Well, I had that in one version of the patch, but people didn't think
> it would be useful.  Maybe you'd like to check out Oliver's comments
> at http://marc.theaimsgroup.com/?l=linux-kernel&m=102644431806734&w=2
> and respond?  If there's demand for MUST_NOT_HOLD, I'd be happy to add
> it since it should be easy.  But if you're using it to enforce lock
> ordering as Oliver suggests, then there are probably more robust
> solutions.
> 

I read Oliver's comments and I do not fully agree.  It is true that often the
MUST_NOT_HOLD macro is used to assert that you are not about to attempt a
recursive lock, which a debugging spinlock implementation would catch as soon
as the recursive attempt is made.  However, it is difficult to make a case
against adding support for this kind of assertion since it has many possible
uses.

It may be useful to catch a recursive spinlock attempt several stack frames
before it actually happens, or to assert that an unusual calling convention
such as "This function is called with the spinlock held and if it returns 0
the spinlock remains held, but if the function returns non-zero the spinlock
is released".  We have such a function in reiser4.

As for preventing deadlock, it is true that (as Oliver says) "Locking order is
larger than functions and should be documented at the point of declaration of
the locks."  We have a mechanism in reiser4 which is not quite the same as
Oliver outlined for making assertions about lock ordering.  We maintain
per-thread counts of each spinlock class and use those counts in a locking
predicate that is applied before a lock of each class is taken.

So I agree that recursive locking should be checked as part of the debugging
spin_lock() routine and that deadlock detection requires more general work,
but the MUST_NOT_HOLD assertion is still useful in some contexts.

-josh