Messages in this thread | | | From | (Eric W. Biederman) | Date | Sun, 03 Feb 2013 07:24:11 -0800 | Subject | Re: [PATCHSET] idr: implement idr_alloc() and convert existing users |
| |
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
| |