Re: [PATCH 00/13] Re: Scalability requirements for sysv ipc

[Nadia Derbey]

Paul E. McKenney wrote:
> On Mon, Apr 14, 2008 at 10:33:23AM +0200, Nadia Derbey wrote:
>
> > Peter Zijlstra wrote:
> >
> > > On Mon, 2008-04-14 at 07:18 +0200, Nadia Derbey wrote:
> > >
> > >
> > > > Peter Zijlstra wrote:
> > > >
> > > >
> > > > > On Fri, 2008-04-11 at 18:17 +0200, Nadia.Derbey@xxxxxxxx wrote:
> > > > >
> > > > >
> > > > >
> > > > > > Here is finally the ipc ridr-based implementation I was talking about 
> > > > > > last
> > > > > > week (see http://lkml.org/lkml/2008/4/4/208).
> > > > > > I couldn't avoid much of the code duplication, but at least made things
> > > > > > incremental.
> > > > > >
> > > > > > Does somebody now a test suite that exists for the idr API, that I could
> > > > > > run on this new api?
> > > > > >
> > > > > > Mike, can you try to run it on your victim: I had such a hard time 
> > > > > > building
> > > > > > this patch, that I couldn't re-run the test on my 8-core with this new
> > > > > > version. So the last results I have are for 2.6.25-rc3-mm1.
> > > > > >
> > > > > > Also, I think a careful review should be done to avoid introducing yet 
> > > > > > other
> > > > > > problems :-(
> > > > >
> > > > >
> > > > > Why duplicate the whole thing, when we converted the Radix tree to be
> > > > > RCU safe we did it in-place. Is there a reason this is not done for idr?
> > > >
> > > > I did that because I wanted to go fast and try to fix the performance 
> > > > problem we have with sysV ipc's. I didn't want to introduce (yet other) 
> > > > regressions in the code that uses idr's today and that works well ;-)
> > > > May be in the future if this rcu based api appears to be ok, we can 
> > > > replace one with the other?
> > >
> > >
> > > > From what I can see the API doesn't change at all,
> >
> > Well, 1 interface changes, 1 is added and another one went away:
> >
> > 1) for the preload part (it becomes like the radix-tree preload part):
> >
> > int idr_pre_get(struct idr *, gfp_t);
> > would become
> > int idr_pre_get(gfp_t);
> >
> > 2) idr_pre_get_end() is added (same as radix_tree_preload_end()).
> >
> > 3) The idr_init() disappears.
> >
> > You might see that other interfaces are not provided by ridr, but this 
> > is only because I've taken those that are useful for the ipc part (so 
> > should not be a problem to make the whole thing rcu safe).
>
>
> Part of this is because you need to allow the caller to choose the
> locking for updates.  Mightn't it be better to have both styles of
> API, and share the bit-twiddling and tree-walking code?

That's what I wanted to get to. But it is very hard to do code 
factorization since
1. the routines use pointers to different structures and access to these 
piinters can be anywhere in the routines.
2. we may have rcu assignment instead of direct pointer assignements 
anywhere in these routines.

In a first try, I finally ended up with huuuuge macros that wouldn't 
have been accepted (I attached one of the patches if interested).

Regards,
Nadia

Attachment: ridr_get_new.patch.bz2
Description: application/redhat-package-manager