Re: 2.6.17-rc5-mm3: bad unlock ordering (reiser4?)

[Ingo Molnar <mingo@elte.hu>]

* Ingo Molnar <mingo@elte.hu> wrote:

> +++ linux/fs/reiser4/txnmgr.h
> @@ -613,7 +613,7 @@ static inline void spin_unlock_txnmgr(tx
>  	LOCK_CNT_DEC(spin_locked_txnmgr);
>  	LOCK_CNT_DEC(spin_locked);
>  
> -	spin_unlock(&(mgr->tmgr_lock));
> +	spin_unlock_non_nested(&(mgr->tmgr_lock));
>  }
>  
>  typedef enum {

Btw., this particular annotation also documents a locking/scalability 
inefficiency. mgr->tmgr_lock is a "global" lock (per superblock it 
seems), while atom->alock is a more "finegrained" lock.

Typical usage: tmgr_lock is used a 'master lock', it's taken, then 
atom->alock is taken, and then ->tmgr_lock is released. Then code runs 
under atom->alock, and atom->alock is released finally.

The scalability problem with such 'master locks' is that they pretty 
much control scalability, so the scalability advantage of the finer 
grained lock is reduced (often eliminated). Since access to the finer 
grained lock goes via the master lock, the master lock cacheline will 
bounce from CPU to CPU.

A much more scalable design is to get to the finer grained lock in some 
read-mostly, lockless way, and then take it. This often necessiates the 
utilization of RCU, but it's well worth it.

There's other kernel code that has been annotated for similar reasons - 
e.g. the netfilter code makes frequent use of master-locks.

All in one, it's a good idea to document such locking constructs via the 
_non_nested() annotation. Often they can be eliminated altogether and 
the code improves. It's not a maintainance problem either, because right 
now there are only 42 such annotations, out of 46,000+ locking API uses 
covered by the lock validator.

	Ingo