On Fri, Feb 21, 2014 at 5:19 AM, Ben Pfaff <b...@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 <b...@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 <b...@nicira.com>
>> > Reported-by: Kmindg G <kmi...@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.hur...@netronome.com
>> > Kevin Mancuso kevin.manc...@rackspace.com
>> > Kiran Shanbhog ki...@vmware.com
>> > Kirill Kabardin
>> > +Kmindg G kmi...@gmail.com
>> > Koichi Yagishita yagishita.koi...@jrc.co.jp
>> > Konstantin Khorenko khore...@openvz.org
>> > Kris zhang zhang.k...@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 <b...@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 <b...@nicira.com>
> Reported-by: Kmindg G <kmi...@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?
> 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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
