| Message ID | 1368750099-14086-3-git-send-email-amwang@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hello. On 17-05-2013 4:21, Cong Wang wrote: > From: Cong Wang <amwang@redhat.com> > It will be used by vxlan module, so move it from ipv6 module > to core kernel. I think it is small enough to be inlined. > Cc: David S. Miller <davem@davemloft.net> > Signed-off-by: Cong Wang <amwang@redhat.com> > --- > include/net/ip6_route.h | 23 +++++++++++++++++++++-- > net/ipv6/route.c | 19 ------------------- > 2 files changed, 21 insertions(+), 21 deletions(-) > diff --git a/include/net/ip6_route.h b/include/net/ip6_route.h > index 260f83f..7e9192e 100644 > --- a/include/net/ip6_route.h > +++ b/include/net/ip6_route.h [...] > @@ -201,4 +200,24 @@ static inline struct in6_addr *rt6_nexthop(struct rt6_info *rt, struct in6_addr > return dest; > } > > +#if IS_ENABLED(CONFIG_IPV6) > +static inline int ip6_dst_hoplimit(struct dst_entry *dst) > +{ > + int hoplimit = dst_metric_raw(dst, RTAX_HOPLIMIT); Empty line wouldn't hurt here, after the declaration, like below... > + if (hoplimit == 0) { > + struct net_device *dev = dst->dev; > + struct inet6_dev *idev; > + > + rcu_read_lock(); > + idev = __in6_dev_get(dev); > + if (idev) > + hoplimit = idev->cnf.hop_limit; > + else > + hoplimit = dev_net(dev)->ipv6.devconf_all->hop_limit; > + rcu_read_unlock(); > + } > + return hoplimit; > +} > +#endif > + WBR, Sergei -- 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: Cong Wang <amwang@redhat.com> Date: Fri, 17 May 2013 08:21:30 +0800 > @@ -201,4 +200,24 @@ static inline struct in6_addr *rt6_nexthop(struct rt6_info *rt, struct in6_addr > return dest; > } > > +#if IS_ENABLED(CONFIG_IPV6) > +static inline int ip6_dst_hoplimit(struct dst_entry *dst) > +{ > + int hoplimit = dst_metric_raw(dst, RTAX_HOPLIMIT); > + if (hoplimit == 0) { > + struct net_device *dev = dst->dev; > + struct inet6_dev *idev; > + > + rcu_read_lock(); > + idev = __in6_dev_get(dev); > + if (idev) > + hoplimit = idev->cnf.hop_limit; > + else > + hoplimit = dev_net(dev)->ipv6.devconf_all->hop_limit; > + rcu_read_unlock(); > + } > + return hoplimit; > +} > +#endif Create a dummy stub version in an #else branch here, so that you have to ifdef less in vxlan.c In fact I think you can avoid nearly every ifdef in vxlan.c if you apply this technique throughout your changes. Please do that and resubmit this series. Thanks. -- 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 Fri, 2013-05-17 at 14:13 -0700, David Miller wrote: > From: Cong Wang <amwang@redhat.com> > Date: Fri, 17 May 2013 08:21:30 +0800 > > > @@ -201,4 +200,24 @@ static inline struct in6_addr *rt6_nexthop(struct rt6_info *rt, struct in6_addr > > return dest; > > } > > > > +#if IS_ENABLED(CONFIG_IPV6) > > +static inline int ip6_dst_hoplimit(struct dst_entry *dst) > > +{ > > + int hoplimit = dst_metric_raw(dst, RTAX_HOPLIMIT); > > + if (hoplimit == 0) { > > + struct net_device *dev = dst->dev; > > + struct inet6_dev *idev; > > + > > + rcu_read_lock(); > > + idev = __in6_dev_get(dev); > > + if (idev) > > + hoplimit = idev->cnf.hop_limit; > > + else > > + hoplimit = dev_net(dev)->ipv6.devconf_all->hop_limit; > > + rcu_read_unlock(); > > + } > > + return hoplimit; > > +} > > +#endif > > Create a dummy stub version in an #else branch here, so that you have > to ifdef less in vxlan.c The reason why we need #if IS_ENABLED(CONFIG_IPV6) here is that dev_net(dev)->ipv6 is defined only in such case, not just for its caller in vxlan. Nor I think anyone will seriously call ip6_dst_hoplimit() for !CONFIG_IPV6 case, since its name is obvious. > > In fact I think you can avoid nearly every ifdef in vxlan.c if you apply > this technique throughout your changes. > > Please do that and resubmit this series. > Actually that is exactly what I _did_ in v1 or RFC, IIRC, it is David Stevens who prefers to use #ifdef inside these functions, so I changed it based on his suggestion. I myself don't have any strong opinion here, either is okay, I just don't like changing it again and again. :) Thanks. -- 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: Cong Wang <amwang@redhat.com> Date: Wed, 22 May 2013 12:54:13 +0800 > Actually that is exactly what I _did_ in v1 or RFC, IIRC, it is David > Stevens who prefers to use #ifdef inside these functions, so I changed > it based on his suggestion. > > I myself don't have any strong opinion here, either is okay, I just > don't like changing it again and again. :) The driver looks like complete shit with all the ifdefs in there, this isn't the BSD kernel. I do not want to seem them there at all. You can abstract everything behind helper functions in a header file, keep the mess there. -- 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 Wed, 2013-05-22 at 00:14 -0700, David Miller wrote: > From: Cong Wang <amwang@redhat.com> > Date: Wed, 22 May 2013 12:54:13 +0800 > > > Actually that is exactly what I _did_ in v1 or RFC, IIRC, it is David > > Stevens who prefers to use #ifdef inside these functions, so I changed > > it based on his suggestion. > > > > I myself don't have any strong opinion here, either is okay, I just > > don't like changing it again and again. :) > > The driver looks like complete shit with all the ifdefs in there, > this isn't the BSD kernel. > > I do not want to seem them there at all. > > You can abstract everything behind helper functions in a header > file, keep the mess there. Alright, I will change all such functions in vxlan.c back to what you are suggesting. For example, change static inline bool vxlan_addr_equal(const union vxlan_addr *a, const union vxlan_addr *b) { #if IS_ENABLED(CONFIG_IPV6) if (a->sa.sa_family != b->sa.sa_family) return false; if (a->sa.sa_family == AF_INET6) return ipv6_addr_equal(&a->sin6.sin6_addr, &b->sin6.sin6_addr); else #endif return a->sin.sin_addr.s_addr == b->sin.sin_addr.s_addr; } to #if IS_ENABLED(CONFIG_IPV6) static inline bool vxlan_addr_equal(const union vxlan_addr *a, const union vxlan_addr *b) { if (a->sa.sa_family != b->sa.sa_family) return false; if (a->sa.sa_family == AF_INET6) return ipv6_addr_equal(&a->sin6.sin6_addr, &b->sin6.sin6_addr); else return a->sin.sin_addr.s_addr == b->sin.sin_addr.s_addr; } #else static inline bool vxlan_addr_equal(const union vxlan_addr *a, const union vxlan_addr *b) { return a->sin.sin_addr.s_addr == b->sin.sin_addr.s_addr; } #endif just in case I misunderstand you. Thanks. -- 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 Wed, May 22, 2013 at 1:28 PM, Cong Wang <amwang@redhat.com> wrote: > On Wed, 2013-05-22 at 00:14 -0700, David Miller wrote: >> From: Cong Wang <amwang@redhat.com> >> Date: Wed, 22 May 2013 12:54:13 +0800 >> > > Alright, I will change all such functions in vxlan.c back to what you > are suggesting. > > For example, change > > static inline > bool vxlan_addr_equal(const union vxlan_addr *a, const union vxlan_addr > *b) > { > #if IS_ENABLED(CONFIG_IPV6) > if (a->sa.sa_family != b->sa.sa_family) > return false; > if (a->sa.sa_family == AF_INET6) > return ipv6_addr_equal(&a->sin6.sin6_addr, > &b->sin6.sin6_addr); > else > #endif > return a->sin.sin_addr.s_addr == b->sin.sin_addr.s_addr; > } > > to > > #if IS_ENABLED(CONFIG_IPV6) > static inline > bool vxlan_addr_equal(const union vxlan_addr *a, const union vxlan_addr > *b) > { > if (a->sa.sa_family != b->sa.sa_family) > return false; > if (a->sa.sa_family == AF_INET6) > return ipv6_addr_equal(&a->sin6.sin6_addr, > &b->sin6.sin6_addr); > else > return a->sin.sin_addr.s_addr == b->sin.sin_addr.s_addr; > } > #else > static inline > bool vxlan_addr_equal(const union vxlan_addr *a, const union vxlan_addr > *b) > { > return a->sin.sin_addr.s_addr == b->sin.sin_addr.s_addr; > } > #endif I think you can just drop #ifdefs in 90% of the cases rather than create two versions of code for IPv4 and IPv6.... > just in case I misunderstand you. > > Thanks. > > -- > 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 -- Sincerely yours, Mike. -- 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
----- Original Message ----- > > I think you can just drop #ifdefs in 90% of the cases rather than > create two versions of code for IPv4 and IPv6.... > I know we can use memcmp(), but comparing 16+ bytes even for IPv4 is not a good idea, also we have to zalloc() every instance of union vxlan_addr. -- 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 Wed, May 22, 2013 at 12:03:23PM -0400, Cong Wang wrote: > > > ----- Original Message ----- > > > > I think you can just drop #ifdefs in 90% of the cases rather than > > create two versions of code for IPv4 and IPv6.... > > > > I know we can use memcmp(), but comparing 16+ bytes even for IPv4 is not > a good idea, also we have to zalloc() every instance of union vxlan_addr. I've lost you here... Why not just: static inline bool vxlan_addr_equal(const union vxlan_addr *a, const union vxlan_addr *b) { if (a->sa.sa_family != b->sa.sa_family) return false; if (a->sa.sa_family == AF_INET6) return ipv6_addr_equal(&a->sin6.sin6_addr, &b->sin6.sin6_addr); else return a->sin.sin_addr.s_addr == b->sin.sin_addr.s_addr; } -- Sincrely yours, Mike. -- 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 Wed, 2013-05-22 at 19:10 +0300, Mike Rapoport wrote: > I've lost you here... Why not just: > > static inline > bool vxlan_addr_equal(const union vxlan_addr *a, const union vxlan_addr *b) > { > if (a->sa.sa_family != b->sa.sa_family) > return false; > if (a->sa.sa_family == AF_INET6) > return ipv6_addr_equal(&a->sin6.sin6_addr, &b->sin6.sin6_addr); > else > return a->sin.sin_addr.s_addr == b->sin.sin_addr.s_addr; > } I see your point now, but for !CONFIG_IPV6, the first two 'if' is obviously useless. Is GCC smart enough to know ->sa.sa_family == AF_INET4 is always true in such case? I doubt... -- 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 Wed, 2013-05-22 at 19:10 +0300, Mike Rapoport wrote: > I've lost you here... Why not just: > > static inline > bool vxlan_addr_equal(const union vxlan_addr *a, const union vxlan_addr *b) > { > if (a->sa.sa_family != b->sa.sa_family) > return false; > if (a->sa.sa_family == AF_INET6) > return ipv6_addr_equal(&a->sin6.sin6_addr, &b->sin6.sin6_addr); > else > return a->sin.sin_addr.s_addr == b->sin.sin_addr.s_addr; > } I see your point now, but for !CONFIG_IPV6, the first two 'if' is obviously useless. Is GCC smart enough to know ->sa.sa_family == AF_INET4 is always true in such case? I doubt... -- 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/include/net/ip6_route.h b/include/net/ip6_route.h index 260f83f..7e9192e 100644 --- a/include/net/ip6_route.h +++ b/include/net/ip6_route.h @@ -21,6 +21,7 @@ struct route_info { #include <net/flow.h> #include <net/ip6_fib.h> #include <net/sock.h> +#include <net/addrconf.h> #include <linux/ip.h> #include <linux/ipv6.h> #include <linux/route.h> @@ -112,8 +113,6 @@ extern struct rt6_info *addrconf_dst_alloc(struct inet6_dev *idev, const struct in6_addr *addr, bool anycast); -extern int ip6_dst_hoplimit(struct dst_entry *dst); - /* * support functions for ND * @@ -201,4 +200,24 @@ static inline struct in6_addr *rt6_nexthop(struct rt6_info *rt, struct in6_addr return dest; } +#if IS_ENABLED(CONFIG_IPV6) +static inline int ip6_dst_hoplimit(struct dst_entry *dst) +{ + int hoplimit = dst_metric_raw(dst, RTAX_HOPLIMIT); + if (hoplimit == 0) { + struct net_device *dev = dst->dev; + struct inet6_dev *idev; + + rcu_read_lock(); + idev = __in6_dev_get(dev); + if (idev) + hoplimit = idev->cnf.hop_limit; + else + hoplimit = dev_net(dev)->ipv6.devconf_all->hop_limit; + rcu_read_unlock(); + } + return hoplimit; +} +#endif + #endif diff --git a/net/ipv6/route.c b/net/ipv6/route.c index ad0aa6b..0d9c531 100644 --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -1310,25 +1310,6 @@ out: return entries > rt_max_size; } -int ip6_dst_hoplimit(struct dst_entry *dst) -{ - int hoplimit = dst_metric_raw(dst, RTAX_HOPLIMIT); - if (hoplimit == 0) { - struct net_device *dev = dst->dev; - struct inet6_dev *idev; - - rcu_read_lock(); - idev = __in6_dev_get(dev); - if (idev) - hoplimit = idev->cnf.hop_limit; - else - hoplimit = dev_net(dev)->ipv6.devconf_all->hop_limit; - rcu_read_unlock(); - } - return hoplimit; -} -EXPORT_SYMBOL(ip6_dst_hoplimit); - /* * */