[PATCH] genalloc: add support of named pool

[PATCH] genalloc: add support of named pool

[Jean-Christophe PLAGNIOL-VILLARD]

so we can get the pool in the driver by name instead of passing it via
parameter

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 include/linux/genalloc.h |   19 ++++++++++++++++++-
 lib/genalloc.c           |   32 +++++++++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
index 5e98eeb..38d3efe 100644
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -33,9 +33,12 @@
  *  General purpose special memory pool descriptor.
  */
 struct gen_pool {
+	char *name;			/* pool name */
 	spinlock_t lock;
 	struct list_head chunks;	/* list of chunks in this pool */
 	int min_alloc_order;		/* minimum allocation order */
+
+	struct list_head list;
 };
 
 /*
@@ -50,7 +53,21 @@ struct gen_pool_chunk {
 	unsigned long bits[0];		/* bitmap for allocating memory chunk */
 };
 
-extern struct gen_pool *gen_pool_create(int, int);
+extern struct gen_pool *gen_pool_create_byname(char*, int, int);
+
+/**
+ * gen_pool_create - create a new special memory pool
+ * @min_alloc_order: log base 2 of number of bytes each bitmap bit represents
+ * @nid: node id of the node the pool structure should be allocated on, or -1
+ *
+ * Create a new special memory pool that can be used to manage special purpose
+ * memory not managed by the regular kmalloc/kfree interface.
+ */
+static inline struct gen_pool *gen_pool_create(int min_alloc_order, int nid)
+{
+	return gen_pool_create_byname(NULL, min_alloc_order, nid);
+}
+extern struct gen_pool* gen_pool_byname(char *name);
 extern phys_addr_t gen_pool_virt_to_phys(struct gen_pool *pool, unsigned long);
 extern int gen_pool_add_virt(struct gen_pool *, unsigned long, phys_addr_t,
 			     size_t, int);
diff --git a/lib/genalloc.c b/lib/genalloc.c
index f352cc4..25ec676 100644
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -35,6 +35,8 @@
 #include <linux/interrupt.h>
 #include <linux/genalloc.h>
 
+static LIST_HEAD(pools);
+
 static int set_bits_ll(unsigned long *addr, unsigned long mask_to_set)
 {
 	unsigned long val, nval;
@@ -136,23 +138,50 @@ static int bitmap_clear_ll(unsigned long *map, int start, int nr)
 }
 
 /**
+ * gen_pool_byname - get pool by name
+ * @name: pool name
+ *
+ */
+struct gen_pool* gen_pool_byname(char *name)
+{
+	struct gen_pool *pool;
+
+	if (!name)
+		return NULL;
+
+	list_for_each_entry(pool, &pools, list) {
+		if(pool->name && strcmp(pool->name, name) == 0)
+			return pool;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(gen_pool_byname);
+
+/**
  * gen_pool_create - create a new special memory pool
+ * @name: pool name if NULL you will not be able to get it via gen_pool_byname
  * @min_alloc_order: log base 2 of number of bytes each bitmap bit represents
  * @nid: node id of the node the pool structure should be allocated on, or -1
  *
  * Create a new special memory pool that can be used to manage special purpose
  * memory not managed by the regular kmalloc/kfree interface.
  */
-struct gen_pool *gen_pool_create(int min_alloc_order, int nid)
+struct gen_pool *gen_pool_create_byname(char *name, int min_alloc_order, int nid)
 {
 	struct gen_pool *pool;
 
+	if (gen_pool_byname(name))
+		return NULL;
+
 	pool = kmalloc_node(sizeof(struct gen_pool), GFP_KERNEL, nid);
 	if (pool != NULL) {
 		spin_lock_init(&pool->lock);
 		INIT_LIST_HEAD(&pool->chunks);
 		pool->min_alloc_order = min_alloc_order;
 	}
+
+	list_add_tail(&pool->list, &pools);
 	return pool;
 }
 EXPORT_SYMBOL(gen_pool_create);
@@ -244,6 +273,7 @@ void gen_pool_destroy(struct gen_pool *pool)
 
 		kfree(chunk);
 	}
+	list_del(&pool->list);
 	kfree(pool);
 	return;
 }
-- 
1.7.7

Re: [PATCH] genalloc: add support of named pool

[Andrew Morton]

On Thu,  8 Dec 2011 16:50:08 +0100
Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:

> so we can get the pool in the driver by name instead of passing it via
> parameter

I'm not seeing any code whcih actually uses this interface, so there's
no reason to apply the patch?

The changelog should tell us why you believe this patch is needed: use
case, end-user value, etc.

> --- a/include/linux/genalloc.h
> +++ b/include/linux/genalloc.h
> @@ -33,9 +33,12 @@
>   *  General purpose special memory pool descriptor.
>   */
>  struct gen_pool {
> +	char *name;			/* pool name */
>  	spinlock_t lock;
>  	struct list_head chunks;	/* list of chunks in this pool */
>  	int min_alloc_order;		/* minimum allocation order */
> +
> +	struct list_head list;

The other fields have nice descriptive comments - why shouldn't this one?

> +static LIST_HEAD(pools);

Except for certain special circumstances (which I don't think apply
here), a global list needs a global lock to protect it.

> +
>  static int set_bits_ll(unsigned long *addr, unsigned long mask_to_set)
>  {
>  	unsigned long val, nval;
> @@ -136,23 +138,50 @@ static int bitmap_clear_ll(unsigned long *map, int start, int nr)
>  }
>  
>  /**
> + * gen_pool_byname - get pool by name
> + * @name: pool name
> + *

Remove that empty comment line.

> + */
> +struct gen_pool* gen_pool_byname(char *name)

The patch has several minor coding-style errors which can be detected
by scripts/checkpatch.pl.

> +{
> +	struct gen_pool *pool;
> +
> +	if (!name)
> +		return NULL;
> +
> +	list_for_each_entry(pool, &pools, list) {
> +		if(pool->name && strcmp(pool->name, name) == 0)
> +			return pool;
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(gen_pool_byname);

This function is racy and cannot be fixed.  If some other process comes
along and deletes *pool immediately after this function has completed,
the caller of this function is left holding a dead pointer.

Re: [PATCH] genalloc: add support of named pool

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1132 bytes --]

Hi,

On Thu,  8 Dec 2011 16:50:08 +0100 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com> wrote:
>
>  struct gen_pool {
> +	char *name;			/* pool name */

Any reason that this can't be "const char *"?

> +extern struct gen_pool *gen_pool_create_byname(char*, int, int);

const char *

> +extern struct gen_pool* gen_pool_byname(char *name);


const char *

> +/**
>   * gen_pool_create - create a new special memory pool

gen_pool_create_byname

> +struct gen_pool *gen_pool_create_byname(char *name, int min_alloc_order, int nid)

const char *

>  {
>  	struct gen_pool *pool;
>  
> +	if (gen_pool_byname(name))
> +		return NULL;
> +
>  	pool = kmalloc_node(sizeof(struct gen_pool), GFP_KERNEL, nid);
>  	if (pool != NULL) {
>  		spin_lock_init(&pool->lock);
>  		INIT_LIST_HEAD(&pool->chunks);
>  		pool->min_alloc_order = min_alloc_order;
>  	}
> +
> +	list_add_tail(&pool->list, &pools);
>  	return pool;
>  }

I don't see where you assign pool->name to anything ...

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]