Messages in this thread | | | Date | Thu, 22 Jun 2006 22:18:07 +0300 (EEST) | From | Pekka J Enberg <> | Subject | Re: [RFC 1/4] slab freeing consolidation |
| |
HI,
On Mon, 19 Jun 2006, Christoph Lameter wrote: > Add a new function drop_freelist that removes slabs with objects > that are already free and use that in various places.
I think you mean drain_freelist(). Anyway, looks good. Some minor comments below.
> -static int __node_shrink(struct kmem_cache *cachep, int node) > +/* > + * Remove slabs from the list of free slabs. > + * Specify the number of slabs to drain in tofree. > + * > + * Returns the actual number of slabs released. > + */ > +static int long drain_freelist(struct kmem_cache *cachep, > + struct kmem_list3 *l3, int tofree)
I have been trying to slowly kill the 'p' prefix so I'd appreciate if you could just call the parameter 'cache'. Also, l3 could be 'lists'.
> + nr_freed = 0; > + while (nr_freed < tofree && !list_empty(&l3->slabs_free)) { > > - p = l3->slabs_free.prev; > - if (p == &l3->slabs_free) > - break; > + spin_lock_irq(&l3->list_lock); > + p = l3->slabs_free.next; > + if (p == &(l3->slabs_free)) {
Please drop the redundant parenthesis.
> + spin_unlock_irq(&l3->list_lock); > + return nr_freed; > + }
Goto to the bottom would be nicer than return here, maybe.
> > - slabp = list_entry(l3->slabs_free.prev, struct slab, list); > + slabp = list_entry(p, struct slab, list); > #if DEBUG > BUG_ON(slabp->inuse); > #endif > list_del(&slabp->list); > - > + /* > + * Safe to drop the lock. The slab is no longer linked > + * to the cache. > + */ > l3->free_objects -= cachep->num; > spin_unlock_irq(&l3->list_lock); > slab_destroy(cachep, slabp); > - spin_lock_irq(&l3->list_lock); > - } > - ret = !list_empty(&l3->slabs_full) || !list_empty(&l3->slabs_partial); > - return ret; > + nr_freed ++;
Redundant whitespace.
> + };
Redundant semicolon.
> + else { > + int x;
nr_freed, would be better.
> > - slabp = list_entry(p, struct slab, list); > - BUG_ON(slabp->inuse); > - list_del(&slabp->list); > - STATS_INC_REAPED(searchp); > - > - /* > - * Safe to drop the lock. The slab is no longer linked > - * to the cache. searchp cannot disappear, we hold > - * cache_chain_lock > - */ > - l3->free_objects -= searchp->num; > - spin_unlock_irq(&l3->list_lock); > - slab_destroy(searchp, slabp); > - } while (--tofree > 0); > + x = drain_freelist(searchp, l3, (l3->free_limit + > + 5 * searchp->num - 1) / (5 * searchp->num)); > + STATS_ADD_REAPED(searchp, x);
Maybe extract a local variable 'to_free' for readability.
Pekka - 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/
| |