Messages in this thread Patch in this message | | | Subject | Re: lockdep complaints in slab allocator | From | Pekka Enberg <> | Date | Mon, 23 Nov 2009 21:00:00 +0200 |
| |
Hi Peter,
On Fri, 2009-11-20 at 16:09 +0100, Peter Zijlstra wrote: > > Uh, ok, so apparently I was right after all. There's a comment in > > free_block() above the slab_destroy() call that refers to the comment > > above alloc_slabmgmt() function definition which explains it all. > > > > Long story short: ->slab_cachep never points to the same kmalloc cache > > we're allocating or freeing from. Where do we need to put the > > spin_lock_nested() annotation? Would it be enough to just use it in > > cache_free_alien() for alien->lock or do we need it in > > cache_flusharray() as well? > > You'd have to somehow push the nested state down from the > kmem_cache_free() call in slab_destroy() to all nc->lock sites below.
That turns out to be _very_ hard. How about something like the following untested patch which delays slab_destroy() while we're under nc->lock.
Pekka
diff --git a/mm/slab.c b/mm/slab.c index 7dfa481..6f522e3 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -316,7 +316,7 @@ struct kmem_list3 __initdata initkmem_list3[NUM_INIT_LISTS]; static int drain_freelist(struct kmem_cache *cache, struct kmem_list3 *l3, int tofree); static void free_block(struct kmem_cache *cachep, void **objpp, int len, - int node); + int node, struct list_head *to_destroy); static int enable_cpucache(struct kmem_cache *cachep, gfp_t gfp); static void cache_reap(struct work_struct *unused); @@ -1002,7 +1002,8 @@ static void free_alien_cache(struct array_cache **ac_ptr) } static void __drain_alien_cache(struct kmem_cache *cachep, - struct array_cache *ac, int node) + struct array_cache *ac, int node, + struct list_head *to_destroy) { struct kmem_list3 *rl3 = cachep->nodelists[node]; @@ -1016,12 +1017,22 @@ static void __drain_alien_cache(struct kmem_cache *cachep, if (rl3->shared) transfer_objects(rl3->shared, ac, ac->limit); - free_block(cachep, ac->entry, ac->avail, node); + free_block(cachep, ac->entry, ac->avail, node, to_destroy); ac->avail = 0; spin_unlock(&rl3->list_lock); } } +static void slab_destroy(struct kmem_cache *, struct slab *); + +static void destroy_slabs(struct kmem_cache *cache, struct list_head *to_destroy) +{ + struct slab *slab, *tmp; + + list_for_each_entry_safe(slab, tmp, to_destroy, list) + slab_destroy(cache, slab); +} + /* * Called from cache_reap() to regularly drain alien caches round robin. */ @@ -1033,8 +1044,11 @@ static void reap_alien(struct kmem_cache *cachep, struct kmem_list3 *l3) struct array_cache *ac = l3->alien[node]; if (ac && ac->avail && spin_trylock_irq(&ac->lock)) { - __drain_alien_cache(cachep, ac, node); + LIST_HEAD(to_destroy); + + __drain_alien_cache(cachep, ac, node, &to_destroy); spin_unlock_irq(&ac->lock); + destroy_slabs(cachep, &to_destroy); } } } @@ -1049,9 +1063,12 @@ static void drain_alien_cache(struct kmem_cache *cachep, for_each_online_node(i) { ac = alien[i]; if (ac) { + LIST_HEAD(to_destroy); + spin_lock_irqsave(&ac->lock, flags); - __drain_alien_cache(cachep, ac, i); + __drain_alien_cache(cachep, ac, i, &to_destroy); spin_unlock_irqrestore(&ac->lock, flags); + destroy_slabs(cachep, &to_destroy); } } } @@ -1076,17 +1093,20 @@ static inline int cache_free_alien(struct kmem_cache *cachep, void *objp) l3 = cachep->nodelists[node]; STATS_INC_NODEFREES(cachep); if (l3->alien && l3->alien[nodeid]) { + LIST_HEAD(to_destroy); + alien = l3->alien[nodeid]; spin_lock(&alien->lock); if (unlikely(alien->avail == alien->limit)) { STATS_INC_ACOVERFLOW(cachep); - __drain_alien_cache(cachep, alien, nodeid); + __drain_alien_cache(cachep, alien, nodeid, &to_destroy); } alien->entry[alien->avail++] = objp; spin_unlock(&alien->lock); + destroy_slabs(cachep, &to_destroy); } else { spin_lock(&(cachep->nodelists[nodeid])->list_lock); - free_block(cachep, &objp, 1, nodeid); + free_block(cachep, &objp, 1, nodeid, NULL); spin_unlock(&(cachep->nodelists[nodeid])->list_lock); } return 1; @@ -1118,7 +1138,7 @@ static void __cpuinit cpuup_canceled(long cpu) /* Free limit for this kmem_list3 */ l3->free_limit -= cachep->batchcount; if (nc) - free_block(cachep, nc->entry, nc->avail, node); + free_block(cachep, nc->entry, nc->avail, node, NULL); if (!cpus_empty(*mask)) { spin_unlock_irq(&l3->list_lock); @@ -1128,7 +1148,7 @@ static void __cpuinit cpuup_canceled(long cpu) shared = l3->shared; if (shared) { free_block(cachep, shared->entry, - shared->avail, node); + shared->avail, node, NULL); l3->shared = NULL; } @@ -2402,7 +2422,7 @@ static void do_drain(void *arg) check_irq_off(); ac = cpu_cache_get(cachep); spin_lock(&cachep->nodelists[node]->list_lock); - free_block(cachep, ac->entry, ac->avail, node); + free_block(cachep, ac->entry, ac->avail, node, NULL); spin_unlock(&cachep->nodelists[node]->list_lock); ac->avail = 0; } @@ -3410,7 +3430,7 @@ __cache_alloc(struct kmem_cache *cachep, gfp_t flags, void *caller) * Caller needs to acquire correct kmem_list's list_lock */ static void free_block(struct kmem_cache *cachep, void **objpp, int nr_objects, - int node) + int node, struct list_head *to_destroy) { int i; struct kmem_list3 *l3; @@ -3439,7 +3459,10 @@ static void free_block(struct kmem_cache *cachep, void **objpp, int nr_objects, * a different cache, refer to comments before * alloc_slabmgmt. */ - slab_destroy(cachep, slabp); + if (to_destroy) + list_add(&slabp->list, to_destroy); + else + slab_destroy(cachep, slabp); } else { list_add(&slabp->list, &l3->slabs_free); } @@ -3479,7 +3502,7 @@ static void cache_flusharray(struct kmem_cache *cachep, struct array_cache *ac) } } - free_block(cachep, ac->entry, batchcount, node); + free_block(cachep, ac->entry, batchcount, node, NULL); free_done: #if STATS { @@ -3822,7 +3845,7 @@ static int alloc_kmemlist(struct kmem_cache *cachep, gfp_t gfp) if (shared) free_block(cachep, shared->entry, - shared->avail, node); + shared->avail, node, NULL); l3->shared = new_shared; if (!l3->alien) { @@ -3925,7 +3948,7 @@ static int do_tune_cpucache(struct kmem_cache *cachep, int limit, if (!ccold) continue; spin_lock_irq(&cachep->nodelists[cpu_to_node(i)]->list_lock); - free_block(cachep, ccold->entry, ccold->avail, cpu_to_node(i)); + free_block(cachep, ccold->entry, ccold->avail, cpu_to_node(i), NULL); spin_unlock_irq(&cachep->nodelists[cpu_to_node(i)]->list_lock); kfree(ccold); } @@ -4007,7 +4030,7 @@ void drain_array(struct kmem_cache *cachep, struct kmem_list3 *l3, tofree = force ? ac->avail : (ac->limit + 4) / 5; if (tofree > ac->avail) tofree = (ac->avail + 1) / 2; - free_block(cachep, ac->entry, tofree, node); + free_block(cachep, ac->entry, tofree, node, NULL); ac->avail -= tofree; memmove(ac->entry, &(ac->entry[tofree]), sizeof(void *) * ac->avail);
| |