Re: [PATCHSET] idr: implement idr_alloc() and convert existing users

[Tejun Heo <tj@kernel.org>]

Hey, Eric.

On Sun, Feb 3, 2013 at 5:41 AM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> Why the deep percpu magic?

Eh? What's magical about it?  You preload percpu buffer if you're
gonna be allocating an id from context which doesn't allow permissive
allocation.  This is the same technique used by radix tree preloading.
 The only reason idr did its own preloading was probably because it
got implemented before we had proper percpu techniques.

> Why don't associate idr_preload with an idr structure.

Because then you end up with a lot more preloaded buffers which are
much less useful.  You can't guarantee your preload target is the only
one who's gonna use the preload buffer so you end up with this ugly
-EAGAIN retry loop.  It's inefficient & inconvenient.

> When reading code with idr_preload I get this deep down creepy feeling.
> What is this magic that is going on?

Seriously, if this gives you deep creepy feeling, you need to get on
with time.  It's a basic percpu technique.

> Can't we just put the preload list_head into struct idr make
> idr_preload and idr_preload_end take an idr argument?

Inefficient & inconvenient.

> Maybe we can have a special structure we put on the stack that has
> the list_head and the preload state instead.
>
> The way this works just weirds me out and I really really don't like it.
>
> I would rather continue to use the existing functions as problematic as
> they are as I don't need a course in deep magic to make sense of them.

Heh, this is the weirdest review I got in quite a while.  Please sit
down and read it again.

Thanks.

-- 
tejun