Re: [PATCH 7/9] Make idr_remove rcu-safe

[Tim Pepper]

On Wed 07 May at 13:36:00 +0200 Nadia.Derbey@xxxxxxxx said:
> [PATCH 07/09]
> 
> This patch introduces the free_layer() routine: it is the one that actually
> frees memory after a grace period has elapsed.
> 
> Signed-off-by: Nadia Derbey <Nadia.Derbey@xxxxxxxx>
> 
> ---
>  lib/idr.c |   59 ++++++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 44 insertions(+), 15 deletions(-)
> 
> Index: linux-2.6.25-mm1/lib/idr.c
> ===================================================================
> --- linux-2.6.25-mm1.orig/lib/idr.c	2008-05-06 18:06:43.000000000 +0200
> +++ linux-2.6.25-mm1/lib/idr.c	2008-05-07 09:07:31.000000000 +0200
> @@ -424,15 +455,13 @@ void idr_remove_all(struct idr *idp)
> 
>  		id += 1 << n;
>  		while (n < fls(id)) {
> -			if (p) {
> -				memset(p, 0, sizeof *p);
> -				move_to_free_list(idp, p);
> -			}
> +			if (p)
> +				free_layer(p);
>  			n += IDR_BITS;
>  			p = *--paa;
>  		}
>  	}
> -	idp->top = NULL;
> +	rcu_assign_pointer(idp->top, NULL);
>  	idp->layers = 0;
>  }
>  EXPORT_SYMBOL(idr_remove_all);

Does idr_remove_all() need an rcu_dereference() in the loop preceeding the
above, where it does:

                while (n > IDR_BITS && p) {
                        n -= IDR_BITS;
                        *paa++ = p;
     ---->              p = p->ary[(id >> n) & IDR_MASK];
                }

idr_replace() also has that loop without rcu_derefernce, but I _think_
I see why that one should be ok.  At least there the comment is clear
that locking at a higher level should be happening.  And then
idr_remove_all() is almost unused and it looks like it is only in
serialized places.

Otherwise, thanks for redoing...This patch set was much easier to digest
and looks reasonable to me.

I've been having some machine issues, but hope to give this patch set a run
still on a 128way machine and mainline to provide some additional
datapoints.

-- 
Tim Pepper  <lnxninja@xxxxxxxxxxxxxxxxxx>
IBM Linux Technology Center
--
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/