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

[Andrew Morton]

On Thu,  8 Dec 2011 16:50:08 +0100
Jean-Christophe PLAGNIOL-VILLARD <plagnioj@xxxxxxxxxxxx> 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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/