Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: makes OVS native tunneling honor tunnel-specified source addresses

2018-04-17 Thread Ben Pfaff
On Tue, Apr 17, 2018 at 10:54:29AM +0800, wenxu wrote:
> 
> 
> 
> 
> 
> 
> 
> 
> 
> At 2018-04-17 04:40:26, "Ben Pfaff"  wrote:
> >On Mon, Apr 16, 2018 at 03:35:57PM +0800, we...@ucloud.cn wrote:
> >> From: wenxu 
> >> 
> >> It makes OVS native tunneling honor tunnel-specified source addresses,
> >> in the same way that Linux kernel tunneling honors them.
> >> 
> >> This patch made valid tun_src specified by flow-action can be used for
> >> tunnel_src of packet. add a "local" property for a route entry.
> >> Like the kernel space when lookup the route, if there are tun_src specified
> >> by flow-action or port options. Check the tun_src wheather is a local
> >> address, then lookup the route.
> >> 
> >> Signed-off-by: wenxu 
> >
> >Thanks for v2.  It resolves the portability issue.  I hope I may ask
> >some more questions, because there are some pieces that I do not
> >understand yet.
> >
> >I think that there is a redundant assignment to change->rd.rtm_dst_len
> >in route_table_parse().  I think that the first assignment here may be
> >removed:
> >
> >change->rd.rtm_dst_len = rtm->rtm_dst_len + (ipv4 ? 96 : 0);
> >if (rtm->rtm_type == RTN_LOCAL) {
> >dst_len_off += 64;
> >change->rd.local = true;
> >} else {
> >change->rd.local = false;
> >}
> >change->rd.rtm_dst_len = rtm->rtm_dst_len + (ipv4 ? dst_len_off : 0);
> >
> >I do not understand dst_len_off yet.  It is normally 96, but for local
> >addresses it increases by 64, to 160.  Then, only for IPv4, it gets
> >added to the prefix length.  I don't understand how a prefix length
> >greater than 128 is to be interpreted, since an IPv6 address is only 128
> >bits long.  (Without the addition of 64, I understand: it is because
> >32-bit IPv4 addresses are mapped into 128-bit IPv6 addresses and 96 =
> >128 - 32.)  rd.rtm_dst_len eventually gets passed to ovs_router_insert()
> >as plen, then to rt_init_match() as plen, and then that is used to
> >create a IPv6 address mask, so it seems like the value should be 128 or
> >less, not 160 or more.
> 
> I want to improve the priority of the local route higher than userdef route
> in the ovs_router_add : the priority of userdef route  is alway higheer than 
> cached route
> ovs_router_insert__(mark, plen + 32, , plen, argv[2], );
> 
> 
> I think I should only enhance the priority of the local route is fine, but 
> not dst_len

That sounds right to me.  Thanks!
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: makes OVS native tunneling honor tunnel-specified source addresses

2018-04-16 Thread wenxu









At 2018-04-17 04:40:26, "Ben Pfaff"  wrote:
>On Mon, Apr 16, 2018 at 03:35:57PM +0800, we...@ucloud.cn wrote:
>> From: wenxu 
>> 
>> It makes OVS native tunneling honor tunnel-specified source addresses,
>> in the same way that Linux kernel tunneling honors them.
>> 
>> This patch made valid tun_src specified by flow-action can be used for
>> tunnel_src of packet. add a "local" property for a route entry.
>> Like the kernel space when lookup the route, if there are tun_src specified
>> by flow-action or port options. Check the tun_src wheather is a local
>> address, then lookup the route.
>> 
>> Signed-off-by: wenxu 
>
>Thanks for v2.  It resolves the portability issue.  I hope I may ask
>some more questions, because there are some pieces that I do not
>understand yet.
>
>I think that there is a redundant assignment to change->rd.rtm_dst_len
>in route_table_parse().  I think that the first assignment here may be
>removed:
>
>change->rd.rtm_dst_len = rtm->rtm_dst_len + (ipv4 ? 96 : 0);
>if (rtm->rtm_type == RTN_LOCAL) {
>dst_len_off += 64;
>change->rd.local = true;
>} else {
>change->rd.local = false;
>}
>change->rd.rtm_dst_len = rtm->rtm_dst_len + (ipv4 ? dst_len_off : 0);
>
>I do not understand dst_len_off yet.  It is normally 96, but for local
>addresses it increases by 64, to 160.  Then, only for IPv4, it gets
>added to the prefix length.  I don't understand how a prefix length
>greater than 128 is to be interpreted, since an IPv6 address is only 128
>bits long.  (Without the addition of 64, I understand: it is because
>32-bit IPv4 addresses are mapped into 128-bit IPv6 addresses and 96 =
>128 - 32.)  rd.rtm_dst_len eventually gets passed to ovs_router_insert()
>as plen, then to rt_init_match() as plen, and then that is used to
>create a IPv6 address mask, so it seems like the value should be 128 or
>less, not 160 or more.

I want to improve the priority of the local route higher than userdef route
in the ovs_router_add : the priority of userdef route  is alway higheer than 
cached route
ovs_router_insert__(mark, plen + 32, , plen, argv[2], );


I think I should only enhance the priority of the local route is fine, but not 
dst_len


>Thanks,
>
>Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev


Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: makes OVS native tunneling honor tunnel-specified source addresses

2018-04-16 Thread Ben Pfaff
On Mon, Apr 16, 2018 at 03:35:57PM +0800, we...@ucloud.cn wrote:
> From: wenxu 
> 
> It makes OVS native tunneling honor tunnel-specified source addresses,
> in the same way that Linux kernel tunneling honors them.
> 
> This patch made valid tun_src specified by flow-action can be used for
> tunnel_src of packet. add a "local" property for a route entry.
> Like the kernel space when lookup the route, if there are tun_src specified
> by flow-action or port options. Check the tun_src wheather is a local
> address, then lookup the route.
> 
> Signed-off-by: wenxu 

Thanks for v2.  It resolves the portability issue.  I hope I may ask
some more questions, because there are some pieces that I do not
understand yet.

I think that there is a redundant assignment to change->rd.rtm_dst_len
in route_table_parse().  I think that the first assignment here may be
removed:

change->rd.rtm_dst_len = rtm->rtm_dst_len + (ipv4 ? 96 : 0);
if (rtm->rtm_type == RTN_LOCAL) {
dst_len_off += 64;
change->rd.local = true;
} else {
change->rd.local = false;
}
change->rd.rtm_dst_len = rtm->rtm_dst_len + (ipv4 ? dst_len_off : 0);

I do not understand dst_len_off yet.  It is normally 96, but for local
addresses it increases by 64, to 160.  Then, only for IPv4, it gets
added to the prefix length.  I don't understand how a prefix length
greater than 128 is to be interpreted, since an IPv6 address is only 128
bits long.  (Without the addition of 64, I understand: it is because
32-bit IPv4 addresses are mapped into 128-bit IPv6 addresses and 96 =
128 - 32.)  rd.rtm_dst_len eventually gets passed to ovs_router_insert()
as plen, then to rt_init_match() as plen, and then that is used to
create a IPv6 address mask, so it seems like the value should be 128 or
less, not 160 or more.

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev