lkml.org 
[lkml]   [2008]   [May]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 02/10] Introduce the ridr structure
On Tue 29 Apr at 16:33:06 +0200 Nadia.Derbey@bull.net said:

> Index: linux-2.6.25-mm1/include/linux/ridr.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.25-mm1/include/linux/ridr.h 2008-04-29 13:08:00.000000000 +0200
> @@ -0,0 +1,50 @@
> +/*
> + * include/linux/ridr.h
> + *
> + * Small id to pointer translation service avoiding fixed sized
> + * tables. RCU-based implmentation of IDRs.
> + */
^^^^^^^
s/implmentation/reimplementation

Additional comment here might be nice here since this amounts to a mostly
copy of idr.h and its backing implementation, with all the associated
maintenance headaches if both are in tree.

> +/*
> + * The idr_layer part should stay at the beginning of the structure
> + */
> +struct ridr_layer {
> + struct idr_layer idr;
> + struct rcu_head rcu_head;
> +};

Why does it have to be at the beginning? So an ridr_layer can be used as
an idr_layer? This doesn't help review. As previous commenters noted,
review would be a lot easier if this was incremental on idr.

> Index: linux-2.6.25-mm1/lib/Makefile
> ===================================================================
> --- linux-2.6.25-mm1.orig/lib/Makefile 2008-04-25 15:29:00.000000000 +0200
> +++ linux-2.6.25-mm1/lib/Makefile 2008-04-25 15:57:39.000000000 +0200
> @@ -6,7 +6,7 @@ lib-y := ctype.o string.o vsprintf.o cmd
> rbtree.o radix-tree.o dump_stack.o \
> idr.o int_sqrt.o extable.o prio_tree.o \
> sha1.o irq_regs.o reciprocal_div.o argv_split.o \
> - proportions.o prio_heap.o ratelimit.o
> + proportions.o prio_heap.o ratelimit.o ridr.o

I vaguely recall past discussion of preferred insertion point, but
lacking something in CodingStyle and especially given the redundancy
here it could be nice to add ridr.o next to idr.o.

> ===================================================================
> --- linux-2.6.25-mm1.orig/lib/idr.c 2008-04-25 17:22:43.000000000 +0200
> +++ linux-2.6.25-mm1/lib/idr.c 2008-04-29 13:08:05.000000000 +0200
> @@ -342,7 +342,7 @@ static void sub_remove(struct idr *idp,
> while ((shift > 0) && p) {
> n = (id >> shift) & IDR_MASK;
> __clear_bit(n, &p->bitmap);
> - *++paa = &p->ary[n];
> + *++paa = (struct idr_layer **) &p->ary[n];
> p = p->ary[n];
> shift -= IDR_BITS;
> }

For shifty use of an ridr_layer as an idr_layer?

> Index: linux-2.6.25-mm1/include/linux/idr.h
> ===================================================================
> --- linux-2.6.25-mm1.orig/include/linux/idr.h 2008-04-25 17:22:43.000000000 +0200
> +++ linux-2.6.25-mm1/include/linux/idr.h 2008-04-29 13:08:00.000000000 +0200
> @@ -48,9 +48,9 @@
> #define IDR_FREE_MAX MAX_LEVEL + MAX_LEVEL
>
> struct idr_layer {
> - unsigned long bitmap; /* A zero bit means "space here" */
> - struct idr_layer *ary[1<<IDR_BITS];
> - int count; /* When zero, we can release it */
> + unsigned long bitmap; /* A zero bit means "space here" */
> + void *ary[1<<IDR_BITS];
> + int count; /* When zero, we can release it */
> };
>
> struct idr {
>

This hunk is white space only...

--
Tim Pepper <lnxninja@linux.vnet.ibm.com>
IBM Linux Technology Center


\
 
 \ /
  Last update: 2008-05-01 06:33    [from the cache]
©2003-2011 Jasper Spaans