Message-ID: <5602E957.2060409@cumulusnetworks.com>
Date:	Wed, 23 Sep 2015 12:03:03 -0600
From:	David Ahern <dsa@...ulusnetworks.com>
To:	Alexander Duyck <alexander.duyck@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/9] net: Remove e_inval label from
 ip_route_input_slow

On 9/23/15 11:38 AM, Alexander Duyck wrote:
> On 09/23/2015 10:13 AM, David Ahern wrote:
>> On 9/23/15 10:31 AM, Alexander Duyck wrote:
>>>
>>> Just as you said, that code would be an intermediate step. Going though
>>> and adding more points where you are updating err and just exchanging
>>> one jump label for another doesn't help anything.  You are better off
>>> pulling apart the spaghetti right from the start and then rearranging
>>> the code.  If nothing else it helps to make things more readable.
>>>
>>> In a couple of patches from here you are going to have to pull out the
>>> local_input helper.  Rather than adding a new jump label inside of it
>>> for out you could save yourself a few steps and just return the error
>>> values.  If you do this correctly what you should end up with is a
>>> series of functions that all converge on one end point anyway.
>>>
>>> Also as far as the multiple returns issue it isn't much of a problem
>>> since ip_output_input_slow ends up being compiled into
>>> ip_route_input_noref anyway.  As such the return statements end up just
>>> being jumps to the bits for the rcu_read_unlock and returning the error
>>> value.
>>
>> I chose this series of steps because it is easy to follow each change
>> to ensure I do not introduce bugs with the patches. Small, focused
>> changes to evolve the code.
>
> Some of the steps just seem like they are busy work and doing them don't
> really add much of anything.

They add a lot of value. They make each change very easy to follow. No 
one is going to pickup a single patch in this series and backport to 
some kernel. Each is dependent on the one before it. Given that I would 
rather waste a few steps and ensure I arrive at the destination without 
error.

>
>> The first 3 patches appear to have *zero* impact on what the compiler
>> generates.
>
> That is kind of my point.  Why mess with something if it has zero
> impact.  At least by just adding the returns we actually gain
> something.  From what I can tell it looks like just going through
> ip_route_input_mc, __mkroute_input, and ip_route_input_slow and simply
> replacing all the spots where we are assigning err, and jumping to
> something that returns it with just a return err I save roughly 80 bytes
> worth of size.

If you have a better suggestion for how to improve this code then please 
submit it. I don't have a religion here. I don't like making mistakes 
and the silly mistake that led to bde6f9ded1 motivated this patch series.

>
>> Do you object to the end result of this patch series? ie. do you have
>> concerns about what the end code looks like?
>
> The biggest thing that caught my eye is the fact that the end result is
> larger than the original.  That tells me something went wrong in your
> process as there shouldn't be any reason for the code footprint to
> actually grow.  If for example moving things into functions has caused
> this then maybe we need to rethink the approach.

It's the last 2 patches that introduce the size change. Top line is 
without IP_VERBOSE; bottom line is with it:

1e03ff19c413 net: Remove no_route label
     19847          2321      32   22200    56b8 kbuild/net/ipv4/route.o
     20711          2321      32   23064    5a18 kbuild/net/ipv4/route.o

3772b7bbac8b net: Remove local_input label
     19781          2321      32   22134    5676 kbuild/net/ipv4/route.o
     20666          2321      32   23019    59eb kbuild/net/ipv4/route.o

cac02ff199fa net: Remove martian_destination label
     19756          2321      32   22109    565d kbuild/net/ipv4/route.o
     20626          2321      32   22979    59c3 kbuild/net/ipv4/route.o

071993ac0735 net: Remove martian_source goto
     19758          2321      32   22111    565f kbuild/net/ipv4/route.o
     20618          2321      32   22971    59bb kbuild/net/ipv4/route.o

6af3805a299e net: Move martian_destination to helper
     19778          2321      32   22131    5673 kbuild/net/ipv4/route.o
     20616          2321      32   22969    59b9 kbuild/net/ipv4/route.o

d56e8f30af4d net: Move rth handling from ip_route_input_slow to helper
     19778          2321      32   22131    5673 kbuild/net/ipv4/route.o
     20605          2321      32   22958    59ae kbuild/net/ipv4/route.o

139df871ec82 net: Remove e_nobufs label from ip_route_input_slow
     19778          2321      32   22131    5673 kbuild/net/ipv4/route.o
     20615          2321      32   22968    59b8 kbuild/net/ipv4/route.o

6daa3b43063b net: Remove e_inval label from ip_route_input_slow
     19778          2321      32   22131    5673 kbuild/net/ipv4/route.o
     20615          2321      32   22968    59b8 kbuild/net/ipv4/route.o

928edcca02e5 net: Remove martian_source_keep_err goto label
     19778          2321      32   22131    5673 kbuild/net/ipv4/route.o
     20615          2321      32   22968    59b8 kbuild/net/ipv4/route.o

227b9e8708b1 usbnet: remove invalid check
     19778          2321      32   22131    5673 kbuild/net/ipv4/route.o
     20615          2321      32   22968    59b8 kbuild/net/ipv4/route.o

This last one is the baseline.

>
> Also it doesn't really feel like you reduce the use of goto statements
> at all.  All you did is reduce the number of labels, but everything is
> still jumping to "out" all over the place.  It just seems kind of silly
> since the compiler will likely take care of that for us anyway since it
> will inline ip_route_input slow into ip_route_input_noref which means
> all of the returns will just end up dumping us out just before the
> rcu_read_unlock in that function.

I am not trying to reduce goto's. I am trying to avoid hops all over the 
place which lead to confusing logic.

David
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists