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

[Tejun Heo]

Hey,

On Sun, Feb 03, 2013 at 07:24:11AM -0800, Eric W. Biederman wrote:
> 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.

People use it as a generic indexing thing.  It gets even used for per
storage command or per network command.  And really think about what?
Preloading disables preemption.  That's the only interface detail that
affects anything.

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

So, is it that you are bothered that idr_preload() should be paired
with _end()?  I mean, it's clearly described so in the interface and
you would even get a nice preemption level mismatch warning pretty
quickly if you miss one unlike when you forget to handle -EAGAIN.

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

And do that for what?  It's less convenient and you're gonna have to
preload it fully every single time and then destroy it.  Why do that?

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

Really, *you* need to lower your cognitive overhead on percpu
operations.  This is fairly basic stuff.  I mean, if this is too hard
or whatever, how do you even use RCU?

> Please sit down and relook at your interface again.  People use idr
> where they need a unique number associatedand they really don't care.

The above is simply untrue.  We already have use cases where idr is
heavily depended upon in hot paths.

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

I can't think of why but if this is somehow cognitively difficult for
someone, just dfon't think about it and use the interface as
described.  That's the whole thing about having an interface, right?

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

How the hell is it harder to use correctly?  Before, you could
theoretically get silent allocation failure very rarely without any
indication.  Now, you don't need silly looping and if you forget
preload_end(), you wlil get a preemption warning very quickly.

> 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, I don't know.  To me, you seem way out of touch on the subject.
It really is a simple thing and percpu is something widely used in the
kernel at this point.  You just need to get used to it like we all
needed to get used to with the idea of finer grained locking way back
and RCU recently.

So, if you can propose an interface which is technically better,
please do so; otherwise, I don't really think everyone should end up
with a worse interface for the supposed congitive overhead that you
have for some reason.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/