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

[ebiederm@xmission.com (Eric W. Biederman)]

Tejun Heo <tj@kernel.org> writes:

> 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.

Maybe.  I haven't been in the parts of the kernel that use the radix
tree preloading and I having just look at a bit of it I don't like it
either.  There is the same strong disconnect between the pieces whose
side effects are interacting.

At least with the radix tree the pieces are core kernel pieces that get
optimized like mad and you expect to have to think about.

idr is a boring thing you use because don't want to think about the
details.  Now you are asking people to really think about the details
when the touch a piece of code using idr.  I think that is the wrong
trade off.

>> 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.

That argument makes sense.

>> 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.

But this isn't a basic percpu technique.  This is a basic percpu
technique hidden behind the wall.

Nothing about reading idr_preload() in a line of code says.  Hey look at
me I am version of preempt disable() that puts things in percpu
variables that you don't know about.

Nothing about idr_preload says don't you dair forget to pair me with
preload_end().

>> 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.

This suggestion you didn't address at all.

If we can't put state in the structure because of the problem of
multiple accesses why can't we take the state out of the structure
entirely and put it into a stack local variable?

That would reduce the cognitive load because it becomes clear how
the pieces connect together.

That would remove the need for disabling preemption and other behind
the scenes magic.

>> 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.

Please sit down and relook at your interface again.  People use idr
where they need a unique number associatedand they really don't care.
Having to think about hidden percpu variables and preemption being
disabled in the background brings with it a lot of extra work to use the
interface.

For the sake of a saving few lines of code you are desiging an interface
that is harder to use correctly, and harder to think about.

I think you are making completely the wrong trade off.  I don't want the
cognitive burden of having to think about percpu and preemption
disabling when using a boring utitlity function like the idr code.

Eric