2002-07-12 00:45:36

by Sandy Harris

[permalink] [raw]
Subject: Re: spinlock assertion macros

Oliver Xymoron wrote:
>
> On Thu, 11 Jul 2002, Daniel Phillips wrote:
>
> > I was thinking of something as simple as:
> >
> > #define spin_assert_locked(LOCK) BUG_ON(!spin_is_locked(LOCK))
> >
> > but in truth I'd be happy regardless of the internal implementation. A note
> > on names: Linus likes to shout the names of his BUG macros. I've never been
> > one for shouting, but it's not my kernel, and anyway, I'm happy he now likes
> > asserts. I bet he'd like it more spelled like this though:
> >
> > MUST_HOLD(&lock);
>
> I prefer that form too.

Is it worth adding MUST_NOT_HOLD(&lock) in an attempt to catch potential
deadlocks?

Say that if two or more of locks A, B and C are to be taken, then
they must be taken in that order. You might then have code like:

MUST_NOT_HOLD(&lock_B) ;
MUST_NOT_HOLD(&lock_C) ;
spinlock(&lock_A) ;

I think you need a separate asertion for this !MUST_NOT_HOLD(&lock)
has different semantics.


2002-07-12 00:53:24

by Daniel Phillips

[permalink] [raw]
Subject: Re: spinlock assertion macros

On Friday 12 July 2002 01:52, Sandy Harris wrote:
> Oliver Xymoron wrote:
> >
> > On Thu, 11 Jul 2002, Daniel Phillips wrote:
> >
> > > I was thinking of something as simple as:
> > >
> > > #define spin_assert_locked(LOCK) BUG_ON(!spin_is_locked(LOCK))
> > >
> > > but in truth I'd be happy regardless of the internal implementation. A note
> > > on names: Linus likes to shout the names of his BUG macros. I've never been
> > > one for shouting, but it's not my kernel, and anyway, I'm happy he now likes
> > > asserts. I bet he'd like it more spelled like this though:
> > >
> > > MUST_HOLD(&lock);
> >
> > I prefer that form too.
>
> Is it worth adding MUST_NOT_HOLD(&lock) in an attempt to catch potential
> deadlocks?
>
> Say that if two or more of locks A, B and C are to be taken, then
> they must be taken in that order. You might then have code like:
>
> MUST_NOT_HOLD(&lock_B) ;
> MUST_NOT_HOLD(&lock_C) ;
> spinlock(&lock_A) ;
>
> I think you need a separate asertion for this !MUST_NOT_HOLD(&lock)
> has different semantics.

MUST_NOT_HOLD is already in Jesse's patch he posted earlier today,
though I imagine it would be used rarely if at all.

!MUST_NOT_HOLD(&lock) is an error, MUST_NOT_HOLD is a statement not a
function.

--
Daniel

2002-07-12 03:20:05

by Oliver Xymoron

[permalink] [raw]
Subject: Re: spinlock assertion macros

On Thu, 11 Jul 2002, Sandy Harris wrote:

> Oliver Xymoron wrote:
> >
> > On Thu, 11 Jul 2002, Daniel Phillips wrote:
> >
> > > I was thinking of something as simple as:
> > >
> > > #define spin_assert_locked(LOCK) BUG_ON(!spin_is_locked(LOCK))
> > >
> > > but in truth I'd be happy regardless of the internal implementation. A note
> > > on names: Linus likes to shout the names of his BUG macros. I've never been
> > > one for shouting, but it's not my kernel, and anyway, I'm happy he now likes
> > > asserts. I bet he'd like it more spelled like this though:
> > >
> > > MUST_HOLD(&lock);
> >
> > I prefer that form too.
>
> Is it worth adding MUST_NOT_HOLD(&lock) in an attempt to catch potential
> deadlocks?

No, you'd rather do that inside the debugging version of spinlock itself.
I see MUST_HOLD happening inside helper functions that presume a lock is
in effect but don't do any on their own to ensure integrity, while
MUST_NOT_HOLD is a matter of forcing ordering and avoiding deadlocks.
Locking order is larger than functions and should be documented at the
point of declaration of the locks.

You can do lock order checking in the spinlock debugging code something
like this:

struct spinlock
{
atomic_t val;
#ifdef SPINLOCK_DEBUG
void *eip;
struct spinlock *previous;
struct spinlock *ordering;
};

DECLARE_SPINLOCK(a, 0); /* a is outermost for this group */
DECLARE_SPINLOCK(b, &a); /* b must nest inside a */
DECLARE_SPINLOCK(c, &b);

Each time you take a spinlock, record the eip, stick the address of the
current lock in the task struct, and stick the outer lock in previous.
Then see if the current lock appears anywhere in the previous lock's
ordering chain (it shouldn't). This may be a bit overkill though.

It might make sense to have a MAY_SLEEP assert in a few key places (high
up in the chains of things that rarely sleep or for which the call path to
sleeping isn't obvious).

--
"Love the dolphins," she advised him. "Write by W.A.S.T.E.."