Re: [ovs-dev] [PATCH 3/3] vxlan: Make vxlan tunnel work in IP_TUNNEL_INFO_BRIDGE mode
On 9/23/2020 6:32 AM, Ilya Maximets wrote:
On 9/17/20 7:14 PM, Greg Rose wrote:
From: wenxu
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
Signed-off-by: Greg Rose
---
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(, "%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(, "%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, _cfg);
const char *full_type = (strcmp(type, "vxlan") ? type
Re: [ovs-dev] [PATCH 3/3] vxlan: Make vxlan tunnel work in IP_TUNNEL_INFO_BRIDGE mode
On 9/17/20 7:14 PM, Greg Rose wrote:
> From: wenxu
>
> 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
> Signed-off-by: Greg Rose
> ---
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(, "%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(, "%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 =
[ovs-dev] [PATCH 3/3] vxlan: Make vxlan tunnel work in IP_TUNNEL_INFO_BRIDGE mode
From: wenxu
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
Signed-off-by: Greg Rose
---
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;
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(, "%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(, "%s: only work for vxlan in remote_ip=flow"
+ " mode\n", node->key);
+}
+}
+
enum tunnel_layers layers = tunnel_supported_layers(type, _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 ||
