| Message ID | 20200917171414.7886-4-gvrose8192@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add IP_TUNNEL_INFO_BRIDGE mode feature | expand |
On 9/17/20 7:14 PM, Greg Rose wrote: > From: wenxu <wenxu@ucloud.cn> > > There is currently no support for the multicast/broadcast aspects > of VXLAN in ovs. In the datapath flow the tun_dst must specific. > But in the IP_TUNNEL_INFO_BRIDGE mode the tun_dst can not be specific. > And the packet can forward through the fdb table of vxlan devcice. In > this mode the broadcast/multicast packet can be sent through the > following ways in ovs. > > It adds an options allow_info_bridge for vxlan tunnel interface to > permit to enable this mode for this vxlan tunnel. > > ovs-vsctl add-port br0 vxlan -- set in vxlan type=vxlan \ > options:key=1000 options:remote_ip=flow > ovs-vsctl set in vxlan options:allow_info_bridge=true > ovs-ofctl add-flow br0 in_port=LOCAL,dl_dst=ff:ff:ff:ff:ff:ff, \ > action=output:vxlan > > bridge fdb append ff:ff:ff:ff:ff:ff dev vxlan_sys_4789 dst 172.168.0.1 \ > src_vni 1000 vni 1000 self > bridge fdb append ff:ff:ff:ff:ff:ff dev vxlan_sys_4789 dst 172.168.0.2 \ > src_vni 1000 vni 1000 self > > Signed-off-by: wenxu <wenxu@ucloud.cn> > Signed-off-by: Greg Rose <gvrose8192@gmail.com> > --- Hi. Thank you very much for rebase and sending new version. However, commnets that I made for the original patch are mostly still valid, so I'll repeat them (and add some new ones) here: 1. This implementation doesn't have support for the userspace datpath and this might produce issues if users will enable new configuration in this case. We need an implementation for the userspace datpath, i.e. in lib/netdev-native-tnl.c, or we need to forbid setting this new configuration for the case where netdev belongs to userspace datapath. This also will need to be documented. 2. Since this is a user-visible change, we need a NEWS entry for it. 3. It'll be good to have a system test for this feature to be sure that we will not break it in the future, especially if userspace/dummy datapaths doesn't support it. 4. We should probbaly check if datapath actually supports this feature before using it, otherwise kernel will fail to install flows with unknown tunnel attribute (the same checking could be used to avoid using this feature with userspace datapath). Couple more comments for the implementation inline. Best regards, Ilya Maximets. > include/openvswitch/packets.h | 3 ++- > lib/netdev-vport.c | 18 ++++++++++++++++++ > lib/netdev.h | 2 ++ > lib/odp-util.c | 21 ++++++++++++++++++--- > lib/packets.h | 6 ++++++ > ofproto/ofproto-dpif-xlate.c | 3 ++- > ofproto/tunnel.c | 5 +++++ > vswitchd/vswitch.xml | 9 +++++++++ > 8 files changed, 62 insertions(+), 5 deletions(-) > > diff --git a/include/openvswitch/packets.h b/include/openvswitch/packets.h > index a65cb0d04..bf3774e84 100644 > --- a/include/openvswitch/packets.h > +++ b/include/openvswitch/packets.h > @@ -43,9 +43,10 @@ struct flow_tnl { > uint32_t erspan_idx; > uint8_t erspan_dir; > uint8_t erspan_hwid; > + uint8_t ipv4_info_bridge; Since this is a flag, why it's not part of 'flags', but a separate field? It seems like we should have something like FLOW_TNL_F_IPV4_INFO_BRIDGE private flag instead. This will also simplify parsing/formatting code, since it's already implemented for flags and will make code cleaner. > uint8_t gtpu_flags; > uint8_t gtpu_msgtype; > - uint8_t pad1[4]; /* Pad to 64 bits. */ > + uint8_t pad1[3]; /* Pad to 64 bits. */ > struct tun_metadata metadata; > }; > > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c > index 0252b61de..4cba7fe20 100644 > --- a/lib/netdev-vport.c > +++ b/lib/netdev-vport.c > @@ -569,6 +569,7 @@ set_tunnel_config(struct netdev *dev_, const struct smap *args, char **errp) > bool needs_dst_port, has_csum, has_seq; > uint16_t dst_proto = 0, src_proto = 0; > struct netdev_tunnel_config tnl_cfg; > + bool allow_info_bridge = false; > struct smap_node *node; > int err; > > @@ -745,12 +746,25 @@ set_tunnel_config(struct netdev *dev_, const struct smap *args, char **errp) > goto out; > } > } > + } else if (!strcmp(node->key, "allow_info_bridge")) { > + if (!strcmp(node->value, "true")) { > + allow_info_bridge = true; > + } > } else { > ds_put_format(&errors, "%s: unknown %s argument '%s'\n", name, > type, node->key); > } > } > > + if (allow_info_bridge) { > + if (!strcmp(type, "vxlan") && tnl_cfg.ip_dst_flow) { > + tnl_cfg.allow_info_bridge = true; > + } else { > + ds_put_format(&errors, "%s: only work for vxlan in remote_ip=flow" > + " mode\n", node->key); 'node' might be different here since we're not breaking from the loop. > + } > + } > + > enum tunnel_layers layers = tunnel_supported_layers(type, &tnl_cfg); > const char *full_type = (strcmp(type, "vxlan") ? type > : (tnl_cfg.exts & (1 << OVS_VXLAN_EXT_GPE) > @@ -983,6 +997,10 @@ get_tunnel_config(const struct netdev *dev, struct smap *args) > } > } > > + if (!strcmp("vxlan", type) && tnl_cfg.allow_info_bridge) { > + smap_add(args, "allow_info_bridge", "true"); > + } > + > return 0; > } > > diff --git a/lib/netdev.h b/lib/netdev.h > index fb5073056..715a8af84 100644 > --- a/lib/netdev.h > +++ b/lib/netdev.h > @@ -139,6 +139,8 @@ struct netdev_tunnel_config { > bool erspan_idx_flow; > bool erspan_dir_flow; > bool erspan_hwid_flow; > + > + bool allow_info_bridge; Maybe 'vxlan_ipv4_info_bridge'? > }; > > void netdev_run(void); > diff --git a/lib/odp-util.c b/lib/odp-util.c > index 5989381e9..b43c599d6 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -2668,6 +2668,7 @@ static const struct attr_len_tbl ovs_tun_key_attr_lens[OVS_TUNNEL_KEY_ATTR_MAX + > [OVS_TUNNEL_KEY_ATTR_IPV6_SRC] = { .len = 16 }, > [OVS_TUNNEL_KEY_ATTR_IPV6_DST] = { .len = 16 }, > [OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS] = { .len = ATTR_LEN_VARIABLE }, > + [OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE] = { .len = 0 }, > [OVS_TUNNEL_KEY_ATTR_GTPU_OPTS] = { .len = ATTR_LEN_VARIABLE }, > }; > > @@ -3081,6 +3082,9 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask, > tun->gtpu_msgtype = opts->msgtype; > break; > } > + case OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE: > + tun->ipv4_info_bridge = 1; > + break; > > default: > /* Allow this to show up as unexpected, if there are unknown > @@ -3128,6 +3132,11 @@ tun_key_to_attr(struct ofpbuf *a, const struct flow_tnl *tun_key, > if (tun_key->tun_id || tun_key->flags & FLOW_TNL_F_KEY) { > nl_msg_put_be64(a, OVS_TUNNEL_KEY_ATTR_ID, tun_key->tun_id); > } > + if (tnl_type && !strcmp(tnl_type, "vxlan") && > + tun_key->ipv4_info_bridge) { > + nl_msg_put_flag(a, OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE); > + goto out; Why this functoin terminated here? What about all other fields of tnl_key? Other ip, checksum, whatever else options? I dont' think we should ignore them. Anyway, this is a utility function and it must construct attributes for everything that is present in tunnel key. > + } > if (tun_key->ip_src) { > nl_msg_put_be32(a, OVS_TUNNEL_KEY_ATTR_IPV4_SRC, tun_key->ip_src); > } > @@ -3204,6 +3213,8 @@ tun_key_to_attr(struct ofpbuf *a, const struct flow_tnl *tun_key, > nl_msg_put_unspec(a, OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS, > &opts, sizeof(opts)); > } > + > +out: > nl_msg_end_nested(a, tun_key_ofs); > } > > @@ -3973,6 +3984,9 @@ format_odp_tun_attr(const struct nlattr *attr, const struct nlattr *mask_attr, > format_odp_tun_gtpu_opt(a, ma, ds, verbose); > ds_put_cstr(ds, ")"); > break; > + case OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE: > + ds_put_cstr(ds, "ipv4_info_bridge,"); > + break; > case __OVS_TUNNEL_KEY_ATTR_MAX: > default: > format_unknown_key(ds, a, ma); > @@ -7687,9 +7701,10 @@ void > commit_odp_tunnel_action(const struct flow *flow, struct flow *base, > struct ofpbuf *odp_actions, const char *tnl_type) > { > - /* A valid IPV4_TUNNEL must have non-zero ip_dst; a valid IPv6 tunnel > - * must have non-zero ipv6_dst. */ > - if (flow_tnl_dst_is_set(&flow->tunnel)) { > + /* A valid IPV4_TUNNEL must have non-zero ip_dst or ipv4 info bridge mode; > + * a valid IPv6 tunnel must have non-zero ipv6_dst. */ > + if (flow_tnl_dst_is_set(&flow->tunnel) || > + flow_tnl_ipv4_info_bridge(&flow->tunnel)) { This check will allow ipv6 with ipv4_info_bridge set, but it should not accroding to the comment. > if (!memcmp(&base->tunnel, &flow->tunnel, sizeof base->tunnel)) { > return; > } > diff --git a/lib/packets.h b/lib/packets.h > index 395bc869e..9edf52113 100644 > --- a/lib/packets.h > +++ b/lib/packets.h > @@ -58,6 +58,12 @@ flow_tnl_src_is_set(const struct flow_tnl *tnl) > return tnl->ip_src || ipv6_addr_is_set(&tnl->ipv6_src); > } > > +static inline bool > +flow_tnl_ipv4_info_bridge(const struct flow_tnl *tnl) > +{ > + return tnl->ipv4_info_bridge; > +} > + > struct in6_addr flow_tnl_dst(const struct flow_tnl *tnl); > struct in6_addr flow_tnl_src(const struct flow_tnl *tnl); > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index e0ede2cab..fb26fb373 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -4172,7 +4172,8 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, > goto out; /* restore flow_nw_tos */ > } > dst = flow_tnl_dst(&flow->tunnel); > - if (ipv6_addr_equals(&dst, &ctx->orig_tunnel_ipv6_dst)) { > + if (!flow_tnl_ipv4_info_bridge(&flow->tunnel) && > + ipv6_addr_equals(&dst, &ctx->orig_tunnel_ipv6_dst)) { Again this check will allow ipv6 address if ipv4_info_bridge is set. > xlate_report(ctx, OFT_WARN, "Not tunneling to our own address"); > goto out; /* restore flow_nw_tos */ > } > diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c > index 3455ed233..18ec93655 100644 > --- a/ofproto/tunnel.c > +++ b/ofproto/tunnel.c > @@ -488,6 +488,11 @@ tnl_port_send(const struct ofport_dpif *ofport, struct flow *flow, > flow->tunnel.erspan_hwid = cfg->erspan_hwid; > } > > + if (cfg->allow_info_bridge && !flow_tnl_dst_is_set(&flow->tunnel) && > + !flow->tunnel.gbp_flags && !flow->tunnel.gbp_id) { This also should check for ipv4, I guess. > + flow->tunnel.ipv4_info_bridge = 1; > + } > + > if (pre_flow_str) { > char *post_flow_str = flow_to_string(flow, NULL); > char *tnl_str = tnl_port_to_string(tnl_port); > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml > index 81c84927f..d9e8f4373 100644 > --- a/vswitchd/vswitch.xml > +++ b/vswitchd/vswitch.xml > @@ -2944,6 +2944,15 @@ > </li> > </ul> > </column> > + > + <column name="options" key="allow_info_bridge" Should it be "ipv4_info_bridge" since it doesn't support ipv6? > + type='{"type": "boolean"}'> > + Optional. Only in remote_ip=flow mode can be enabled. If enabled, > + the tunnel dst is not set in flow. It will ignore all other > + parameters except VNI. It makes the packet forward through the fdb > + of vxlan_sys_x device. Default is disable; set to <code>true</code> > + to enable. > + </column> > </group> > > <group title="Tunnel Options: gre only"> >
On 9/23/2020 6:32 AM, Ilya Maximets wrote: > On 9/17/20 7:14 PM, Greg Rose wrote: >> From: wenxu <wenxu@ucloud.cn> >> >> There is currently no support for the multicast/broadcast aspects >> of VXLAN in ovs. In the datapath flow the tun_dst must specific. >> But in the IP_TUNNEL_INFO_BRIDGE mode the tun_dst can not be specific. >> And the packet can forward through the fdb table of vxlan devcice. In >> this mode the broadcast/multicast packet can be sent through the >> following ways in ovs. >> >> It adds an options allow_info_bridge for vxlan tunnel interface to >> permit to enable this mode for this vxlan tunnel. >> >> ovs-vsctl add-port br0 vxlan -- set in vxlan type=vxlan \ >> options:key=1000 options:remote_ip=flow >> ovs-vsctl set in vxlan options:allow_info_bridge=true >> ovs-ofctl add-flow br0 in_port=LOCAL,dl_dst=ff:ff:ff:ff:ff:ff, \ >> action=output:vxlan >> >> bridge fdb append ff:ff:ff:ff:ff:ff dev vxlan_sys_4789 dst 172.168.0.1 \ >> src_vni 1000 vni 1000 self >> bridge fdb append ff:ff:ff:ff:ff:ff dev vxlan_sys_4789 dst 172.168.0.2 \ >> src_vni 1000 vni 1000 self >> >> Signed-off-by: wenxu <wenxu@ucloud.cn> >> Signed-off-by: Greg Rose <gvrose8192@gmail.com> >> --- > > Hi. Thank you very much for rebase and sending new version. However, > commnets that I made for the original patch are mostly still valid, so > I'll repeat them (and add some new ones) here: > > 1. This implementation doesn't have support for the userspace datpath > and this might produce issues if users will enable new configuration > in this case. We need an implementation for the userspace datpath, i.e. > in lib/netdev-native-tnl.c, or we need to forbid setting this new > configuration for the case where netdev belongs to userspace datapath. > This also will need to be documented. > > 2. Since this is a user-visible change, we need a NEWS entry for it. > > 3. It'll be good to have a system test for this feature to be sure that > we will not break it in the future, especially if userspace/dummy > datapaths doesn't support it. > > 4. We should probbaly check if datapath actually supports this feature > before using it, otherwise kernel will fail to install flows with > unknown tunnel attribute (the same checking could be used to avoid > using this feature with userspace datapath). > > Couple more comments for the implementation inline. > > Best regards, Ilya Maximets. > >> include/openvswitch/packets.h | 3 ++- >> lib/netdev-vport.c | 18 ++++++++++++++++++ >> lib/netdev.h | 2 ++ >> lib/odp-util.c | 21 ++++++++++++++++++--- >> lib/packets.h | 6 ++++++ >> ofproto/ofproto-dpif-xlate.c | 3 ++- >> ofproto/tunnel.c | 5 +++++ >> vswitchd/vswitch.xml | 9 +++++++++ >> 8 files changed, 62 insertions(+), 5 deletions(-) >> >> diff --git a/include/openvswitch/packets.h b/include/openvswitch/packets.h >> index a65cb0d04..bf3774e84 100644 >> --- a/include/openvswitch/packets.h >> +++ b/include/openvswitch/packets.h >> @@ -43,9 +43,10 @@ struct flow_tnl { >> uint32_t erspan_idx; >> uint8_t erspan_dir; >> uint8_t erspan_hwid; >> + uint8_t ipv4_info_bridge; > > Since this is a flag, why it's not part of 'flags', but a separate field? > It seems like we should have something like FLOW_TNL_F_IPV4_INFO_BRIDGE > private flag instead. This will also simplify parsing/formatting code, > since it's already implemented for flags and will make code cleaner. > >> uint8_t gtpu_flags; >> uint8_t gtpu_msgtype; >> - uint8_t pad1[4]; /* Pad to 64 bits. */ >> + uint8_t pad1[3]; /* Pad to 64 bits. */ >> struct tun_metadata metadata; >> }; >> >> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c >> index 0252b61de..4cba7fe20 100644 >> --- a/lib/netdev-vport.c >> +++ b/lib/netdev-vport.c >> @@ -569,6 +569,7 @@ set_tunnel_config(struct netdev *dev_, const struct smap *args, char **errp) >> bool needs_dst_port, has_csum, has_seq; >> uint16_t dst_proto = 0, src_proto = 0; >> struct netdev_tunnel_config tnl_cfg; >> + bool allow_info_bridge = false; >> struct smap_node *node; >> int err; >> >> @@ -745,12 +746,25 @@ set_tunnel_config(struct netdev *dev_, const struct smap *args, char **errp) >> goto out; >> } >> } >> + } else if (!strcmp(node->key, "allow_info_bridge")) { >> + if (!strcmp(node->value, "true")) { >> + allow_info_bridge = true; >> + } >> } else { >> ds_put_format(&errors, "%s: unknown %s argument '%s'\n", name, >> type, node->key); >> } >> } >> >> + if (allow_info_bridge) { >> + if (!strcmp(type, "vxlan") && tnl_cfg.ip_dst_flow) { >> + tnl_cfg.allow_info_bridge = true; >> + } else { >> + ds_put_format(&errors, "%s: only work for vxlan in remote_ip=flow" >> + " mode\n", node->key); > > 'node' might be different here since we're not breaking from the loop. > >> + } >> + } >> + >> enum tunnel_layers layers = tunnel_supported_layers(type, &tnl_cfg); >> const char *full_type = (strcmp(type, "vxlan") ? type >> : (tnl_cfg.exts & (1 << OVS_VXLAN_EXT_GPE) >> @@ -983,6 +997,10 @@ get_tunnel_config(const struct netdev *dev, struct smap *args) >> } >> } >> >> + if (!strcmp("vxlan", type) && tnl_cfg.allow_info_bridge) { >> + smap_add(args, "allow_info_bridge", "true"); >> + } >> + >> return 0; >> } >> >> diff --git a/lib/netdev.h b/lib/netdev.h >> index fb5073056..715a8af84 100644 >> --- a/lib/netdev.h >> +++ b/lib/netdev.h >> @@ -139,6 +139,8 @@ struct netdev_tunnel_config { >> bool erspan_idx_flow; >> bool erspan_dir_flow; >> bool erspan_hwid_flow; >> + >> + bool allow_info_bridge; > > Maybe 'vxlan_ipv4_info_bridge'? > >> }; >> >> void netdev_run(void); >> diff --git a/lib/odp-util.c b/lib/odp-util.c >> index 5989381e9..b43c599d6 100644 >> --- a/lib/odp-util.c >> +++ b/lib/odp-util.c >> @@ -2668,6 +2668,7 @@ static const struct attr_len_tbl ovs_tun_key_attr_lens[OVS_TUNNEL_KEY_ATTR_MAX + >> [OVS_TUNNEL_KEY_ATTR_IPV6_SRC] = { .len = 16 }, >> [OVS_TUNNEL_KEY_ATTR_IPV6_DST] = { .len = 16 }, >> [OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS] = { .len = ATTR_LEN_VARIABLE }, >> + [OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE] = { .len = 0 }, >> [OVS_TUNNEL_KEY_ATTR_GTPU_OPTS] = { .len = ATTR_LEN_VARIABLE }, >> }; >> >> @@ -3081,6 +3082,9 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask, >> tun->gtpu_msgtype = opts->msgtype; >> break; >> } >> + case OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE: >> + tun->ipv4_info_bridge = 1; >> + break; >> >> default: >> /* Allow this to show up as unexpected, if there are unknown >> @@ -3128,6 +3132,11 @@ tun_key_to_attr(struct ofpbuf *a, const struct flow_tnl *tun_key, >> if (tun_key->tun_id || tun_key->flags & FLOW_TNL_F_KEY) { >> nl_msg_put_be64(a, OVS_TUNNEL_KEY_ATTR_ID, tun_key->tun_id); >> } >> + if (tnl_type && !strcmp(tnl_type, "vxlan") && >> + tun_key->ipv4_info_bridge) { >> + nl_msg_put_flag(a, OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE); >> + goto out; > > Why this functoin terminated here? > What about all other fields of tnl_key? Other ip, checksum, > whatever else options? I dont' think we should ignore them. > > Anyway, this is a utility function and it must construct attributes for everything > that is present in tunnel key. > >> + } >> if (tun_key->ip_src) { >> nl_msg_put_be32(a, OVS_TUNNEL_KEY_ATTR_IPV4_SRC, tun_key->ip_src); >> } >> @@ -3204,6 +3213,8 @@ tun_key_to_attr(struct ofpbuf *a, const struct flow_tnl *tun_key, >> nl_msg_put_unspec(a, OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS, >> &opts, sizeof(opts)); >> } >> + >> +out: >> nl_msg_end_nested(a, tun_key_ofs); >> } >> >> @@ -3973,6 +3984,9 @@ format_odp_tun_attr(const struct nlattr *attr, const struct nlattr *mask_attr, >> format_odp_tun_gtpu_opt(a, ma, ds, verbose); >> ds_put_cstr(ds, ")"); >> break; >> + case OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE: >> + ds_put_cstr(ds, "ipv4_info_bridge,"); >> + break; >> case __OVS_TUNNEL_KEY_ATTR_MAX: >> default: >> format_unknown_key(ds, a, ma); >> @@ -7687,9 +7701,10 @@ void >> commit_odp_tunnel_action(const struct flow *flow, struct flow *base, >> struct ofpbuf *odp_actions, const char *tnl_type) >> { >> - /* A valid IPV4_TUNNEL must have non-zero ip_dst; a valid IPv6 tunnel >> - * must have non-zero ipv6_dst. */ >> - if (flow_tnl_dst_is_set(&flow->tunnel)) { >> + /* A valid IPV4_TUNNEL must have non-zero ip_dst or ipv4 info bridge mode; >> + * a valid IPv6 tunnel must have non-zero ipv6_dst. */ >> + if (flow_tnl_dst_is_set(&flow->tunnel) || >> + flow_tnl_ipv4_info_bridge(&flow->tunnel)) { > > This check will allow ipv6 with ipv4_info_bridge set, but it should not accroding > to the comment. > >> if (!memcmp(&base->tunnel, &flow->tunnel, sizeof base->tunnel)) { >> return; >> } >> diff --git a/lib/packets.h b/lib/packets.h >> index 395bc869e..9edf52113 100644 >> --- a/lib/packets.h >> +++ b/lib/packets.h >> @@ -58,6 +58,12 @@ flow_tnl_src_is_set(const struct flow_tnl *tnl) >> return tnl->ip_src || ipv6_addr_is_set(&tnl->ipv6_src); >> } >> >> +static inline bool >> +flow_tnl_ipv4_info_bridge(const struct flow_tnl *tnl) >> +{ >> + return tnl->ipv4_info_bridge; >> +} >> + >> struct in6_addr flow_tnl_dst(const struct flow_tnl *tnl); >> struct in6_addr flow_tnl_src(const struct flow_tnl *tnl); >> >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index e0ede2cab..fb26fb373 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -4172,7 +4172,8 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, >> goto out; /* restore flow_nw_tos */ >> } >> dst = flow_tnl_dst(&flow->tunnel); >> - if (ipv6_addr_equals(&dst, &ctx->orig_tunnel_ipv6_dst)) { >> + if (!flow_tnl_ipv4_info_bridge(&flow->tunnel) && >> + ipv6_addr_equals(&dst, &ctx->orig_tunnel_ipv6_dst)) { > > Again this check will allow ipv6 address if ipv4_info_bridge is set. > >> xlate_report(ctx, OFT_WARN, "Not tunneling to our own address"); >> goto out; /* restore flow_nw_tos */ >> } >> diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c >> index 3455ed233..18ec93655 100644 >> --- a/ofproto/tunnel.c >> +++ b/ofproto/tunnel.c >> @@ -488,6 +488,11 @@ tnl_port_send(const struct ofport_dpif *ofport, struct flow *flow, >> flow->tunnel.erspan_hwid = cfg->erspan_hwid; >> } >> >> + if (cfg->allow_info_bridge && !flow_tnl_dst_is_set(&flow->tunnel) && >> + !flow->tunnel.gbp_flags && !flow->tunnel.gbp_id) { > > This also should check for ipv4, I guess. > >> + flow->tunnel.ipv4_info_bridge = 1; >> + } >> + >> if (pre_flow_str) { >> char *post_flow_str = flow_to_string(flow, NULL); >> char *tnl_str = tnl_port_to_string(tnl_port); >> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml >> index 81c84927f..d9e8f4373 100644 >> --- a/vswitchd/vswitch.xml >> +++ b/vswitchd/vswitch.xml >> @@ -2944,6 +2944,15 @@ >> </li> >> </ul> >> </column> >> + >> + <column name="options" key="allow_info_bridge" > > Should it be "ipv4_info_bridge" since it doesn't support ipv6? > >> + type='{"type": "boolean"}'> >> + Optional. Only in remote_ip=flow mode can be enabled. If enabled, >> + the tunnel dst is not set in flow. It will ignore all other >> + parameters except VNI. It makes the packet forward through the fdb >> + of vxlan_sys_x device. Default is disable; set to <code>true</code> >> + to enable. >> + </column> >> </group> >> >> <group title="Tunnel Options: gre only"> >> > Hi Ilya, Thanks for the review and suggestions. I've been out sick for a while but have returned to work and will work up a V2 of this series. - Greg
diff --git a/include/openvswitch/packets.h b/include/openvswitch/packets.h index a65cb0d04..bf3774e84 100644 --- a/include/openvswitch/packets.h +++ b/include/openvswitch/packets.h @@ -43,9 +43,10 @@ struct flow_tnl { uint32_t erspan_idx; uint8_t erspan_dir; uint8_t erspan_hwid; + uint8_t ipv4_info_bridge; uint8_t gtpu_flags; uint8_t gtpu_msgtype; - uint8_t pad1[4]; /* Pad to 64 bits. */ + uint8_t pad1[3]; /* Pad to 64 bits. */ struct tun_metadata metadata; }; diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index 0252b61de..4cba7fe20 100644 --- a/lib/netdev-vport.c +++ b/lib/netdev-vport.c @@ -569,6 +569,7 @@ set_tunnel_config(struct netdev *dev_, const struct smap *args, char **errp) bool needs_dst_port, has_csum, has_seq; uint16_t dst_proto = 0, src_proto = 0; struct netdev_tunnel_config tnl_cfg; + bool allow_info_bridge = false; struct smap_node *node; int err; @@ -745,12 +746,25 @@ set_tunnel_config(struct netdev *dev_, const struct smap *args, char **errp) goto out; } } + } else if (!strcmp(node->key, "allow_info_bridge")) { + if (!strcmp(node->value, "true")) { + allow_info_bridge = true; + } } else { ds_put_format(&errors, "%s: unknown %s argument '%s'\n", name, type, node->key); } } + if (allow_info_bridge) { + if (!strcmp(type, "vxlan") && tnl_cfg.ip_dst_flow) { + tnl_cfg.allow_info_bridge = true; + } else { + ds_put_format(&errors, "%s: only work for vxlan in remote_ip=flow" + " mode\n", node->key); + } + } + enum tunnel_layers layers = tunnel_supported_layers(type, &tnl_cfg); const char *full_type = (strcmp(type, "vxlan") ? type : (tnl_cfg.exts & (1 << OVS_VXLAN_EXT_GPE) @@ -983,6 +997,10 @@ get_tunnel_config(const struct netdev *dev, struct smap *args) } } + if (!strcmp("vxlan", type) && tnl_cfg.allow_info_bridge) { + smap_add(args, "allow_info_bridge", "true"); + } + return 0; } diff --git a/lib/netdev.h b/lib/netdev.h index fb5073056..715a8af84 100644 --- a/lib/netdev.h +++ b/lib/netdev.h @@ -139,6 +139,8 @@ struct netdev_tunnel_config { bool erspan_idx_flow; bool erspan_dir_flow; bool erspan_hwid_flow; + + bool allow_info_bridge; }; void netdev_run(void); diff --git a/lib/odp-util.c b/lib/odp-util.c index 5989381e9..b43c599d6 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -2668,6 +2668,7 @@ static const struct attr_len_tbl ovs_tun_key_attr_lens[OVS_TUNNEL_KEY_ATTR_MAX + [OVS_TUNNEL_KEY_ATTR_IPV6_SRC] = { .len = 16 }, [OVS_TUNNEL_KEY_ATTR_IPV6_DST] = { .len = 16 }, [OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS] = { .len = ATTR_LEN_VARIABLE }, + [OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE] = { .len = 0 }, [OVS_TUNNEL_KEY_ATTR_GTPU_OPTS] = { .len = ATTR_LEN_VARIABLE }, }; @@ -3081,6 +3082,9 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask, tun->gtpu_msgtype = opts->msgtype; break; } + case OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE: + tun->ipv4_info_bridge = 1; + break; default: /* Allow this to show up as unexpected, if there are unknown @@ -3128,6 +3132,11 @@ tun_key_to_attr(struct ofpbuf *a, const struct flow_tnl *tun_key, if (tun_key->tun_id || tun_key->flags & FLOW_TNL_F_KEY) { nl_msg_put_be64(a, OVS_TUNNEL_KEY_ATTR_ID, tun_key->tun_id); } + if (tnl_type && !strcmp(tnl_type, "vxlan") && + tun_key->ipv4_info_bridge) { + nl_msg_put_flag(a, OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE); + goto out; + } if (tun_key->ip_src) { nl_msg_put_be32(a, OVS_TUNNEL_KEY_ATTR_IPV4_SRC, tun_key->ip_src); } @@ -3204,6 +3213,8 @@ tun_key_to_attr(struct ofpbuf *a, const struct flow_tnl *tun_key, nl_msg_put_unspec(a, OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS, &opts, sizeof(opts)); } + +out: nl_msg_end_nested(a, tun_key_ofs); } @@ -3973,6 +3984,9 @@ format_odp_tun_attr(const struct nlattr *attr, const struct nlattr *mask_attr, format_odp_tun_gtpu_opt(a, ma, ds, verbose); ds_put_cstr(ds, ")"); break; + case OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE: + ds_put_cstr(ds, "ipv4_info_bridge,"); + break; case __OVS_TUNNEL_KEY_ATTR_MAX: default: format_unknown_key(ds, a, ma); @@ -7687,9 +7701,10 @@ void commit_odp_tunnel_action(const struct flow *flow, struct flow *base, struct ofpbuf *odp_actions, const char *tnl_type) { - /* A valid IPV4_TUNNEL must have non-zero ip_dst; a valid IPv6 tunnel - * must have non-zero ipv6_dst. */ - if (flow_tnl_dst_is_set(&flow->tunnel)) { + /* A valid IPV4_TUNNEL must have non-zero ip_dst or ipv4 info bridge mode; + * a valid IPv6 tunnel must have non-zero ipv6_dst. */ + if (flow_tnl_dst_is_set(&flow->tunnel) || + flow_tnl_ipv4_info_bridge(&flow->tunnel)) { if (!memcmp(&base->tunnel, &flow->tunnel, sizeof base->tunnel)) { return; } diff --git a/lib/packets.h b/lib/packets.h index 395bc869e..9edf52113 100644 --- a/lib/packets.h +++ b/lib/packets.h @@ -58,6 +58,12 @@ flow_tnl_src_is_set(const struct flow_tnl *tnl) return tnl->ip_src || ipv6_addr_is_set(&tnl->ipv6_src); } +static inline bool +flow_tnl_ipv4_info_bridge(const struct flow_tnl *tnl) +{ + return tnl->ipv4_info_bridge; +} + struct in6_addr flow_tnl_dst(const struct flow_tnl *tnl); struct in6_addr flow_tnl_src(const struct flow_tnl *tnl); diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index e0ede2cab..fb26fb373 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -4172,7 +4172,8 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t ofp_port, goto out; /* restore flow_nw_tos */ } dst = flow_tnl_dst(&flow->tunnel); - if (ipv6_addr_equals(&dst, &ctx->orig_tunnel_ipv6_dst)) { + if (!flow_tnl_ipv4_info_bridge(&flow->tunnel) && + ipv6_addr_equals(&dst, &ctx->orig_tunnel_ipv6_dst)) { xlate_report(ctx, OFT_WARN, "Not tunneling to our own address"); goto out; /* restore flow_nw_tos */ } diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c index 3455ed233..18ec93655 100644 --- a/ofproto/tunnel.c +++ b/ofproto/tunnel.c @@ -488,6 +488,11 @@ tnl_port_send(const struct ofport_dpif *ofport, struct flow *flow, flow->tunnel.erspan_hwid = cfg->erspan_hwid; } + if (cfg->allow_info_bridge && !flow_tnl_dst_is_set(&flow->tunnel) && + !flow->tunnel.gbp_flags && !flow->tunnel.gbp_id) { + flow->tunnel.ipv4_info_bridge = 1; + } + if (pre_flow_str) { char *post_flow_str = flow_to_string(flow, NULL); char *tnl_str = tnl_port_to_string(tnl_port); diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 81c84927f..d9e8f4373 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -2944,6 +2944,15 @@ </li> </ul> </column> + + <column name="options" key="allow_info_bridge" + type='{"type": "boolean"}'> + Optional. Only in remote_ip=flow mode can be enabled. If enabled, + the tunnel dst is not set in flow. It will ignore all other + parameters except VNI. It makes the packet forward through the fdb + of vxlan_sys_x device. Default is disable; set to <code>true</code> + to enable. + </column> </group> <group title="Tunnel Options: gre only">