netdev - Re: [patch net-next 0/3] net/sched: Improve getting objects by indexes

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]

Date:   Wed, 16 Aug 2017 11:41:11 +0200
From:   Christian König <christian.koenig@....com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     Chris Mi <chrism@...lanox.com>, netdev@...r.kernel.org
Subject: Re: [patch net-next 0/3] net/sched: Improve getting objects by
 indexes

Am 16.08.2017 um 11:31 schrieb Jiri Pirko:
> [SNIP]
> I don't. It is an API change, maintainers of the individual drivers are
> not expected to review the patches like this.

Yeah, completely agree.

>> If yes then it somehow makes sense to send the patch bit by bit, if no then
>> it doesn't seem to make to much sense to CC them all individually.
>>
>>>>>> I've never read the bsg code before, but that's certainly not correct. And
>>>>>> that incorrect pattern repeats over and over again in this code.
>>>>>>
>>>>>> Apart from that why the heck do you want to allocate more than 1<<31 handles?
>>>>> tc action indexes for example. That is part of this patchset.
>>>> Well, let me refine the question: Why does tc action indexes need more than
>>>> 31 bits? From an outside view that looks like pure overkill.
>>> That is current state, uapi. We have to live with it.
>> Is the range to allocate from part of the uapi or what is the issue here?
> Yes.

A bit strange uapi design, but ok in this case that change actually 
makes sense.

>> If the issue is that userspace can specify the handle then I suggest that you
>> use the radix tree directly instead of the idr wrapper around it.
> But why? idr is exactly the tool we need. Only signed int does not suit
> us. In fact, it does not make sense idr is using signed int when it
> uses radix tree with unsigned long under the hood.

Well it always depends on what you do and how to use it.

In amdgpu for example for have very very short lived objects and only 
few of them are active at the same time.

The solution was not to use and idr, but rather 64bit identifiers and a 
ring buffer with the last 128 entries.

But in your case changing the idr calling convention actually makes 
sense (at least from the tn mile high view), feel free to add an 
Acked-by: Christian König <christian.koenig@....com> on it.

Regards,
Christian.