[ovs-dev] [PATCH 3/3] vxlan: Make vxlan tunnel work in IP_TUNNEL_INFO_BRIDGE mode
Gregory Rose
gvrose8192 at gmail.com
Mon Sep 28 15:19:48 UTC 2020
On 9/23/2020 6:32 AM, Ilya Maximets wrote:
> On 9/17/20 7:14 PM, Greg Rose wrote:
>> From: wenxu <wenxu at 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 at ucloud.cn>
>> Signed-off-by: Greg Rose <gvrose8192 at 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
More information about the dev
mailing list