| Message ID | 51E7B522.4070105@windriver.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
Le 18/07/2013 11:28, Fan Du a écrit : > > Thanks for replying :) > > On 2013年07月18日 17:13, Nicolas Dichtel wrote: >> Le 18/07/2013 05:22, Fan Du a écrit : >>> Hello Nicolas >>> >>> Commit 6f3118b571b8a4c06c7985dc3172c3526cb86253: "ipv6: use net->rt_genid to >>> check dst validity" >>> makes ip6_dst_check to check rt6i_genid against with struct net->rt_genid, >>> As a matter of fact, struct net->rt_genid could only be modified by two places, >>> first is adding/delete IPv4 address, second is inserting new XFRM policy. >>> >>> Is there any other considerations that adding/deleting IPv4 address would >>> invalid all IPv6 dst >>> as well? because I'm working a patch which actually depends on the result of >>> this question. >> No, the goal was to cover the IPsec case, ie invalidate dst entries when an >> xfrm policy is inserted/deleted. > > Ok, then how about we only checking rt6i_genid against rt_genid *only* > when XFRM is enabled for IPv6, because when XFRM is not enabled for IPv6 > ip6_dst_check for rt_genid is really not necessary. > > So what do you think of below modifications? Seems good. Just a small comment below. > > diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h > index 2a601e7..4ae35fd 100644 > --- a/include/net/ip6_fib.h > +++ b/include/net/ip6_fib.h > @@ -119,7 +119,9 @@ struct rt6_info { > struct inet6_dev *rt6i_idev; > unsigned long _rt6i_peer; > > +#ifdef CONFIG_XFRM > u32 rt6i_genid; > +#endif > > /* more non-fragment space at head required */ > unsigned short rt6i_nfheader_len; > diff --git a/net/ipv6/route.c b/net/ipv6/route.c > index bd5fd70..1b64406 100644 > --- a/net/ipv6/route.c > +++ b/net/ipv6/route.c > @@ -277,7 +277,9 @@ static inline struct rt6_info *ip6_dst_alloc(struct net *net, > > memset(dst + 1, 0, sizeof(*rt) - sizeof(*dst)); > rt6_init_peer(rt, table ? &table->tb6_peers : net->ipv6.peers); > +#ifdef CONFIG_XFRM > rt->rt6i_genid = rt_genid(net); > +#endif > INIT_LIST_HEAD(&rt->rt6i_siblings); > rt->rt6i_nsiblings = 0; > } > @@ -1041,12 +1043,15 @@ static struct dst_entry *ip6_dst_check(struct dst_entry > *dst, u32 cookie) > > rt = (struct rt6_info *) dst; > > +#ifdef CONFIG_XFRM > /* All IPV6 dsts are created with ->obsolete set to the value > * DST_OBSOLETE_FORCE_CHK which forces validation calls down > * into this function always. > + * Note: for IPv6, rt6i_genid is noly used when XFRM enabled. indentation is wrong, but I'm not sure this comment is really needed ... you've put the ifdef 4 lines above. > */ > if (rt->rt6i_genid != rt_genid(dev_net(rt->dst.dev))) > return NULL; > +#endif > > if (rt->rt6i_node && (rt->rt6i_node->fn_sernum == cookie)) > return dst; -- 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 2013年07月18日 23:12, Nicolas Dichtel wrote: > Le 18/07/2013 11:28, Fan Du a écrit : >> >> Thanks for replying :) >> >> On 2013年07月18日 17:13, Nicolas Dichtel wrote: >>> Le 18/07/2013 05:22, Fan Du a écrit : >>>> Hello Nicolas >>>> >>>> Commit 6f3118b571b8a4c06c7985dc3172c3526cb86253: "ipv6: use net->rt_genid to >>>> check dst validity" >>>> makes ip6_dst_check to check rt6i_genid against with struct net->rt_genid, >>>> As a matter of fact, struct net->rt_genid could only be modified by two places, >>>> first is adding/delete IPv4 address, second is inserting new XFRM policy. >>>> >>>> Is there any other considerations that adding/deleting IPv4 address would >>>> invalid all IPv6 dst >>>> as well? because I'm working a patch which actually depends on the result of >>>> this question. >>> No, the goal was to cover the IPsec case, ie invalidate dst entries when an >>> xfrm policy is inserted/deleted. >> >> Ok, then how about we only checking rt6i_genid against rt_genid *only* >> when XFRM is enabled for IPv6, because when XFRM is not enabled for IPv6 >> ip6_dst_check for rt_genid is really not necessary. >> >> So what do you think of below modifications? > Seems good. Just a small comment below. Will send v2 for your reviewing when net-next is reopen. Thanks >> >> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h >> index 2a601e7..4ae35fd 100644 >> --- a/include/net/ip6_fib.h >> +++ b/include/net/ip6_fib.h >> @@ -119,7 +119,9 @@ struct rt6_info { >> struct inet6_dev *rt6i_idev; >> unsigned long _rt6i_peer; >> >> +#ifdef CONFIG_XFRM >> u32 rt6i_genid; >> +#endif >> >> /* more non-fragment space at head required */ >> unsigned short rt6i_nfheader_len; >> diff --git a/net/ipv6/route.c b/net/ipv6/route.c >> index bd5fd70..1b64406 100644 >> --- a/net/ipv6/route.c >> +++ b/net/ipv6/route.c >> @@ -277,7 +277,9 @@ static inline struct rt6_info *ip6_dst_alloc(struct net *net, >> >> memset(dst + 1, 0, sizeof(*rt) - sizeof(*dst)); >> rt6_init_peer(rt, table ? &table->tb6_peers : net->ipv6.peers); >> +#ifdef CONFIG_XFRM >> rt->rt6i_genid = rt_genid(net); >> +#endif >> INIT_LIST_HEAD(&rt->rt6i_siblings); >> rt->rt6i_nsiblings = 0; >> } >> @@ -1041,12 +1043,15 @@ static struct dst_entry *ip6_dst_check(struct dst_entry >> *dst, u32 cookie) >> >> rt = (struct rt6_info *) dst; >> >> +#ifdef CONFIG_XFRM >> /* All IPV6 dsts are created with ->obsolete set to the value >> * DST_OBSOLETE_FORCE_CHK which forces validation calls down >> * into this function always. >> + * Note: for IPv6, rt6i_genid is noly used when XFRM enabled. > indentation is wrong, but I'm not sure this comment is really needed ... you've put the ifdef 4 lines above. > >> */ >> if (rt->rt6i_genid != rt_genid(dev_net(rt->dst.dev))) >> return NULL; >> +#endif >> >> if (rt->rt6i_node && (rt->rt6i_node->fn_sernum == cookie)) >> return dst; >
From: Fan Du <fan.du@windriver.com> Date: Fri, 19 Jul 2013 08:01:47 +0800 > > > On 2013年07月18日 23:12, Nicolas Dichtel wrote: >> Le 18/07/2013 11:28, Fan Du a écrit : >>> >>> Thanks for replying :) >>> >>> On 2013年07月18日 17:13, Nicolas Dichtel wrote: >>>> Le 18/07/2013 05:22, Fan Du a écrit : >>>>> Hello Nicolas >>>>> >>>>> Commit 6f3118b571b8a4c06c7985dc3172c3526cb86253: "ipv6: use >>>>> net->rt_genid to >>>>> check dst validity" >>>>> makes ip6_dst_check to check rt6i_genid against with struct >>>>> net->rt_genid, >>>>> As a matter of fact, struct net->rt_genid could only be modified by >>>>> two places, >>>>> first is adding/delete IPv4 address, second is inserting new XFRM >>>>> policy. >>>>> >>>>> Is there any other considerations that adding/deleting IPv4 address >>>>> would >>>>> invalid all IPv6 dst >>>>> as well? because I'm working a patch which actually depends on the >>>>> result of >>>>> this question. >>>> No, the goal was to cover the IPsec case, ie invalidate dst entries >>>> when an >>>> xfrm policy is inserted/deleted. >>> >>> Ok, then how about we only checking rt6i_genid against rt_genid *only* >>> when XFRM is enabled for IPv6, because when XFRM is not enabled for >>> IPv6 >>> ip6_dst_check for rt_genid is really not necessary. >>> >>> So what do you think of below modifications? >> Seems good. Just a small comment below. > > Will send v2 for your reviewing when net-next is reopen. Although it's a correct change, it is of almost no value. %99.9999999 of users will be running kernels with CONFIG_XFRM enabled. So your savings are essentially for no-one.
On 2013年07月19日 11:18, David Miller wrote: > From: Fan Du<fan.du@windriver.com> > Date: Fri, 19 Jul 2013 08:01:47 +0800 > >> >> >> On 2013年07月18日 23:12, Nicolas Dichtel wrote: >>> Le 18/07/2013 11:28, Fan Du a écrit : >>>> >>>> Thanks for replying :) >>>> >>>> On 2013年07月18日 17:13, Nicolas Dichtel wrote: >>>>> Le 18/07/2013 05:22, Fan Du a écrit : >>>>>> Hello Nicolas >>>>>> >>>>>> Commit 6f3118b571b8a4c06c7985dc3172c3526cb86253: "ipv6: use >>>>>> net->rt_genid to >>>>>> check dst validity" >>>>>> makes ip6_dst_check to check rt6i_genid against with struct >>>>>> net->rt_genid, >>>>>> As a matter of fact, struct net->rt_genid could only be modified by >>>>>> two places, >>>>>> first is adding/delete IPv4 address, second is inserting new XFRM >>>>>> policy. >>>>>> >>>>>> Is there any other considerations that adding/deleting IPv4 address >>>>>> would >>>>>> invalid all IPv6 dst >>>>>> as well? because I'm working a patch which actually depends on the >>>>>> result of >>>>>> this question. >>>>> No, the goal was to cover the IPsec case, ie invalidate dst entries >>>>> when an >>>>> xfrm policy is inserted/deleted. >>>> >>>> Ok, then how about we only checking rt6i_genid against rt_genid *only* >>>> when XFRM is enabled for IPv6, because when XFRM is not enabled for >>>> IPv6 >>>> ip6_dst_check for rt_genid is really not necessary. >>>> >>>> So what do you think of below modifications? >>> Seems good. Just a small comment below. >> >> Will send v2 for your reviewing when net-next is reopen. > > Although it's a correct change, it is of almost no value. %99.9999999 > of users will be running kernels with CONFIG_XFRM enabled. Thanks. Good to know %99.99999999 users protect their networking with IPsec. > So your savings are essentially for no-one.
From: Fan Du <fan.du@windriver.com> Date: Fri, 19 Jul 2013 11:28:51 +0800 > On 2013年07月19日 11:18, David Miller wrote: >> Although it's a correct change, it is of almost no value. %99.9999999 >> of users will be running kernels with CONFIG_XFRM enabled. > > Thanks. Good to know %99.99999999 users protect their networking with > IPsec. That is not what I said. I said that nearly every user will be running a kernel with that config option enabled, I did not say that they will actually be using IPSEC. Distributions enable all options, so that users may use any facility that they want. So optimizing for things like this are almost pointless. -- 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 2013年07月19日 11:31, David Miller wrote: > From: Fan Du<fan.du@windriver.com> > Date: Fri, 19 Jul 2013 11:28:51 +0800 > >> On 2013年07月19日 11:18, David Miller wrote: >>> Although it's a correct change, it is of almost no value. %99.9999999 >>> of users will be running kernels with CONFIG_XFRM enabled. >> >> Thanks. Good to know %99.99999999 users protect their networking with >> IPsec. > > That is not what I said. > > I said that nearly every user will be running a kernel with that > config option enabled, I did not say that they will actually be > using IPSEC. > > Distributions enable all options, so that users may use any facility > that they want. > > So optimizing for things like this are almost pointless. > I've understood the situation/point you're trying to describe. No problem, I will drop this almost-pointless patch :) The original commit is targeted for XFRM policy inserting/removing, but it uses net genid shared by both IPv4 and IPv6, the side effect is add/delete IPv4 address will invalidate IPv6 dst in all. We *do* need to bump genid when add/delete IPv6 address in scenario I described here: http://www.spinics.net/lists/netdev/msg243398.html, but definitely not from add/delete IPv4 address. Moreover test shows that DCCP still push thousands of packets on wire after delete its IPv6 address in the same scenario I describe before. The impulse to bump genid for IPv6 is much more stronger after this commit even do it unintentionally. So am I missing some thing more important inside IPv6, Dave?
From: Fan Du <fan.du@windriver.com> Date: Fri, 19 Jul 2013 15:50:20 +0800 > The original commit is targeted for XFRM policy inserting/removing, > but it uses net genid shared by both IPv4 and IPv6, the side effect is > add/delete IPv4 address will invalidate IPv6 dst in all. > > We *do* need to bump genid when add/delete IPv6 address in scenario I > described here: http://www.spinics.net/lists/netdev/msg243398.html, > but definitely not from add/delete IPv4 address. Moreover test shows > that DCCP still push thousands of packets on wire after delete its > IPv6 > address in the same scenario I describe before. > > The impulse to bump genid for IPv6 is much more stronger after this > commit even do it unintentionally. If you really think it will help, and it will still handle the IPSEC case, you can make a seperate genid for ipv4 and ipv6 but that might not work out so cleanly. -- 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_fib.h b/include/net/ip6_fib.h index 2a601e7..4ae35fd 100644 --- a/include/net/ip6_fib.h +++ b/include/net/ip6_fib.h @@ -119,7 +119,9 @@ struct rt6_info { struct inet6_dev *rt6i_idev; unsigned long _rt6i_peer; +#ifdef CONFIG_XFRM u32 rt6i_genid; +#endif /* more non-fragment space at head required */ unsigned short rt6i_nfheader_len; diff --git a/net/ipv6/route.c b/net/ipv6/route.c index bd5fd70..1b64406 100644 --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -277,7 +277,9 @@ static inline struct rt6_info *ip6_dst_alloc(struct net *net, memset(dst + 1, 0, sizeof(*rt) - sizeof(*dst)); rt6_init_peer(rt, table ? &table->tb6_peers : net->ipv6.peers); +#ifdef CONFIG_XFRM rt->rt6i_genid = rt_genid(net); +#endif INIT_LIST_HEAD(&rt->rt6i_siblings); rt->rt6i_nsiblings = 0; } @@ -1041,12 +1043,15 @@ static struct dst_entry *ip6_dst_check(struct dst_entry *dst, u32 cookie) rt = (struct rt6_info *) dst; +#ifdef CONFIG_XFRM /* All IPV6 dsts are created with ->obsolete set to the value * DST_OBSOLETE_FORCE_CHK which forces validation calls down * into this function always. + * Note: for IPv6, rt6i_genid is noly used when XFRM enabled. */ if (rt->rt6i_genid != rt_genid(dev_net(rt->dst.dev))) return NULL; +#endif if (rt->rt6i_node && (rt->rt6i_node->fn_sernum == cookie)) return dst;