From: Ben Pfaff Date: Wed, 26 Feb 2014 19:12:57 +0000 (-0800) Subject: ofproto: Send port status message for port-mods, right away. X-Git-Tag: v2.3~605 X-Git-Url: http://git.cascardo.info/?a=commitdiff_plain;h=2a6f78e0fae96b07b5f328ee071652f4a525b16f;p=cascardo%2Fovs.git 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. EXT-338. Signed-off-by: Ben Pfaff Reported-by: Kmindg G --- diff --git a/include/openflow/openflow-common.h b/include/openflow/openflow-common.h index bf16d59c3..53aa67eb7 100644 --- a/include/openflow/openflow-common.h +++ b/include/openflow/openflow-common.h @@ -1,4 +1,4 @@ -/* Copyright (c) 2008, 2011, 2012, 2013 The Board of Trustees of The Leland Stanford +/* Copyright (c) 2008, 2011, 2012, 2013, 2014 The Board of Trustees of The Leland Stanford * Junior University * * We are making the OpenFlow specification and associated documentation @@ -32,7 +32,7 @@ */ /* - * Copyright (c) 2008, 2009, 2010, 2011 Nicira, Inc. + * Copyright (c) 2008-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. @@ -76,6 +76,12 @@ enum ofp_version { OFP11_VERSION = 0x02, OFP12_VERSION = 0x03, OFP13_VERSION = 0x04 + + /* When we add real support for these versions, add them to the enum so + * that we get compiler warnings everywhere we might forget to provide + * support. Until then, keep them as macros to avoid those warnings. */ +#define OFP14_VERSION 0x05 +#define OFP15_VERSION 0x06 }; /* Vendor (aka experimenter) IDs. diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c index a58e785a2..033ab7d5c 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.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, + * so that one could expect the controller to already know the + * changes it had made. + * + * EXT-338 changes OpenFlow 1.5 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.4 and earlier, 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) < OFP15_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 170d8721d..3c9216ffb 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 02e628ae6..7117e1fb6 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); }