[ovs-dev] [PATCH] ofproto-dpif : propagate may_enable flag as link aliveness

Ben Pfaff blp at ovn.org
Fri Mar 17 18:20:38 UTC 2017


On Fri, Mar 03, 2017 at 01:00:05PM +0000, László Sürü wrote:
> hereby I'm sending the implementation of link liveness propagation functionality 
> including the additional functionalities requested last year (last activity at Wed Mar 16 23:13:24 UTC 2016).
> 
> The idea is to use OFPPS_LIVE bit to propagate link aliveness state towards the controller also when sending port status.
> The ofport->may_enable flag could be used for this purpose, thus any change in LIVE bit is propagated towards conrtoller in OFPT_PORT_STATUS message.
> OFPPS_LIVE bit is set only when links is not down not administratively, neither operationally as recommended in OF papers.
> I added 9 new unit tests to verify link state changes when monitored with cfm, bfd or lacp for OF 1.3, OF 1.4 and OF 1.5.
> I updated related unit tests according to the changes of ofproto-dpif.

Thanks a lot for the revision!

I am a little concerned about ofport_modified().  It seems like this
does not honor the status of protocols like BFD or CFM.  I think this
means that, if a port changes for any reason, liveness will initially
ignore the status of these protocols, until the next time their status
changes.  That is probably not a good situation.  Do you have any
comments on that?

I'm passing along an incremental I recommend folding into the next
revision of the patch, to make it better fit the usual OVS coding style.

(I do hope that the next revision will take less than a year!)

Thanks,

Ben.

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

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index b3f3a56921d3..450123a043a2 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -3453,7 +3453,6 @@ port_run(struct ofport_dpif *ofport)
 
     if (ofport->may_enable != enable) {
         struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
-        enum ofputil_port_state of_state;
 
         ofproto->backer->need_revalidate = REV_PORT_TOGGLED;
 
@@ -3461,13 +3460,16 @@ port_run(struct ofport_dpif *ofport)
             rstp_port_set_mac_operational(ofport->rstp_port, enable);
         }
 
-        /* liveness is propagated only,
-         * when link is nor administratively neither operatively down */
+        /* Propagate liveness, unless the link is administratively or
+         * operationally down. */
         if (!(ofport->up.pp.config & OFPUTIL_PC_PORT_DOWN) &&
             !(ofport->up.pp.state & OFPUTIL_PS_LINK_DOWN)) {
-            of_state = ofport->up.pp.state;
-            (enable) ? (of_state |= OFPUTIL_PS_LIVE)
-                     : (of_state &= ~OFPUTIL_PS_LIVE);
+            enum ofputil_port_state of_state = ofport->up.pp.state;
+            if (enable) {
+                of_state |= OFPUTIL_PS_LIVE;
+            } else {
+                of_state &= ~OFPUTIL_PS_LIVE;
+            }
             ofproto_port_set_state(&ofport->up, of_state);
         }
     }
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 65cb558355bc..3bb88dcb270a 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2464,7 +2464,7 @@ ofport_modified(struct ofport *port, struct ofputil_phy_port *pp)
                         | (pp->config & OFPUTIL_PC_PORT_DOWN));
     port->pp.state = ((port->pp.state & ~OFPUTIL_PS_LINK_DOWN)
                       | (pp->state & OFPUTIL_PS_LINK_DOWN));
-    /* set LIVE bit aligned with PORT_DOWN and LINK_DOWN bits */
+    /* Initialize LIVE bit aligned with PORT_DOWN and LINK_DOWN bits. */
     if ((port->pp.config & OFPUTIL_PC_PORT_DOWN) ||
         (port->pp.state & OFPUTIL_PS_LINK_DOWN)) {
         port->pp.state &= ~OFPUTIL_PS_LIVE;



More information about the dev mailing list