lkml.org 
[lkml]   [2008]   [May]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 7/9] Make idr_remove rcu-safe
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



\
 
 \ /
  Last update: 2008-05-15 09:43    [W:0.138 / U:0.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site