Re: spinlock assertion macros

[Daniel Phillips]

On Friday 12 July 2002 22:41, Oliver Xymoron wrote:
> On Fri, 12 Jul 2002, Daniel Phillips wrote:
> > On Friday 12 July 2002 14:07, Dave Jones wrote:
> > > Something I've been meaning to hack up for a while is some spinlock
> > > debugging code that adds a FUNCTION_SLEEPS() to (ta-da) functions that
> > > may sleep.
> >
> > Yesss.  May I suggest simply SLEEPS()?  (Chances are, we know it's a
> > function.)
> >
> > > This macro then checks whether we're currently holding any
> > > locks, and if so printk's the names of locks held, and where they were taken.
> >
> > And then oopes?
> >
> > > When I came up with the idea[1] I envisioned some linked-lists frobbing,
> > > but in more recent times, we can now check the preempt_count for a
> > > quick-n-dirty implementation (without the additional info of which locks
> > > we hold, lock-taker, etc).
> >
> > Spin_lock just has to store the address/location of the lock in a
> > per-cpu vector, and the assert prints that out when it oopses.  Such
> > bugs won't live too long under those conditions.
> 
> Store it in the task struct, and store a pointer to the previous (outer
> lock) in that lock, then you've got a linked list of locks per task - very
> useful.

Yes, thanks, I was in fact feeling a little guilty about proposing that
non-nested solution, even if it would in practice point you at the
correct culprit most of the time.

So in schedule() we check the task struct field and oops if it's non-null.
In other words, one instance of SLEEPS goes in shedule() and others are
sprinkled around the kernel as executable documentation.

> You'll need a helper function - current() is hard to get at from
> spinlock.h due to tangled dependencies.

What is the problem?  It looks like (struct thread_info *) can be forward
declared, and hence current_thread_info and get_current can be defined
early.  Maybe I didn't sniff at enough architectures.

Anyway, if there are nasty dependencies then they are probably similar
to the ones I unravelled with my early-page patch, which allows struct
page to be declared right at the start of mm.h instead of way later,
below a lot of things that want to reference it.

> As I mentioned before, it can also
> be very handy to stash the address of the code that took the lock in the
> lock itself.

And since the stack is chained through the locks, that just works.
 
> > Any idea how one might implement NEVER_SLEEPS()?  Maybe as:
> >
> >    NEVER_ [code goes here] _SLEEPS
> >
> > which inc/dec the preeempt count, triggering a BUG in schedule().
> 
> NEVER_SLEEPS will only trigger on the rare events that blow up anyway,
> while the MAY_SLEEP version catches _potential_ problems even when the
> fatal sleep doesn't happen.

Yes, that one's already been rejected as largely pointless.

-- 
Daniel