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

Wed, Aug 16, 2017 at 10:55:56AM CEST, christian.koenig@....com wrote:
>Am 16.08.2017 um 10:39 schrieb Jiri Pirko:
>> Wed, Aug 16, 2017 at 10:31:35AM CEST, christian.koenig@....com wrote:
>> > 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.
>> Understood. I think is is good to avoid having some "idr_alloc2". That
>> is why I suggested to do this in one go, to avoid "idr_alloc2" and then
>> patch to rename "idr_alloc2" to "idr_alloc" once nobody uses the original
>> "idr_alloc". In fact, if you do it driver, by driver, the review burden
>> would be the same, probably even bigger, you'll just have 100+ patches.
>> Why would it help?
>
>Because it would give each maintainer only the part of the change he is
>interested in.
>
>Current status of this change is that you send a mail with nearly 300 people
>on CC.

That was a mistake to cc all.


>
>Do you really expect to get an reviewed-by or acked-by on this single patch
>from all of them?

I don't. It is an API change, maintainers of the individual drivers are
not expected to review the patches like this.


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

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



>
>Regards,
>Christian.