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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOS58YPhSz4DQT5-hTkH-wW_P3b5LK1WuEbWiR8uyNZPZdf_Gw@mail.gmail.com>
Date:	Sun, 3 Feb 2013 06:13:53 -0800
From:	Tejun Heo <tj@...nel.org>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>
Cc:	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
	rusty@...tcorp.com.au, bfields@...ldses.org,
	skinsbursky@...allels.com, jmorris@...ei.org, axboe@...nel.dk
Subject: Re: [PATCHSET] idr: implement idr_alloc() and convert existing users

Hey, Eric.

On Sun, Feb 3, 2013 at 5:41 AM, Eric W. Biederman <ebiederm@...ssion.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/