[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