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

Ingo Molnar
2006-06-05T07:37:43Z

* 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