Messages in this thread | | | Date | Thu, 13 Jul 2006 21:32:28 +0200 | From | Ingo Molnar <> | Subject | Re: [patch] lockdep: annotate mm/slab.c |
| |
* 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |