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

[Nadia Derbey <Nadia.Derbey@bull.net>]

Tim Pepper wrote:
> On Wed 07 May at 13:36:00 +0200 Nadia.Derbey@bull.net 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@bull.net>
>>
>>---
>> 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