| Message ID | 4a368304.ChlIzzc+4MKVMZmr%heinzm@redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Delegated to: | Jonthan Brassow |
| Headers | show |
This patch looks good to me. Just a couple questions about object accounting... brassow On Jun 15, 2009, at 12:21 PM, heinzm@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@redhat.com>"); > +MODULE_LICENSE("GPL"); -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Fri, 2009-06-19 at 11:45 -0500, Jonathan Brassow wrote: > This patch looks good to me. Just a couple questions about object > accounting... Jon, see below. > > > brassow > > On Jun 15, 2009, at 12:21 PM, heinzm@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'? Thanks, good catch. The reason why this didn't cause trouble so far is, that the caller only requested one object per call on allocate/free. > > > + 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'? Yes, likewise. > > > + 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? Yes. > > > + } > > + > > + 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? dm_mem_cache_alloc() adjusts cl->free_pages holding cl->lock before it allocates the pages from the list via alloc_chunks(), where the cl->lock is held very short to pop one page off the list in order to allow for allocation parallelism with multiple thread in the allocator function chain. Hence a dm_mem_cache_alloc() call can run in parallel with the actual allocation of pages of another caller. On the dm_mem_cache_free() side of things, again cl->lock is hold for a short period of time to enhance parallelism. All that not playing out performance-wise with this caller (yet) because of single threaded allocate/free calls. Heinz > > > + > > + 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@redhat.com>"); > > +MODULE_LICENSE("GPL"); > > > > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/dm-memcache.h.orig b/drivers/md/dm-memcache.h index e69de29..87e4256 100644 --- a/drivers/md/dm-memcache.h.orig +++ b/drivers/md/dm-memcache.h @@ -0,0 +1,68 @@ +/* + * Copyright (C) 2006-2008 Red Hat, Inc. All rights reserved. + * + * Module Author: Heinz Mauelshagen <Mauelshagen@RedHat.com> + * + * Device-mapper memory object handling: + * + * o allocate/free total_pages in a per client page pool. + * + * o allocate/free memory objects with chunks (1..n) of + * pages_per_chunk pages hanging off. + * + * This file is released under the GPL. + */ + +#ifndef _DM_MEM_CACHE_H +#define _DM_MEM_CACHE_H + +#define DM_MEM_CACHE_H_VERSION "0.1" + +#include "dm.h" +#include <linux/dm-io.h> + +static inline struct page_list *pl_elem(struct page_list *pl, unsigned p) +{ + while (pl && p--) + pl = pl->next; + + return pl; +} + +struct dm_mem_cache_object { + struct page_list *pl; /* Dynamically allocated array */ + void *private; /* Caller context reference */ +}; + +struct dm_mem_cache_client; + +/* + * Create/destroy dm memory cache client resources. + * + * On creation, a number of @objects with @chunks of + * @pages_per_chunk pages will be allocated. + */ +struct dm_mem_cache_client * +dm_mem_cache_client_create(unsigned objects, unsigned chunks, + unsigned pages_per_chunk); +void dm_mem_cache_client_destroy(struct dm_mem_cache_client *client); + +/* + * Grow/shrink a dm memory cache client resources + * by @objetcs amount of objects. + */ +int dm_mem_cache_grow(struct dm_mem_cache_client *client, unsigned objects); +int dm_mem_cache_shrink(struct dm_mem_cache_client *client, unsigned objects); + +/* + * Allocate/free a memory object + * + * On allocation one object with an amount of chunks and + * an amount of pages per chunk will be returned on success. + */ +struct dm_mem_cache_object * +dm_mem_cache_alloc(struct dm_mem_cache_client *client); +void dm_mem_cache_free(struct dm_mem_cache_client *client, + struct dm_mem_cache_object *object); + +#endif drivers/md/{dm-memcache.c.orig => dm-memcache.c} | 301 ++++++++++++++++++++++ 1 files changed, 301 insertions(+), 0 deletions(-) diff --git a/drivers/md/dm-memcache.c.orig b/drivers/md/dm-memcache.c index e69de29..4e3731c 100644 --- a/drivers/md/dm-memcache.c.orig +++ b/drivers/md/dm-memcache.c @@ -0,0 +1,301 @@ +/* + * Copyright (C) 2006-2008 Red Hat, Inc. All rights reserved. + * + * Module Author: Heinz Mauelshagen <heinzm@redhat.com> + * + * Device-mapper memory object handling: + * + * o allocate/free total_pages in a per client page pool. + * + * o allocate/free memory objects with chunks (1..n) of + * pages_per_chunk pages hanging off. + * + * This file is released under the GPL. + */ + +#define DM_MEM_CACHE_VERSION "0.2" + +#include "dm.h" +#include "dm-memcache.h" +#include <linux/dm-io.h> + +struct dm_mem_cache_client { + spinlock_t lock; + mempool_t *objs_pool; + struct page_list *free_list; + unsigned objects; + unsigned chunks; + unsigned pages_per_chunk; + unsigned free_pages; + unsigned total_pages; +}; + +/* + * Free pages and page_list elements of client. + */ +static void free_cache_pages(struct page_list *list) +{ + while (list) { + struct page_list *pl = list; + + list = pl->next; + BUG_ON(!pl->page); + __free_page(pl->page); + kfree(pl); + } +} + +/* + * Alloc number of pages and page_list elements as required by client. + */ +static struct page_list *alloc_cache_pages(unsigned pages) +{ + struct page_list *pl, *ret = NULL; + struct page *page; + + while (pages--) { + page = alloc_page(GFP_NOIO); + if (!page) + goto err; + + pl = kmalloc(sizeof(*pl), GFP_NOIO); + if (!pl) { + __free_page(page); + goto err; + } + + pl->page = page; + pl->next = ret; + ret = pl; + } + + return ret; + +err: + free_cache_pages(ret); + return NULL; +} + +/* + * Allocate page_list elements from the pool to chunks of the memory object. + */ +static void alloc_chunks(struct dm_mem_cache_client *cl, + struct dm_mem_cache_object *obj) +{ + unsigned chunks = cl->chunks; + unsigned long flags; + + local_irq_save(flags); + local_irq_disable(); + while (chunks--) { + unsigned p = cl->pages_per_chunk; + + obj[chunks].pl = NULL; + + while (p--) { + struct page_list *pl; + + /* Take next element from free list */ + spin_lock(&cl->lock); + pl = cl->free_list; + BUG_ON(!pl); + cl->free_list = pl->next; + spin_unlock(&cl->lock); + + pl->next = obj[chunks].pl; + obj[chunks].pl = pl; + } + } + + local_irq_restore(flags); +} + +/* + * Free page_list elements putting them back onto free list + */ +static void free_chunks(struct dm_mem_cache_client *cl, + struct dm_mem_cache_object *obj) +{ + unsigned chunks = cl->chunks; + unsigned long flags; + struct page_list *next, *pl; + + local_irq_save(flags); + local_irq_disable(); + while (chunks--) { + for (pl = obj[chunks].pl; pl; pl = next) { + next = pl->next; + + spin_lock(&cl->lock); + pl->next = cl->free_list; + cl->free_list = pl; + cl->free_pages++; + spin_unlock(&cl->lock); + } + } + + local_irq_restore(flags); +} + +/* + * Create/destroy dm memory cache client resources. + */ +struct dm_mem_cache_client * +dm_mem_cache_client_create(unsigned objects, unsigned chunks, + unsigned pages_per_chunk) +{ + unsigned total_pages = objects * chunks * pages_per_chunk; + struct dm_mem_cache_client *client; + + BUG_ON(!total_pages); + client = kzalloc(sizeof(*client), GFP_KERNEL); + if (!client) + return ERR_PTR(-ENOMEM); + + client->objs_pool = mempool_create_kmalloc_pool(objects, + chunks * sizeof(struct dm_mem_cache_object)); + if (!client->objs_pool) + goto err; + + client->free_list = alloc_cache_pages(total_pages); + if (!client->free_list) + goto err1; + + spin_lock_init(&client->lock); + client->objects = objects; + client->chunks = chunks; + client->pages_per_chunk = pages_per_chunk; + client->free_pages = client->total_pages = total_pages; + return client; + +err1: + mempool_destroy(client->objs_pool); +err: + kfree(client); + return ERR_PTR(-ENOMEM); +} +EXPORT_SYMBOL(dm_mem_cache_client_create); + +void dm_mem_cache_client_destroy(struct dm_mem_cache_client *cl) +{ + BUG_ON(cl->free_pages != cl->total_pages); + free_cache_pages(cl->free_list); + mempool_destroy(cl->objs_pool); + kfree(cl); +} +EXPORT_SYMBOL(dm_mem_cache_client_destroy); + +/* + * Grow a clients cache by an amount of pages. + * + * Don't call from interrupt context! + */ +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++; + spin_unlock_irq(&cl->lock); + + mempool_resize(cl->objs_pool, cl->objects, GFP_NOIO); + 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--; + last->next = NULL; + } + spin_unlock_irqrestore(&cl->lock, flags); + + if (!r) { + free_cache_pages(pl); + mempool_resize(cl->objs_pool, cl->objects, GFP_NOIO); + } + + 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); + + 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@redhat.com>"); +MODULE_LICENSE("GPL");
This patch adds the dm-memcache memory cache module. Heinz drivers/md/{dm-memcache.h.orig => dm-memcache.h} | 68 ++++++++++++++++++++++ 1 files changed, 68 insertions(+), 0 deletions(-) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel