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

Alexander Zarochentsev
2006-06-05T11:27:45Z

Hello,

On Monday 05 June 2006 11:37, Ingo Molnar wrote:
> * 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.

please note that the master lock is taken by try_caputure only if new
atom is created. It is likely than current thread has an atom already
or the block already captured.

> 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 was a code to measure lock contention for reiser4 locks which
showed that the tmgr lock was contented less than atom and jnode spin
locks were.

> 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.

I think the txnh lock and the tmgr lock are _non_nested. And, there is
a place where two atom locks are taken in deadlock-free order w/o
always keeping correct order of unlocking. The latest thing can be
made lock-validator-friendly.

> Ingo

Best,
Alex.