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

Ben Pfaff blp at nicira.com
Wed Feb 26 19:13:37 UTC 2014


On Sat, Feb 22, 2014 at 01:14:00PM +0800, Kmindg G wrote:
> On Sat, Feb 22, 2014 at 12:42 AM, Ben Pfaff <blp at nicira.com> wrote:
> > On Fri, Feb 21, 2014 at 10:07:14AM +0800, Kmindg G wrote:
> >> On Fri, Feb 21, 2014 at 5:19 AM, Ben Pfaff <blp at nicira.com> wrote:
> >> > 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;
> >> > +            }
> >> > +
> >>
> >>
> >> I'm not very familiar with the openflow specification, but I find that
> >> it says: "If the port config bits are changed by the switch through
> >> another administrative interface, the switch sends an OFPT_PORT_STATUS
> >> message to notify the controller of the change."in openflow-spec-1.1.0
> >>  A.2.1. Did I misunderstand something?
> >
> > "another administrative interface" means "other than OpenFlow".
> 
> All right, this patch is good to me.
> Thanks.

I'll apply this in a minute, thanks.



More information about the dev mailing list