Messages in this thread | | | Date | Thu, 15 May 2008 09:40:13 +0200 | From | Nadia Derbey <> | Subject | Re: [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
| |