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]

Message-ID: <e3cb8f8e-7808-48b4-d90d-67f30ab39687@amd.com>
Date:   Wed, 16 Aug 2017 10:31:35 +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 10:16 schrieb Jiri Pirko:
> Wed, Aug 16, 2017 at 09:49:07AM CEST, christian.koenig@....com wrote:
>> Am 16.08.2017 um 04:12 schrieb Chris Mi:
>>> Using current TC code, it is very slow to insert a lot of rules.
>>>
>>> In order to improve the rules update rate in TC,
>>> we introduced the following two changes:
>>>           1) changed cls_flower to use IDR to manage the filters.
>>>           2) changed all act_xxx modules to use IDR instead of
>>>              a small hash table
>>>
>>> But IDR has a limitation that it uses int. TC handle uses u32.
>>> To make sure there is no regression, we also changed IDR to use
>>> unsigned long. All clients of IDR are changed to use new IDR API.
>> WOW, wait a second. The idr change is touching a lot of drivers and to be
>> honest doesn't looks correct at all.
>>
>> Just look at the first chunk of your modification:
>>> @@ -998,8 +999,9 @@ int bsg_register_queue(struct request_queue *q, struct device *parent,
>>>    	mutex_lock(&bsg_mutex);
>>> -	ret = idr_alloc(&bsg_minor_idr, bcd, 0, BSG_MAX_DEVS, GFP_KERNEL);
>>> -	if (ret < 0) {
>>> +	ret = idr_alloc(&bsg_minor_idr, bcd, &idr_index, 0, BSG_MAX_DEVS,
>>> +			GFP_KERNEL);
>>> +	if (ret) {
>>>    		if (ret == -ENOSPC) {
>>>    			printk(KERN_ERR "bsg: too many bsg devices\n");
>>>    			ret = -EINVAL;
>> The condition "if (ret)" will now always be true after the first allocation
>> and so we always run into the error handling after that.
> On success, idr_alloc returns 0.

Ah, I see. You change the idr_alloc to return the resulting index as 
separate parameter.

You should explicit note that in the commit message, cause that is 
something easily overlooked.

In general I strongly suggest to add a separate interface for allocating 
unsigned long handles, use that for the while being and then move the 
existing drivers over bit by bit.

A single patch which touches so many different driver is practically 
impossible to review consequently.

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

Regards,
Christian.