| Message ID | 1443021322-48621-3-git-send-email-dsa@cumulusnetworks.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- 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
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. -- 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
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 -- 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
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? -- 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
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 -- 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
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- 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
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. -- 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
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 -- 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
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:
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(-)