[ovs-dev] [PATCH] datapath: Introduce reference counter in datapath tunnel port
Balazs Nemeth
balazs.nemeth at ericsson.com
Wed Jul 5 14:11:14 UTC 2017
There is a problem with deleting one out of more OF tunnel ports with
the same port type. In OVS all OF tunnel ports sharing the same tunnel
type and dst_port setting are mapped to a common tunnel port instance
in the datapath. Available tunnel types are GRE, VXLAN, GENEVE and
STT. dst_port is optional and only present for UDP/TCP-based tunnels.
OVS has no counter for storing how many OF tunnel ports exist of the
same port type, thus when deleting a tunnel port, current OVS
implementation removes tunnel port from datapath too, independently
from eventual other existing OF tunnel ports mapped to that datapath
port. As a consequence datapath traffic is stopped (100% packet loss)
on further tunnel ports of the same type as the deleted one.
In addition to this there is another issue with tunnel port deletion
when reconfiguring OF tunnels. If the dst_port value is changed, the
old tunnel map entry will not be deleted, due to the tp_port argument
of tnl_port_map_delete() has the new dst_port setting, and the tunnel
can not be found in the list of tnl_port structures.
This patch has a fix for the first issue with adding a reference
counter to the tnl_ports object in tnl-ports.c which tracks the
number of OF tunnel ports that are bound to the same datapath port
and only deletes the tunnel map entries for the datapath tunnel port
when the reference counter is decreased to zero.
The second issue is fixed with adding a new argument, 'old_odp_port'
to tnl_port_reconfigure(). This value is used to identify the
datapath tunnel port which is being reconfigured. In connection with
this fix, to unify the tunnel port map handling, odp_port value
is used to search the proper port to insert and delete tunnel map
entries as well. This variable can be used instead of tp_port, as it
is unique for all datapath tunnel ports, and there is no need to
reach dst_port from netdev_tunnel_config structure.
Extending OVS unit test cases to have ref_cnt values in the expected
dump. Adding new test cases to check if packet receiving is still
working in the case of OF tunnel port deletion. Adding new test cases
to check the reference counter in case of OF tunnel deletion or
reconfiguration.
Signed-off-by: Balazs Nemeth <balazs.nemeth at ericsson.com>
Co-authored-by: Jan Scheurich <jan.scheurich at ericsson.com>
Signed-off-by: Jan Scheurich <jan.scheurich at ericsson.com>
---
lib/tnl-ports.c | 26 +++++++++++++++++---------
lib/tnl-ports.h | 4 ++--
ofproto/ofproto-dpif.c | 8 +++++---
ofproto/tunnel.c | 37 ++++++++++++++++++++-----------------
ofproto/tunnel.h | 7 ++++---
tests/tunnel-push-pop-ipv6.at | 33 ++++++++++++++++++++++++++++++---
tests/tunnel-push-pop.at | 35 ++++++++++++++++++++++++++++++++---
7 files changed, 110 insertions(+), 40 deletions(-)
diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c
index 4a07dcb..3f87575 100644
--- a/lib/tnl-ports.c
+++ b/lib/tnl-ports.c
@@ -55,6 +55,8 @@ struct tnl_port {
uint8_t nw_proto;
char dev_name[IFNAMSIZ];
struct ovs_list node;
+ struct ovs_refcount ref_cnt; /* Number of 'ofport's using
+ * this datapath tunnel port. */
};
static struct ovs_list port_list;
@@ -194,8 +196,9 @@ tnl_port_map_insert(odp_port_t port, ovs_be16 tp_port,
ovs_mutex_lock(&mutex);
LIST_FOR_EACH(p, node, &port_list) {
- if (tp_port == p->tp_port && p->nw_proto == nw_proto) {
- goto out;
+ if (p->port == port && p->nw_proto == nw_proto) {
+ ovs_refcount_ref(&p->ref_cnt);
+ goto out;
}
}
@@ -205,6 +208,7 @@ tnl_port_map_insert(odp_port_t port, ovs_be16 tp_port,
p->nw_proto = nw_proto;
ovs_strlcpy(p->dev_name, dev_name, sizeof p->dev_name);
ovs_list_insert(&port_list, &p->node);
+ ovs_refcount_init(&p->ref_cnt);
LIST_FOR_EACH(ip_dev, node, &addr_list) {
map_insert_ipdev__(ip_dev, p->dev_name, p->port, p->nw_proto, p->tp_port);
@@ -252,25 +256,28 @@ ipdev_map_delete(struct ip_device *ip_dev, ovs_be16 tp_port, uint8_t nw_proto)
}
void
-tnl_port_map_delete(ovs_be16 tp_port, const char type[])
+tnl_port_map_delete(odp_port_t port, const char type[])
{
struct tnl_port *p, *next;
struct ip_device *ip_dev;
- bool found = false;
+ bool deleted = false;
uint8_t nw_proto;
nw_proto = tnl_type_to_nw_proto(type);
ovs_mutex_lock(&mutex);
LIST_FOR_EACH_SAFE(p, next, node, &port_list) {
- if (p->tp_port == tp_port && p->nw_proto == nw_proto) {
- ovs_list_remove(&p->node);
- found = true;
+ if (p->port == port && p->nw_proto == nw_proto) {
+ if (ovs_refcount_unref(&p->ref_cnt) == 1) {
+ /* Time to delete the tnl_port from the list and clean up. */
+ ovs_list_remove(&p->node);
+ deleted = true;
+ }
break;
}
}
- if (!found) {
+ if (!deleted) {
goto out;
}
LIST_FOR_EACH(ip_dev, node, &addr_list) {
@@ -352,7 +359,8 @@ tnl_port_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
}
LIST_FOR_EACH(p, node, &port_list) {
- ds_put_format(&ds, "%s (%"PRIu32")\n", p->dev_name, p->port);
+ ds_put_format(&ds, "%s (%"PRIu32") ref_cnt=%u\n", p->dev_name, p->port,
+ ovs_refcount_read(&p->ref_cnt));
}
out:
diff --git a/lib/tnl-ports.h b/lib/tnl-ports.h
index 58b048a..61ca0f8 100644
--- a/lib/tnl-ports.h
+++ b/lib/tnl-ports.h
@@ -26,10 +26,10 @@
odp_port_t tnl_port_map_lookup(struct flow *flow, struct flow_wildcards *wc);
-void tnl_port_map_insert(odp_port_t port, ovs_be16 udp_port,
+void tnl_port_map_insert(odp_port_t, ovs_be16 tp_port,
const char dev_name[], const char type[]);
-void tnl_port_map_delete(ovs_be16 udp_port, const char type[]);
+void tnl_port_map_delete(odp_port_t, const char type[]);
void tnl_port_map_insert_ipdev(const char dev[]);
void tnl_port_map_delete_ipdev(const char dev[]);
void tnl_port_map_run(void);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index d19d486..bd914ef 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -378,6 +378,7 @@ type_run(const char *type)
HMAP_FOR_EACH (iter, up.hmap_node, &ofproto->up.ports) {
char namebuf[NETDEV_VPORT_NAME_BUFSIZE];
const char *dp_port;
+ odp_port_t old_odp_port;
if (!iter->is_tunnel) {
continue;
@@ -385,6 +386,7 @@ type_run(const char *type)
dp_port = netdev_vport_get_dpif_port(iter->up.netdev,
namebuf, sizeof namebuf);
+ old_odp_port = iter->odp_port;
node = simap_find(&tmp_backers, dp_port);
if (node) {
simap_put(&backer->tnl_backers, dp_port, node->data);
@@ -406,7 +408,7 @@ type_run(const char *type)
iter->odp_port = node ? u32_to_odp(node->data) : ODPP_NONE;
if (tnl_port_reconfigure(iter, iter->up.netdev,
- iter->odp_port,
+ iter->odp_port, old_odp_port,
ovs_native_tunneling_is_on(ofproto), dp_port)) {
backer->need_revalidate = REV_RECONFIGURE;
}
@@ -1935,7 +1937,7 @@ port_destruct(struct ofport *port_, bool del)
dpif_ipfix_del_tunnel_port(ofproto->ipfix, port->odp_port);
}
- tnl_port_del(port);
+ tnl_port_del(port, port->odp_port);
sset_find_and_delete(&ofproto->ports, devname);
sset_find_and_delete(&ofproto->ghost_ports, devname);
bundle_remove(port_);
@@ -1991,7 +1993,7 @@ port_modified(struct ofport *port_)
if (port->is_tunnel) {
struct ofproto_dpif *ofproto = ofproto_dpif_cast(port->up.ofproto);
- if (tnl_port_reconfigure(port, netdev, port->odp_port,
+ if (tnl_port_reconfigure(port, netdev, port->odp_port, port->odp_port,
ovs_native_tunneling_is_on(ofproto),
dp_port_name)) {
ofproto->backer->need_revalidate = REV_RECONFIGURE;
diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
index c6856a0..1676f4d 100644
--- a/ofproto/tunnel.c
+++ b/ofproto/tunnel.c
@@ -126,7 +126,8 @@ static void tnl_port_mod_log(const struct tnl_port *, const char *action)
OVS_REQ_RDLOCK(rwlock);
static const char *tnl_port_get_name(const struct tnl_port *)
OVS_REQ_RDLOCK(rwlock);
-static void tnl_port_del__(const struct ofport_dpif *) OVS_REQ_WRLOCK(rwlock);
+static void tnl_port_del__(const struct ofport_dpif *, odp_port_t)
+ OVS_REQ_WRLOCK(rwlock);
void
ofproto_tunnel_init(void)
@@ -221,13 +222,15 @@ tnl_port_add(const struct ofport_dpif *ofport, const struct netdev *netdev,
}
/* Checks if the tunnel represented by 'ofport' reconfiguration due to changes
- * in its netdev_tunnel_config. If it does, returns true. Otherwise, returns
- * false. 'ofport' and 'odp_port' should be the same as would be passed to
- * tnl_port_add(). */
+ * in its netdev_tunnel_config. If it does, returns true. Otherwise, returns
+ * false. 'new_odp_port' should be the port number coming from 'ofport' that
+ * is passed to tnl_port_add__(). 'old_odp_port' should be the port number
+ * that is passed to tnl_port_del__(). */
bool
tnl_port_reconfigure(const struct ofport_dpif *ofport,
- const struct netdev *netdev, odp_port_t odp_port,
- bool native_tnl, const char name[])
+ const struct netdev *netdev, odp_port_t new_odp_port,
+ odp_port_t old_odp_port, bool native_tnl,
+ const char name[])
OVS_EXCLUDED(rwlock)
{
struct tnl_port *tnl_port;
@@ -236,14 +239,14 @@ tnl_port_reconfigure(const struct ofport_dpif *ofport,
fat_rwlock_wrlock(&rwlock);
tnl_port = tnl_find_ofport(ofport);
if (!tnl_port) {
- changed = tnl_port_add__(ofport, netdev, odp_port, false, native_tnl,
- name);
+ changed = tnl_port_add__(ofport, netdev, new_odp_port, false,
+ native_tnl, name);
} else if (tnl_port->netdev != netdev
- || tnl_port->match.odp_port != odp_port
+ || tnl_port->match.odp_port != new_odp_port
|| tnl_port->change_seq != netdev_get_change_seq(tnl_port->netdev)) {
VLOG_DBG("reconfiguring %s", tnl_port_get_name(tnl_port));
- tnl_port_del__(ofport);
- tnl_port_add__(ofport, netdev, odp_port, true, native_tnl, name);
+ tnl_port_del__(ofport, old_odp_port);
+ tnl_port_add__(ofport, netdev, new_odp_port, true, native_tnl, name);
changed = true;
}
fat_rwlock_unlock(&rwlock);
@@ -251,7 +254,8 @@ tnl_port_reconfigure(const struct ofport_dpif *ofport,
}
static void
-tnl_port_del__(const struct ofport_dpif *ofport) OVS_REQ_WRLOCK(rwlock)
+tnl_port_del__(const struct ofport_dpif *ofport, odp_port_t odp_port)
+ OVS_REQ_WRLOCK(rwlock)
{
struct tnl_port *tnl_port;
@@ -261,11 +265,9 @@ tnl_port_del__(const struct ofport_dpif *ofport) OVS_REQ_WRLOCK(rwlock)
tnl_port = tnl_find_ofport(ofport);
if (tnl_port) {
- const struct netdev_tunnel_config *cfg =
- netdev_get_tunnel_config(tnl_port->netdev);
struct hmap **map;
- tnl_port_map_delete(cfg->dst_port, netdev_get_type(tnl_port->netdev));
+ tnl_port_map_delete(odp_port, netdev_get_type(tnl_port->netdev));
tnl_port_mod_log(tnl_port, "removing");
map = tnl_match_map(&tnl_port->match);
hmap_remove(*map, &tnl_port->match_node);
@@ -282,10 +284,11 @@ tnl_port_del__(const struct ofport_dpif *ofport) OVS_REQ_WRLOCK(rwlock)
/* Removes 'ofport' from the module. */
void
-tnl_port_del(const struct ofport_dpif *ofport) OVS_EXCLUDED(rwlock)
+tnl_port_del(const struct ofport_dpif *ofport, odp_port_t odp_port)
+ OVS_EXCLUDED(rwlock)
{
fat_rwlock_wrlock(&rwlock);
- tnl_port_del__(ofport);
+ tnl_port_del__(ofport, odp_port);
fat_rwlock_unlock(&rwlock);
}
diff --git a/ofproto/tunnel.h b/ofproto/tunnel.h
index b0ec67c..47d3dd5 100644
--- a/ofproto/tunnel.h
+++ b/ofproto/tunnel.h
@@ -32,11 +32,12 @@ struct netdev_tnl_build_header_params;
void ofproto_tunnel_init(void);
bool tnl_port_reconfigure(const struct ofport_dpif *, const struct netdev *,
- odp_port_t, bool native_tnl, const char name[]);
+ odp_port_t new_odp_port, odp_port_t old_odp_port,
+ bool native_tnl, const char name[]);
int tnl_port_add(const struct ofport_dpif *, const struct netdev *,
- odp_port_t odp_port, bool native_tnl, const char name[]);
-void tnl_port_del(const struct ofport_dpif *);
+ odp_port_t, bool native_tnl, const char name[]);
+void tnl_port_del(const struct ofport_dpif *, odp_port_t);
const struct ofport_dpif *tnl_port_receive(const struct flow *);
void tnl_wc_init(struct flow *, struct flow_wildcards *);
diff --git a/tests/tunnel-push-pop-ipv6.at b/tests/tunnel-push-pop-ipv6.at
index 9ff7c89..8f51c06 100644
--- a/tests/tunnel-push-pop-ipv6.at
+++ b/tests/tunnel-push-pop-ipv6.at
@@ -66,9 +66,9 @@ AT_CHECK([ovs-appctl tnl/arp/show | tail -n+3 | sort], [0], [dnl
AT_CHECK([ovs-appctl tnl/ports/show |sort], [0], [dnl
Listening ports:
-genev_sys_6081 (6081)
-gre_sys (3)
-vxlan_sys_4789 (4789)
+genev_sys_6081 (6081) ref_cnt=1
+gre_sys (3) ref_cnt=2
+vxlan_sys_4789 (4789) ref_cnt=2
])
dnl Check VXLAN tunnel pop
@@ -167,5 +167,32 @@ AT_CHECK([ovs-appctl dpif/dump-flows int-br | grep 'in_port(6081)'], [0], [dnl
tunnel(tun_id=0x7b,ipv6_src=2001:cafe::92,ipv6_dst=2001:cafe::88,geneve({class=0xffff,type=0x80,len=4,0xa/0xf}{class=0xffff,type=0,len=4}),flags(-df-csum+key)),recirc_id(0),in_port(6081),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:userspace(pid=0,slow_path(controller))
])
+ovs-appctl time/warp 10000
+
+AT_CHECK([ovs-vsctl del-port int-br t3 \
+ -- set Interface t1 type=vxlan \
+ -- set Interface t2 options:dst_port=4790 \
+ ], [0])
+
+dnl Check tunnel lookup entries after deleting/reconfiguring some ports
+AT_CHECK([ovs-appctl tnl/ports/show |sort], [0], [dnl
+Listening ports:
+genev_sys_6081 (6081) ref_cnt=1
+gre_sys (3) ref_cnt=1
+vxlan_sys_4789 (4789) ref_cnt=1
+vxlan_sys_4790 (4790) ref_cnt=1
+])
+
+AT_CHECK([ovs-vsctl del-port int-br t1 \
+ -- del-port int-br t2 \
+ -- del-port int-br t4 \
+ -- del-port int-br t5 \
+ ], [0])
+
+dnl Check tunnel lookup entries after deleting all remaining tunnel ports
+AT_CHECK([ovs-appctl tnl/ports/show |sort], [0], [dnl
+Listening ports:
+])
+
OVS_VSWITCHD_STOP
AT_CLEANUP
diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
index c376e71..5f2f172 100644
--- a/tests/tunnel-push-pop.at
+++ b/tests/tunnel-push-pop.at
@@ -80,9 +80,9 @@ AT_CHECK([ovs-appctl tnl/neigh/show | tail -n+3 | sort], [0], [dnl
AT_CHECK([ovs-appctl tnl/ports/show |sort], [0], [dnl
Listening ports:
-genev_sys_6081 (6081)
-gre_sys (3)
-vxlan_sys_4789 (4789)
+genev_sys_6081 (6081) ref_cnt=2
+gre_sys (3) ref_cnt=2
+vxlan_sys_4789 (4789) ref_cnt=3
])
dnl Check VXLAN tunnel pop
@@ -218,6 +218,35 @@ AT_CHECK([ovs-appctl dpif/dump-flows int-br | grep 'in_port(6081)'], [0], [dnl
tunnel(tun_id=0x7b,src=1.1.2.92,dst=1.1.2.88,geneve({class=0xffff,type=0x80,len=4,0xa/0xf}{class=0xffff,type=0,len=4}),flags(-df-csum+key)),recirc_id(0),in_port(6081),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:userspace(pid=0,slow_path(controller))
])
+ovs-appctl time/warp 10000
+
+AT_CHECK([ovs-vsctl del-port int-br t3 \
+ -- del-port int-br t5 \
+ -- set Interface t1 type=vxlan \
+ -- set Interface t2 options:dst_port=4790 \
+ ], [0])
+
+dnl Check tunnel lookup entries after deleting/reconfiguring some ports
+AT_CHECK([ovs-appctl tnl/ports/show |sort], [0], [dnl
+Listening ports:
+genev_sys_6081 (6081) ref_cnt=1
+gre_sys (3) ref_cnt=1
+vxlan_sys_4789 (4789) ref_cnt=2
+vxlan_sys_4790 (4790) ref_cnt=1
+])
+
+AT_CHECK([ovs-vsctl del-port int-br t1 \
+ -- del-port int-br t2 \
+ -- del-port int-br t4 \
+ -- del-port int-br t6 \
+ -- del-port int-br t7 \
+ ], [0])
+
+dnl Check tunnel lookup entries after deleting all remaining tunnel ports
+AT_CHECK([ovs-appctl tnl/ports/show |sort], [0], [dnl
+Listening ports:
+])
+
OVS_VSWITCHD_STOP
AT_CLEANUP
--
1.9.1
More information about the dev
mailing list