Re: [patch] lockdep: annotate mm/slab.c

[Ingo Molnar <mingo@elte.hu>]

* Pekka Enberg <penberg@cs.helsinki.fi> wrote:

> On 7/13/06, Ingo Molnar <mingo@elte.hu> wrote:
> >mm/slab.c uses nested locking when dealing with 'off-slab'
> >caches, in that case it allocates the slab header from the
> >(on-slab) kmalloc caches. Teach the lock validator about
> >this by putting all on-slab caches into a separate class.
> 
> What's "nested lock" btw? If I understood from the other patch, you're 
> talking about ac->lock. Surely you can't take the same lock twice but 
> it's perfectly legal to take lock as long as the ac instance is 
> different...

yeah - there's some ambiguity of the term "nested lock". For the lock 
validator it means "holding two instances of the same lock class". A 
"lock class" is something like inode->i_mutex. (standalone static locks 
like cache_chain_mutex form their own singleton lock class. See 
Documentation/lockdep-design.txt for more details.)

"trying to lock the same lock instance twice" we call "recursive 
locking", and that's a bug for everything except read_lock() on rwlocks. 
There is no "recursive locking" in slab.c, but there is "nested 
locking". For the case of "nested locking" the lock validator needs to 
be taught of the relation between instances. Most of the cases the 
relation is "static" and can thus be assigned build-time via the use of 
separate lock-keys that "split up" a class into subclasses. In rare 
cases the relation is dynamic (for example the VFS has such nesting 
rules).

initially i annotated slab.c via dynamic nesting - but as Arjan has 
correctly observed, most of the nested locking in slab.c is of static 
type: we first take the off-slab lock, then the on-slab lock. [or we 
only take an on-slab lock, if the cache is on-slab]

Note: nested locking annotations arent just done to "shut up" lockdep, 
but rather to enable lockdep from now on to enforce this dependency 
rule: if anywhere we take an off-slab lock after an on-slab lock it will 
print a warning. That's why we go the trouble of identifying all the 
nested locking cases instead of going the easy path of "shutting lockdep 
off" (we could trivially ignore nesting within lockdep.c) - there were a 
couple of locking bugs found already that were related to nested 
locking.

btw., there's still one nested locking construct in slab.c that could 
cause problems:

 Call Trace:
  [<ffffffff8020b0b9>] show_trace+0xaa/0x23d
  [<ffffffff8020b261>] dump_stack+0x15/0x17
  [<ffffffff802456c4>] __lock_acquire+0x127/0x9c4
  [<ffffffff8024648b>] lock_acquire+0x4b/0x6a
  [<ffffffff804aead3>] _spin_lock+0x25/0x31
  [<ffffffff8027301a>] __drain_alien_cache+0x34/0x78
  [<ffffffff80272b1f>] __cache_free+0xe8/0x215
  [<ffffffff80272dc4>] slab_destroy+0x9e/0xc2
  [<ffffffff80272ed3>] free_block+0xeb/0x135
  [<ffffffff80273043>] __drain_alien_cache+0x5d/0x78
  [<ffffffff80272b1f>] __cache_free+0xe8/0x215
  [<ffffffff80273203>] kfree+0x8c/0xb0

what guarantees that while we are 'draining' another node's cache, that 
other node does not drain our cache (and thus we'd deadlock)?

	Ingo