Re: [RFC 1/4] slab freeing consolidation

[Pekka J Enberg <penberg@cs.Helsinki.FI>]

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/