lkml.org 
[lkml]   [2008]   [May]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    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    [W:0.024 / U:61.372 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site