From patchwork Fri Aug 24 17:35:16 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 961963 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=openvswitch.org (client-ip=140.211.169.12; helo=mail.linuxfoundation.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=ovn.org Received: from mail.linuxfoundation.org (mail.linuxfoundation.org [140.211.169.12]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 41xpMp4Fgnz9s0n for ; Sat, 25 Aug 2018 03:35:34 +1000 (AEST) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 58D64E9F; Fri, 24 Aug 2018 17:35:30 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@mail.linuxfoundation.org Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id E7FFDE4A for ; Fri, 24 Aug 2018 17:35:28 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net [217.70.183.199]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id B4A9E2C3 for ; Fri, 24 Aug 2018 17:35:27 +0000 (UTC) X-Originating-IP: 208.91.3.26 Received: from sigabrt.benpfaff.org (unknown [208.91.3.26]) (Authenticated sender: blp@ovn.org) by relay9-d.mail.gandi.net (Postfix) with ESMTPSA id EF68FFF807; Fri, 24 Aug 2018 17:35:24 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Fri, 24 Aug 2018 10:35:16 -0700 Message-Id: <20180824173521.19922-1-blp@ovn.org> X-Mailer: git-send-email 2.16.1 X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on smtp1.linux-foundation.org Cc: Ben Pfaff Subject: [ovs-dev] [PATCH 1/6] connmgr: Suppress duplicate port status notifications. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.12 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: ovs-dev-bounces@openvswitch.org Errors-To: ovs-dev-bounces@openvswitch.org When the status of a port changes, ofproto calls into connmgr to notify controllers. Sometimes, particular changes are only visible to controllers running specific versions of OpenFlow. Until now, OVS would send those controllers duplicate port status notifications. This is unnecessary and somewhat confusing. This commit eliminates it. This commit updates one of the tests not to expect duplicate notifications. Signed-off-by: Ben Pfaff Acked-by: Numan Siddique for all the patches in this --- ofproto/connmgr.c | 38 ++++++++++++++++++++++++++++---------- ofproto/connmgr.h | 4 +++- ofproto/ofproto.c | 18 ++++++++++-------- tests/lacp.at | 3 --- 4 files changed, 41 insertions(+), 22 deletions(-) diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c index f78b4c5ff411..50e0b8f991b5 100644 --- a/ofproto/connmgr.c +++ b/ofproto/connmgr.c @@ -1609,24 +1609,28 @@ ofconn_send(const struct ofconn *ofconn, struct ofpbuf *msg, /* Sending asynchronous messages. */ -/* Sends an OFPT_PORT_STATUS message with 'opp' and 'reason' to appropriate +/* Sends an OFPT_PORT_STATUS message with 'new_pp' and 'reason' to appropriate * 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. */ + * request; otherwise, specify 'source' as NULL. + * + * If 'reason' is OFPPR_MODIFY and 'old_pp' is nonnull, then messages are + * suppressed in the case where the change would not be visible to a particular + * controller. For example, OpenFlow 1.0 does not have the OFPPS_LIVE flag, so + * this would suppress a change solely to that flag from being sent to an + * OpenFlow 1.0 controller. */ void connmgr_send_port_status(struct connmgr *mgr, struct ofconn *source, - const struct ofputil_phy_port *pp, uint8_t reason) + const struct ofputil_phy_port *old_pp, + const struct ofputil_phy_port *new_pp, + uint8_t reason) { /* XXX Should limit the number of queued port status change messages. */ - struct ofputil_port_status ps; - struct ofconn *ofconn; + struct ofputil_port_status new_ps = { reason, *new_pp }; - ps.reason = reason; - ps.desc = *pp; + struct ofconn *ofconn; LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { 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, @@ -1651,7 +1655,21 @@ connmgr_send_port_status(struct connmgr *mgr, struct ofconn *source, continue; } - msg = ofputil_encode_port_status(&ps, ofconn_get_protocol(ofconn)); + enum ofputil_protocol protocol = ofconn_get_protocol(ofconn); + struct ofpbuf *msg = ofputil_encode_port_status(&new_ps, protocol); + if (reason == OFPPR_MODIFY && old_pp) { + struct ofputil_port_status old_ps = { reason, *old_pp }; + struct ofpbuf *old_msg = ofputil_encode_port_status(&old_ps, + protocol); + bool suppress = ofpbuf_equal(msg, old_msg); + ofpbuf_delete(old_msg); + + if (suppress) { + ofpbuf_delete(msg); + continue; + } + } + ofconn_send(ofconn, msg, NULL); } } diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h index eb3be16686c5..4a22f1c26611 100644 --- a/ofproto/connmgr.h +++ b/ofproto/connmgr.h @@ -147,7 +147,9 @@ void ofconn_report_flow_mod(struct ofconn *, enum ofp_flow_mod_command); /* Sending asynchronous messages. */ bool connmgr_wants_packet_in_on_miss(struct connmgr *mgr); void connmgr_send_port_status(struct connmgr *, struct ofconn *source, - const struct ofputil_phy_port *, uint8_t reason); + const struct ofputil_phy_port *old_pp, + const struct ofputil_phy_port *new_pp, + uint8_t reason); void connmgr_send_flow_removed(struct connmgr *, const struct ofputil_flow_removed *) OVS_REQUIRES(ofproto_mutex); diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 9552a585d096..95235f21232c 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -2417,7 +2417,7 @@ ofport_install(struct ofproto *p, if (error) { goto error; } - connmgr_send_port_status(p->connmgr, NULL, pp, OFPPR_ADD); + connmgr_send_port_status(p->connmgr, NULL, NULL, pp, OFPPR_ADD); return 0; error: @@ -2438,7 +2438,7 @@ ofport_remove(struct ofport *ofport) struct ofproto *p = ofport->ofproto; bool is_mtu_overridden = ofport_is_mtu_overridden(p, ofport); - connmgr_send_port_status(ofport->ofproto->connmgr, NULL, &ofport->pp, + connmgr_send_port_status(ofport->ofproto->connmgr, NULL, NULL, &ofport->pp, OFPPR_DELETE); ofport_destroy(ofport, true); if (!is_mtu_overridden) { @@ -2483,8 +2483,9 @@ void ofproto_port_set_state(struct ofport *port, enum ofputil_port_state state) { if (port->pp.state != state) { + struct ofputil_phy_port old_pp = port->pp; port->pp.state = state; - connmgr_send_port_status(port->ofproto->connmgr, NULL, + connmgr_send_port_status(port->ofproto->connmgr, NULL, &old_pp, &port->pp, OFPPR_MODIFY); } } @@ -2630,6 +2631,7 @@ update_port(struct ofproto *ofproto, const char *name) port = ofproto_get_port(ofproto, ofproto_port.ofp_port); if (port && !strcmp(netdev_get_name(port->netdev), name)) { struct netdev *old_netdev = port->netdev; + struct ofputil_phy_port old_pp = port->pp; /* 'name' hasn't changed location. Any properties changed? */ bool port_changed = !ofport_equal(&port->pp, &pp); @@ -2652,7 +2654,7 @@ update_port(struct ofproto *ofproto, const char *name) /* Send status update, if any port property changed */ if (port_changed) { connmgr_send_port_status(port->ofproto->connmgr, NULL, - &port->pp, OFPPR_MODIFY); + &old_pp, &port->pp, OFPPR_MODIFY); } netdev_close(old_netdev); @@ -3648,11 +3650,11 @@ update_port_config(struct ofconn *ofconn, struct ofport *port, } if (toggle) { - enum ofputil_port_config old_config = port->pp.config; + struct ofputil_phy_port old_pp = port->pp; 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); + port->ofproto->ofproto_class->port_reconfigured(port, old_pp.config); + connmgr_send_port_status(port->ofproto->connmgr, ofconn, &old_pp, + &port->pp, OFPPR_MODIFY); } } diff --git a/tests/lacp.at b/tests/lacp.at index a7f75ac67acb..7b460d7be35e 100644 --- a/tests/lacp.at +++ b/tests/lacp.at @@ -805,7 +805,6 @@ check_liveness 1 LIVE AT_CHECK([ovs-vsctl \ -- add-port br0 null0 -- set int null0 type=patch options:peer=p2 -- set int p2 options:peer=null0 \ -- add-port br1 null1 -- set int null1 type=patch options:peer=p0 -- set int p0 options:peer=null1]) -check_liveness 2 LIVE # Wait 4 more simulated seconds. The LACP state should become "defaulted" for p0 and p2. ovs-appctl time/warp 4100 100 @@ -901,7 +900,6 @@ check_liveness 1 LIVE AT_CHECK([ovs-vsctl \ -- add-port br0 null0 -- set int null0 type=patch options:peer=p2 -- set int p2 options:peer=null0 \ -- add-port br1 null1 -- set int null1 type=patch options:peer=p0 -- set int p0 options:peer=null1]) -check_liveness 2 LIVE # Wait 4 more simulated seconds. The LACP state should become "defaulted" for p0 and p2. ovs-appctl time/warp 4100 100 @@ -997,7 +995,6 @@ check_liveness 1 LIVE AT_CHECK([ovs-vsctl \ -- add-port br0 null0 -- set int null0 type=patch options:peer=p2 -- set int p2 options:peer=null0 \ -- add-port br1 null1 -- set int null1 type=patch options:peer=p0 -- set int p0 options:peer=null1]) -check_liveness 2 LIVE # Wait 4 more simulated seconds. The LACP state should become "defaulted" for p0 and p2. ovs-appctl time/warp 4100 100