[dm-devel] [PATCH 2/6] dm raid45 target: introduce memory cache

Jonathan Brassow jbrassow at redhat.com
Fri Jun 19 16:45:39 UTC 2009


This patch looks good to me.  Just a couple questions about object  
accounting...

  brassow

On Jun 15, 2009, at 12:21 PM, heinzm at redhat.com wrote:

> +int dm_mem_cache_grow(struct dm_mem_cache_client *cl, unsigned  
> objects)
> +{
> +	unsigned pages = objects * cl->chunks * cl->pages_per_chunk;
> +	struct page_list *pl, *last;
> +
> +	BUG_ON(!pages);
> +	pl = alloc_cache_pages(pages);
> +	if (!pl)
> +		return -ENOMEM;
> +
> +	last = pl;
> +	while (last->next)
> +		last = last->next;
> +
> +	spin_lock_irq(&cl->lock);
> +	last->next = cl->free_list;
> +	cl->free_list = pl;
> +	cl->free_pages += pages;
> +	cl->total_pages += pages;
> +	cl->objects++;

Should this be 'cl->objects += objects'?

> +	spin_unlock_irq(&cl->lock);
> +
> +	mempool_resize(cl->objs_pool, cl->objects, GFP_NOIO);

Do we need to check return value here?

> +	return 0;
> +}
> +EXPORT_SYMBOL(dm_mem_cache_grow);
> +
> +/* Shrink a clients cache by an amount of pages */
> +int dm_mem_cache_shrink(struct dm_mem_cache_client *cl, unsigned  
> objects)
> +{
> +	int r;
> +	unsigned pages = objects * cl->chunks * cl->pages_per_chunk, p =  
> pages;
> +	unsigned long flags;
> +	struct page_list *last = NULL, *pl, *pos;
> +
> +	BUG_ON(!pages);
> +
> +	spin_lock_irqsave(&cl->lock, flags);
> +	pl = pos = cl->free_list;
> +	while (p-- && pos->next) {
> +		last = pos;
> +		pos = pos->next;
> +	}
> +
> +	if (++p)
> +		r = -ENOMEM;
> +	else {
> +		r = 0;
> +		cl->free_list = pos;
> +		cl->free_pages -= pages;
> +		cl->total_pages -= pages;
> +		cl->objects--;

'cl->objects -= objects'?

> +		last->next = NULL;
> +	}
> +	spin_unlock_irqrestore(&cl->lock, flags);
> +
> +	if (!r) {
> +		free_cache_pages(pl);
> +		mempool_resize(cl->objs_pool, cl->objects, GFP_NOIO);

When shrinking, 'mempool_resize' will not fail... no need to check  
return value?

> +	}
> +
> +	return r;
> +}
> +EXPORT_SYMBOL(dm_mem_cache_shrink);
> +
> +/*
> + * Allocate/free a memory object
> + *
> + * Can be called from interrupt context
> + */
> +struct dm_mem_cache_object *dm_mem_cache_alloc(struct  
> dm_mem_cache_client *cl)
> +{
> +	int r = 0;
> +	unsigned pages = cl->chunks * cl->pages_per_chunk;
> +	unsigned long flags;
> +	struct dm_mem_cache_object *obj;
> +
> +	obj = mempool_alloc(cl->objs_pool, GFP_NOIO);
> +	if (!obj)
> +		return ERR_PTR(-ENOMEM);
> +
> +	spin_lock_irqsave(&cl->lock, flags);
> +	if (pages > cl->free_pages)
> +		r = -ENOMEM;
> +	else
> +		cl->free_pages -= pages;
> +	spin_unlock_irqrestore(&cl->lock, flags);

It's not important, but 'free_chunks' adjusts the 'cl->free_pages'  
count for us...  That made me look for where 'cl->free_pages' was  
adjusted in 'alloc_chunks', but that is done here.  Could we push this  
critical section into alloc_chunks and then just check the return code  
of that?

> +
> +	if (r) {
> +		mempool_free(obj, cl->objs_pool);
> +		return ERR_PTR(r);
> +	}
> +
> +	alloc_chunks(cl, obj);
> +	return obj;
> +}
> +EXPORT_SYMBOL(dm_mem_cache_alloc);
> +
> +void dm_mem_cache_free(struct dm_mem_cache_client *cl,
> +		       struct dm_mem_cache_object *obj)
> +{
> +	free_chunks(cl, obj);
> +	mempool_free(obj, cl->objs_pool);
> +}
> +EXPORT_SYMBOL(dm_mem_cache_free);
> +
> +MODULE_DESCRIPTION(DM_NAME " dm memory cache");
> +MODULE_AUTHOR("Heinz Mauelshagen <hjm at redhat.com>");
> +MODULE_LICENSE("GPL");


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/dm-devel/attachments/20090619/1085bdec/attachment.htm>


More information about the dm-devel mailing list