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

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

[Alexander Duyck]

On 09/22/2015 03:55 PM, David Ahern 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>

The comments above and the code below don't sync up.  The function 
fib_validate_source can return either -EINVAL, -EXDEV, 0, or 1.  The 
fact is this may be acceptable as long as all callers of 
ip_route_input_slow will handle a non-zero value as an error and it 
doesn't care about what the actual return value is.  If that is what you 
are going for here at least the comment should be updated, and we should 
be explicit somewhere about documenting the return values.

> ---
>   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;
>   }
>

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

[Alexander Duyck]

On 09/22/2015 06:04 PM, Alexander Duyck wrote:
> On 09/22/2015 03:55 PM, David Ahern 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>
>
> The comments above and the code below don't sync up.  The function
> fib_validate_source can return either -EINVAL, -EXDEV, 0, or 1.  The
> fact is this may be acceptable as long as all callers of
> ip_route_input_slow will handle a non-zero value as an error and it
> doesn't care about what the actual return value is.  If that is what you
> are going for here at least the comment should be updated, and we should
> be explicit somewhere about documenting the return values.

Actually this patch is correct.  I got my wires a bit crossed and 
misread martian_source vs martian_source_keep_err.

[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

[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

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

[Eric W. Biederman]

David Ahern <dsa@cumulusnetworks.com> writes:

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

Ick you are pessimizing the code.

You will almost certainly have better code generation if you hoist
the assignment of "err = -ENOBUFS" above the rt_dst_alloc then you
don't need to do anything in your error path except "goto out;"

Eric

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

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

[David Ahern]

On 9/22/15 8:15 PM, Eric W. Biederman wrote:
> David Ahern <dsa@cumulusnetworks.com> writes:
>
>> 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.
>
> Ick you are pessimizing the code.
>
> You will almost certainly have better code generation if you hoist
> the assignment of "err = -ENOBUFS" above the rt_dst_alloc then you
> don't need to do anything in your error path except "goto out;"

Can't do that here because the current value of err is used later in:
rth->dst.error = -err;

Besides, as mentioned above this is a stepping stone patch all of this 
is moved to a helper in patch 4. Where I could do the assignment before 
the rt_dst_alloc since I put the original err as an input arg rth_err.

David

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

[David Laight]

From: Eric W. Biederman
> Sent: 23 September 2015 03:15
> David Ahern <dsa@cumulusnetworks.com> writes:
> 
> > 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.
> 
> Ick you are pessimizing the code.
> 
> You will almost certainly have better code generation if you hoist
> the assignment of "err = -ENOBUFS" above the rt_dst_alloc then you
> don't need to do anything in your error path except "goto out;"

Unlikely to make a difference.
The compiler is very likely to set 'err' before the conditional
branch anyway. It will use an extra register to make it all work.

	David

[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 | 104 ++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 60 insertions(+), 44 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index e3b18cc1952f..79c4cecdb7a1 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1667,6 +1667,63 @@ 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_device *loopback_dev,
+				unsigned int flags,
+				u32 itag,
+				int rth_err,
+				bool nopolicy)
+{
+	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(loopback_dev, flags | RTCF_LOCAL, res->type,
+			   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 = -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 +1744,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 +1845,9 @@ 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->loopback_dev,
+				   flags, itag, err,
+				   IN_DEV_CONF_GET(in_dev, NOPOLICY));
 	goto out;
 
 no_route:
-- 
1.9.1

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

[Alexander Duyck]

On 09/22/2015 03:55 PM, David Ahern wrote:
> 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 | 104 ++++++++++++++++++++++++++++++++-----------------------
>   1 file changed, 60 insertions(+), 44 deletions(-)
>
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index e3b18cc1952f..79c4cecdb7a1 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1667,6 +1667,63 @@ 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_device *loopback_dev,
> +				unsigned int flags,
> +				u32 itag,
> +				int rth_err,
> +				bool nopolicy)

Why pass loopback_dev instead of just passing the net pointer?  Seems 
like in the path below there are cases where you end up not needing to 
dereference it and it might be worth the effort to avoid dereferencing 
it if you don't need to.

Same thing for nopolicy.  Why not just pass the in_dev pointer and 
handle getting the policy if you need it.

> +{
> +	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(loopback_dev, flags | RTCF_LOCAL, res->type,
> +			   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 = -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 +1744,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 +1845,9 @@ 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->loopback_dev,
> +				   flags, itag, err,
> +				   IN_DEV_CONF_GET(in_dev, NOPOLICY));
>   	goto out;
>
>   no_route:
>

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

[David Ahern]

On 9/22/15 8:33 PM, Alexander Duyck wrote:
> Why pass loopback_dev instead of just passing the net pointer?  Seems
> like in the path below there are cases where you end up not needing to
> dereference it and it might be worth the effort to avoid dereferencing
> it if you don't need to.
>
> Same thing for nopolicy.  Why not just pass the in_dev pointer and
> handle getting the policy if you need it.

Sure, I was passing in what was needed but I get your point. If the 
cache version is valid it is an unnecessary dereference.

[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

[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 3ee37835e1f5..b5c3844b85d3 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1777,20 +1777,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;
 
@@ -1801,8 +1801,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;
+		}
 	}
 
 	/*
@@ -1828,8 +1830,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;
 	}
 
@@ -1851,8 +1855,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;
@@ -1875,10 +1881,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 b5c3844b85d3..efc8e58c1964 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1791,15 +1791,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);
@@ -1841,8 +1845,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;
@@ -1876,11 +1883,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

[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 | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index efc8e58c1964..454c38bd23a8 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1836,9 +1836,13 @@ 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->loopback_dev,
+						   flags, itag, err,
+						   IN_DEV_CONF_GET(in_dev, NOPOLICY));
 		}
-		goto local_input;
+		goto out;
 	}
 
 	if (!IN_DEV_FORWARD(in_dev)) {
@@ -1871,7 +1875,6 @@ out:	return err;
 	res.type = RTN_BROADCAST;
 	RT_CACHE_STAT_INC(in_brd);
 
-local_input:
 	err = ip_route_local_input(skb, &res, net->loopback_dev,
 				   flags, itag, err,
 				   IN_DEV_CONF_GET(in_dev, NOPOLICY));
@@ -1882,7 +1885,11 @@ out:	return err;
 	res.type = RTN_UNREACHABLE;
 	res.fi = NULL;
 	res.table = NULL;
-	goto local_input;
+
+	err = ip_route_local_input(skb, &res, net->loopback_dev,
+				   flags, itag, err,
+				   IN_DEV_CONF_GET(in_dev, NOPOLICY));
+	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 | 40 ++++++++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index 454c38bd23a8..e486a3fb3081 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1738,6 +1738,23 @@ 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_device *loopback_dev,
+			     unsigned int flags,
+			     u32 itag, int err,
+			     bool nopolicy)
+{
+	RT_CACHE_STAT_INC(in_no_route);
+
+	res->type = RTN_UNREACHABLE;
+	res->fi = NULL;
+	res->table = NULL;
+
+	return ip_route_local_input(skb, res, loopback_dev,
+				    flags, itag, err, nopolicy);
+}
+
 /*
  *	NOTE. We drop all the packets that has local source
  *	addresses, because every properly looped back packet
@@ -1825,7 +1842,11 @@ 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->loopback_dev,
+					flags, itag, err,
+					IN_DEV_CONF_GET(in_dev, NOPOLICY));
+		goto out;
 	}
 
 	if (res.type == RTN_BROADCAST)
@@ -1846,8 +1867,10 @@ 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->loopback_dev,
+					flags, itag, -EHOSTUNREACH,
+					IN_DEV_CONF_GET(in_dev, NOPOLICY));
+		goto out;
 	}
 	if (res.type != RTN_UNICAST) {
 		err = -EINVAL;
@@ -1879,17 +1902,6 @@ out:	return err;
 				   flags, itag, err,
 				   IN_DEV_CONF_GET(in_dev, NOPOLICY));
 	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->loopback_dev,
-				   flags, itag, err,
-				   IN_DEV_CONF_GET(in_dev, NOPOLICY));
-	goto out;
 }
 
 int ip_route_input_noref(struct sk_buff *skb, __be32 daddr, __be32 saddr,
-- 
1.9.1

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

[Alexander Duyck]

On 09/22/2015 03:55 PM, 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.
>
> 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(-)
>

One option you might consider when untangling this is to just return the 
values instead of leaving any labels.  I just did a quick test on my 
system with gcc version 5.1.1 and going through and just replacing all 
of the labels with returns actually resulted in smaller code since the 
compiler was smart enough to just combine the returns anyway.

You may also want to increase the scope of this patch set to include 
__mkroute_input as it ends up being compiled into this function as 
well.  From what I have seen there is a bit of redundancy with some of 
the code from local_input.

- Alex

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

[David Ahern]

On 9/22/15 11:51 PM, Alexander Duyck wrote:
>
> One option you might consider when untangling this is to just return the
> values instead of leaving any labels.  I just did a quick test on my
> system with gcc version 5.1.1 and going through and just replacing all
> of the labels with returns actually resulted in smaller code since the
> compiler was smart enough to just combine the returns anyway.

You have to unwrap it the way I did because there are a number of places 
with multiple jumps and a couple of places with fallthrough -- one label 
falling into the next.

>
> You may also want to increase the scope of this patch set to include
> __mkroute_input as it ends up being compiled into this function as
> well.  From what I have seen there is a bit of redundancy with some of
> the code from local_input.

Topic of another series. As is ip_route_input_mc. (and many other 
functions in this file)

Thanks for the review; new version coming soon.

David

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