[PATCH net-next 0/9 v2] net: Refactor ip_route_input_slow

[PATCH net-next 0/9 v2] net: Refactor ip_route_input_slow

[David Ahern]

ip_route_input_slow is a maze of gotos (9 of them!) making it error
prone and difficult to read. This patchset refactors it, removing all
but 2 of the labels. The brd_input label for broadcast path requires
too many inputs to make a reasonble helper out of it so I left it as is.

None of these patches change functionality, only move code around
to make ip_route_input_slow more readable.

Size comparison using gcc version 4.7.2 (Debian 4.7.2-5)

With CONFIG_IP_ROUTE_VERBOSE:
    Before patches:
    $ size kbuild2/net/ipv4/route.o
       text	   data	    bss	    dec	    hex	filename
      20615	   2321	     32	  22968	   59b8	kbuild2/net/ipv4/route.o

    After patches:
    $ size kbuild/net/ipv4/route.o
       text	   data	    bss	    dec	    hex	filename
      20711	   2321	     32	  23064	   5a18	kbuild/net/ipv4/route.o

An increase of 96 bytes with CONFIG_IP_ROUTE_VERBOSE.

Without CONFIG_IP_ROUTE_VERBOSE:
    Before patches:
    $ size kbuild2/net/ipv4/route.o
       text	   data	    bss	    dec	    hex	filename
      19778	   2321	     32	  22131	   5673	kbuild2/net/ipv4/route.o

    After patches:
    $ size kbuild/net/ipv4/route.o
       text	   data	    bss	    dec	    hex	filename
      19847	   2321	     32	  22200	   56b8	kbuild/net/ipv4/route.o

An increase of 69 bytes without CONFIG_IP_ROUTE_VERBOSE.

v2
- Addressed comments from Eric B and Alex Duyck

David Ahern (9):
  net: Remove martian_source_keep_err goto label
  net: Remove e_inval label from ip_route_input_slow
  net: Remove e_nobufs label from ip_route_input_slow
  net: Move rth handling from ip_route_input_slow to helper
  net: Move martian_destination to helper
  net: Remove martian_source goto
  net: Remove martian_destination label
  net: Remove local_input label
  net: Remove no_route label

 net/ipv4/route.c | 233 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 133 insertions(+), 100 deletions(-)

-- 
1.9.1

[PATCH net-next 1/9] net: Remove martian_source_keep_err goto label

[David Ahern]

err is initialized to -EINVAL when it is declared. It is not reset until
fib_lookup which is well after the 3 users of the martian_source jump. So
resetting err to -EINVAL at martian_source label is not needed.

Removing that line obviates the need for the martian_source_keep_err label
so delete it.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/ipv4/route.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 80f7c5b7b832..ef36dfed24da 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1760,7 +1760,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 		err = fib_validate_source(skb, saddr, daddr, tos,
 					  0, dev, in_dev, &itag);
 		if (err < 0)
-			goto martian_source_keep_err;
+			goto martian_source;
 		goto local_input;
 	}
 
@@ -1782,7 +1782,7 @@ out:	return err;
 		err = fib_validate_source(skb, saddr, 0, tos, 0, dev,
 					  in_dev, &itag);
 		if (err < 0)
-			goto martian_source_keep_err;
+			goto martian_source;
 	}
 	flags |= RTCF_BROADCAST;
 	res.type = RTN_BROADCAST;
@@ -1858,8 +1858,6 @@ out:	return err;
 	goto out;
 
 martian_source:
-	err = -EINVAL;
-martian_source_keep_err:
 	ip_handle_martian_source(dev, in_dev, skb, daddr, saddr);
 	goto out;
 }
-- 
1.9.1

Re: [PATCH net-next 1/9] net: Remove martian_source_keep_err goto label

[Tom Herbert]

On Wed, Sep 23, 2015 at 8:15 AM, David Ahern <dsa@cumulusnetworks.com> wrote:
> err is initialized to -EINVAL when it is declared. It is not reset until
> fib_lookup which is well after the 3 users of the martian_source jump. So
> resetting err to -EINVAL at martian_source label is not needed.
>
> Removing that line obviates the need for the martian_source_keep_err label
> so delete it.
>
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
> ---
>  net/ipv4/route.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 80f7c5b7b832..ef36dfed24da 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1760,7 +1760,7 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
>                 err = fib_validate_source(skb, saddr, daddr, tos,
>                                           0, dev, in_dev, &itag);
>                 if (err < 0)
> -                       goto martian_source_keep_err;
> +                       goto martian_source;
>                 goto local_input;
>         }
>
> @@ -1782,7 +1782,7 @@ out:      return err;
>                 err = fib_validate_source(skb, saddr, 0, tos, 0, dev,
>                                           in_dev, &itag);
>                 if (err < 0)
> -                       goto martian_source_keep_err;
> +                       goto martian_source;
>         }
>         flags |= RTCF_BROADCAST;
>         res.type = RTN_BROADCAST;
> @@ -1858,8 +1858,6 @@ out:      return err;
>         goto out;
>
>  martian_source:
> -       err = -EINVAL;
Just remove t=above line, no need to rename label.

> -martian_source_keep_err:
>         ip_handle_martian_source(dev, in_dev, skb, daddr, saddr);
>         goto out;
>  }
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 1/9] net: Remove martian_source_keep_err goto label

[David Ahern]

On 9/23/15 10:19 AM, Tom Herbert wrote:
>> @@ -1782,7 +1782,7 @@ out:      return err;
>>                  err = fib_validate_source(skb, saddr, 0, tos, 0, dev,
>>                                            in_dev, &itag);
>>                  if (err < 0)
>> -                       goto martian_source_keep_err;
>> +                       goto martian_source;
>>          }
>>          flags |= RTCF_BROADCAST;
>>          res.type = RTN_BROADCAST;
>> @@ -1858,8 +1858,6 @@ out:      return err;
>>          goto out;
>>
>>   martian_source:
>> -       err = -EINVAL;
> Just remove t=above line, no need to rename label.
>

This is a stepping stone patch. It removes the unnecessary EINVAL and 
then drops one of the duplicate labels as setup for later patches. By 
patch 9 all of this is gone.

[PATCH net-next 2/9] net: Remove e_inval label from ip_route_input_slow

[David Ahern]

e_inval has 1 explicit user and 1 fallthrough user -- martian_destination.
Move setting err to EINVAL before the 2 users and use the goto out label
instead of e_inval.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/ipv4/route.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index ef36dfed24da..3c5000db5972 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1775,8 +1775,9 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 out:	return err;
 
 brd_input:
+	err = -EINVAL;
 	if (skb->protocol != htons(ETH_P_IP))
-		goto e_inval;
+		goto out;
 
 	if (!ipv4_is_zeronet(saddr)) {
 		err = fib_validate_source(skb, saddr, 0, tos, 0, dev,
@@ -1842,15 +1843,13 @@ out:	return err;
 	 *	Do not cache martian addresses: they should be logged (RFC1812)
 	 */
 martian_destination:
+	err = -EINVAL;
 	RT_CACHE_STAT_INC(in_martian_dst);
 #ifdef CONFIG_IP_ROUTE_VERBOSE
 	if (IN_DEV_LOG_MARTIANS(in_dev))
 		net_warn_ratelimited("martian destination %pI4 from %pI4, dev %s\n",
 				     &daddr, &saddr, dev->name);
 #endif
-
-e_inval:
-	err = -EINVAL;
 	goto out;
 
 e_nobufs:
-- 
1.9.1

Re: [PATCH net-next 2/9] net: Remove e_inval label from ip_route_input_slow

[Alexander Duyck]

On 09/23/2015 08:15 AM, David Ahern wrote:
> e_inval has 1 explicit user and 1 fallthrough user -- martian_destination.
> Move setting err to EINVAL before the 2 users and use the goto out label
> instead of e_inval.
>
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
> ---
>   net/ipv4/route.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index ef36dfed24da..3c5000db5972 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1775,8 +1775,9 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
>   out:	return err;
>   
>   brd_input:
> +	err = -EINVAL;
>   	if (skb->protocol != htons(ETH_P_IP))
> -		goto e_inval;
> +		goto out;
>   
>   	if (!ipv4_is_zeronet(saddr)) {
>   		err = fib_validate_source(skb, saddr, 0, tos, 0, dev,
> @@ -1842,15 +1843,13 @@ out:	return err;
>   	 *	Do not cache martian addresses: they should be logged (RFC1812)
>   	 */
>   martian_destination:
> +	err = -EINVAL;
>   	RT_CACHE_STAT_INC(in_martian_dst);
>   #ifdef CONFIG_IP_ROUTE_VERBOSE
>   	if (IN_DEV_LOG_MARTIANS(in_dev))
>   		net_warn_ratelimited("martian destination %pI4 from %pI4, dev %s\n",
>   				     &daddr, &saddr, dev->name);
>   #endif
> -
> -e_inval:
> -	err = -EINVAL;
>   	goto out;
>   
>   e_nobufs:

This is adding unnecessary bloat to the code.  I really think if you 
want to simplify this just replace "goto e_inval;" with "return -EINVAL;".

Also why is it you end up leaving the out label through all of these 
patches?  It seems like that would be one of the first places to start 
in terms of removing the labels.

- Alex

Re: [PATCH net-next 2/9] net: Remove e_inval label from ip_route_input_slow

[David Ahern]

On 9/23/15 9:45 AM, Alexander Duyck wrote:
> On 09/23/2015 08:15 AM, David Ahern wrote:
>> e_inval has 1 explicit user and 1 fallthrough user --
>> martian_destination.
>> Move setting err to EINVAL before the 2 users and use the goto out label
>> instead of e_inval.
>>
>> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
>> ---
>>   net/ipv4/route.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
>> index ef36dfed24da..3c5000db5972 100644
>> --- a/net/ipv4/route.c
>> +++ b/net/ipv4/route.c
>> @@ -1775,8 +1775,9 @@ static int ip_route_input_slow(struct sk_buff
>> *skb, __be32 daddr, __be32 saddr,
>>   out:    return err;
>>   brd_input:
>> +    err = -EINVAL;
>>       if (skb->protocol != htons(ETH_P_IP))
>> -        goto e_inval;
>> +        goto out;
>>       if (!ipv4_is_zeronet(saddr)) {
>>           err = fib_validate_source(skb, saddr, 0, tos, 0, dev,
>> @@ -1842,15 +1843,13 @@ out:    return err;
>>        *    Do not cache martian addresses: they should be logged
>> (RFC1812)
>>        */
>>   martian_destination:
>> +    err = -EINVAL;
>>       RT_CACHE_STAT_INC(in_martian_dst);
>>   #ifdef CONFIG_IP_ROUTE_VERBOSE
>>       if (IN_DEV_LOG_MARTIANS(in_dev))
>>           net_warn_ratelimited("martian destination %pI4 from %pI4,
>> dev %s\n",
>>                        &daddr, &saddr, dev->name);
>>   #endif
>> -
>> -e_inval:
>> -    err = -EINVAL;
>>       goto out;
>>   e_nobufs:
>
> This is adding unnecessary bloat to the code.  I really think if you
> want to simplify this just replace "goto e_inval;" with "return -EINVAL;".

Not sure why you think this is bloating the code. Before this patch:

$ size kbuild/net/ipv4/route.o
text data bss dec hex filename
19778 2321 32 22131 5673 kbuild/net/ipv4/route.o

With this patch:
$ size kbuild/net/ipv4/route.o
    text	   data	    bss	    dec	    hex	filename
   19778	   2321	     32	  22131	   5673	kbuild/net/ipv4/route.o

So no difference at all.

Please consider this is an intermediate step; the code goes away in 
later ones.

>
> Also why is it you end up leaving the out label through all of these
> patches?  It seems like that would be one of the first places to start
> in terms of removing the labels.

jump to return is normal usage. Why would I start there and end up with 
N exit points? There are a lot of functions in the route code where 
doing that will burn you.

Re: [PATCH net-next 2/9] net: Remove e_inval label from ip_route_input_slow

[Alexander Duyck]

On 09/23/2015 09:02 AM, David Ahern wrote:
> On 9/23/15 9:45 AM, Alexander Duyck wrote:
>> On 09/23/2015 08:15 AM, David Ahern wrote:
>>> e_inval has 1 explicit user and 1 fallthrough user --
>>> martian_destination.
>>> Move setting err to EINVAL before the 2 users and use the goto out 
>>> label
>>> instead of e_inval.
>>>
>>> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
>>> ---
>>>   net/ipv4/route.c | 7 +++----
>>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
>>> index ef36dfed24da..3c5000db5972 100644
>>> --- a/net/ipv4/route.c
>>> +++ b/net/ipv4/route.c
>>> @@ -1775,8 +1775,9 @@ static int ip_route_input_slow(struct sk_buff
>>> *skb, __be32 daddr, __be32 saddr,
>>>   out:    return err;
>>>   brd_input:
>>> +    err = -EINVAL;
>>>       if (skb->protocol != htons(ETH_P_IP))
>>> -        goto e_inval;
>>> +        goto out;
>>>       if (!ipv4_is_zeronet(saddr)) {
>>>           err = fib_validate_source(skb, saddr, 0, tos, 0, dev,
>>> @@ -1842,15 +1843,13 @@ out:    return err;
>>>        *    Do not cache martian addresses: they should be logged
>>> (RFC1812)
>>>        */
>>>   martian_destination:
>>> +    err = -EINVAL;
>>>       RT_CACHE_STAT_INC(in_martian_dst);
>>>   #ifdef CONFIG_IP_ROUTE_VERBOSE
>>>       if (IN_DEV_LOG_MARTIANS(in_dev))
>>>           net_warn_ratelimited("martian destination %pI4 from %pI4,
>>> dev %s\n",
>>>                        &daddr, &saddr, dev->name);
>>>   #endif
>>> -
>>> -e_inval:
>>> -    err = -EINVAL;
>>>       goto out;
>>>   e_nobufs:
>>
>> This is adding unnecessary bloat to the code.  I really think if you
>> want to simplify this just replace "goto e_inval;" with "return 
>> -EINVAL;".
>
> Not sure why you think this is bloating the code. Before this patch:
>
> $ size kbuild/net/ipv4/route.o
> text data bss dec hex filename
> 19778 2321 32 22131 5673 kbuild/net/ipv4/route.o
>
> With this patch:
> $ size kbuild/net/ipv4/route.o
>    text       data        bss        dec        hex    filename
>   19778       2321         32      22131       5673 
> kbuild/net/ipv4/route.o
>
> So no difference at all.
>
> Please consider this is an intermediate step; the code goes away in 
> later ones.

Total code size for a change this small doesn't tell you anything. The 
fact is you are doubling the number of spots where you are having to 
reinitialize err.  It is the general direction of this I don't like 
since it is just making the code uglier before you attempt to fix it.

>
>>
>> Also why is it you end up leaving the out label through all of these
>> patches?  It seems like that would be one of the first places to start
>> in terms of removing the labels.
>
> jump to return is normal usage. Why would I start there and end up 
> with N exit points? There are a lot of functions in the route code 
> where doing that will burn you.
>

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.

- Alex

Re: [PATCH net-next 2/9] net: Remove e_inval label from ip_route_input_slow

[David Ahern]

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.

The first 3 patches appear to have *zero* impact on what the compiler 
generates.

Do you object to the end result of this patch series? ie. do you have 
concerns about what the end code looks like?

Re: [PATCH net-next 2/9] net: Remove e_inval label from ip_route_input_slow

[Alexander Duyck]

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.

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

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

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.

- Alex

Re: [PATCH net-next 2/9] net: Remove e_inval label from ip_route_input_slow

[David Ahern]

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

Re: [PATCH net-next 2/9] net: Remove e_inval label from ip_route_input_slow

[David Miller]

From: David Ahern <dsa@cumulusnetworks.com>
Date: Wed, 23 Sep 2015 12:03:03 -0600

> 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 problem is that people will, in the future, have the backport bug
fixes _THROUGH_ all of these code rearrangements.

And the person who is going to have to do that, is _ME_.

And if that kind of pain is going to be bestowed upon me, it has to
be for something of great value.

I do not see great value in your rearrangements here, not at all.

Re: [PATCH net-next 2/9] net: Remove e_inval label from ip_route_input_slow

[Alexander Duyck]

On 09/23/2015 11:03 AM, David Ahern wrote:
> 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.

I'll take a stab at it.  If you don't mind I might pull in a couple of 
your patches and make a few tweaks here and there and I will resubmit 
them with a few others.

Thanks.

- Alex

Re: [PATCH net-next 2/9] net: Remove e_inval label from ip_route_input_slow

[David Miller]

From: Alexander Duyck <alexander.duyck@gmail.com>
Date: Wed, 23 Sep 2015 09:31:06 -0700

> It is the general direction of this I don't like since it is just
> making the code uglier before you attempt to fix it.

I agree, I don't like these refactorings at all.

Some of these code paths have been this way for a decade or longer,
and are etched into the back of our skulls.

Unless there is _true_ benefit, just leave the code alone, really.

I also really don't like "un-goto" conversions at all, they generally
make the code harder to read.  And as Eric Dumazet said, unless you
carefully pair such changes with appropriate likely()/unlikely()
taggin you pessimize the code.

[PATCH net-next 3/9] net: Remove e_nobufs label from ip_route_input_slow

[David Ahern]

e_nobufs has 1 user. Move setting err to -ENOBUFS for the 1 user and
use the goto out label instead of e_nobufs. Stepping stone patch; next
one moves rth code into a helper function.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/ipv4/route.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 3c5000db5972..e3b18cc1952f 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1805,8 +1805,10 @@ out:	return err;
 
 	rth = rt_dst_alloc(net->loopback_dev, flags | RTCF_LOCAL, res.type,
 			   IN_DEV_CONF_GET(in_dev, NOPOLICY), false, do_cache);
-	if (!rth)
-		goto e_nobufs;
+	if (!rth) {
+		err = -ENOBUFS;
+		goto out;
+	}
 
 	rth->dst.output= ip_rt_bug;
 #ifdef CONFIG_IP_ROUTE_CLASSID
@@ -1852,10 +1854,6 @@ out:	return err;
 #endif
 	goto out;
 
-e_nobufs:
-	err = -ENOBUFS;
-	goto out;
-
 martian_source:
 	ip_handle_martian_source(dev, in_dev, skb, daddr, saddr);
 	goto out;
-- 
1.9.1

[PATCH net-next 4/9] net: Move rth handling from ip_route_input_slow to helper

[David Ahern]

Move the rth lookup and allocation from ip_route_input_slow into a
helper function. Code move only; no operational change intended.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/ipv4/route.c | 102 +++++++++++++++++++++++++++++++------------------------
 1 file changed, 58 insertions(+), 44 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index e3b18cc1952f..35a19d15232a 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1667,6 +1667,62 @@ static int ip_mkroute_input(struct sk_buff *skb,
 	return __mkroute_input(skb, res, in_dev, daddr, saddr, tos);
 }
 
+static int ip_route_local_input(struct sk_buff *skb,
+				struct fib_result *res,
+				struct net *net,
+				struct in_device *in_dev,
+				unsigned int flags,
+				u32 itag,
+				int rth_err)
+{
+	bool do_cache = false;
+	struct rtable *rth;
+	int err = 0;
+
+	if (res->fi) {
+		if (!itag) {
+			rth = rcu_dereference(FIB_RES_NH(*res).nh_rth_input);
+			if (rt_cache_valid(rth)) {
+				skb_dst_set_noref(skb, &rth->dst);
+				goto out;
+			}
+			do_cache = true;
+		}
+	}
+
+	rth = rt_dst_alloc(net->loopback_dev, flags | RTCF_LOCAL, res->type,
+			   IN_DEV_CONF_GET(in_dev, NOPOLICY), false, do_cache);
+	err = -ENOBUFS;
+	if (!rth)
+		goto out;
+
+	err = 0;
+	rth->dst.output = ip_rt_bug;
+#ifdef CONFIG_IP_ROUTE_CLASSID
+	rth->dst.tclassid = itag;
+#endif
+	rth->rt_is_input = 1;
+	if (res->table)
+		rth->rt_table_id = res->table->tb_id;
+
+	RT_CACHE_STAT_INC(in_slow_tot);
+	if (res->type == RTN_UNREACHABLE) {
+		rth->dst.input = ip_error;
+		rth->dst.error = -rth_err;
+		rth->rt_flags &= ~RTCF_LOCAL;
+	}
+
+	if (do_cache) {
+		if (unlikely(!rt_cache_route(&FIB_RES_NH(*res), rth))) {
+			rth->dst.flags |= DST_NOCACHE;
+			rt_add_uncached_list(rth);
+		}
+	}
+	skb_dst_set(skb, &rth->dst);
+out:
+	return err;
+}
+
 /*
  *	NOTE. We drop all the packets that has local source
  *	addresses, because every properly looped back packet
@@ -1687,10 +1743,8 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 	struct flowi4	fl4;
 	unsigned int	flags = 0;
 	u32		itag = 0;
-	struct rtable	*rth;
 	int		err = -EINVAL;
 	struct net    *net = dev_net(dev);
-	bool do_cache;
 
 	/* IP on this device is disabled. */
 
@@ -1790,48 +1844,8 @@ out:	return err;
 	RT_CACHE_STAT_INC(in_brd);
 
 local_input:
-	do_cache = false;
-	if (res.fi) {
-		if (!itag) {
-			rth = rcu_dereference(FIB_RES_NH(res).nh_rth_input);
-			if (rt_cache_valid(rth)) {
-				skb_dst_set_noref(skb, &rth->dst);
-				err = 0;
-				goto out;
-			}
-			do_cache = true;
-		}
-	}
-
-	rth = rt_dst_alloc(net->loopback_dev, flags | RTCF_LOCAL, res.type,
-			   IN_DEV_CONF_GET(in_dev, NOPOLICY), false, do_cache);
-	if (!rth) {
-		err = -ENOBUFS;
-		goto out;
-	}
-
-	rth->dst.output= ip_rt_bug;
-#ifdef CONFIG_IP_ROUTE_CLASSID
-	rth->dst.tclassid = itag;
-#endif
-	rth->rt_is_input = 1;
-	if (res.table)
-		rth->rt_table_id = res.table->tb_id;
-
-	RT_CACHE_STAT_INC(in_slow_tot);
-	if (res.type == RTN_UNREACHABLE) {
-		rth->dst.input= ip_error;
-		rth->dst.error= -err;
-		rth->rt_flags 	&= ~RTCF_LOCAL;
-	}
-	if (do_cache) {
-		if (unlikely(!rt_cache_route(&FIB_RES_NH(res), rth))) {
-			rth->dst.flags |= DST_NOCACHE;
-			rt_add_uncached_list(rth);
-		}
-	}
-	skb_dst_set(skb, &rth->dst);
-	err = 0;
+	err = ip_route_local_input(skb, &res, net, in_dev,
+				   flags, itag, err);
 	goto out;
 
 no_route:
-- 
1.9.1

[PATCH net-next 5/9] net: Move martian_destination to helper

[David Ahern]

Move stat and logging for martian_destination error into helper function
similar to ip_handle_martian_source. Code move only; no functional change.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/ipv4/route.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 35a19d15232a..8e0fb1e4de72 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1531,6 +1531,20 @@ static int ip_route_input_mc(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 	return err;
 }
 
+/*
+ *	Do not cache martian addresses: they should be logged (RFC1812)
+ */
+static void ip_handle_martian_dest(struct net_device *dev,
+				   struct in_device *in_dev,
+				   __be32 daddr, __be32 saddr)
+{
+	RT_CACHE_STAT_INC(in_martian_dst);
+#ifdef CONFIG_IP_ROUTE_VERBOSE
+	if (IN_DEV_LOG_MARTIANS(in_dev))
+		net_warn_ratelimited("martian destination %pI4 from %pI4, dev %s\n",
+				     &daddr, &saddr, dev->name);
+#endif
+}
 
 static void ip_handle_martian_source(struct net_device *dev,
 				     struct in_device *in_dev,
@@ -1855,17 +1869,9 @@ out:	return err;
 	res.table = NULL;
 	goto local_input;
 
-	/*
-	 *	Do not cache martian addresses: they should be logged (RFC1812)
-	 */
 martian_destination:
 	err = -EINVAL;
-	RT_CACHE_STAT_INC(in_martian_dst);
-#ifdef CONFIG_IP_ROUTE_VERBOSE
-	if (IN_DEV_LOG_MARTIANS(in_dev))
-		net_warn_ratelimited("martian destination %pI4 from %pI4, dev %s\n",
-				     &daddr, &saddr, dev->name);
-#endif
+	ip_handle_martian_dest(dev, in_dev, daddr, saddr);
 	goto out;
 
 martian_source:
-- 
1.9.1

[PATCH net-next 6/9] net: Remove martian_source goto

[David Ahern]

Move call to ip_handle_martian_source to various jump sites. The
function only increments a counter and possibly does additional
logging if CONFIG_IP_ROUTE_VERBOSE is enabled.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/ipv4/route.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 8e0fb1e4de72..0fdcb0539795 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1776,20 +1776,20 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 		fl4.flowi4_tun_key.tun_id = 0;
 	skb_dst_drop(skb);
 
-	if (ipv4_is_multicast(saddr) || ipv4_is_lbcast(saddr))
-		goto martian_source;
+	/* Accept zero addresses only to limited broadcast;
+	 * I even do not know to fix it or not. Waiting for complains :-)
+	 */
+	if (ipv4_is_multicast(saddr) || ipv4_is_lbcast(saddr) ||
+	    ipv4_is_zeronet(saddr)) {
+		ip_handle_martian_source(dev, in_dev, skb, daddr, saddr);
+		goto out;
+	}
 
 	res.fi = NULL;
 	res.table = NULL;
 	if (ipv4_is_lbcast(daddr) || (saddr == 0 && daddr == 0))
 		goto brd_input;
 
-	/* Accept zero addresses only to limited broadcast;
-	 * I even do not know to fix it or not. Waiting for complains :-)
-	 */
-	if (ipv4_is_zeronet(saddr))
-		goto martian_source;
-
 	if (ipv4_is_zeronet(daddr))
 		goto martian_destination;
 
@@ -1800,8 +1800,10 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 		if (!IN_DEV_NET_ROUTE_LOCALNET(in_dev, net))
 			goto martian_destination;
 	} else if (ipv4_is_loopback(saddr)) {
-		if (!IN_DEV_NET_ROUTE_LOCALNET(in_dev, net))
-			goto martian_source;
+		if (!IN_DEV_NET_ROUTE_LOCALNET(in_dev, net)) {
+			ip_handle_martian_source(dev, in_dev, skb, daddr, saddr);
+			goto out;
+		}
 	}
 
 	/*
@@ -1827,8 +1829,10 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 	if (res.type == RTN_LOCAL) {
 		err = fib_validate_source(skb, saddr, daddr, tos,
 					  0, dev, in_dev, &itag);
-		if (err < 0)
-			goto martian_source;
+		if (err < 0) {
+			ip_handle_martian_source(dev, in_dev, skb, daddr, saddr);
+			goto out;
+		}
 		goto local_input;
 	}
 
@@ -1850,8 +1854,10 @@ out:	return err;
 	if (!ipv4_is_zeronet(saddr)) {
 		err = fib_validate_source(skb, saddr, 0, tos, 0, dev,
 					  in_dev, &itag);
-		if (err < 0)
-			goto martian_source;
+		if (err < 0) {
+			ip_handle_martian_source(dev, in_dev, skb, daddr, saddr);
+			goto out;
+		}
 	}
 	flags |= RTCF_BROADCAST;
 	res.type = RTN_BROADCAST;
@@ -1873,10 +1879,6 @@ out:	return err;
 	err = -EINVAL;
 	ip_handle_martian_dest(dev, in_dev, daddr, saddr);
 	goto out;
-
-martian_source:
-	ip_handle_martian_source(dev, in_dev, skb, daddr, saddr);
-	goto out;
 }
 
 int ip_route_input_noref(struct sk_buff *skb, __be32 daddr, __be32 saddr,
-- 
1.9.1

[PATCH net-next 7/9] net: Remove martian_destination label

[David Ahern]

Move call to ip_handle_martian_dest to various jump sites. The
function only increments a counter and possibly does additional
logging if CONFIG_IP_ROUTE_VERBOSE is enabled.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/ipv4/route.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 0fdcb0539795..c23bb0965966 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1790,15 +1790,19 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 	if (ipv4_is_lbcast(daddr) || (saddr == 0 && daddr == 0))
 		goto brd_input;
 
-	if (ipv4_is_zeronet(daddr))
-		goto martian_destination;
+	if (ipv4_is_zeronet(daddr)) {
+		ip_handle_martian_dest(dev, in_dev, daddr, saddr);
+		goto out;
+	}
 
 	/* Following code try to avoid calling IN_DEV_NET_ROUTE_LOCALNET(),
 	 * and call it once if daddr or/and saddr are loopback addresses
 	 */
 	if (ipv4_is_loopback(daddr)) {
-		if (!IN_DEV_NET_ROUTE_LOCALNET(in_dev, net))
-			goto martian_destination;
+		if (!IN_DEV_NET_ROUTE_LOCALNET(in_dev, net)) {
+			ip_handle_martian_dest(dev, in_dev, daddr, saddr);
+			goto out;
+		}
 	} else if (ipv4_is_loopback(saddr)) {
 		if (!IN_DEV_NET_ROUTE_LOCALNET(in_dev, net)) {
 			ip_handle_martian_source(dev, in_dev, skb, daddr, saddr);
@@ -1840,8 +1844,11 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 		err = -EHOSTUNREACH;
 		goto no_route;
 	}
-	if (res.type != RTN_UNICAST)
-		goto martian_destination;
+	if (res.type != RTN_UNICAST) {
+		err = -EINVAL;
+		ip_handle_martian_dest(dev, in_dev, daddr, saddr);
+		goto out;
+	}
 
 	err = ip_mkroute_input(skb, &res, &fl4, in_dev, daddr, saddr, tos);
 out:	return err;
@@ -1874,11 +1881,6 @@ out:	return err;
 	res.fi = NULL;
 	res.table = NULL;
 	goto local_input;
-
-martian_destination:
-	err = -EINVAL;
-	ip_handle_martian_dest(dev, in_dev, daddr, saddr);
-	goto out;
 }
 
 int ip_route_input_noref(struct sk_buff *skb, __be32 daddr, __be32 saddr,
-- 
1.9.1

Re: [PATCH net-next 7/9] net: Remove martian_destination label

[Eric Dumazet]

On Wed, 2015-09-23 at 08:15 -0700, David Ahern wrote:
> Move call to ip_handle_martian_dest to various jump sites. The
> function only increments a counter and possibly does additional
> logging if CONFIG_IP_ROUTE_VERBOSE is enabled.
> 
> Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
> ---
>  net/ipv4/route.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 0fdcb0539795..c23bb0965966 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1790,15 +1790,19 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
>  	if (ipv4_is_lbcast(daddr) || (saddr == 0 && daddr == 0))
>  		goto brd_input;
>  
> -	if (ipv4_is_zeronet(daddr))
> -		goto martian_destination;
> +	if (ipv4_is_zeronet(daddr)) {
> +		ip_handle_martian_dest(dev, in_dev, daddr, saddr);
> +		goto out;
> +	}
>  

As requested, here is an example of code for which compiler might
compile in a non efficient way, ie inserting code at the wrong place,
increasing icache pressure.

Removing a goto is removing an implicit unlikely.

[PATCH net-next 8/9] net: Remove local_input label

[David Ahern]

Move calls to ip_route_local_input to jump sites and remove
local_input label.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/ipv4/route.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index c23bb0965966..340246414545 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1835,9 +1835,11 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 					  0, dev, in_dev, &itag);
 		if (err < 0) {
 			ip_handle_martian_source(dev, in_dev, skb, daddr, saddr);
-			goto out;
+		} else {
+			err = ip_route_local_input(skb, &res, net, in_dev,
+						   flags, itag, err);
 		}
-		goto local_input;
+		goto out;
 	}
 
 	if (!IN_DEV_FORWARD(in_dev)) {
@@ -1870,7 +1872,6 @@ out:	return err;
 	res.type = RTN_BROADCAST;
 	RT_CACHE_STAT_INC(in_brd);
 
-local_input:
 	err = ip_route_local_input(skb, &res, net, in_dev,
 				   flags, itag, err);
 	goto out;
@@ -1880,7 +1881,10 @@ out:	return err;
 	res.type = RTN_UNREACHABLE;
 	res.fi = NULL;
 	res.table = NULL;
-	goto local_input;
+
+	err = ip_route_local_input(skb, &res, net, in_dev,
+				   flags, itag, err);
+	goto out;
 }
 
 int ip_route_input_noref(struct sk_buff *skb, __be32 daddr, __be32 saddr,
-- 
1.9.1

[PATCH net-next 9/9] net: Remove no_route label

[David Ahern]

Move no_route code into helper. Add call to helper at jump sites and
remove goto label.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/ipv4/route.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 340246414545..2709ee6e292e 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1737,6 +1737,22 @@ static int ip_route_local_input(struct sk_buff *skb,
 	return err;
 }
 
+static int ip_input_no_route(struct sk_buff *skb,
+			     struct fib_result *res,
+			     struct net *net,
+			     struct in_device *in_dev,
+			     unsigned int flags,
+			     u32 itag, int err)
+{
+	RT_CACHE_STAT_INC(in_no_route);
+
+	res->type = RTN_UNREACHABLE;
+	res->fi = NULL;
+	res->table = NULL;
+
+	return ip_route_local_input(skb, res, net, in_dev, flags, itag, err);
+}
+
 /*
  *	NOTE. We drop all the packets that has local source
  *	addresses, because every properly looped back packet
@@ -1824,7 +1840,10 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 	if (err != 0) {
 		if (!IN_DEV_FORWARD(in_dev))
 			err = -EHOSTUNREACH;
-		goto no_route;
+
+		err = ip_input_no_route(skb, &res, net, in_dev,
+					flags, itag, err);
+		goto out;
 	}
 
 	if (res.type == RTN_BROADCAST)
@@ -1843,8 +1862,9 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 	}
 
 	if (!IN_DEV_FORWARD(in_dev)) {
-		err = -EHOSTUNREACH;
-		goto no_route;
+		err = ip_input_no_route(skb, &res, net, in_dev,
+					flags, itag, -EHOSTUNREACH);
+		goto out;
 	}
 	if (res.type != RTN_UNICAST) {
 		err = -EINVAL;
@@ -1875,16 +1895,6 @@ out:	return err;
 	err = ip_route_local_input(skb, &res, net, in_dev,
 				   flags, itag, err);
 	goto out;
-
-no_route:
-	RT_CACHE_STAT_INC(in_no_route);
-	res.type = RTN_UNREACHABLE;
-	res.fi = NULL;
-	res.table = NULL;
-
-	err = ip_route_local_input(skb, &res, net, in_dev,
-				   flags, itag, err);
-	goto out;
 }
 
 int ip_route_input_noref(struct sk_buff *skb, __be32 daddr, __be32 saddr,
-- 
1.9.1

Re: [PATCH net-next 0/9 v2] net: Refactor ip_route_input_slow

[Eric Dumazet]

On Wed, 2015-09-23 at 08:15 -0700, David Ahern wrote:
> ip_route_input_slow is a maze of gotos (9 of them!) making it error
> prone and difficult to read. This patchset refactors it, removing all
> but 2 of the labels. The brd_input label for broadcast path requires
> too many inputs to make a reasonble helper out of it so I left it as is.
> 
> None of these patches change functionality, only move code around
> to make ip_route_input_slow more readable.

This kind of work, might ease your job (pushing new functions in the
future kernels), but make stable teams work a nightmare.

You also remove a lot of goto, but do not place likely() or unlikely()
clauses that would help compiler to emit the same optimal code.

gotos have quite nice properties, forget what you might have learn in
some CS classes.

Re: [PATCH net-next 0/9 v2] net: Refactor ip_route_input_slow

[David Ahern]

On 9/23/15 9:30 AM, Eric Dumazet wrote:
>
> You also remove a lot of goto, but do not place likely() or unlikely()
> clauses that would help compiler to emit the same optimal code.

I do not see where I have changed the likely/unlikely aspect of the 
logic. Can you point to an example in the patches?

Re: [PATCH net-next 0/9 v2] net: Refactor ip_route_input_slow

[Eric Dumazet]

On Wed, 2015-09-23 at 10:08 -0600, David Ahern wrote:
> On 9/23/15 9:30 AM, Eric Dumazet wrote:
> >
> > You also remove a lot of goto, but do not place likely() or unlikely()
> > clauses that would help compiler to emit the same optimal code.
> 
> I do not see where I have changed the likely/unlikely aspect of the 
> logic. Can you point to an example in the patches?
> 

Sure, I did for PATCH net-next 7/9

Copied here :

> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index 0fdcb0539795..c23bb0965966 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1790,15 +1790,19 @@ static int ip_route_input_slow(struct sk_buff
*skb, __be32 daddr, __be32 saddr,
>       if (ipv4_is_lbcast(daddr) || (saddr == 0 && daddr == 0))
>               goto brd_input;
>  
> -     if (ipv4_is_zeronet(daddr))
> -             goto martian_destination;
> +     if (ipv4_is_zeronet(daddr)) {
> +             ip_handle_martian_dest(dev, in_dev, daddr, saddr);
> +             goto out;
> +     }
>  

As requested, here is an example of code for which compiler might
compile in a non efficient way, ie inserting code at the wrong place,
increasing icache pressure.

Removing a goto is removing an implicit unlikely.

[PATCH net-next 0/9] net: Refactor ip_route_input_slow

[David Ahern]

ip_route_input_slow is a maze of gotos (9 of them!) making it error
prone and difficult to read. This patchset refactors it, removing all
but 2 of the labels. The brd_input label for broadcast path requires
too many inputs to make a reasonble helper out of it so I left it as is.

None of these patches change functionality, only move code around
to make ip_route_input_slow more readable.

Size comparison using gcc version 4.7.2 (Debian 4.7.2-5)

With CONFIG_IP_ROUTE_VERBOSE:
    Before patches:
    $ size kbuild2/net/ipv4/route.o
       text	   data	    bss	    dec	    hex	filename
      20615	   2321	     32	  22968	   59b8	kbuild2/net/ipv4/route.o

    After patches:
    $ size kbuild/net/ipv4/route.o
       text	   data	    bss	    dec	    hex	filename
      20774	   2321	     32	  23127	   5a57	kbuild/net/ipv4/route.o

An increase of 159 bytes with CONFIG_IP_ROUTE_VERBOSE.

Without CONFIG_IP_ROUTE_VERBOSE:
    Before patches:
    $ size kbuild2/net/ipv4/route.o
       text	   data	    bss	    dec	    hex	filename
      19778	   2321	     32	  22131	   5673	kbuild2/net/ipv4/route.o

    After patches:
    $ size kbuild/net/ipv4/route.o
       text	   data	    bss	    dec	    hex	filename
      19858	   2321	     32	  22211	   56c3	kbuild/net/ipv4/route.o

An increase of 80 bytes without CONFIG_IP_ROUTE_VERBOSE.

David Ahern (9):
  net: Remove martian_source_keep_err goto label
  net: Remove e_inval label from ip_route_input_slow
  net: Remove e_nobufs label from ip_route_input_slow
  net: Move rth handling from ip_route_input_slow to helper
  net: Move martian_destination to helper
  net: Remove martian_source goto
  net: Remove martian_destination label
  net: Remove local_input label
  net: Remove no_route label

 net/ipv4/route.c | 239 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 139 insertions(+), 100 deletions(-)

-- 
1.9.1

[PATCH net-next 5/9] net: Move martian_destination to helper

[David Ahern]

Move stat and logging for martian_destination error into helper function
similar to ip_handle_martian_source. Code move only; no functional change.

Signed-off-by: David Ahern <dsa@cumulusnetworks.com>
---
 net/ipv4/route.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 79c4cecdb7a1..3ee37835e1f5 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1531,6 +1531,20 @@ static int ip_route_input_mc(struct sk_buff *skb, __be32 daddr, __be32 saddr,
 	return err;
 }
 
+/*
+ *	Do not cache martian addresses: they should be logged (RFC1812)
+ */
+static void ip_handle_martian_dest(struct net_device *dev,
+				   struct in_device *in_dev,
+				   __be32 daddr, __be32 saddr)
+{
+	RT_CACHE_STAT_INC(in_martian_dst);
+#ifdef CONFIG_IP_ROUTE_VERBOSE
+	if (IN_DEV_LOG_MARTIANS(in_dev))
+		net_warn_ratelimited("martian destination %pI4 from %pI4, dev %s\n",
+				     &daddr, &saddr, dev->name);
+#endif
+}
 
 static void ip_handle_martian_source(struct net_device *dev,
 				     struct in_device *in_dev,
@@ -1857,17 +1871,9 @@ out:	return err;
 	res.table = NULL;
 	goto local_input;
 
-	/*
-	 *	Do not cache martian addresses: they should be logged (RFC1812)
-	 */
 martian_destination:
 	err = -EINVAL;
-	RT_CACHE_STAT_INC(in_martian_dst);
-#ifdef CONFIG_IP_ROUTE_VERBOSE
-	if (IN_DEV_LOG_MARTIANS(in_dev))
-		net_warn_ratelimited("martian destination %pI4 from %pI4, dev %s\n",
-				     &daddr, &saddr, dev->name);
-#endif
+	ip_handle_martian_dest(dev, in_dev, daddr, saddr);
 	goto out;
 
 martian_source:
-- 
1.9.1