LWN.net Logo

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

From:  ebiederm-AT-xmission.com (Eric W. Biederman)
To:  Tejun Heo <tj-AT-kernel.org>
Subject:  Re: [PATCHSET] idr: implement idr_alloc() and convert existing users
Date:  Sun, 03 Feb 2013 05:41:16 -0800
Message-ID:  <87a9rltszn.fsf@xmission.com>
Cc:  akpm-AT-linux-foundation.org, linux-kernel-AT-vger.kernel.org, rusty-AT-rustcorp.com.au, bfields-AT-fieldses.org, skinsbursky-AT-parallels.com, jmorris-AT-namei.org, axboe-AT-kernel.dk
Archive-link:  Article, Thread

Tejun Heo <tj@kernel.org> writes:

> Hello,

So my first response after looking at the ipc patch is ick.

Why the deep percpu magic?
Why don't associate idr_preload with an idr structure.

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

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

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.

> 	idr_preload(GFP_KERNEL);
> 	spin_lock(lock);
>
> 	id = idr_alloc(idr, ptr, lower_limit, upper_limit, GFP_NOWAIT);
>
> 	spin_unlock(lock);
> 	idr_preload_end();
> 	if (id < 0)
> 		return id;

Eric


(Log in to post comments)

Copyright © 2013, Eklektix, Inc.
Comments and public postings are copyrighted by their creators.
Linux is a registered trademark of Linus Torvalds