linux-kernel - Re: [PATCH 6/8] idr: avoid ping-pong

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: <CACvQF53zXjTRgL_uHxp=J04efqBuD6L7VbCOpRvSyxQRiBoA_w@mail.gmail.com>
Date:	Sat, 19 Apr 2014 22:23:58 +0800
From:	Lai Jiangshan <laijs@...fujitsu.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Jean Delvare <jdelvare@...e.de>,
	Monam Agarwal <monamagarwal123@...il.com>,
	Jeff Layton <jlayton@...hat.com>,
	Andreas Gruenbacher <agruen@...bit.com>,
	Stephen Hemminger <stephen@...workplumber.org>
Subject: Re: [PATCH 6/8] idr: avoid ping-pong

On Sat, Apr 19, 2014 at 9:01 PM, Tejun Heo <tj@...nel.org> wrote:
> Hello,
>
> On Sat, Apr 19, 2014 at 06:43:41PM +0800, Lai Jiangshan wrote:
>> On 04/19/2014 01:17 AM, Tejun Heo wrote:
>> It only frees one layer. And the ida_pre_get() for the next ida_get_new*()
>> will allocation it back again. The aim "Throw away extra resources one by one"
>> can't be achieved. It can't keep its memory footprint minimal.
>
> Isn't the point not keeping the memory around per-ida between
> allocations which can be arbitrarily long?  Why isn't it achieved?

"Throw away one layer" is achieved.
"one by one" can't be achieved.

>
>> > I think the right
>> > thing to do is implementing ida_preload() which is simliar to
>> > idr_preload() and do away with per-ida layer cache.
>>
>> Yes and no.

Yes: I agree to add ida_preload().
No: Because I think we don't need to expose ida_preload().

>>
>> We need a static private ida_preload() for ida_simple_get() only.
>>
>> Because the IDA doesn't have any query-function, so IDA's own synchronization
>> is enough for all use cases, IDA should off-loads the caller's
>> synchronization burden.
>
> Hmmm?  And why can't that use preload on its own?

We need to use  preload  for ida_simple_get().
But we don't need to expose the ida_preload() if we kill the
ida_get_new*() APIs.
(The reason why we can kill the ida_get_new*() is explained below.)

>
>> In my todo-list, IDA only needs the following functions. other functions
>> will be deprecated and scheduled to be removed:
>>       void ida_destroy(struct ida *ida);
>>       void ida_init(struct ida *ida);
>>       int ida_simple_get(struct ida *ida, unsigned int start, unsigned int end,
>>                          gfp_t gfp_mask);
>>       void ida_simple_remove(struct ida *ida, unsigned int id);
>>
>> (I don't think we need any query-function, But...)
>> If in the future we need some query-functions such as:
>>       ida_is_this_id_allocated()
>>       ida_find_next_[un]allocated_id(),
>> In this case, we can expose the ida_preload() and ida_alloc() at the same time that
>> we introduce the query-functions.
>
> Regardless of interface, we can still switch to per-cpu layer caching
> which would make this per-ida allocation issue go away.

agree.

>  I'd much
> prefer to see that than debating the (dis)merits of this patch.

agree.

Please drop this patch.
It will be better if we will cleanup it(and all free list related code)
after ida_preload() is added.

Thanks
Lai

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