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

[Nadia Derbey]

Tim Pepper wrote:
> 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];
>                 }

I assumed here that idr_remove_all() was called in the "typical cleanup 
sequence" mentioned in the comment describing the routine.
And actually, that's how it is called in drivers/char/drm.

> 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.

I didn't use rcu_dereference here since the caller should anyway 
serialize with other writers. So the tree should remain unchanged during 
the replace operation.

>  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.

That would be kind, indeed (hope I didn't break anything).

Regards,
Nadia

--
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/