[ovs-dev] [PATCH] ofproto: Send port status message for port-mods, right away.

Ben Pfaff blp at nicira.com
Thu Feb 20 21:19:08 UTC 2014


On Thu, Feb 20, 2014 at 12:45:49PM +0800, Kmindg G wrote:
> On Thu, Feb 20, 2014 at 3:20 AM, Ben Pfaff <blp at nicira.com> wrote:
> > Until now, when it processes OFPT_PORT_MOD message, Open vSwitch has waited
> > for ports to notify it that their status has changed before it sends a
> > port status update to controllers.
> >
> > Also, Open vSwitch never sent port config updates at all for port
> > modifications other than OFPPC_PORT_DOWN.  I guess that this is a relic
> > from the era when there was only one controller, since presumably the
> > controller already knew that it changed the port configuration.  But in the
> > multi-controller world, it makes sense to send such port status updates,
> > and I couldn't quickly find anything in OF1.3 or OF1.4 that said they
> > shouldn't be sent.
> >
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > Reported-by: Kmindg G <kmindg at gmail.com>
> > ---
> >  AUTHORS           |    1 +
> >  ofproto/ofproto.c |   25 +++++++++++++------------
> >  2 files changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/AUTHORS b/AUTHORS
> > index c557303..2fda8d7 100644
> > --- a/AUTHORS
> > +++ b/AUTHORS
> > @@ -197,6 +197,7 @@ John Hurley             john.hurley at netronome.com
> >  Kevin Mancuso           kevin.mancuso at rackspace.com
> >  Kiran Shanbhog          kiran at vmware.com
> >  Kirill Kabardin
> > +Kmindg G                kmindg at gmail.com
> >  Koichi Yagishita        yagishita.koichi at jrc.co.jp
> >  Konstantin Khorenko     khorenko at openvz.org
> >  Kris zhang              zhang.kris at gmail.com
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index 62c97a2..48e10ca 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -2986,22 +2986,23 @@ update_port_config(struct ofport *port,
> >                     enum ofputil_port_config config,
> >                     enum ofputil_port_config mask)
> >  {
> > -    enum ofputil_port_config old_config = port->pp.config;
> > -    enum ofputil_port_config toggle;
> > -
> > -    toggle = (config ^ port->pp.config) & mask;
> > -    if (toggle & OFPUTIL_PC_PORT_DOWN) {
> > -        if (config & OFPUTIL_PC_PORT_DOWN) {
> > -            netdev_turn_flags_off(port->netdev, NETDEV_UP, NULL);
> > -        } else {
> > -            netdev_turn_flags_on(port->netdev, NETDEV_UP, NULL);
> > -        }
> > +    enum ofputil_port_config toggle = (config ^ port->pp.config) & mask;
> > +
> > +    if (toggle & OFPUTIL_PC_PORT_DOWN
> > +        && (config & OFPUTIL_PC_PORT_DOWN
> > +            ? netdev_turn_flags_off(port->netdev, NETDEV_UP, NULL)
> > +            : netdev_turn_flags_on(port->netdev, NETDEV_UP, NULL))) {
> > +        /* We tried to bring the port up or down, but it failed, so don't
> > +         * update the "down" bit. */
> >          toggle &= ~OFPUTIL_PC_PORT_DOWN;
> >      }
> >
> > -    port->pp.config ^= toggle;
> > -    if (port->pp.config != old_config) {
> > +    if (toggle) {
> > +        enum ofputil_port_config old_config = port->pp.config;
> > +        port->pp.config ^= toggle;
> >          port->ofproto->ofproto_class->port_reconfigured(port, old_config);
> > +        connmgr_send_port_status(port->ofproto->connmgr, &port->pp,
> > +                                 OFPPR_MODIFY);
> >      }
> >  }
> >
> > --
> > 1.7.10.4
> >
> 
> looks good to me.

Thanks.  What do you think of this version?  I think that it retains
better compatibility with OpenFlow 1.0 in particular.

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Thu, 20 Feb 2014 13:18:51 -0800
Subject: [PATCH] ofproto: Send port status message for port-mods, right away.

Until now, when it processes OFPT_PORT_MOD message, Open vSwitch has waited
for ports to notify it that their status has changed before it sends a
port status update to controllers.

Also, Open vSwitch never sent port config updates at all for port
modifications other than OFPPC_PORT_DOWN.  I guess that this is a relic
from the era when there was only one controller, since presumably the
controller already knew that it changed the port configuration.  But in the
multi-controller world, it makes sense to send such port status updates,
and I couldn't quickly find anything in OF1.3 or OF1.4 that said they
shouldn't be sent.

Signed-off-by: Ben Pfaff <blp at nicira.com>
Reported-by: Kmindg G <kmindg at gmail.com>
---
 ofproto/connmgr.c | 30 ++++++++++++++++++++++++++++--
 ofproto/connmgr.h |  4 ++--
 ofproto/ofproto.c | 38 ++++++++++++++++++++------------------
 3 files changed, 50 insertions(+), 22 deletions(-)

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index a58e785..7a25828 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1456,9 +1456,11 @@ static void schedule_packet_in(struct ofconn *, struct ofproto_packet_in,
                                enum ofp_packet_in_reason wire_reason);
 
 /* Sends an OFPT_PORT_STATUS message with 'opp' and 'reason' to appropriate
- * controllers managed by 'mgr'. */
+ * 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. */
 void
-connmgr_send_port_status(struct connmgr *mgr,
+connmgr_send_port_status(struct connmgr *mgr, struct ofconn *source,
                          const struct ofputil_phy_port *pp, uint8_t reason)
 {
     /* XXX Should limit the number of queued port status change messages. */
@@ -1471,6 +1473,30 @@ connmgr_send_port_status(struct connmgr *mgr,
         if (ofconn_receives_async_msg(ofconn, OAM_PORT_STATUS, reason)) {
             struct ofpbuf *msg;
 
+            /* Before 1.3.4, 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,
+             * so that one could expect the controller to already know the
+             * changes it had made.
+             *
+             * OpenFlow 1.3.4 and OpenFlow 1.5 changed OFPT_PORT_MOD to send
+             * OFPT_PORT_STATUS messages to every controller.  This is
+             * obviously more useful in the multi-controller case.  We could
+             * always implement it that way in OVS, but that would risk
+             * confusing controllers that are intended for single-controller
+             * use only.  (Imagine a controller that generates an
+             * OFPT_PORT_MOD in response to any OFPT_PORT_STATUS!)
+             *
+             * So this compromises: for OpenFlow 1.0, 1.1, and 1.2, it
+             * generates OFPT_PORT_STATUS for OFPT_PORT_MOD, but not back to
+             * the originating controller.  In a single-controller environment,
+             * in particular, this means that it will never generate
+             * OFPT_PORT_STATUS for OFPT_PORT_MOD at all. */
+            if (ofconn == source
+                && rconn_get_version(ofconn->rconn) < OFP13_VERSION) {
+                continue;
+            }
+
             msg = ofputil_encode_port_status(&ps, ofconn_get_protocol(ofconn));
             ofconn_send(ofconn, msg, NULL);
         }
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 170d872..3c9216f 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
+ * Copyright (c) 2009, 2010, 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.
@@ -147,7 +147,7 @@ void ofconn_remove_opgroup(struct ofconn *, struct list *,
                            const struct ofp_header *request, int error);
 
 /* Sending asynchronous messages. */
-void connmgr_send_port_status(struct connmgr *,
+void connmgr_send_port_status(struct connmgr *, struct ofconn *source,
                               const struct ofputil_phy_port *, uint8_t reason);
 void connmgr_send_flow_removed(struct connmgr *,
                                const struct ofputil_flow_removed *);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 02e628a..7117e1f 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2193,7 +2193,7 @@ ofport_install(struct ofproto *p,
     if (error) {
         goto error;
     }
-    connmgr_send_port_status(p->connmgr, pp, OFPPR_ADD);
+    connmgr_send_port_status(p->connmgr, NULL, pp, OFPPR_ADD);
     return;
 
 error:
@@ -2210,7 +2210,7 @@ error:
 static void
 ofport_remove(struct ofport *ofport)
 {
-    connmgr_send_port_status(ofport->ofproto->connmgr, &ofport->pp,
+    connmgr_send_port_status(ofport->ofproto->connmgr, NULL, &ofport->pp,
                              OFPPR_DELETE);
     ofport_destroy(ofport);
 }
@@ -2245,7 +2245,8 @@ ofport_modified(struct ofport *port, struct ofputil_phy_port *pp)
     port->pp.curr_speed = pp->curr_speed;
     port->pp.max_speed = pp->max_speed;
 
-    connmgr_send_port_status(port->ofproto->connmgr, &port->pp, OFPPR_MODIFY);
+    connmgr_send_port_status(port->ofproto->connmgr, NULL,
+                             &port->pp, OFPPR_MODIFY);
 }
 
 /* Update OpenFlow 'state' in 'port' and notify controller. */
@@ -2254,8 +2255,8 @@ ofproto_port_set_state(struct ofport *port, enum ofputil_port_state state)
 {
     if (port->pp.state != state) {
         port->pp.state = state;
-        connmgr_send_port_status(port->ofproto->connmgr, &port->pp,
-                                 OFPPR_MODIFY);
+        connmgr_send_port_status(port->ofproto->connmgr, NULL,
+                                 &port->pp, OFPPR_MODIFY);
     }
 }
 
@@ -2975,26 +2976,27 @@ exit:
 }
 
 static void
-update_port_config(struct ofport *port,
+update_port_config(struct ofconn *ofconn, struct ofport *port,
                    enum ofputil_port_config config,
                    enum ofputil_port_config mask)
 {
-    enum ofputil_port_config old_config = port->pp.config;
-    enum ofputil_port_config toggle;
+    enum ofputil_port_config toggle = (config ^ port->pp.config) & mask;
 
-    toggle = (config ^ port->pp.config) & mask;
-    if (toggle & OFPUTIL_PC_PORT_DOWN) {
-        if (config & OFPUTIL_PC_PORT_DOWN) {
-            netdev_turn_flags_off(port->netdev, NETDEV_UP, NULL);
-        } else {
-            netdev_turn_flags_on(port->netdev, NETDEV_UP, NULL);
-        }
+    if (toggle & OFPUTIL_PC_PORT_DOWN
+        && (config & OFPUTIL_PC_PORT_DOWN
+            ? netdev_turn_flags_off(port->netdev, NETDEV_UP, NULL)
+            : netdev_turn_flags_on(port->netdev, NETDEV_UP, NULL))) {
+        /* We tried to bring the port up or down, but it failed, so don't
+         * update the "down" bit. */
         toggle &= ~OFPUTIL_PC_PORT_DOWN;
     }
 
-    port->pp.config ^= toggle;
-    if (port->pp.config != old_config) {
+    if (toggle) {
+        enum ofputil_port_config old_config = port->pp.config;
+        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);
     }
 }
 
@@ -3022,7 +3024,7 @@ handle_port_mod(struct ofconn *ofconn, const struct ofp_header *oh)
     } else if (!eth_addr_equals(port->pp.hw_addr, pm.hw_addr)) {
         return OFPERR_OFPPMFC_BAD_HW_ADDR;
     } else {
-        update_port_config(port, pm.config, pm.mask);
+        update_port_config(ofconn, port, pm.config, pm.mask);
         if (pm.advertise) {
             netdev_set_advertisements(port->netdev, pm.advertise);
         }
-- 
1.8.5.3




More information about the dev mailing list