[ovs-dev,1/6] connmgr: Suppress duplicate port status notifications.

Message ID 20180824173521.19922-1-blp@ovn.org
State New
Headers show
Series
  • [ovs-dev,1/6] connmgr: Suppress duplicate port status notifications.
Related show

Commit Message

Ben Pfaff Aug. 24, 2018, 5:35 p.m.
When the status of a port changes, ofproto calls into connmgr to notify
controllers.  Sometimes, particular changes are only visible to controllers
running specific versions of OpenFlow.  Until now, OVS would send those
controllers duplicate port status notifications.  This is unnecessary and
somewhat confusing.  This commit eliminates it.

This commit updates one of the tests not to expect duplicate notifications.

Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 ofproto/connmgr.c | 38 ++++++++++++++++++++++++++++----------
 ofproto/connmgr.h |  4 +++-
 ofproto/ofproto.c | 18 ++++++++++--------
 tests/lacp.at     |  3 ---
 4 files changed, 41 insertions(+), 22 deletions(-)

Patch

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index f78b4c5ff411..50e0b8f991b5 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1609,24 +1609,28 @@  ofconn_send(const struct ofconn *ofconn, struct ofpbuf *msg,
 
 /* Sending asynchronous messages. */
 
-/* Sends an OFPT_PORT_STATUS message with 'opp' and 'reason' to appropriate
+/* Sends an OFPT_PORT_STATUS message with 'new_pp' and 'reason' to appropriate
  * controllers managed by 'mgr'.  For messages caused by a controller
  * OFPT_PORT_MOD, specify 'source' as the controller connection that sent the
- * request; otherwise, specify 'source' as NULL. */
+ * request; otherwise, specify 'source' as NULL.
+ *
+ * If 'reason' is OFPPR_MODIFY and 'old_pp' is nonnull, then messages are
+ * suppressed in the case where the change would not be visible to a particular
+ * controller.  For example, OpenFlow 1.0 does not have the OFPPS_LIVE flag, so
+ * this would suppress a change solely to that flag from being sent to an
+ * OpenFlow 1.0 controller. */
 void
 connmgr_send_port_status(struct connmgr *mgr, struct ofconn *source,
-                         const struct ofputil_phy_port *pp, uint8_t reason)
+                         const struct ofputil_phy_port *old_pp,
+                         const struct ofputil_phy_port *new_pp,
+                         uint8_t reason)
 {
     /* XXX Should limit the number of queued port status change messages. */
-    struct ofputil_port_status ps;
-    struct ofconn *ofconn;
+    struct ofputil_port_status new_ps = { reason, *new_pp };
 
-    ps.reason = reason;
-    ps.desc = *pp;
+    struct ofconn *ofconn;
     LIST_FOR_EACH (ofconn, node, &mgr->all_conns) {
         if (ofconn_receives_async_msg(ofconn, OAM_PORT_STATUS, reason)) {
-            struct ofpbuf *msg;
-
             /* Before 1.5, OpenFlow specified that OFPT_PORT_MOD should not
              * generate OFPT_PORT_STATUS messages.  That requirement was a
              * relic of how OpenFlow originally supported a single controller,
@@ -1651,7 +1655,21 @@  connmgr_send_port_status(struct connmgr *mgr, struct ofconn *source,
                 continue;
             }
 
-            msg = ofputil_encode_port_status(&ps, ofconn_get_protocol(ofconn));
+            enum ofputil_protocol protocol = ofconn_get_protocol(ofconn);
+            struct ofpbuf *msg = ofputil_encode_port_status(&new_ps, protocol);
+            if (reason == OFPPR_MODIFY && old_pp) {
+                struct ofputil_port_status old_ps = { reason, *old_pp };
+                struct ofpbuf *old_msg = ofputil_encode_port_status(&old_ps,
+                                                                    protocol);
+                bool suppress = ofpbuf_equal(msg, old_msg);
+                ofpbuf_delete(old_msg);
+
+                if (suppress) {
+                    ofpbuf_delete(msg);
+                    continue;
+                }
+            }
+
             ofconn_send(ofconn, msg, NULL);
         }
     }
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index eb3be16686c5..4a22f1c26611 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -147,7 +147,9 @@  void ofconn_report_flow_mod(struct ofconn *, enum ofp_flow_mod_command);
 /* Sending asynchronous messages. */
 bool connmgr_wants_packet_in_on_miss(struct connmgr *mgr);
 void connmgr_send_port_status(struct connmgr *, struct ofconn *source,
-                              const struct ofputil_phy_port *, uint8_t reason);
+                              const struct ofputil_phy_port *old_pp,
+                              const struct ofputil_phy_port *new_pp,
+                              uint8_t reason);
 void connmgr_send_flow_removed(struct connmgr *,
                                const struct ofputil_flow_removed *)
     OVS_REQUIRES(ofproto_mutex);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 9552a585d096..95235f21232c 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2417,7 +2417,7 @@  ofport_install(struct ofproto *p,
     if (error) {
         goto error;
     }
-    connmgr_send_port_status(p->connmgr, NULL, pp, OFPPR_ADD);
+    connmgr_send_port_status(p->connmgr, NULL, NULL, pp, OFPPR_ADD);
     return 0;
 
 error:
@@ -2438,7 +2438,7 @@  ofport_remove(struct ofport *ofport)
     struct ofproto *p = ofport->ofproto;
     bool is_mtu_overridden = ofport_is_mtu_overridden(p, ofport);
 
-    connmgr_send_port_status(ofport->ofproto->connmgr, NULL, &ofport->pp,
+    connmgr_send_port_status(ofport->ofproto->connmgr, NULL, NULL, &ofport->pp,
                              OFPPR_DELETE);
     ofport_destroy(ofport, true);
     if (!is_mtu_overridden) {
@@ -2483,8 +2483,9 @@  void
 ofproto_port_set_state(struct ofport *port, enum ofputil_port_state state)
 {
     if (port->pp.state != state) {
+        struct ofputil_phy_port old_pp = port->pp;
         port->pp.state = state;
-        connmgr_send_port_status(port->ofproto->connmgr, NULL,
+        connmgr_send_port_status(port->ofproto->connmgr, NULL, &old_pp,
                                  &port->pp, OFPPR_MODIFY);
     }
 }
@@ -2630,6 +2631,7 @@  update_port(struct ofproto *ofproto, const char *name)
         port = ofproto_get_port(ofproto, ofproto_port.ofp_port);
         if (port && !strcmp(netdev_get_name(port->netdev), name)) {
             struct netdev *old_netdev = port->netdev;
+            struct ofputil_phy_port old_pp = port->pp;
 
             /* 'name' hasn't changed location.  Any properties changed? */
             bool port_changed = !ofport_equal(&port->pp, &pp);
@@ -2652,7 +2654,7 @@  update_port(struct ofproto *ofproto, const char *name)
             /* Send status update, if any port property changed */
             if (port_changed) {
                 connmgr_send_port_status(port->ofproto->connmgr, NULL,
-                                         &port->pp, OFPPR_MODIFY);
+                                         &old_pp, &port->pp, OFPPR_MODIFY);
             }
 
             netdev_close(old_netdev);
@@ -3648,11 +3650,11 @@  update_port_config(struct ofconn *ofconn, struct ofport *port,
     }
 
     if (toggle) {
-        enum ofputil_port_config old_config = port->pp.config;
+        struct ofputil_phy_port old_pp = port->pp;
         port->pp.config ^= toggle;
-        port->ofproto->ofproto_class->port_reconfigured(port, old_config);
-        connmgr_send_port_status(port->ofproto->connmgr, ofconn, &port->pp,
-                                 OFPPR_MODIFY);
+        port->ofproto->ofproto_class->port_reconfigured(port, old_pp.config);
+        connmgr_send_port_status(port->ofproto->connmgr, ofconn, &old_pp,
+                                 &port->pp, OFPPR_MODIFY);
     }
 }
 
diff --git a/tests/lacp.at b/tests/lacp.at
index a7f75ac67acb..7b460d7be35e 100644
--- a/tests/lacp.at
+++ b/tests/lacp.at
@@ -805,7 +805,6 @@  check_liveness 1 LIVE
 AT_CHECK([ovs-vsctl \
 -- add-port br0 null0 -- set int null0 type=patch options:peer=p2 -- set int p2 options:peer=null0 \
 -- add-port br1 null1 -- set int null1 type=patch options:peer=p0 -- set int p0 options:peer=null1])
-check_liveness 2 LIVE
 
 # Wait 4 more simulated seconds.  The LACP state should become "defaulted" for p0 and p2.
 ovs-appctl time/warp 4100 100
@@ -901,7 +900,6 @@  check_liveness 1 LIVE
 AT_CHECK([ovs-vsctl \
 -- add-port br0 null0 -- set int null0 type=patch options:peer=p2 -- set int p2 options:peer=null0 \
 -- add-port br1 null1 -- set int null1 type=patch options:peer=p0 -- set int p0 options:peer=null1])
-check_liveness 2 LIVE
 
 # Wait 4 more simulated seconds.  The LACP state should become "defaulted" for p0 and p2.
 ovs-appctl time/warp 4100 100
@@ -997,7 +995,6 @@  check_liveness 1 LIVE
 AT_CHECK([ovs-vsctl \
 -- add-port br0 null0 -- set int null0 type=patch options:peer=p2 -- set int p2 options:peer=null0 \
 -- add-port br1 null1 -- set int null1 type=patch options:peer=p0 -- set int p0 options:peer=null1])
-check_liveness 2 LIVE
 
 # Wait 4 more simulated seconds.  The LACP state should become "defaulted" for p0 and p2.
 ovs-appctl time/warp 4100 100