[ovs-dev] [add-remove 7/7] vswitchd: Better tolerate changes in datapath ports.

Ben Pfaff blp at nicira.com
Tue Sep 28 18:58:41 UTC 2010


Until now, a command that removed and added ports in a single change to
the database, e.g.:
	ovs-vsctl del-port br0 vif1.0 -- add-port br0 vif2.0
typically failed, because of this sequence of events:

1. Bridge code removes vif1.0 from br0.
2. Bridge code adds vif2.0 to br0.
3. ofproto_run() receives kernel notification that vif1.0 was deleted, so
   it notifies the bridge by calling back to bridge_port_changed_ofhook_cb,
   which sees that it has an interface with the specified port number, and
   deletes it.  Oops--this is where the problem occurs.  For completeness:
4. ofproto_run() receives kernel notification that vif2.0 was added, so
   it notifies the bridge by calling back to ,
   which sees that it has no interface with the specified port number, and
   does nothing.

This commit fixes the problem by adding the ifindex to the callbacks.  In
step 3, bridge_port_changed_ofhook_cb sees that the port that was deleted
was not the same as the one that it has in its interface and ignores it.

Thanks to Jesse Gross for identifying the problem.

Bug #3671.
---
 ofproto/ofproto.c |   57 ++++++++++++++++++++++++++++++++++++++--------------
 ofproto/ofproto.h |    2 +-
 vswitchd/bridge.c |    5 ++-
 3 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 5fb5f41..9bebd58 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -71,6 +71,7 @@ enum {
 struct ofport {
     struct netdev *netdev;
     struct ofp_phy_port opp;    /* In host byte order. */
+    int ifindex;
 };
 
 static void ofport_free(struct ofport *);
@@ -1438,6 +1439,7 @@ make_ofport(const struct odp_port *odp_port)
     struct ofport *ofport;
     struct netdev *netdev;
     bool carrier;
+    int ifindex;
     int error;
 
     memset(&netdev_options, 0, sizeof netdev_options);
@@ -1453,8 +1455,20 @@ make_ofport(const struct odp_port *odp_port)
         return NULL;
     }
 
+    ifindex = netdev_get_ifindex(netdev);
+    if (ifindex <= 0 && ifindex != -EOPNOTSUPP) {
+        VLOG_WARN_RL(&rl, "ignoring port %s (%"PRIu16") because netdev %s "
+                     "ifindex cannot be obtained (%s)",
+                     odp_port->devname, odp_port->port, odp_port->devname,
+                     !ifindex ? "zero ifindex" : strerror(-ifindex));
+        netdev_close(netdev);
+        return NULL;
+    }
+
     ofport = xmalloc(sizeof *ofport);
     ofport->netdev = netdev;
+    ofport->ifindex = ifindex;
+
     ofport->opp.port_no = odp_port_to_ofp_port(odp_port->port);
     netdev_get_etheraddr(netdev, ofport->opp.hw_addr);
     memcpy(ofport->opp.name, odp_port->devname,
@@ -1490,10 +1504,10 @@ ofport_conflicts(const struct ofproto *p, const struct odp_port *odp_port)
 }
 
 static int
-ofport_equal(const struct ofport *a_, const struct ofport *b_)
+ofport_equal(const struct ofport *ap, const struct ofport *bp)
 {
-    const struct ofp_phy_port *a = &a_->opp;
-    const struct ofp_phy_port *b = &b_->opp;
+    const struct ofp_phy_port *a = &ap->opp;
+    const struct ofp_phy_port *b = &bp->opp;
 
     BUILD_ASSERT_DECL(sizeof *a == 48); /* Detect ofp_phy_port changes. */
     return (a->port_no == b->port_no
@@ -1504,7 +1518,8 @@ ofport_equal(const struct ofport *a_, const struct ofport *b_)
             && a->curr == b->curr
             && a->advertised == b->advertised
             && a->supported == b->supported
-            && a->peer == b->peer);
+            && a->peer == b->peer
+            && ap->ifindex == bp->ifindex);
 }
 
 static void
@@ -1528,7 +1543,8 @@ send_port_status(struct ofproto *p, const struct ofport *ofport,
         queue_tx(b, ofconn, NULL);
     }
     if (p->ofhooks->port_changed_cb) {
-        p->ofhooks->port_changed_cb(reason, &ofport->opp, p->aux);
+        p->ofhooks->port_changed_cb(reason, &ofport->opp, ofport->ifindex,
+                                    p->aux);
     }
 }
 
@@ -1589,13 +1605,7 @@ update_port(struct ofproto *p, const char *devname)
             /* There's no port named 'devname' but there might be a port with
              * the same port number.  This could happen if a port is deleted
              * and then a new one added in its place very quickly, or if a port
-             * is renamed.  In the former case we want to send an OFPPR_DELETE
-             * and an OFPPR_ADD, and in the latter case we want to send a
-             * single OFPPR_MODIFY.  We can distinguish the cases by comparing
-             * the old port's ifindex against the new port, or perhaps less
-             * reliably but more portably by comparing the old port's MAC
-             * against the new port's MAC.  However, this code isn't that smart
-             * and always sends an OFPPR_MODIFY (XXX). */
+             * is renamed. */
             old_ofport = port_array_get(&p->ports, odp_port.port);
         }
     } else if (error != ENOENT && error != ENODEV) {
@@ -1631,10 +1641,25 @@ update_port(struct ofproto *p, const char *devname)
     if (new_ofport) {
         ofport_install(p, new_ofport);
     }
-    send_port_status(p, new_ofport ? new_ofport : old_ofport,
-                     (!old_ofport ? OFPPR_ADD
-                      : !new_ofport ? OFPPR_DELETE
-                      : OFPPR_MODIFY));
+
+    /* Send OpenFlow update and notify hooks. */
+    if (old_ofport && new_ofport
+        && (old_ofport->opp.port_no != new_ofport->opp.port_no
+            || old_ofport->ifindex != new_ofport->ifindex)) {
+        /* new_port and old_port coincidentally have the same name or the same
+         * OpenFlow port number, but they are not the same port, so express the
+         * change via OpenFlow as a deletion of the old port followed by an add
+         * of the new one. */
+        send_port_status(p, old_ofport, OFPPR_DELETE);
+        send_port_status(p, new_ofport, OFPPR_ADD);
+    } else {
+        struct ofport *port = new_ofport ? new_ofport : old_ofport;
+        uint8_t reason = (!old_ofport ? OFPPR_ADD
+                          : !new_ofport ? OFPPR_DELETE
+                          : OFPPR_MODIFY);
+
+        send_port_status(p, port, reason);
+    }
     ofport_free(old_ofport);
 
     /* Update port groups. */
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index e39c6c0..4a6be75 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -141,7 +141,7 @@ void ofproto_flush_flows(struct ofproto *);
 /* Hooks for ovs-vswitchd. */
 struct ofhooks {
     void (*port_changed_cb)(enum ofp_port_reason, const struct ofp_phy_port *,
-                            void *aux);
+                            int ifindex, void *aux);
     bool (*normal_cb)(const flow_t *, const struct ofpbuf *packet,
                       struct odp_actions *, tag_type *,
                       uint16_t *nf_output_iface, void *aux);
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index ce0faee..2ef1870 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -87,6 +87,7 @@ struct iface {
      * be initialized. */
     int dp_ifidx;               /* Index within kernel datapath. */
     struct netdev *netdev;      /* Network device. */
+    int ifindex;                /* Network device's ifindex. */
     bool enabled;               /* May be chosen for flows? */
     const char *type;           /* Usually same as cfg->type. */
     const struct ovsrec_interface *cfg;
@@ -2556,7 +2557,7 @@ done:
 static void
 bridge_port_changed_ofhook_cb(enum ofp_port_reason reason,
                               const struct ofp_phy_port *opp,
-                              void *br_)
+                              int ifindex, void *br_)
 {
     struct bridge *br = br_;
     struct iface *iface;
@@ -2568,7 +2569,7 @@ bridge_port_changed_ofhook_cb(enum ofp_port_reason reason,
     }
     port = iface->port;
 
-    if (reason == OFPPR_DELETE) {
+    if (reason == OFPPR_DELETE && ifindex == iface->ifindex) {
         VLOG_WARN("bridge %s: interface %s deleted unexpectedly",
                   br->name, iface->name);
         iface_destroy(iface);
-- 
1.7.1





More information about the dev mailing list