[ovs-dev] [PATCH] vxlan: Make vxlan tunnel work in IP_TUNNEL_INFO_BRIDGE mode
Ilya Maximets
i.maximets at ovn.org
Wed Sep 9 16:48:46 UTC 2020
On 3/30/19 6:28 AM, wenxu at ucloud.cn 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>
> ---
Hi. I'm looking through old patches after the patchwork cleanup and it
seems like this patch was never actually accepted or even reviewed.
However, the kernel part of this feature was accepted to kernel like
1.5 years ago. It'll be good to have this feature support in userspace
code so it could be actually used.
I have a few comments for this implementation:
1. Comment in openvswitch.h differs from one that accepted to kernel, so
it should be updated.
2. This implementation doens't implement 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 firbid setting this new configuration
for the case where netdev belongs to userspace datapath. This also
will need to be documanted.
3. Since this is a user-visible change, we need a NEWS entry for it.
4. 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. See tests/system-traffic.at.
Anyway, this patch is very old and can not be applied cleanly, it'll be
great if you could rebase it, address above issues and send v2.
What do you think?
Best regards, Ilya Maximets.
> datapath/linux/compat/include/linux/openvswitch.h | 1 +
> include/openvswitch/packets.h | 3 ++-
> lib/netdev-vport.c | 20 +++++++++++++++++++-
> lib/netdev.h | 2 ++
> lib/odp-util.c | 19 ++++++++++++++++---
> lib/packets.h | 6 ++++++
> ofproto/ofproto-dpif-xlate.c | 3 ++-
> ofproto/tunnel.c | 5 +++++
> vswitchd/vswitch.xml | 9 +++++++++
> 9 files changed, 62 insertions(+), 6 deletions(-)
>
> diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
> index d5aa09d..3b23590 100644
> --- a/datapath/linux/compat/include/linux/openvswitch.h
> +++ b/datapath/linux/compat/include/linux/openvswitch.h
> @@ -400,6 +400,7 @@ enum ovs_tunnel_key_attr {
> OVS_TUNNEL_KEY_ATTR_IPV6_DST, /* struct in6_addr dst IPv6 address. */
> OVS_TUNNEL_KEY_ATTR_PAD,
> OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS, /* struct erspan_metadata */
> + OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE, /* No argument.ipv4 info bridge mode */
> __OVS_TUNNEL_KEY_ATTR_MAX
> };
>
> diff --git a/include/openvswitch/packets.h b/include/openvswitch/packets.h
> index 925844e..bad5618 100644
> --- a/include/openvswitch/packets.h
> +++ b/include/openvswitch/packets.h
> @@ -43,7 +43,8 @@ struct flow_tnl {
> uint32_t erspan_idx;
> uint8_t erspan_dir;
> uint8_t erspan_hwid;
> - uint8_t pad1[6]; /* Pad to 64 bits. */
> + uint8_t ipv4_info_bridge;
> + uint8_t pad1[5]; /* Pad to 64 bits. */
> struct tun_metadata metadata;
> };
>
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 808a43f..43ca503 100644
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -553,6 +553,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;
>
> @@ -725,12 +726,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)
> @@ -962,9 +976,13 @@ 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;
> }
> -
> +
> /* Code specific to patch ports. */
>
> /* If 'netdev' is a patch port, returns the name of its peer as a malloc()'d
> diff --git a/lib/netdev.h b/lib/netdev.h
> index d94817f..1fcee9c 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 b6552c5..e3ad8ee 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -2480,6 +2480,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 },
> };
>
> const struct attr_len_tbl ovs_flow_key_attr_lens[OVS_KEY_ATTR_MAX + 1] = {
> @@ -2871,6 +2872,8 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool is_mask,
> }
> break;
> }
> + case OVS_TUNNEL_KEY_ATTR_IPV4_INFO_BRIDGE:
> + tun->ipv4_info_bridge = 1;
>
> default:
> /* Allow this to show up as unexpected, if there are unknown
> @@ -2918,6 +2921,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);
> }
> @@ -2985,6 +2993,7 @@ tun_key_to_attr(struct ofpbuf *a, const struct flow_tnl *tun_key,
> &opts, sizeof(opts));
> }
>
> +out:
> nl_msg_end_nested(a, tun_key_ofs);
> }
>
> @@ -3733,6 +3742,9 @@ format_odp_tun_attr(const struct nlattr *attr, const struct nlattr *mask_attr,
> format_odp_tun_erspan_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);
> @@ -7351,9 +7363,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 d293b35..9b86be2 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -51,6 +51,12 @@ flow_tnl_dst_is_set(const struct flow_tnl *tnl)
> return tnl->ip_dst || ipv6_addr_is_set(&tnl->ipv6_dst);
> }
>
> +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 c4014d7..63eaaa1 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -4076,7 +4076,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 03f0ab7..86204d3 100644
> --- a/ofproto/tunnel.c
> +++ b/ofproto/tunnel.c
> @@ -490,6 +490,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 08001db..5c5728c 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -2848,6 +2848,15 @@
> </li>
> </ul>
> </column>
> +
> + <column name="options" key="allow_info_bridge"
> + type='{"type": "boolean"}'>
> + Optional. Only in reomte_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">
>
More information about the dev
mailing list