[ovs-dev] [PATCH V2] netdev-vport: Checks tunnel status change when route-table is reset.

Alex Wang alexw at nicira.com
Fri May 2 18:15:46 UTC 2014


Commit 3e912ffcbb (netdev: Add 'change_seq' back to netdev.) added per-
netdev change number for indicating status change.  Future commits used
this change number to optimize the netdev status update to database.
However, the work also introduced the bug in the following scenario:

- assume interface eth0 has address 1.2.3.4, eth1 has adddress 10.0.0.1.
- assume tunnel port p1 is set with remote_ip=10.0.0.5.
- after setup, 'ovs-vsctl list interface p1 status' should show the
  'tunnel_egress_iface="eth1"'.
- now if the address of eth1 is change to 0 via 'ifconfig eth1 0'.
- expectedly, after change, 'ovs-vsctl list interface p1 status' should
  show the 'tunnel_egress_iface="eth0"'

However, 'tunnel_egress_iface' will not be updated on current master.
This is in that, the 'netdev-vport' module corresponding to p1 does
not react to routing related changes.

To fix the bug, this commit adds a change sequence number in the route-
table module and makes netdev-vport check the sequence number for
tunnel status update.

Bug #1240626

Signed-off-by: Alex Wang <alexw at nicira.com>
---
PATCH -> V2:
- use strcmp() and ovs_strlcpy() in netdev_vport_route_changed().
- change the netdev_vport_route_changed() to return array of pointers
  to 'netdev-vports'.
---
 lib/netdev-provider.h  |    1 +
 lib/netdev-vport.c     |   96 +++++++++++++++++++++++++++++++++++++++++++-----
 lib/netdev-vport.h     |    1 +
 lib/netdev.c           |   35 ++++++++++++++++++
 lib/route-table-bsd.c  |    6 +++
 lib/route-table-stub.c |    8 +++-
 lib/route-table.c      |   17 ++++++++-
 lib/route-table.h      |    3 +-
 8 files changed, 154 insertions(+), 13 deletions(-)

diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
index 9d8e67f..c2123dc 100644
--- a/lib/netdev-provider.h
+++ b/lib/netdev-provider.h
@@ -71,6 +71,7 @@ const char *netdev_get_name(const struct netdev *);
 struct netdev *netdev_from_name(const char *name);
 void netdev_get_devices(const struct netdev_class *,
                         struct shash *device_list);
+struct netdev **netdev_get_vports(size_t *size);
 
 /* A data structure for capturing packets received by a network device.
  *
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 9e92a88..c214bf7 100644
--- a/lib/netdev-vport.c
+++ b/lib/netdev-vport.c
@@ -34,6 +34,7 @@
 #include "netdev-provider.h"
 #include "ofpbuf.h"
 #include "packets.h"
+#include "poll-loop.h"
 #include "route-table.h"
 #include "shash.h"
 #include "socket-util.h"
@@ -57,6 +58,8 @@ struct netdev_vport {
 
     /* Tunnels. */
     struct netdev_tunnel_config tnl_cfg;
+    char egress_iface[IFNAMSIZ];
+    bool carrier_status;
 
     /* Patch Ports. */
     char *peer;
@@ -67,9 +70,13 @@ struct vport_class {
     struct netdev_class netdev_class;
 };
 
+/* Last read of the route-table's change number. */
+static uint64_t rt_change_seqno;
+
 static int netdev_vport_construct(struct netdev *);
 static int get_patch_config(const struct netdev *netdev, struct smap *args);
 static int get_tunnel_config(const struct netdev *, struct smap *args);
+static bool tunnel_check_status_change__(struct netdev_vport *);
 
 static bool
 is_vport_class(const struct netdev_class *class)
@@ -77,6 +84,12 @@ is_vport_class(const struct netdev_class *class)
     return class->construct == netdev_vport_construct;
 }
 
+bool
+netdev_vport_is_vport_class(const struct netdev_class *class)
+{
+    return is_vport_class(class);
+}
+
 static const struct vport_class *
 vport_class_cast(const struct netdev_class *class)
 {
@@ -164,6 +177,34 @@ netdev_vport_get_dpif_port_strdup(const struct netdev *netdev)
                                               sizeof namebuf));
 }
 
+/* Whenever the route-table change number is incremented,
+ * netdev_vport_route_changed() should be called to update
+ * the corresponding tunnel interface status. */
+static void
+netdev_vport_route_changed(void)
+{
+    struct netdev **vports;
+    size_t i, n_vports;
+
+    vports = netdev_get_vports(&n_vports);
+    for (i = 0; i < n_vports; i++) {
+        struct netdev *netdev_ = vports[i];
+        struct netdev_vport *netdev = netdev_vport_cast(netdev_);
+
+        ovs_mutex_lock(&netdev->mutex);
+        /* Finds all tunnel vports. */
+        if (netdev->tnl_cfg.ip_dst) {
+            if (tunnel_check_status_change__(netdev)) {
+                netdev_change_seq_changed(netdev_);
+            }
+        }
+        netdev_close(netdev_);
+        ovs_mutex_unlock(&netdev->mutex);
+    }
+
+    free(vports);
+}
+
 static struct netdev *
 netdev_vport_alloc(void)
 {
@@ -228,29 +269,50 @@ netdev_vport_get_etheraddr(const struct netdev *netdev_,
     return 0;
 }
 
-static int
-tunnel_get_status(const struct netdev *netdev_, struct smap *smap)
+/* Checks if the tunnel status has changed and returns a boolean.
+ * Updates the tunnel status if it has changed. */
+static bool
+tunnel_check_status_change__(struct netdev_vport *netdev)
+    OVS_REQUIRES(netdev->mutex)
 {
-    struct netdev_vport *netdev = netdev_vport_cast(netdev_);
     char iface[IFNAMSIZ];
+    bool status = false;
     ovs_be32 route;
 
-    ovs_mutex_lock(&netdev->mutex);
+    iface[0] = '\0';
     route = netdev->tnl_cfg.ip_dst;
-    ovs_mutex_unlock(&netdev->mutex);
-
     if (route_table_get_name(route, iface)) {
         struct netdev *egress_netdev;
 
-        smap_add(smap, "tunnel_egress_iface", iface);
-
         if (!netdev_open(iface, "system", &egress_netdev)) {
-            smap_add(smap, "tunnel_egress_iface_carrier",
-                     netdev_get_carrier(egress_netdev) ? "up" : "down");
+            status = netdev_get_carrier(egress_netdev);
             netdev_close(egress_netdev);
         }
     }
 
+    if (strcmp(netdev->egress_iface, iface)
+        || netdev->carrier_status != status) {
+        ovs_strlcpy(netdev->egress_iface, iface, IFNAMSIZ);
+        netdev->carrier_status = status;
+
+        return true;
+    }
+
+    return false;
+}
+
+static int
+tunnel_get_status(const struct netdev *netdev_, struct smap *smap)
+{
+    struct netdev_vport *netdev = netdev_vport_cast(netdev_);
+
+    if (netdev->egress_iface[0]) {
+        smap_add(smap, "tunnel_egress_iface", netdev->egress_iface);
+
+        smap_add(smap, "tunnel_egress_iface_carrier",
+                 netdev->carrier_status ? "up" : "down");
+    }
+
     return 0;
 }
 
@@ -271,13 +333,26 @@ netdev_vport_update_flags(struct netdev *netdev OVS_UNUSED,
 static void
 netdev_vport_run(void)
 {
+    uint64_t seq;
+
     route_table_run();
+    seq = route_table_get_change_seq();
+    if (rt_change_seqno != seq) {
+        rt_change_seqno = seq;
+        netdev_vport_route_changed();
+    }
 }
 
 static void
 netdev_vport_wait(void)
 {
+    uint64_t seq;
+
     route_table_wait();
+    seq = route_table_get_change_seq();
+    if (rt_change_seqno != seq) {
+        poll_immediate_wake();
+    }
 }
 
 /* Code specific to tunnel types. */
@@ -484,6 +559,7 @@ set_tunnel_config(struct netdev *dev_, const struct smap *args)
 
     ovs_mutex_lock(&dev->mutex);
     dev->tnl_cfg = tnl_cfg;
+    tunnel_check_status_change__(dev);
     netdev_change_seq_changed(dev_);
     ovs_mutex_unlock(&dev->mutex);
 
diff --git a/lib/netdev-vport.h b/lib/netdev-vport.h
index 833fee8..d79ef98 100644
--- a/lib/netdev-vport.h
+++ b/lib/netdev-vport.h
@@ -39,6 +39,7 @@ void netdev_vport_inc_rx(const struct netdev *,
 void netdev_vport_inc_tx(const struct netdev *,
                          const struct dpif_flow_stats *);
 
+bool netdev_vport_is_vport_class(const struct netdev_class *);
 const char *netdev_vport_class_get_dpif_port(const struct netdev_class *);
 
 #ifndef _WIN32
diff --git a/lib/netdev.c b/lib/netdev.c
index 2fc1834..9aaf107 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -1613,6 +1613,41 @@ netdev_get_devices(const struct netdev_class *netdev_class,
     ovs_mutex_unlock(&netdev_mutex);
 }
 
+/* Extracts pointers to all 'netdev-vports' into an array 'vports'
+ * and returns it.  Stores the size of the array into '*size'.
+ *
+ * The caller is responsible for freeing 'vports' and must close
+ * each 'netdev-vport' in the list. */
+struct netdev **
+netdev_get_vports(size_t *size)
+    OVS_EXCLUDED(netdev_mutex)
+{
+    struct netdev **vports;
+    struct shash_node *node;
+    size_t n = 0;
+
+    if (!size) {
+        return NULL;
+    }
+
+    /* Explicitly allocates big enough chunk of memory. */
+    vports = xmalloc(shash_count(&netdev_shash) * sizeof *vports);
+    ovs_mutex_lock(&netdev_mutex);
+    SHASH_FOR_EACH (node, &netdev_shash) {
+        struct netdev *dev = node->data;
+
+        if (netdev_vport_is_vport_class(dev->netdev_class)) {
+            dev->ref_cnt++;
+            vports[n] = dev;
+            n++;
+        }
+    }
+    ovs_mutex_unlock(&netdev_mutex);
+    *size = n;
+
+    return vports;
+}
+
 const char *
 netdev_get_type_from_name(const char *name)
 {
diff --git a/lib/route-table-bsd.c b/lib/route-table-bsd.c
index 38cd2c9..4cdf2ac 100644
--- a/lib/route-table-bsd.c
+++ b/lib/route-table-bsd.c
@@ -109,6 +109,12 @@ route_table_get_name(ovs_be32 ip, char name[IFNAMSIZ])
     return false;
 }
 
+uint64_t
+route_table_get_change_seq(void)
+{
+    return 0;
+}
+
 void
 route_table_register(void)
 {
diff --git a/lib/route-table-stub.c b/lib/route-table-stub.c
index a4a0991..9acc81c 100644
--- a/lib/route-table-stub.c
+++ b/lib/route-table-stub.c
@@ -1,4 +1,4 @@
-/* Copyright (c) 2012 Nicira, Inc.
+/* Copyright (c) 2012, 2013, 2014 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -31,6 +31,12 @@ route_table_get_ifindex(ovs_be32 ip OVS_UNUSED, int *ifindex)
     return false;
 }
 
+uint64_t
+route_table_get_change_seq(void)
+{
+    return 0;
+}
+
 void
 route_table_register(void)
 {
diff --git a/lib/route-table.c b/lib/route-table.c
index 8f5b733..2986d3d 100644
--- a/lib/route-table.c
+++ b/lib/route-table.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2012, 2013 Nicira, Inc.
+ * Copyright (c) 2011, 2012, 2013, 2014 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -65,6 +65,10 @@ struct name_node {
 
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
 
+/* Global change number for route-table, which should be incremented
+ * every time route_table_reset() is called.  */
+static uint64_t rt_change_seq;
+
 static unsigned int register_count = 0;
 static struct nln *nln = NULL;
 static struct route_table_msg rtmsg;
@@ -154,6 +158,12 @@ route_table_get_ifindex(ovs_be32 ip_, int *ifindex)
     return false;
 }
 
+uint64_t
+route_table_get_change_seq(void)
+{
+    return rt_change_seq;
+}
+
 /* Users of the route_table module should register themselves with this
  * function before making any other route_table function calls. */
 void
@@ -205,6 +215,10 @@ route_table_run(void)
     if (nln) {
         rtnetlink_link_run();
         nln_run(nln);
+
+        if (!route_table_valid) {
+            route_table_reset();
+        }
     }
 }
 
@@ -228,6 +242,7 @@ route_table_reset(void)
 
     route_map_clear();
     route_table_valid = true;
+    rt_change_seq++;
 
     ofpbuf_init(&request, 0);
 
diff --git a/lib/route-table.h b/lib/route-table.h
index 6403b6d..2ed4b91 100644
--- a/lib/route-table.h
+++ b/lib/route-table.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011 Nicira, Inc.
+ * Copyright (c) 2011, 2012, 2013, 2014 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -27,6 +27,7 @@
 
 bool route_table_get_ifindex(ovs_be32 ip, int *ifindex);
 bool route_table_get_name(ovs_be32 ip, char name[IFNAMSIZ]);
+uint64_t route_table_get_change_seq(void);
 void route_table_register(void);
 void route_table_unregister(void);
 void route_table_run(void);
-- 
1.7.9.5




More information about the dev mailing list