From patchwork Mon Oct 29 22:57:46 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 990597 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 42kVQD64Nbz9s7W for ; Tue, 30 Oct 2018 09:58:44 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id A2CF03462; Mon, 29 Oct 2018 22:58:15 +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 E87BD3451 for ; Mon, 29 Oct 2018 22:58:00 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay12.mail.gandi.net (relay12.mail.gandi.net [217.70.178.232]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id D5606735 for ; Mon, 29 Oct 2018 22:57:58 +0000 (UTC) Received: from sigabrt.benpfaff.org (unknown [208.91.3.26]) (Authenticated sender: blp@ovn.org) by relay12.mail.gandi.net (Postfix) with ESMTPSA id 35BE0200007; Mon, 29 Oct 2018 22:57:54 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Mon, 29 Oct 2018 15:57:46 -0700 Message-Id: <20181029225751.5936-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: Modernize coding style. 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 This moves declarations closer to first use and merges them with initialization when possible, moves "for" loop variable declarations into the "for" statements where possible, and otherwise makes this code look like it was written a little more recently than it was. Signed-off-by: Ben Pfaff Reviewed-by: Yifeng Sun --- ofproto/connmgr.c | 280 ++++++++++++++++++++++-------------------------------- 1 file changed, 112 insertions(+), 168 deletions(-) diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c index db9f6bad9a74..c7532cf01217 100644 --- a/ofproto/connmgr.c +++ b/ofproto/connmgr.c @@ -242,9 +242,7 @@ struct connmgr * connmgr_create(struct ofproto *ofproto, const char *name, const char *local_port_name) { - struct connmgr *mgr; - - mgr = xmalloc(sizeof *mgr); + struct connmgr *mgr = xmalloc(sizeof *mgr); mgr->ofproto = ofproto; mgr->name = xstrdup(name); mgr->local_port_name = xstrdup(local_port_name); @@ -304,26 +302,24 @@ void connmgr_destroy(struct connmgr *mgr) OVS_REQUIRES(ofproto_mutex) { - struct ofservice *ofservice, *next_ofservice; - struct ofconn *ofconn, *next_ofconn; - size_t i; - if (!mgr) { return; } + struct ofconn *ofconn, *next_ofconn; LIST_FOR_EACH_SAFE (ofconn, next_ofconn, node, &mgr->all_conns) { ofconn_destroy(ofconn); } hmap_destroy(&mgr->controllers); + struct ofservice *ofservice, *next_ofservice; HMAP_FOR_EACH_SAFE (ofservice, next_ofservice, node, &mgr->services) { ofservice_destroy(mgr, ofservice); } hmap_destroy(&mgr->services); - for (i = 0; i < mgr->n_snoops; i++) { + for (size_t i = 0; i < mgr->n_snoops; i++) { pvconn_close(mgr->snoops[i]); } free(mgr->snoops); @@ -350,10 +346,6 @@ connmgr_run(struct connmgr *mgr, const struct ofpbuf *ofp_msg)) OVS_EXCLUDED(ofproto_mutex) { - struct ofconn *ofconn, *next_ofconn; - struct ofservice *ofservice; - size_t i; - if (mgr->in_band) { if (!in_band_run(mgr->in_band)) { in_band_destroy(mgr->in_band); @@ -361,6 +353,7 @@ connmgr_run(struct connmgr *mgr, } } + struct ofconn *ofconn, *next_ofconn; LIST_FOR_EACH_SAFE (ofconn, next_ofconn, node, &mgr->all_conns) { ofconn_run(ofconn, handle_openflow); } @@ -372,19 +365,16 @@ connmgr_run(struct connmgr *mgr, fail_open_run(mgr->fail_open); } + struct ofservice *ofservice; HMAP_FOR_EACH (ofservice, node, &mgr->services) { struct vconn *vconn; - int retval; - - retval = pvconn_accept(ofservice->pvconn, &vconn); + int retval = pvconn_accept(ofservice->pvconn, &vconn); if (!retval) { - struct rconn *rconn; - char *name; - /* Passing default value for creation of the rconn */ - rconn = rconn_create(ofservice->probe_interval, 0, ofservice->dscp, - vconn_get_allowed_versions(vconn)); - name = ofconn_make_name(mgr, vconn_get_name(vconn)); + struct rconn *rconn = rconn_create( + ofservice->probe_interval, 0, ofservice->dscp, + vconn_get_allowed_versions(vconn)); + char *name = ofconn_make_name(mgr, vconn_get_name(vconn)); rconn_connect_unreliably(rconn, vconn, name); free(name); @@ -400,11 +390,9 @@ connmgr_run(struct connmgr *mgr, } } - for (i = 0; i < mgr->n_snoops; i++) { + for (size_t i = 0; i < mgr->n_snoops; i++) { struct vconn *vconn; - int retval; - - retval = pvconn_accept(mgr->snoops[i], &vconn); + int retval = pvconn_accept(mgr->snoops[i], &vconn); if (!retval) { add_snooper(mgr, vconn); } else if (retval != EAGAIN) { @@ -417,24 +405,27 @@ connmgr_run(struct connmgr *mgr, void connmgr_wait(struct connmgr *mgr) { - struct ofservice *ofservice; struct ofconn *ofconn; - size_t i; - LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { ofconn_wait(ofconn); } + ofmonitor_wait(mgr); + if (mgr->in_band) { in_band_wait(mgr->in_band); } + if (mgr->fail_open) { fail_open_wait(mgr->fail_open); } + + struct ofservice *ofservice; HMAP_FOR_EACH (ofservice, node, &mgr->services) { pvconn_wait(ofservice->pvconn); } - for (i = 0; i < mgr->n_snoops; i++) { + + for (size_t i = 0; i < mgr->n_snoops; i++) { pvconn_wait(mgr->snoops[i]); } } @@ -444,17 +435,15 @@ connmgr_wait(struct connmgr *mgr) void connmgr_get_memory_usage(const struct connmgr *mgr, struct simap *usage) { - const struct ofconn *ofconn; unsigned int packets = 0; unsigned int ofconns = 0; + const struct ofconn *ofconn; LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { - int i; - ofconns++; packets += rconn_count_txqlen(ofconn->rconn); - for (i = 0; i < N_SCHEDULERS; i++) { + for (int i = 0; i < N_SCHEDULERS; i++) { struct pinsched_stats stats; pinsched_get_stats(ofconn->schedulers[i], &stats); @@ -597,10 +586,6 @@ connmgr_set_controllers(struct connmgr *mgr, OVS_EXCLUDED(ofproto_mutex) { bool had_controllers = connmgr_has_controllers(mgr); - struct shash new_controllers; - struct ofconn *ofconn, *next_ofconn; - struct ofservice *ofservice, *next_ofservice; - size_t i; /* Required to add and remove ofconns. This could probably be narrowed to * cover a smaller amount of code, if that yielded some benefit. */ @@ -608,13 +593,13 @@ connmgr_set_controllers(struct connmgr *mgr, /* Create newly configured controllers and services. * Create a name to ofproto_controller mapping in 'new_controllers'. */ - shash_init(&new_controllers); - for (i = 0; i < n_controllers; i++) { + struct shash new_controllers = SHASH_INITIALIZER(&new_controllers); + for (size_t i = 0; i < n_controllers; i++) { const struct ofproto_controller *c = &controllers[i]; if (!vconn_verify_name(c->target)) { bool add = false; - ofconn = find_controller_by_target(mgr, c->target); + struct ofconn *ofconn = find_controller_by_target(mgr, c->target); if (!ofconn) { VLOG_INFO("%s: added primary controller \"%s\"", mgr->name, c->target); @@ -631,7 +616,7 @@ connmgr_set_controllers(struct connmgr *mgr, } } else if (!pvconn_verify_name(c->target)) { bool add = false; - ofservice = ofservice_lookup(mgr, c->target); + struct ofservice *ofservice = ofservice_lookup(mgr, c->target); if (!ofservice) { VLOG_INFO("%s: added service controller \"%s\"", mgr->name, c->target); @@ -656,11 +641,11 @@ connmgr_set_controllers(struct connmgr *mgr, /* Delete controllers that are no longer configured. * Update configuration of all now-existing controllers. */ + struct ofconn *ofconn, *next_ofconn; HMAP_FOR_EACH_SAFE (ofconn, next_ofconn, hmap_node, &mgr->controllers) { const char *target = ofconn_get_target(ofconn); - struct ofproto_controller *c; - - c = shash_find_data(&new_controllers, target); + struct ofproto_controller *c = shash_find_data(&new_controllers, + target); if (!c) { VLOG_INFO("%s: removed primary controller \"%s\"", mgr->name, target); @@ -672,11 +657,11 @@ connmgr_set_controllers(struct connmgr *mgr, /* Delete services that are no longer configured. * Update configuration of all now-existing services. */ + struct ofservice *ofservice, *next_ofservice; HMAP_FOR_EACH_SAFE (ofservice, next_ofservice, node, &mgr->services) { const char *target = pvconn_get_name(ofservice->pvconn); - struct ofproto_controller *c; - - c = shash_find_data(&new_controllers, target); + struct ofproto_controller *c = shash_find_data(&new_controllers, + target); if (!c) { VLOG_INFO("%s: removed service controller \"%s\"", mgr->name, target); @@ -723,9 +708,7 @@ connmgr_set_snoops(struct connmgr *mgr, const struct sset *snoops) void connmgr_get_snoops(const struct connmgr *mgr, struct sset *snoops) { - size_t i; - - for (i = 0; i < mgr->n_snoops; i++) { + for (size_t i = 0; i < mgr->n_snoops; i++) { sset_add(snoops, pvconn_get_name(mgr->snoops[i])); } } @@ -745,10 +728,9 @@ add_controller(struct connmgr *mgr, const char *target, uint8_t dscp, OVS_REQUIRES(ofproto_mutex) { char *name = ofconn_make_name(mgr, target); - struct ofconn *ofconn; - - ofconn = ofconn_create(mgr, rconn_create(5, 8, dscp, allowed_versions), - OFCONN_PRIMARY, true); + struct ofconn *ofconn = ofconn_create(mgr, rconn_create(5, 8, dscp, + allowed_versions), + OFCONN_PRIMARY, true); rconn_connect(ofconn->rconn, target, name); hmap_insert(&mgr->controllers, &ofconn->hmap_node, hash_string(target, 0)); @@ -772,17 +754,13 @@ find_controller_by_target(struct connmgr *mgr, const char *target) static void update_in_band_remotes(struct connmgr *mgr) { - struct sockaddr_in *addrs; - size_t max_addrs, n_addrs; - struct ofconn *ofconn; - size_t i; - /* Allocate enough memory for as many remotes as we could possibly have. */ - max_addrs = mgr->n_extra_remotes + hmap_count(&mgr->controllers); - addrs = xmalloc(max_addrs * sizeof *addrs); - n_addrs = 0; + size_t max_addrs = mgr->n_extra_remotes + hmap_count(&mgr->controllers); + struct sockaddr_in *addrs = xmalloc(max_addrs * sizeof *addrs); + size_t n_addrs = 0; /* Add all the remotes. */ + struct ofconn *ofconn; HMAP_FOR_EACH (ofconn, hmap_node, &mgr->controllers) { const char *target = rconn_get_target(ofconn->rconn); union { @@ -796,7 +774,7 @@ update_in_band_remotes(struct connmgr *mgr) addrs[n_addrs++] = sa.in; } } - for (i = 0; i < mgr->n_extra_remotes; i++) { + for (size_t i = 0; i < mgr->n_extra_remotes; i++) { addrs[n_addrs++] = mgr->extra_in_band_remotes[i]; } @@ -839,25 +817,26 @@ static int set_pvconns(struct pvconn ***pvconnsp, size_t *n_pvconnsp, const struct sset *sset) { - struct pvconn **pvconns = *pvconnsp; - size_t n_pvconns = *n_pvconnsp; - const char *name; - int retval = 0; - size_t i; - - for (i = 0; i < n_pvconns; i++) { - pvconn_close(pvconns[i]); + /* Free the old pvconns. */ + struct pvconn **old_pvconns = *pvconnsp; + size_t old_n_pvconns = *n_pvconnsp; + for (size_t i = 0; i < old_n_pvconns; i++) { + pvconn_close(old_pvconns[i]); } - free(pvconns); + free(old_pvconns); + + /* Populate the new pvconns. */ + struct pvconn **new_pvconns = xmalloc(sset_count(sset) + * sizeof *new_pvconns); + size_t new_n_pvconns = 0; - pvconns = xmalloc(sset_count(sset) * sizeof *pvconns); - n_pvconns = 0; + int retval = 0; + const char *name; SSET_FOR_EACH (name, sset) { struct pvconn *pvconn; - int error; - error = pvconn_open(name, 0, 0, &pvconn); + int error = pvconn_open(name, 0, 0, &pvconn); if (!error) { - pvconns[n_pvconns++] = pvconn; + new_pvconns[new_n_pvconns++] = pvconn; } else { VLOG_ERR("failed to listen on %s: %s", name, ovs_strerror(error)); if (!retval) { @@ -866,8 +845,8 @@ set_pvconns(struct pvconn ***pvconnsp, size_t *n_pvconnsp, } } - *pvconnsp = pvconns; - *n_pvconnsp = n_pvconns; + *pvconnsp = new_pvconns; + *n_pvconnsp = new_n_pvconns; return retval; } @@ -897,10 +876,9 @@ snoop_preference(const struct ofconn *ofconn) static void add_snooper(struct connmgr *mgr, struct vconn *vconn) { - struct ofconn *ofconn, *best; - /* Pick a controller for monitoring. */ - best = NULL; + struct ofconn *best = NULL; + struct ofconn *ofconn; LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { if (ofconn->type == OFCONN_PRIMARY && (!best || snoop_preference(ofconn) > snoop_preference(best))) { @@ -969,13 +947,12 @@ void ofconn_send_role_status(struct ofconn *ofconn, uint32_t role, uint8_t reason) { struct ofputil_role_status status; - struct ofpbuf *buf; - status.reason = reason; status.role = role; ofconn_get_master_election_id(ofconn, &status.generation_id); - buf = ofputil_encode_role_status(&status, ofconn_get_protocol(ofconn)); + struct ofpbuf *buf + = ofputil_encode_role_status(&status, ofconn_get_protocol(ofconn)); if (buf) { ofconn_send(ofconn, buf, NULL); } @@ -992,7 +969,8 @@ ofconn_set_role(struct ofconn *ofconn, enum ofp12_controller_role role) LIST_FOR_EACH (other, node, &ofconn->connmgr->all_conns) { if (other->role == OFPCR12_ROLE_MASTER) { other->role = OFPCR12_ROLE_SLAVE; - ofconn_send_role_status(other, OFPCR12_ROLE_SLAVE, OFPCRR_MASTER_REQUEST); + ofconn_send_role_status(other, OFPCR12_ROLE_SLAVE, + OFPCRR_MASTER_REQUEST); } } } @@ -1159,19 +1137,15 @@ ofconn_send_error(const struct ofconn *ofconn, const struct ofp_header *request, enum ofperr error) { static struct vlog_rate_limit err_rl = VLOG_RATE_LIMIT_INIT(10, 10); - struct ofpbuf *reply; - - reply = ofperr_encode_reply(error, request); + struct ofpbuf *reply = ofperr_encode_reply(error, request); if (!VLOG_DROP_INFO(&err_rl)) { - const char *type_name; - size_t request_len; - enum ofpraw raw; + size_t request_len = ntohs(request->length); - request_len = ntohs(request->length); - type_name = (!ofpraw_decode_partial(&raw, request, - MIN(64, request_len)) - ? ofpraw_get_name(raw) - : "invalid"); + enum ofpraw raw; + const char *type_name = (!ofpraw_decode_partial(&raw, request, + MIN(64, request_len)) + ? ofpraw_get_name(raw) + : "invalid"); VLOG_INFO("%s: sending %s error reply to %s message", rconn_get_name(ofconn->rconn), ofperr_to_string(error), @@ -1186,8 +1160,6 @@ void ofconn_report_flow_mod(struct ofconn *ofconn, enum ofp_flow_mod_command command) { - long long int now; - switch (command) { case OFPFC_ADD: ofconn->n_add++; @@ -1204,7 +1176,7 @@ ofconn_report_flow_mod(struct ofconn *ofconn, break; } - now = time_msec(); + long long int now = time_msec(); if (ofconn->next_op_report == LLONG_MAX) { ofconn->first_op = now; ofconn->next_op_report = MAX(now + 10 * 1000, ofconn->op_backoff); @@ -1260,9 +1232,9 @@ bundle_remove_all(struct ofconn *ofconn) static void bundle_remove_expired(struct ofconn *ofconn, long long int now) { - struct ofp_bundle *b, *next; long long int limit = now - bundle_idle_timeout; + struct ofp_bundle *b, *next; HMAP_FOR_EACH_SAFE (b, next, node, &ofconn->bundles) { if (b->used <= limit) { ofconn_send_error(ofconn, b->msg, OFPERR_OFPBFC_TIMEOUT); @@ -1284,9 +1256,7 @@ ofconn_create(struct connmgr *mgr, struct rconn *rconn, enum ofconn_type type, bool enable_async_msgs) OVS_REQUIRES(ofproto_mutex) { - struct ofconn *ofconn; - - ofconn = xzalloc(sizeof *ofconn); + struct ofconn *ofconn = xzalloc(sizeof *ofconn); ofconn->connmgr = mgr; ovs_list_push_back(&mgr->all_conns, &ofconn->node); ofconn->rconn = rconn; @@ -1310,9 +1280,6 @@ static void ofconn_flush(struct ofconn *ofconn) OVS_REQUIRES(ofproto_mutex) { - struct ofmonitor *monitor, *next_monitor; - int i; - ofconn_log_flow_mods(ofconn); ofconn->role = OFPCR12_ROLE_EQUAL; @@ -1321,7 +1288,7 @@ ofconn_flush(struct ofconn *ofconn) rconn_packet_counter_destroy(ofconn->packet_in_counter); ofconn->packet_in_counter = rconn_packet_counter_create(); - for (i = 0; i < N_SCHEDULERS; i++) { + for (int i = 0; i < N_SCHEDULERS; i++) { if (ofconn->schedulers[i]) { int rate, burst; @@ -1346,6 +1313,7 @@ ofconn_flush(struct ofconn *ofconn) ofconn->next_op_report = LLONG_MAX; ofconn->op_backoff = LLONG_MIN; + struct ofmonitor *monitor, *next_monitor; HMAP_FOR_EACH_SAFE (monitor, next_monitor, ofconn_node, &ofconn->monitors) { ofmonitor_destroy(monitor); @@ -1387,14 +1355,12 @@ ofconn_destroy(struct ofconn *ofconn) static void ofconn_reconfigure(struct ofconn *ofconn, const struct ofproto_controller *c) { - int probe_interval; - ofconn->band = c->band; ofconn->enable_async_msgs = c->enable_async_msgs; rconn_set_max_backoff(ofconn->rconn, c->max_backoff); - probe_interval = c->probe_interval ? MAX(c->probe_interval, 5) : 0; + int probe_interval = c->probe_interval ? MAX(c->probe_interval, 5) : 0; rconn_set_probe_interval(ofconn->rconn, probe_interval); ofconn_set_rate_limit(ofconn, c->rate_limit, c->burst_limit); @@ -1421,9 +1387,8 @@ ofconn_run(struct ofconn *ofconn, const struct ofpbuf *ofp_msg)) { struct connmgr *mgr = ofconn->connmgr; - size_t i; - for (i = 0; i < N_SCHEDULERS; i++) { + for (size_t i = 0; i < N_SCHEDULERS; i++) { struct ovs_list txq; pinsched_run(ofconn->schedulers[i], &txq); @@ -1433,7 +1398,7 @@ ofconn_run(struct ofconn *ofconn, rconn_run(ofconn->rconn); /* Limit the number of iterations to avoid starving other tasks. */ - for (i = 0; i < 50 && ofconn_may_recv(ofconn); i++) { + for (int i = 0; i < 50 && ofconn_may_recv(ofconn); i++) { struct ofpbuf *of_msg = rconn_recv(ofconn->rconn); if (!of_msg) { break; @@ -1470,9 +1435,7 @@ ofconn_run(struct ofconn *ofconn, static void ofconn_wait(struct ofconn *ofconn) { - int i; - - for (i = 0; i < N_SCHEDULERS; i++) { + for (int i = 0; i < N_SCHEDULERS; i++) { pinsched_wait(ofconn->schedulers[i]); } rconn_run_wait(ofconn->rconn); @@ -1585,9 +1548,7 @@ ofconn_make_name(const struct connmgr *mgr, const char *target) static void ofconn_set_rate_limit(struct ofconn *ofconn, int rate, int burst) { - int i; - - for (i = 0; i < N_SCHEDULERS; i++) { + for (int i = 0; i < N_SCHEDULERS; i++) { struct pinsched **s = &ofconn->schedulers[i]; if (rate > 0) { @@ -1719,14 +1680,13 @@ connmgr_send_flow_removed(struct connmgr *mgr, LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { if (ofconn_receives_async_msg(ofconn, OAM_FLOW_REMOVED, fr->reason)) { - struct ofpbuf *msg; - /* Account flow expirations as replies to OpenFlow requests. That * works because preventing OpenFlow requests from being processed * also prevents new flows from being added (and expiring). (It * also prevents processing OpenFlow requests that would not add * new flows, so it is imperfect.) */ - msg = ofputil_encode_flow_removed(fr, ofconn_get_protocol(ofconn)); + struct ofpbuf *msg = ofputil_encode_flow_removed( + fr, ofconn_get_protocol(ofconn)); ofconn_send_reply(ofconn, msg); } } @@ -1744,12 +1704,12 @@ connmgr_send_table_status(struct connmgr *mgr, const struct ofputil_table_desc *td, uint8_t reason) { - struct ofputil_table_status ts; - struct ofconn *ofconn; - - ts.reason = reason; - ts.desc = *td; + struct ofputil_table_status ts = { + .reason = reason, + .desc = *td + }; + struct ofconn *ofconn; LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { if (ofconn_receives_async_msg(ofconn, OAM_TABLE_STATUS, reason)) { struct ofpbuf *msg; @@ -1841,10 +1801,9 @@ connmgr_set_fail_mode(struct connmgr *mgr, enum ofproto_fail_mode fail_mode) int connmgr_get_max_probe_interval(const struct connmgr *mgr) { - const struct ofconn *ofconn; - int max_probe_interval; + int max_probe_interval = 0; - max_probe_interval = 0; + const struct ofconn *ofconn; HMAP_FOR_EACH (ofconn, hmap_node, &mgr->controllers) { int probe_interval = rconn_get_probe_interval(ofconn->rconn); max_probe_interval = MAX(max_probe_interval, probe_interval); @@ -1857,14 +1816,13 @@ connmgr_get_max_probe_interval(const struct connmgr *mgr) int connmgr_failure_duration(const struct connmgr *mgr) { - const struct ofconn *ofconn; - int min_failure_duration; - if (!connmgr_has_controllers(mgr)) { return 0; } - min_failure_duration = INT_MAX; + int min_failure_duration = INT_MAX; + + const struct ofconn *ofconn; HMAP_FOR_EACH (ofconn, hmap_node, &mgr->controllers) { int failure_duration = rconn_failure_duration(ofconn->rconn); min_failure_duration = MIN(min_failure_duration, failure_duration); @@ -1942,13 +1900,11 @@ static bool any_extras_changed(const struct connmgr *mgr, const struct sockaddr_in *extras, size_t n) { - size_t i; - if (n != mgr->n_extra_remotes) { return true; } - for (i = 0; i < n; i++) { + for (size_t i = 0; i < n; i++) { const struct sockaddr_in *old = &mgr->extra_in_band_remotes[i]; const struct sockaddr_in *new = &extras[i]; @@ -2029,16 +1985,13 @@ static int ofservice_create(struct connmgr *mgr, const char *target, uint32_t allowed_versions, uint8_t dscp) { - struct ofservice *ofservice; struct pvconn *pvconn; - int error; - - error = pvconn_open(target, allowed_versions, dscp, &pvconn); + int error = pvconn_open(target, allowed_versions, dscp, &pvconn); if (error) { return error; } - ofservice = xzalloc(sizeof *ofservice); + struct ofservice *ofservice = xzalloc(sizeof *ofservice); hmap_insert(&mgr->services, &ofservice->node, hash_string(target, 0)); ofservice->pvconn = pvconn; ofservice->allowed_versions = allowed_versions; @@ -2111,11 +2064,9 @@ ofmonitor_create(const struct ofputil_flow_monitor_request *request, struct ofconn *ofconn, struct ofmonitor **monitorp) OVS_REQUIRES(ofproto_mutex) { - struct ofmonitor *m; - *monitorp = NULL; - m = ofmonitor_lookup(ofconn, request->id); + struct ofmonitor *m = ofmonitor_lookup(ofconn, request->id); if (m) { return OFPERR_OFPMOFC_MONITOR_EXISTS; } @@ -2167,13 +2118,11 @@ ofmonitor_report(struct connmgr *mgr, struct rule *rule, const struct rule_actions *old_actions) OVS_REQUIRES(ofproto_mutex) { - enum nx_flow_monitor_flags update; - struct ofconn *ofconn; - if (rule_is_hidden(rule)) { return; } + enum nx_flow_monitor_flags update; switch (event) { case NXFME_ADDED: update = NXFMF_ADD; @@ -2194,10 +2143,8 @@ ofmonitor_report(struct connmgr *mgr, struct rule *rule, OVS_NOT_REACHED(); } + struct ofconn *ofconn; LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { - enum nx_flow_monitor_flags flags = 0; - struct ofmonitor *m; - if (ofconn->monitor_paused) { /* Only send NXFME_DELETED notifications for flows that were added * before we paused. */ @@ -2207,6 +2154,8 @@ ofmonitor_report(struct connmgr *mgr, struct rule *rule, } } + enum nx_flow_monitor_flags flags = 0; + struct ofmonitor *m; HMAP_FOR_EACH (m, ofconn_node, &ofconn->monitors) { if (m->flags & update && (m->table_id == 0xff || m->table_id == rule->table_id) @@ -2243,7 +2192,8 @@ ofmonitor_report(struct connmgr *mgr, struct rule *rule, ovs_mutex_unlock(&rule->mutex); if (flags & NXFMF_ACTIONS) { - const struct rule_actions *actions = rule_get_actions(rule); + const struct rule_actions *actions + = rule_get_actions(rule); fu.ofpacts = actions->ofpacts; fu.ofpacts_len = actions->ofpacts_len; } else { @@ -2282,12 +2232,10 @@ ofmonitor_flush(struct connmgr *mgr) if (!ofconn->monitor_paused && rconn_packet_counter_n_bytes(counter) > 128 * 1024) { - struct ofpbuf *pause; - COVERAGE_INC(ofmonitor_pause); ofconn->monitor_paused = monitor_seqno++; - pause = ofpraw_alloc_xid(OFPRAW_NXT_FLOW_MONITOR_PAUSED, - OFP10_VERSION, htonl(0), 0); + struct ofpbuf *pause = ofpraw_alloc_xid( + OFPRAW_NXT_FLOW_MONITOR_PAUSED, OFP10_VERSION, htonl(0), 0); ofconn_send(ofconn, pause, counter); } } @@ -2298,20 +2246,18 @@ ofmonitor_resume(struct ofconn *ofconn) OVS_REQUIRES(ofproto_mutex) { struct rule_collection rules; - struct ofpbuf *resumed; - struct ofmonitor *m; - struct ovs_list msgs; - rule_collection_init(&rules); + + struct ofmonitor *m; HMAP_FOR_EACH (m, ofconn_node, &ofconn->monitors) { ofmonitor_collect_resume_rules(m, ofconn->monitor_paused, &rules); } - ovs_list_init(&msgs); + struct ovs_list msgs = OVS_LIST_INITIALIZER(&msgs); ofmonitor_compose_refresh_updates(&rules, &msgs); - resumed = ofpraw_alloc_xid(OFPRAW_NXT_FLOW_MONITOR_RESUMED, OFP10_VERSION, - htonl(0), 0); + struct ofpbuf *resumed = ofpraw_alloc_xid(OFPRAW_NXT_FLOW_MONITOR_RESUMED, + OFP10_VERSION, htonl(0), 0); ovs_list_push_back(&msgs, &resumed->list_node); ofconn_send_replies(ofconn, &msgs); @@ -2329,9 +2275,8 @@ ofmonitor_may_resume(const struct ofconn *ofconn) static void ofmonitor_run(struct connmgr *mgr) { - struct ofconn *ofconn; - ovs_mutex_lock(&ofproto_mutex); + struct ofconn *ofconn; LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { if (ofmonitor_may_resume(ofconn)) { COVERAGE_INC(ofmonitor_resume); @@ -2344,9 +2289,8 @@ ofmonitor_run(struct connmgr *mgr) static void ofmonitor_wait(struct connmgr *mgr) { - struct ofconn *ofconn; - ovs_mutex_lock(&ofproto_mutex); + struct ofconn *ofconn; LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { if (ofmonitor_may_resume(ofconn)) { poll_immediate_wake(); From patchwork Mon Oct 29 22:57:47 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 990596 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 42kVPj6hkZz9s5c for ; Tue, 30 Oct 2018 09:58:17 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id C436D3460; Mon, 29 Oct 2018 22:58:14 +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 3E7D93451 for ; Mon, 29 Oct 2018 22:58:00 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay12.mail.gandi.net (relay12.mail.gandi.net [217.70.178.232]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 7E63E75B for ; Mon, 29 Oct 2018 22:57:59 +0000 (UTC) Received: from sigabrt.benpfaff.org (unknown [208.91.3.26]) (Authenticated sender: blp@ovn.org) by relay12.mail.gandi.net (Postfix) with ESMTPSA id 26628200002; Mon, 29 Oct 2018 22:57:56 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Mon, 29 Oct 2018 15:57:47 -0700 Message-Id: <20181029225751.5936-2-blp@ovn.org> X-Mailer: git-send-email 2.16.1 In-Reply-To: <20181029225751.5936-1-blp@ovn.org> References: <20181029225751.5936-1-blp@ovn.org> 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 2/6] rconn: New function rconn_is_reliable(). 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 This will have its first user in an upcoming commit. Signed-off-by: Ben Pfaff Reviewed-by: Yifeng Sun --- include/openvswitch/rconn.h | 1 + lib/rconn.c | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/include/openvswitch/rconn.h b/include/openvswitch/rconn.h index 5dc988af1dda..fd60a6ce1dea 100644 --- a/include/openvswitch/rconn.h +++ b/include/openvswitch/rconn.h @@ -79,6 +79,7 @@ const char *rconn_get_name(const struct rconn *); void rconn_set_name(struct rconn *, const char *new_name); const char *rconn_get_target(const struct rconn *); +bool rconn_is_reliable(const struct rconn *); bool rconn_is_alive(const struct rconn *); bool rconn_is_connected(const struct rconn *); bool rconn_is_admitted(const struct rconn *); diff --git a/lib/rconn.c b/lib/rconn.c index 3fabc504fba0..48ae8c6a72e5 100644 --- a/lib/rconn.c +++ b/lib/rconn.c @@ -883,6 +883,13 @@ rconn_get_target(const struct rconn *rc) return rc->target; } +/* Returns true if 'rconn' will reconnect if it disconnects. */ +bool +rconn_is_reliable(const struct rconn *rconn) +{ + return rconn->reliable; +} + /* Returns true if 'rconn' is connected or in the process of reconnecting, * false if 'rconn' is disconnected and will not reconnect on its own. */ bool From patchwork Mon Oct 29 22:57:48 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 990598 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 42kVQl4m8hz9s9m for ; Tue, 30 Oct 2018 09:59:11 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 86377345E; Mon, 29 Oct 2018 22:58:16 +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 53AEB3451 for ; Mon, 29 Oct 2018 22:58:02 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay12.mail.gandi.net (relay12.mail.gandi.net [217.70.178.232]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 0EA7E735 for ; Mon, 29 Oct 2018 22:58:00 +0000 (UTC) Received: from sigabrt.benpfaff.org (unknown [208.91.3.26]) (Authenticated sender: blp@ovn.org) by relay12.mail.gandi.net (Postfix) with ESMTPSA id A2FDE200006; Mon, 29 Oct 2018 22:57:58 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Mon, 29 Oct 2018 15:57:48 -0700 Message-Id: <20181029225751.5936-3-blp@ovn.org> X-Mailer: git-send-email 2.16.1 In-Reply-To: <20181029225751.5936-1-blp@ovn.org> References: <20181029225751.5936-1-blp@ovn.org> 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 3/6] connmgr: Improve interface for setting controllers. 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 Using an shash instead of an array simplifies the code for both the caller and the callee. Putting the set of allowed OpenFlow versions into the ofproto_controller data structure also simplifies the overall function interface slightly. Signed-off-by: Ben Pfaff Tested-by: Yifeng Sun Reviewed-by: Yifeng Sun --- ofproto/connmgr.c | 51 +++++++++++---------------- ofproto/connmgr.h | 5 ++- ofproto/ofproto.c | 7 ++-- ofproto/ofproto.h | 7 ++-- vswitchd/bridge.c | 102 ++++++++++++++++++++++-------------------------------- 5 files changed, 69 insertions(+), 103 deletions(-) diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c index c7532cf01217..f5576d50524d 100644 --- a/ofproto/connmgr.c +++ b/ofproto/connmgr.c @@ -580,9 +580,7 @@ connmgr_free_controller_info(struct shash *info) /* Changes 'mgr''s set of controllers to the 'n_controllers' controllers in * 'controllers'. */ void -connmgr_set_controllers(struct connmgr *mgr, - const struct ofproto_controller *controllers, - size_t n_controllers, uint32_t allowed_versions) +connmgr_set_controllers(struct connmgr *mgr, struct shash *controllers) OVS_EXCLUDED(ofproto_mutex) { bool had_controllers = connmgr_has_controllers(mgr); @@ -591,52 +589,49 @@ connmgr_set_controllers(struct connmgr *mgr, * cover a smaller amount of code, if that yielded some benefit. */ ovs_mutex_lock(&ofproto_mutex); - /* Create newly configured controllers and services. - * Create a name to ofproto_controller mapping in 'new_controllers'. */ - struct shash new_controllers = SHASH_INITIALIZER(&new_controllers); - for (size_t i = 0; i < n_controllers; i++) { - const struct ofproto_controller *c = &controllers[i]; + /* Create newly configured controllers and services. */ + struct shash_node *node; + SHASH_FOR_EACH (node, controllers) { + const char *target = node->name; + const struct ofproto_controller *c = node->data; - if (!vconn_verify_name(c->target)) { + if (!vconn_verify_name(target)) { bool add = false; - struct ofconn *ofconn = find_controller_by_target(mgr, c->target); + struct ofconn *ofconn = find_controller_by_target(mgr, target); if (!ofconn) { VLOG_INFO("%s: added primary controller \"%s\"", - mgr->name, c->target); + mgr->name, target); add = true; } else if (rconn_get_allowed_versions(ofconn->rconn) != - allowed_versions) { + c->allowed_versions) { VLOG_INFO("%s: re-added primary controller \"%s\"", - mgr->name, c->target); + mgr->name, target); add = true; ofconn_destroy(ofconn); } if (add) { - add_controller(mgr, c->target, c->dscp, allowed_versions); + add_controller(mgr, target, c->dscp, c->allowed_versions); } - } else if (!pvconn_verify_name(c->target)) { + } else if (!pvconn_verify_name(target)) { bool add = false; - struct ofservice *ofservice = ofservice_lookup(mgr, c->target); + struct ofservice *ofservice = ofservice_lookup(mgr, target); if (!ofservice) { VLOG_INFO("%s: added service controller \"%s\"", - mgr->name, c->target); + mgr->name, target); add = true; - } else if (ofservice->allowed_versions != allowed_versions) { + } else if (ofservice->allowed_versions != c->allowed_versions) { VLOG_INFO("%s: re-added service controller \"%s\"", - mgr->name, c->target); + mgr->name, target); ofservice_destroy(mgr, ofservice); add = true; } if (add) { - ofservice_create(mgr, c->target, allowed_versions, c->dscp); + ofservice_create(mgr, target, c->allowed_versions, c->dscp); } } else { VLOG_WARN_RL(&rl, "%s: unsupported controller \"%s\"", - mgr->name, c->target); - continue; + mgr->name, target); } - - shash_add_once(&new_controllers, c->target, &controllers[i]); } /* Delete controllers that are no longer configured. @@ -644,8 +639,7 @@ connmgr_set_controllers(struct connmgr *mgr, struct ofconn *ofconn, *next_ofconn; HMAP_FOR_EACH_SAFE (ofconn, next_ofconn, hmap_node, &mgr->controllers) { const char *target = ofconn_get_target(ofconn); - struct ofproto_controller *c = shash_find_data(&new_controllers, - target); + struct ofproto_controller *c = shash_find_data(controllers, target); if (!c) { VLOG_INFO("%s: removed primary controller \"%s\"", mgr->name, target); @@ -660,8 +654,7 @@ connmgr_set_controllers(struct connmgr *mgr, struct ofservice *ofservice, *next_ofservice; HMAP_FOR_EACH_SAFE (ofservice, next_ofservice, node, &mgr->services) { const char *target = pvconn_get_name(ofservice->pvconn); - struct ofproto_controller *c = shash_find_data(&new_controllers, - target); + struct ofproto_controller *c = shash_find_data(controllers, target); if (!c) { VLOG_INFO("%s: removed service controller \"%s\"", mgr->name, target); @@ -671,8 +664,6 @@ connmgr_set_controllers(struct connmgr *mgr, } } - shash_destroy(&new_controllers); - ovs_mutex_unlock(&ofproto_mutex); update_in_band_remotes(mgr); diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h index 4a22f1c26611..11c8f9a85121 100644 --- a/ofproto/connmgr.h +++ b/ofproto/connmgr.h @@ -56,6 +56,7 @@ enum ofconn_type { OFCONN_PRIMARY, /* An ordinary OpenFlow controller. */ OFCONN_SERVICE /* A service connection, e.g. "ovs-ofctl". */ }; +const char *ofconn_type_to_string(enum ofconn_type); /* An asynchronous message that might need to be queued between threads. */ struct ofproto_async_msg { @@ -94,9 +95,7 @@ void connmgr_retry(struct connmgr *); bool connmgr_has_controllers(const struct connmgr *); void connmgr_get_controller_info(struct connmgr *, struct shash *); void connmgr_free_controller_info(struct shash *); -void connmgr_set_controllers(struct connmgr *, - const struct ofproto_controller[], size_t n, - uint32_t allowed_versions); +void connmgr_set_controllers(struct connmgr *, struct shash *); void connmgr_reconnect(const struct connmgr *); int connmgr_set_snoops(struct connmgr *, const struct sset *snoops); diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 8b2e3ca97a2b..222c749940ec 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -639,12 +639,9 @@ ofproto_set_datapath_id(struct ofproto *p, uint64_t datapath_id) } void -ofproto_set_controllers(struct ofproto *p, - const struct ofproto_controller *controllers, - size_t n_controllers, uint32_t allowed_versions) +ofproto_set_controllers(struct ofproto *p, struct shash *controllers) { - connmgr_set_controllers(p->connmgr, controllers, n_controllers, - allowed_versions); + connmgr_set_controllers(p->connmgr, controllers); } void diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h index 3ca88baf018f..595729dd61f1 100644 --- a/ofproto/ofproto.h +++ b/ofproto/ofproto.h @@ -208,11 +208,12 @@ enum ofproto_band { }; struct ofproto_controller { - char *target; /* e.g. "tcp:127.0.0.1" */ int max_backoff; /* Maximum reconnection backoff, in seconds. */ int probe_interval; /* Max idle time before probing, in seconds. */ enum ofproto_band band; /* In-band or out-of-band? */ bool enable_async_msgs; /* Initially enable asynchronous messages? */ + uint32_t allowed_versions; /* OpenFlow protocol versions that may + * be negotiated for a session. */ /* OpenFlow packet-in rate-limiting. */ int rate_limit; /* Max packet-in rate in packets per second. */ @@ -304,9 +305,7 @@ int ofproto_port_query_by_name(const struct ofproto *, const char *devname, /* Top-level configuration. */ uint64_t ofproto_get_datapath_id(const struct ofproto *); void ofproto_set_datapath_id(struct ofproto *, uint64_t datapath_id); -void ofproto_set_controllers(struct ofproto *, - const struct ofproto_controller *, size_t n, - uint32_t allowed_versions); +void ofproto_set_controllers(struct ofproto *, struct shash *controllers); void ofproto_set_fail_mode(struct ofproto *, enum ofproto_fail_mode fail_mode); void ofproto_reconnect_controllers(struct ofproto *); void ofproto_set_extra_in_band_remotes(struct ofproto *, diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 9d230b20641c..83708ee51c7a 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -3462,48 +3462,6 @@ bridge_del_ports(struct bridge *br, const struct shash *wanted_ports) } } -/* Initializes 'oc' appropriately as a management service controller for - * 'br'. - * - * The caller must free oc->target when it is no longer needed. */ -static void -bridge_ofproto_controller_for_mgmt(const struct bridge *br, - struct ofproto_controller *oc) -{ - oc->target = xasprintf("punix:%s/%s.mgmt", ovs_rundir(), br->name); - oc->max_backoff = 0; - oc->probe_interval = 60; - oc->band = OFPROTO_OUT_OF_BAND; - oc->rate_limit = 0; - oc->burst_limit = 0; - oc->enable_async_msgs = true; - oc->dscp = 0; -} - -/* Converts ovsrec_controller 'c' into an ofproto_controller in 'oc'. */ -static void -bridge_ofproto_controller_from_ovsrec(const struct ovsrec_controller *c, - struct ofproto_controller *oc) -{ - int dscp; - - oc->target = c->target; - oc->max_backoff = c->max_backoff ? *c->max_backoff / 1000 : 8; - oc->probe_interval = c->inactivity_probe ? *c->inactivity_probe / 1000 : 5; - oc->band = (!c->connection_mode || !strcmp(c->connection_mode, "in-band") - ? OFPROTO_IN_BAND : OFPROTO_OUT_OF_BAND); - oc->rate_limit = c->controller_rate_limit ? *c->controller_rate_limit : 0; - oc->burst_limit = (c->controller_burst_limit - ? *c->controller_burst_limit : 0); - oc->enable_async_msgs = (!c->enable_async_messages - || *c->enable_async_messages); - dscp = smap_get_int(&c->other_config, "dscp", DSCP_DEFAULT); - if (dscp < 0 || dscp > 63) { - dscp = DSCP_DEFAULT; - } - oc->dscp = dscp; -} - /* Configures the IP stack for 'br''s local interface properly according to the * configuration in 'c'. */ static void @@ -3589,10 +3547,6 @@ bridge_configure_remotes(struct bridge *br, enum ofproto_fail_mode fail_mode; - struct ofproto_controller *ocs; - size_t n_ocs; - size_t i; - /* Check if we should disable in-band control on this bridge. */ disable_in_band = smap_get_bool(&br->cfg->other_config, "disable-in-band", false); @@ -3611,13 +3565,22 @@ bridge_configure_remotes(struct bridge *br, n_controllers = (ofproto_get_flow_restore_wait() ? 0 : bridge_get_controllers(br, &controllers)); - ocs = xmalloc((n_controllers + 1) * sizeof *ocs); - n_ocs = 0; - - bridge_ofproto_controller_for_mgmt(br, &ocs[n_ocs++]); - for (i = 0; i < n_controllers; i++) { + /* The set of controllers to pass down to ofproto. */ + struct shash ocs = SHASH_INITIALIZER(&ocs); + + /* Add managment controller. */ + struct ofproto_controller *oc = xmalloc(sizeof *oc); + *oc = (struct ofproto_controller) { + .probe_interval = 60, + .band = OFPROTO_OUT_OF_BAND, + .enable_async_msgs = true, + .allowed_versions = bridge_get_allowed_versions(br), + }; + shash_add_nocopy( + &ocs, xasprintf("punix:%s/%s.mgmt", ovs_rundir(), br->name), oc); + + for (size_t i = 0; i < n_controllers; i++) { struct ovsrec_controller *c = controllers[i]; - if (daemon_should_self_confine() && (!strncmp(c->target, "punix:", 6) || !strncmp(c->target, "unix:", 5))) { @@ -3668,17 +3631,34 @@ bridge_configure_remotes(struct bridge *br, } bridge_configure_local_iface_netdev(br, c); - bridge_ofproto_controller_from_ovsrec(c, &ocs[n_ocs]); - if (disable_in_band) { - ocs[n_ocs].band = OFPROTO_OUT_OF_BAND; + + int dscp = smap_get_int(&c->other_config, "dscp", DSCP_DEFAULT); + if (dscp < 0 || dscp > 63) { + dscp = DSCP_DEFAULT; } - n_ocs++; - } - ofproto_set_controllers(br->ofproto, ocs, n_ocs, - bridge_get_allowed_versions(br)); - free(ocs[0].target); /* From bridge_ofproto_controller_for_mgmt(). */ - free(ocs); + oc = xmalloc(sizeof *oc); + *oc = (struct ofproto_controller) { + .max_backoff = c->max_backoff ? *c->max_backoff / 1000 : 8, + .probe_interval = (c->inactivity_probe + ? *c->inactivity_probe / 1000 : 5), + .band = ((!c->connection_mode + || !strcmp(c->connection_mode, "in-band")) + && !disable_in_band + ? OFPROTO_IN_BAND : OFPROTO_OUT_OF_BAND), + .enable_async_msgs = (!c->enable_async_messages + || *c->enable_async_messages), + .allowed_versions = bridge_get_allowed_versions(br), + .rate_limit = (c->controller_rate_limit + ? *c->controller_rate_limit : 0), + .burst_limit = (c->controller_burst_limit + ? *c->controller_burst_limit : 0), + .dscp = dscp, + }; + shash_add(&ocs, c->target, oc); + } + ofproto_set_controllers(br->ofproto, &ocs); + shash_destroy_free_data(&ocs); /* Set the fail-mode. */ fail_mode = !br->cfg->fail_mode From patchwork Mon Oct 29 22:57:49 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 990599 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 42kVRG0nD7z9s5c for ; Tue, 30 Oct 2018 09:59:38 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 5E24A3481; Mon, 29 Oct 2018 22:58:25 +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 80F7F3451 for ; Mon, 29 Oct 2018 22:58:05 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay12.mail.gandi.net (relay12.mail.gandi.net [217.70.178.232]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id CA299735 for ; Mon, 29 Oct 2018 22:58:02 +0000 (UTC) Received: from sigabrt.benpfaff.org (unknown [208.91.3.26]) (Authenticated sender: blp@ovn.org) by relay12.mail.gandi.net (Postfix) with ESMTPSA id 21FB8200008; Mon, 29 Oct 2018 22:57:59 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Mon, 29 Oct 2018 15:57:49 -0700 Message-Id: <20181029225751.5936-4-blp@ovn.org> X-Mailer: git-send-email 2.16.1 In-Reply-To: <20181029225751.5936-1-blp@ovn.org> References: <20181029225751.5936-1-blp@ovn.org> 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 4/6] connmgr: Make treatment of active and passive connections more uniform. 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 Until now, connmgr has handled active and passive OpenFlow connections in quite different ways. Any active connection, whether it was currently connected or not, was always maintained as an ofconn. Whenever such a connection (re)connected, its settings were cleared. On the other hand, passive connections had a separate listener which created an ofconn when a new connection came in, and these ofconns would be deleted when such a connection was closed. This approach is inelegant and has occasionally led to bugs when reconnection didn't clear all of the state that it should have. There's another motivation here. Currently, active connections are always primary controllers and passive connections are always service controllers (as documented in ovs-vswitchd.conf.db(5)). Sometimes it would be useful to have passive primary controllers (maybe active service controllers too but I haven't personally run into that use case). As is, this is difficult to implement because there is so much different code in use between active and passive connections. This commit will make it easier. Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- ofproto/connmgr.c | 615 +++++++++++++++++++++++++++--------------------------- vswitchd/bridge.c | 2 +- 2 files changed, 310 insertions(+), 307 deletions(-) diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c index f5576d50524d..30d543d220e5 100644 --- a/ofproto/connmgr.c +++ b/ofproto/connmgr.c @@ -57,20 +57,17 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); * connmgr. 'ofproto_mutex' doesn't protect the data inside the ofconn, except * as specifically noted below. */ struct ofconn { -/* Configuration that persists from one connection to the next. */ + struct connmgr *connmgr; /* Connection's manager. */ + struct ovs_list connmgr_node; /* In connmgr->conns. */ - struct ovs_list node; /* In struct connmgr's "all_conns" list. */ - struct hmap_node hmap_node; /* In struct connmgr's "controllers" map. */ + struct ofservice *ofservice; /* Connection's service. */ + struct ovs_list ofservice_node; /* In service->conns. */ - struct connmgr *connmgr; /* Connection's manager. */ struct rconn *rconn; /* OpenFlow connection. */ enum ofconn_type type; /* Type. */ enum ofproto_band band; /* In-band or out-of-band? */ - bool enable_async_msgs; /* Initially enable async messages? */ bool want_packet_in_on_miss; -/* State that should be cleared from one connection to the next. */ - /* OpenFlow state. */ enum ofp12_controller_role role; /* Role. */ enum ofputil_protocol protocol; /* Current protocol variant. */ @@ -145,11 +142,10 @@ struct ofconn { static unsigned int bundle_idle_timeout = BUNDLE_IDLE_TIMEOUT_DEFAULT; -static struct ofconn *ofconn_create(struct connmgr *, struct rconn *, - enum ofconn_type, bool enable_async_msgs) - OVS_REQUIRES(ofproto_mutex); +static void ofconn_create(struct ofservice *, struct rconn *, enum ofconn_type, + const struct ofproto_controller *settings) + OVS_EXCLUDED(ofproto_mutex); static void ofconn_destroy(struct ofconn *) OVS_REQUIRES(ofproto_mutex); -static void ofconn_flush(struct ofconn *) OVS_REQUIRES(ofproto_mutex); static void ofconn_reconfigure(struct ofconn *, const struct ofproto_controller *); @@ -161,7 +157,6 @@ static void ofconn_wait(struct ofconn *); static void ofconn_log_flow_mods(struct ofconn *); -static const char *ofconn_get_target(const struct ofconn *); static char *ofconn_make_name(const struct connmgr *, const char *target); static void ofconn_set_rate_limit(struct ofconn *, int rate, int burst); @@ -171,27 +166,33 @@ static void ofconn_send(const struct ofconn *, struct ofpbuf *, static void do_send_packet_ins(struct ofconn *, struct ovs_list *txq); -/* A listener for incoming OpenFlow "service" connections. */ +/* A listener for incoming OpenFlow connections or for establishing an + * outgoing connection. */ struct ofservice { - struct hmap_node node; /* In struct connmgr's "services" hmap. */ - struct pvconn *pvconn; /* OpenFlow connection listener. */ - - /* These are not used by ofservice directly. They are settings for - * accepted "struct ofconn"s from the pvconn. */ - int probe_interval; /* Max idle time before probing, in seconds. */ - int rate_limit; /* Max packet-in rate in packets per second. */ - int burst_limit; /* Limit on accumulating packet credits. */ - bool enable_async_msgs; /* Initially enable async messages? */ - uint8_t dscp; /* DSCP Value for controller connection */ - uint32_t allowed_versions; /* OpenFlow protocol versions that may - * be negotiated for a session. */ + struct hmap_node hmap_node; /* In connmgr->services, by target. */ + struct connmgr *connmgr; + + char *target; /* e.g. "tcp:..." or "pssl:...". */ + struct ovs_list conns; /* "ofconn"s generated by this service. */ + enum ofconn_type type; /* OFCONN_PRIMARY or OFCONN_SERVICE. */ + + /* Source of connections. */ + struct rconn *rconn; /* Active connection only. */ + struct pvconn *pvconn; /* Passive listener only. */ + + /* Settings for "struct ofconn"s established by this service. */ + struct ofproto_controller s; }; +static void ofservice_run(struct ofservice *); +static void ofservice_wait(struct ofservice *); static void ofservice_reconfigure(struct ofservice *, - const struct ofproto_controller *); -static int ofservice_create(struct connmgr *mgr, const char *target, - uint32_t allowed_versions, uint8_t dscp); -static void ofservice_destroy(struct connmgr *, struct ofservice *); + const struct ofproto_controller *) + OVS_REQUIRES(ofproto_mutex); +static void ofservice_create(struct connmgr *mgr, const char *target, + const struct ofproto_controller *) + OVS_REQUIRES(ofproto_mutex); +static void ofservice_destroy(struct ofservice *) OVS_REQUIRES(ofproto_mutex); static struct ofservice *ofservice_lookup(struct connmgr *, const char *target); @@ -201,17 +202,17 @@ struct connmgr { char *name; char *local_port_name; - /* OpenFlow connections. */ - struct hmap controllers; /* All OFCONN_PRIMARY controllers. */ - struct ovs_list all_conns; /* All controllers. All modifications are - protected by ofproto_mutex, so that any - traversals from other threads can be made - safe by holding the ofproto_mutex. */ + /* OpenFlow connections. + * + * All modifications to 'conns' protected by ofproto_mutex, so that any + * traversals from other threads can be made safe by holding the + * ofproto_mutex.*/ + struct ovs_list conns; /* All ofconns. */ uint64_t master_election_id; /* monotonically increasing sequence number * for master election */ bool master_election_id_defined; - /* OpenFlow listeners. */ + /* OpenFlow connection establishment. */ struct hmap services; /* Contains "struct ofservice"s. */ struct pvconn **snoops; size_t n_snoops; @@ -247,8 +248,7 @@ connmgr_create(struct ofproto *ofproto, mgr->name = xstrdup(name); mgr->local_port_name = xstrdup(local_port_name); - hmap_init(&mgr->controllers); - ovs_list_init(&mgr->all_conns); + ovs_list_init(&mgr->conns); mgr->master_election_id = 0; mgr->master_election_id_defined = false; @@ -306,18 +306,12 @@ connmgr_destroy(struct connmgr *mgr) return; } - struct ofconn *ofconn, *next_ofconn; - LIST_FOR_EACH_SAFE (ofconn, next_ofconn, node, &mgr->all_conns) { - ofconn_destroy(ofconn); - } - - hmap_destroy(&mgr->controllers); - struct ofservice *ofservice, *next_ofservice; - HMAP_FOR_EACH_SAFE (ofservice, next_ofservice, node, &mgr->services) { - ofservice_destroy(mgr, ofservice); + HMAP_FOR_EACH_SAFE (ofservice, next_ofservice, hmap_node, &mgr->services) { + ofservice_destroy(ofservice); } hmap_destroy(&mgr->services); + ovs_assert(ovs_list_is_empty(&mgr->conns)); for (size_t i = 0; i < mgr->n_snoops; i++) { pvconn_close(mgr->snoops[i]); @@ -354,7 +348,7 @@ connmgr_run(struct connmgr *mgr, } struct ofconn *ofconn, *next_ofconn; - LIST_FOR_EACH_SAFE (ofconn, next_ofconn, node, &mgr->all_conns) { + LIST_FOR_EACH_SAFE (ofconn, next_ofconn, connmgr_node, &mgr->conns) { ofconn_run(ofconn, handle_openflow); } ofmonitor_run(mgr); @@ -366,28 +360,8 @@ connmgr_run(struct connmgr *mgr, } struct ofservice *ofservice; - HMAP_FOR_EACH (ofservice, node, &mgr->services) { - struct vconn *vconn; - int retval = pvconn_accept(ofservice->pvconn, &vconn); - if (!retval) { - /* Passing default value for creation of the rconn */ - struct rconn *rconn = rconn_create( - ofservice->probe_interval, 0, ofservice->dscp, - vconn_get_allowed_versions(vconn)); - char *name = ofconn_make_name(mgr, vconn_get_name(vconn)); - rconn_connect_unreliably(rconn, vconn, name); - free(name); - - ovs_mutex_lock(&ofproto_mutex); - ofconn = ofconn_create(mgr, rconn, OFCONN_SERVICE, - ofservice->enable_async_msgs); - ovs_mutex_unlock(&ofproto_mutex); - - ofconn_set_rate_limit(ofconn, ofservice->rate_limit, - ofservice->burst_limit); - } else if (retval != EAGAIN) { - VLOG_WARN_RL(&rl, "accept failed (%s)", ovs_strerror(retval)); - } + HMAP_FOR_EACH (ofservice, hmap_node, &mgr->services) { + ofservice_run(ofservice); } for (size_t i = 0; i < mgr->n_snoops; i++) { @@ -406,7 +380,7 @@ void connmgr_wait(struct connmgr *mgr) { struct ofconn *ofconn; - LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { + LIST_FOR_EACH (ofconn, connmgr_node, &mgr->conns) { ofconn_wait(ofconn); } @@ -421,8 +395,8 @@ connmgr_wait(struct connmgr *mgr) } struct ofservice *ofservice; - HMAP_FOR_EACH (ofservice, node, &mgr->services) { - pvconn_wait(ofservice->pvconn); + HMAP_FOR_EACH (ofservice, hmap_node, &mgr->services) { + ofservice_wait(ofservice); } for (size_t i = 0; i < mgr->n_snoops; i++) { @@ -438,8 +412,8 @@ connmgr_get_memory_usage(const struct connmgr *mgr, struct simap *usage) unsigned int packets = 0; unsigned int ofconns = 0; - const struct ofconn *ofconn; - LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { + struct ofconn *ofconn; + LIST_FOR_EACH (ofconn, connmgr_node, &mgr->conns) { ofconns++; packets += rconn_count_txqlen(ofconn->rconn); @@ -475,11 +449,6 @@ connmgr_set_bundle_idle_timeout(unsigned timeout) /* OpenFlow configuration. */ -static void add_controller(struct connmgr *, const char *target, uint8_t dscp, - uint32_t allowed_versions) - OVS_REQUIRES(ofproto_mutex); -static struct ofconn *find_controller_by_target(struct connmgr *, - const char *target); static void update_fail_open(struct connmgr *) OVS_EXCLUDED(ofproto_mutex); static int set_pvconns(struct pvconn ***pvconnsp, size_t *n_pvconnsp, const struct sset *); @@ -491,7 +460,22 @@ static int set_pvconns(struct pvconn ***pvconnsp, size_t *n_pvconnsp, bool connmgr_has_controllers(const struct connmgr *mgr) { - return !hmap_is_empty(&mgr->controllers); + struct ofservice *ofservice; + HMAP_FOR_EACH (ofservice, hmap_node, &mgr->services) { + if (ofservice->type == OFCONN_PRIMARY) { + return true; + } + } + return false; +} + +static struct ofconn * +ofservice_first_conn(const struct ofservice *ofservice) +{ + return (ovs_list_is_empty(&ofservice->conns) + ? NULL + : CONTAINER_OF(ofservice->conns.next, + struct ofconn, ofservice_node)); } /* Initializes 'info' and populates it with information about each configured @@ -503,13 +487,16 @@ connmgr_has_controllers(const struct connmgr *mgr) void connmgr_get_controller_info(struct connmgr *mgr, struct shash *info) { - const struct ofconn *ofconn; - - HMAP_FOR_EACH (ofconn, hmap_node, &mgr->controllers) { - const struct rconn *rconn = ofconn->rconn; + struct ofservice *ofservice; + HMAP_FOR_EACH (ofservice, hmap_node, &mgr->services) { + const struct rconn *rconn = ofservice->rconn; + if (!rconn) { + continue; + } const char *target = rconn_get_target(rconn); if (!shash_find(info, target)) { + struct ofconn *ofconn = ofservice_first_conn(ofservice); struct ofproto_controller_info *cinfo = xmalloc(sizeof *cinfo); time_t now = time_now(); time_t last_connection = rconn_get_last_connection(rconn); @@ -520,7 +507,7 @@ connmgr_get_controller_info(struct connmgr *mgr, struct shash *info) shash_add(info, target, cinfo); cinfo->is_connected = rconn_is_connected(rconn); - cinfo->role = ofconn->role; + cinfo->role = ofconn ? ofconn->role : OFPCR12_ROLE_NOCHANGE; smap_init(&cinfo->pairs); if (last_error) { @@ -541,7 +528,7 @@ connmgr_get_controller_info(struct connmgr *mgr, struct shash *info) } for (i = 0; i < N_SCHEDULERS; i++) { - if (ofconn->schedulers[i]) { + if (ofconn && ofconn->schedulers[i]) { const char *name = i ? "miss" : "action"; struct pinsched_stats stats; @@ -589,76 +576,27 @@ connmgr_set_controllers(struct connmgr *mgr, struct shash *controllers) * cover a smaller amount of code, if that yielded some benefit. */ ovs_mutex_lock(&ofproto_mutex); - /* Create newly configured controllers and services. */ + /* Create newly configured services. */ struct shash_node *node; SHASH_FOR_EACH (node, controllers) { const char *target = node->name; const struct ofproto_controller *c = node->data; - - if (!vconn_verify_name(target)) { - bool add = false; - struct ofconn *ofconn = find_controller_by_target(mgr, target); - if (!ofconn) { - VLOG_INFO("%s: added primary controller \"%s\"", - mgr->name, target); - add = true; - } else if (rconn_get_allowed_versions(ofconn->rconn) != - c->allowed_versions) { - VLOG_INFO("%s: re-added primary controller \"%s\"", - mgr->name, target); - add = true; - ofconn_destroy(ofconn); - } - if (add) { - add_controller(mgr, target, c->dscp, c->allowed_versions); - } - } else if (!pvconn_verify_name(target)) { - bool add = false; - struct ofservice *ofservice = ofservice_lookup(mgr, target); - if (!ofservice) { - VLOG_INFO("%s: added service controller \"%s\"", - mgr->name, target); - add = true; - } else if (ofservice->allowed_versions != c->allowed_versions) { - VLOG_INFO("%s: re-added service controller \"%s\"", - mgr->name, target); - ofservice_destroy(mgr, ofservice); - add = true; - } - if (add) { - ofservice_create(mgr, target, c->allowed_versions, c->dscp); - } - } else { - VLOG_WARN_RL(&rl, "%s: unsupported controller \"%s\"", - mgr->name, target); - } - } - - /* Delete controllers that are no longer configured. - * Update configuration of all now-existing controllers. */ - struct ofconn *ofconn, *next_ofconn; - HMAP_FOR_EACH_SAFE (ofconn, next_ofconn, hmap_node, &mgr->controllers) { - const char *target = ofconn_get_target(ofconn); - struct ofproto_controller *c = shash_find_data(controllers, target); - if (!c) { - VLOG_INFO("%s: removed primary controller \"%s\"", - mgr->name, target); - ofconn_destroy(ofconn); - } else { - ofconn_reconfigure(ofconn, c); + if (!ofservice_lookup(mgr, target)) { + ofservice_create(mgr, target, c); } } /* Delete services that are no longer configured. * Update configuration of all now-existing services. */ struct ofservice *ofservice, *next_ofservice; - HMAP_FOR_EACH_SAFE (ofservice, next_ofservice, node, &mgr->services) { - const char *target = pvconn_get_name(ofservice->pvconn); + HMAP_FOR_EACH_SAFE (ofservice, next_ofservice, hmap_node, &mgr->services) { + const char *target = ofservice->target; struct ofproto_controller *c = shash_find_data(controllers, target); if (!c) { - VLOG_INFO("%s: removed service controller \"%s\"", - mgr->name, target); - ofservice_destroy(mgr, ofservice); + VLOG_INFO("%s: removed %s controller \"%s\"", + mgr->name, ofconn_type_to_string(ofservice->type), + target); + ofservice_destroy(ofservice); } else { ofservice_reconfigure(ofservice, c); } @@ -680,7 +618,7 @@ connmgr_reconnect(const struct connmgr *mgr) { struct ofconn *ofconn; - LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { + LIST_FOR_EACH (ofconn, connmgr_node, &mgr->conns) { rconn_reconnect(ofconn->rconn); } } @@ -711,55 +649,24 @@ connmgr_has_snoops(const struct connmgr *mgr) return mgr->n_snoops > 0; } -/* Creates a new controller for 'target' in 'mgr'. update_controller() needs - * to be called later to finish the new ofconn's configuration. */ -static void -add_controller(struct connmgr *mgr, const char *target, uint8_t dscp, - uint32_t allowed_versions) - OVS_REQUIRES(ofproto_mutex) -{ - char *name = ofconn_make_name(mgr, target); - struct ofconn *ofconn = ofconn_create(mgr, rconn_create(5, 8, dscp, - allowed_versions), - OFCONN_PRIMARY, true); - rconn_connect(ofconn->rconn, target, name); - hmap_insert(&mgr->controllers, &ofconn->hmap_node, hash_string(target, 0)); - - free(name); -} - -static struct ofconn * -find_controller_by_target(struct connmgr *mgr, const char *target) -{ - struct ofconn *ofconn; - - HMAP_FOR_EACH_WITH_HASH (ofconn, hmap_node, - hash_string(target, 0), &mgr->controllers) { - if (!strcmp(ofconn_get_target(ofconn), target)) { - return ofconn; - } - } - return NULL; -} - static void update_in_band_remotes(struct connmgr *mgr) { /* Allocate enough memory for as many remotes as we could possibly have. */ - size_t max_addrs = mgr->n_extra_remotes + hmap_count(&mgr->controllers); + size_t max_addrs = mgr->n_extra_remotes + hmap_count(&mgr->services); struct sockaddr_in *addrs = xmalloc(max_addrs * sizeof *addrs); size_t n_addrs = 0; /* Add all the remotes. */ - struct ofconn *ofconn; - HMAP_FOR_EACH (ofconn, hmap_node, &mgr->controllers) { - const char *target = rconn_get_target(ofconn->rconn); + struct ofservice *ofservice; + HMAP_FOR_EACH (ofservice, hmap_node, &mgr->services) { + const char *target = ofservice->target; union { struct sockaddr_storage ss; struct sockaddr_in in; } sa; - if (ofconn->band == OFPROTO_IN_BAND + if (ofservice->s.band == OFPROTO_IN_BAND && stream_parse_target_with_default_port(target, OFP_PORT, &sa.ss) && sa.ss.ss_family == AF_INET) { addrs[n_addrs++] = sa.in; @@ -846,8 +753,13 @@ set_pvconns(struct pvconn ***pvconnsp, size_t *n_pvconnsp, * means that 'ofconn' is more interesting for monitoring than a lower return * value. */ static int -snoop_preference(const struct ofconn *ofconn) +snoop_preference(const struct ofservice *ofservice) { + struct ofconn *ofconn = ofservice_first_conn(ofservice); + if (!ofconn) { + return 0; + } + switch (ofconn->role) { case OFPCR12_ROLE_MASTER: return 3; @@ -868,12 +780,12 @@ static void add_snooper(struct connmgr *mgr, struct vconn *vconn) { /* Pick a controller for monitoring. */ - struct ofconn *best = NULL; - struct ofconn *ofconn; - LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { - if (ofconn->type == OFCONN_PRIMARY - && (!best || snoop_preference(ofconn) > snoop_preference(best))) { - best = ofconn; + struct ofservice *best = NULL; + struct ofservice *ofservice; + HMAP_FOR_EACH (ofservice, hmap_node, &mgr->services) { + if (ofservice->rconn && + (!best || snoop_preference(ofservice) > snoop_preference(best))) { + best = ofservice; } } @@ -957,7 +869,7 @@ ofconn_set_role(struct ofconn *ofconn, enum ofp12_controller_role role) if (role != ofconn->role && role == OFPCR12_ROLE_MASTER) { struct ofconn *other; - LIST_FOR_EACH (other, node, &ofconn->connmgr->all_conns) { + LIST_FOR_EACH (other, connmgr_node, &ofconn->connmgr->conns) { if (other->role == OFPCR12_ROLE_MASTER) { other->role = OFPCR12_ROLE_SLAVE; ofconn_send_role_status(other, OFPCR12_ROLE_SLAVE, @@ -1095,7 +1007,7 @@ ofconn_get_async_config(const struct ofconn *ofconn) } int version = rconn_get_version(ofconn->rconn); - return (version < 0 || !ofconn->enable_async_msgs + return (version < 0 || !ofconn->ofservice->s.enable_async_msgs ? OFPUTIL_ASYNC_CFG_INIT : ofputil_async_cfg_default(version)); } @@ -1236,67 +1148,37 @@ bundle_remove_expired(struct ofconn *ofconn, long long int now) /* Private ofconn functions. */ -static const char * -ofconn_get_target(const struct ofconn *ofconn) +static void +ofconn_create(struct ofservice *ofservice, struct rconn *rconn, + enum ofconn_type type, const struct ofproto_controller *settings) + OVS_EXCLUDED(ofproto_mutex) { - return rconn_get_target(ofconn->rconn); -} + ovs_mutex_lock(&ofproto_mutex); -static struct ofconn * -ofconn_create(struct connmgr *mgr, struct rconn *rconn, enum ofconn_type type, - bool enable_async_msgs) - OVS_REQUIRES(ofproto_mutex) -{ struct ofconn *ofconn = xzalloc(sizeof *ofconn); - ofconn->connmgr = mgr; - ovs_list_push_back(&mgr->all_conns, &ofconn->node); - ofconn->rconn = rconn; - ofconn->type = type; - ofconn->enable_async_msgs = enable_async_msgs; - - hmap_init(&ofconn->monitors); - ovs_list_init(&ofconn->updates); - hmap_init(&ofconn->bundles); - ofconn->next_bundle_expiry_check = time_msec() + BUNDLE_EXPIRY_INTERVAL; + ofconn->connmgr = ofservice->connmgr; + ovs_list_push_back(&ofservice->connmgr->conns, &ofconn->connmgr_node); - ofconn_flush(ofconn); + ofconn->ofservice = ofservice; + ovs_list_push_back(&ofservice->conns, &ofconn->ofservice_node); - return ofconn; -} - -/* Clears all of the state in 'ofconn' that should not persist from one - * connection to the next. */ -static void -ofconn_flush(struct ofconn *ofconn) - OVS_REQUIRES(ofproto_mutex) -{ - ofconn_log_flow_mods(ofconn); + ofconn->rconn = rconn; + ofconn->type = type; + ofconn->band = settings->band; ofconn->role = OFPCR12_ROLE_EQUAL; ofconn_set_protocol(ofconn, OFPUTIL_P_NONE); ofconn->packet_in_format = OFPUTIL_PACKET_IN_STD; - rconn_packet_counter_destroy(ofconn->packet_in_counter); ofconn->packet_in_counter = rconn_packet_counter_create(); - for (int i = 0; i < N_SCHEDULERS; i++) { - if (ofconn->schedulers[i]) { - int rate, burst; - - pinsched_get_limits(ofconn->schedulers[i], &rate, &burst); - pinsched_destroy(ofconn->schedulers[i]); - ofconn->schedulers[i] = pinsched_create(rate, burst); - } - } ofconn->miss_send_len = (ofconn->type == OFCONN_PRIMARY ? OFP_DEFAULT_MISS_SEND_LEN : 0); ofconn->controller_id = 0; - rconn_packet_counter_destroy(ofconn->reply_counter); ofconn->reply_counter = rconn_packet_counter_create(); - free(ofconn->async_cfg); ofconn->async_cfg = NULL; ofconn->n_add = ofconn->n_delete = ofconn->n_modify = 0; @@ -1304,59 +1186,81 @@ ofconn_flush(struct ofconn *ofconn) ofconn->next_op_report = LLONG_MAX; ofconn->op_backoff = LLONG_MIN; - struct ofmonitor *monitor, *next_monitor; - HMAP_FOR_EACH_SAFE (monitor, next_monitor, ofconn_node, - &ofconn->monitors) { - ofmonitor_destroy(monitor); - } - rconn_packet_counter_destroy(ofconn->monitor_counter); + hmap_init(&ofconn->monitors); ofconn->monitor_counter = rconn_packet_counter_create(); - ofpbuf_list_delete(&ofconn->updates); /* ...but it should be empty. */ + + ovs_list_init(&ofconn->updates); + + hmap_init(&ofconn->bundles); + ofconn->next_bundle_expiry_check = time_msec() + BUNDLE_EXPIRY_INTERVAL; + + ofconn_set_rate_limit(ofconn, settings->rate_limit, settings->burst_limit); + + ovs_mutex_unlock(&ofproto_mutex); } static void ofconn_destroy(struct ofconn *ofconn) OVS_REQUIRES(ofproto_mutex) { - ofconn_flush(ofconn); + if (!ofconn) { + return; + } + + ofconn_log_flow_mods(ofconn); + + ovs_list_remove(&ofconn->connmgr_node); + ovs_list_remove(&ofconn->ofservice_node); + + if (ofconn->rconn != ofconn->ofservice->rconn) { + rconn_destroy(ofconn->rconn); + } /* Force clearing of want_packet_in_on_miss to keep the global count * accurate. */ ofconn->controller_id = 1; update_want_packet_in_on_miss(ofconn); - if (ofconn->type == OFCONN_PRIMARY) { - hmap_remove(&ofconn->connmgr->controllers, &ofconn->hmap_node); + rconn_packet_counter_destroy(ofconn->packet_in_counter); + for (int i = 0; i < N_SCHEDULERS; i++) { + if (ofconn->schedulers[i]) { + pinsched_destroy(ofconn->schedulers[i]); + } } - bundle_remove_all(ofconn); - hmap_destroy(&ofconn->bundles); + rconn_packet_counter_destroy(ofconn->reply_counter); + + free(ofconn->async_cfg); + struct ofmonitor *monitor, *next_monitor; + HMAP_FOR_EACH_SAFE (monitor, next_monitor, ofconn_node, + &ofconn->monitors) { + ofmonitor_destroy(monitor); + } hmap_destroy(&ofconn->monitors); - ovs_list_remove(&ofconn->node); - rconn_destroy(ofconn->rconn); - rconn_packet_counter_destroy(ofconn->packet_in_counter); - rconn_packet_counter_destroy(ofconn->reply_counter); rconn_packet_counter_destroy(ofconn->monitor_counter); + + ofpbuf_list_delete(&ofconn->updates); /* ...but it should be empty. */ + + bundle_remove_all(ofconn); + hmap_destroy(&ofconn->bundles); + free(ofconn); } -/* Reconfigures 'ofconn' to match 'c'. 'ofconn' and 'c' must have the same - * target. */ +/* Reconfigures 'ofconn' to match 'c'. */ static void ofconn_reconfigure(struct ofconn *ofconn, const struct ofproto_controller *c) { - ofconn->band = c->band; - ofconn->enable_async_msgs = c->enable_async_msgs; - rconn_set_max_backoff(ofconn->rconn, c->max_backoff); int probe_interval = c->probe_interval ? MAX(c->probe_interval, 5) : 0; rconn_set_probe_interval(ofconn->rconn, probe_interval); + ofconn->band = c->band; + ofconn_set_rate_limit(ofconn, c->rate_limit, c->burst_limit); - /* If dscp value changed reconnect. */ if (c->dscp != rconn_get_dscp(ofconn->rconn)) { rconn_set_dscp(ofconn->rconn, c->dscp); rconn_reconnect(ofconn->rconn); @@ -1415,10 +1319,10 @@ ofconn_run(struct ofconn *ofconn, } ovs_mutex_lock(&ofproto_mutex); - if (!rconn_is_alive(ofconn->rconn)) { + if (rconn_is_reliable(ofconn->rconn) + ? !rconn_is_connected(ofconn->rconn) + : !rconn_is_alive(ofconn->rconn)) { ofconn_destroy(ofconn); - } else if (!rconn_is_connected(ofconn->rconn)) { - ofconn_flush(ofconn); } ovs_mutex_unlock(&ofproto_mutex); } @@ -1585,7 +1489,7 @@ connmgr_send_port_status(struct connmgr *mgr, struct ofconn *source, struct ofputil_port_status new_ps = { reason, *new_pp }; struct ofconn *ofconn; - LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { + LIST_FOR_EACH (ofconn, connmgr_node, &mgr->conns) { if (ofconn_receives_async_msg(ofconn, OAM_PORT_STATUS, reason)) { /* Before 1.5, OpenFlow specified that OFPT_PORT_MOD should not * generate OFPT_PORT_STATUS messages. That requirement was a @@ -1642,7 +1546,7 @@ connmgr_send_requestforward(struct connmgr *mgr, const struct ofconn *source, { struct ofconn *ofconn; - LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { + LIST_FOR_EACH (ofconn, connmgr_node, &mgr->conns) { /* METER_MOD only supported in OF13 and up. */ if (rf->reason == OFPRFR_METER_MOD && rconn_get_version(ofconn->rconn) < OFP13_VERSION) { @@ -1669,7 +1573,7 @@ connmgr_send_flow_removed(struct connmgr *mgr, { struct ofconn *ofconn; - LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { + LIST_FOR_EACH (ofconn, connmgr_node, &mgr->conns) { if (ofconn_receives_async_msg(ofconn, OAM_FLOW_REMOVED, fr->reason)) { /* Account flow expirations as replies to OpenFlow requests. That * works because preventing OpenFlow requests from being processed @@ -1701,7 +1605,7 @@ connmgr_send_table_status(struct connmgr *mgr, }; struct ofconn *ofconn; - LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { + LIST_FOR_EACH (ofconn, connmgr_node, &mgr->conns) { if (ofconn_receives_async_msg(ofconn, OAM_TABLE_STATUS, reason)) { struct ofpbuf *msg; @@ -1722,7 +1626,7 @@ connmgr_send_async_msg(struct connmgr *mgr, { struct ofconn *ofconn; - LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { + LIST_FOR_EACH (ofconn, connmgr_node, &mgr->conns) { enum ofputil_protocol protocol = ofconn_get_protocol(ofconn); if (protocol == OFPUTIL_P_NONE || !rconn_is_connected(ofconn->rconn) || ofconn->controller_id != am->controller_id @@ -1794,10 +1698,12 @@ connmgr_get_max_probe_interval(const struct connmgr *mgr) { int max_probe_interval = 0; - const struct ofconn *ofconn; - HMAP_FOR_EACH (ofconn, hmap_node, &mgr->controllers) { - int probe_interval = rconn_get_probe_interval(ofconn->rconn); - max_probe_interval = MAX(max_probe_interval, probe_interval); + struct ofservice *ofservice; + HMAP_FOR_EACH (ofservice, hmap_node, &mgr->services) { + if (ofservice->type == OFCONN_PRIMARY) { + int probe_interval = ofservice->s.probe_interval; + max_probe_interval = MAX(max_probe_interval, probe_interval); + } } return max_probe_interval; } @@ -1807,18 +1713,17 @@ connmgr_get_max_probe_interval(const struct connmgr *mgr) int connmgr_failure_duration(const struct connmgr *mgr) { - if (!connmgr_has_controllers(mgr)) { - return 0; - } - int min_failure_duration = INT_MAX; - const struct ofconn *ofconn; - HMAP_FOR_EACH (ofconn, hmap_node, &mgr->controllers) { - int failure_duration = rconn_failure_duration(ofconn->rconn); - min_failure_duration = MIN(min_failure_duration, failure_duration); + struct ofservice *ofservice; + HMAP_FOR_EACH (ofservice, hmap_node, &mgr->services) { + if (ofservice->rconn) { + int failure_duration = rconn_failure_duration(ofservice->rconn); + min_failure_duration = MIN(min_failure_duration, failure_duration); + } } - return min_failure_duration; + + return min_failure_duration != INT_MAX ? min_failure_duration : 0; } /* Returns true if at least one primary controller is connected (regardless of @@ -1827,10 +1732,9 @@ connmgr_failure_duration(const struct connmgr *mgr) bool connmgr_is_any_controller_connected(const struct connmgr *mgr) { - const struct ofconn *ofconn; - - HMAP_FOR_EACH (ofconn, hmap_node, &mgr->controllers) { - if (rconn_is_connected(ofconn->rconn)) { + struct ofservice *ofservice; + HMAP_FOR_EACH (ofservice, hmap_node, &mgr->services) { + if (ofservice->rconn && rconn_is_connected(ofservice->rconn)) { return true; } } @@ -1844,8 +1748,9 @@ connmgr_is_any_controller_admitted(const struct connmgr *mgr) { const struct ofconn *ofconn; - HMAP_FOR_EACH (ofconn, hmap_node, &mgr->controllers) { - if (rconn_is_admitted(ofconn->rconn)) { + LIST_FOR_EACH (ofconn, connmgr_node, &mgr->conns) { + if (ofconn->type == OFCONN_PRIMARY + && rconn_is_admitted(ofconn->rconn)) { return true; } } @@ -1972,41 +1877,133 @@ connmgr_count_hidden_rules(const struct connmgr *mgr) * * ofservice_reconfigure() must be called to fully configure the new * ofservice. */ -static int +static void ofservice_create(struct connmgr *mgr, const char *target, - uint32_t allowed_versions, uint8_t dscp) + const struct ofproto_controller *c) + OVS_REQUIRES(ofproto_mutex) { - struct pvconn *pvconn; - int error = pvconn_open(target, allowed_versions, dscp, &pvconn); - if (error) { - return error; + struct pvconn *pvconn = NULL; + struct rconn *rconn = NULL; + if (!vconn_verify_name(target)) { + char *name = ofconn_make_name(mgr, target); + rconn = rconn_create(5, 8, c->dscp, c->allowed_versions); + rconn_connect(rconn, target, name); + free(name); + } else if (!pvconn_verify_name(target)) { + int error = pvconn_open(target, c->allowed_versions, c->dscp, &pvconn); + if (error) { + return; + } + } else { + VLOG_WARN_RL(&rl, "%s: unsupported controller \"%s\"", + mgr->name, target); + return; } struct ofservice *ofservice = xzalloc(sizeof *ofservice); - hmap_insert(&mgr->services, &ofservice->node, hash_string(target, 0)); + hmap_insert(&mgr->services, &ofservice->hmap_node, hash_string(target, 0)); + ofservice->connmgr = mgr; + ofservice->target = xstrdup(target); + ovs_list_init(&ofservice->conns); + ofservice->type = rconn ? OFCONN_PRIMARY : OFCONN_SERVICE; + ofservice->rconn = rconn; ofservice->pvconn = pvconn; - ofservice->allowed_versions = allowed_versions; + ofservice->s = *c; + ofservice_reconfigure(ofservice, c); - return 0; + VLOG_INFO("%s: added %s controller \"%s\"", + mgr->name, ofconn_type_to_string(ofservice->type), target); } static void -ofservice_destroy(struct connmgr *mgr, struct ofservice *ofservice) +ofservice_close_all(struct ofservice *ofservice) + OVS_REQUIRES(ofproto_mutex) { - hmap_remove(&mgr->services, &ofservice->node); - pvconn_close(ofservice->pvconn); + struct ofconn *ofconn, *next; + LIST_FOR_EACH_SAFE (ofconn, next, ofservice_node, &ofservice->conns) { + ofconn_destroy(ofconn); + } +} + +static void +ofservice_destroy(struct ofservice *ofservice) + OVS_REQUIRES(ofproto_mutex) +{ + if (!ofservice) { + return; + } + + ofservice_close_all(ofservice); + + hmap_remove(&ofservice->connmgr->services, &ofservice->hmap_node); + free(ofservice->target); + if (ofservice->pvconn) { + pvconn_close(ofservice->pvconn); + } + if (ofservice->rconn) { + rconn_destroy(ofservice->rconn); + } free(ofservice); } +static void +ofservice_run(struct ofservice *ofservice) +{ + if (ofservice->pvconn) { + struct vconn *vconn; + int retval = pvconn_accept(ofservice->pvconn, &vconn); + if (!retval) { + /* Passing default value for creation of the rconn */ + struct rconn *rconn = rconn_create( + ofservice->s.probe_interval, ofservice->s.max_backoff, + ofservice->s.dscp, ofservice->s.allowed_versions); + char *name = ofconn_make_name(ofservice->connmgr, + vconn_get_name(vconn)); + rconn_connect_unreliably(rconn, vconn, name); + free(name); + + ofconn_create(ofservice, rconn, OFCONN_SERVICE, &ofservice->s); + } else if (retval != EAGAIN) { + VLOG_WARN_RL(&rl, "accept failed (%s)", ovs_strerror(retval)); + } + } else { + rconn_run(ofservice->rconn); + + bool connected = rconn_is_connected(ofservice->rconn); + bool has_ofconn = !ovs_list_is_empty(&ofservice->conns); + if (connected && !has_ofconn) { + ofconn_create(ofservice, ofservice->rconn, OFCONN_PRIMARY, + &ofservice->s); + } + } +} + +static void +ofservice_wait(struct ofservice *ofservice) +{ + if (ofservice->pvconn) { + pvconn_wait(ofservice->pvconn); + } +} + static void ofservice_reconfigure(struct ofservice *ofservice, - const struct ofproto_controller *c) + const struct ofproto_controller *settings) + OVS_REQUIRES(ofproto_mutex) { - ofservice->probe_interval = c->probe_interval; - ofservice->rate_limit = c->rate_limit; - ofservice->burst_limit = c->burst_limit; - ofservice->enable_async_msgs = c->enable_async_msgs; - ofservice->dscp = c->dscp; + /* If the allowed OpenFlow versions change, close all of the existing + * connections to allow them to reconnect and possibly negotiate a new + * version. */ + if (ofservice->s.allowed_versions != settings->allowed_versions) { + ofservice_close_all(ofservice); + } + + ofservice->s = *settings; + + struct ofconn *ofconn; + LIST_FOR_EACH (ofconn, ofservice_node, &ofservice->conns) { + ofconn_reconfigure(ofconn, settings); + } } /* Finds and returns the ofservice within 'mgr' that has the given @@ -2016,9 +2013,9 @@ ofservice_lookup(struct connmgr *mgr, const char *target) { struct ofservice *ofservice; - HMAP_FOR_EACH_WITH_HASH (ofservice, node, hash_string(target, 0), + HMAP_FOR_EACH_WITH_HASH (ofservice, hmap_node, hash_string(target, 0), &mgr->services) { - if (!strcmp(pvconn_get_name(ofservice->pvconn), target)) { + if (!strcmp(ofservice->target, target)) { return ofservice; } } @@ -2135,7 +2132,7 @@ ofmonitor_report(struct connmgr *mgr, struct rule *rule, } struct ofconn *ofconn; - LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { + LIST_FOR_EACH (ofconn, connmgr_node, &mgr->conns) { if (ofconn->monitor_paused) { /* Only send NXFME_DELETED notifications for flows that were added * before we paused. */ @@ -2213,7 +2210,7 @@ ofmonitor_flush(struct connmgr *mgr) { struct ofconn *ofconn; - LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { + LIST_FOR_EACH (ofconn, connmgr_node, &mgr->conns) { struct rconn_packet_counter *counter = ofconn->monitor_counter; struct ofpbuf *msg; @@ -2268,7 +2265,7 @@ ofmonitor_run(struct connmgr *mgr) { ovs_mutex_lock(&ofproto_mutex); struct ofconn *ofconn; - LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { + LIST_FOR_EACH (ofconn, connmgr_node, &mgr->conns) { if (ofmonitor_may_resume(ofconn)) { COVERAGE_INC(ofmonitor_resume); ofmonitor_resume(ofconn); @@ -2282,7 +2279,7 @@ ofmonitor_wait(struct connmgr *mgr) { ovs_mutex_lock(&ofproto_mutex); struct ofconn *ofconn; - LIST_FOR_EACH (ofconn, node, &mgr->all_conns) { + LIST_FOR_EACH (ofconn, connmgr_node, &mgr->conns) { if (ofmonitor_may_resume(ofconn)) { poll_immediate_wake(); } @@ -2290,6 +2287,12 @@ ofmonitor_wait(struct connmgr *mgr) ovs_mutex_unlock(&ofproto_mutex); } +const char * +ofconn_type_to_string(enum ofconn_type type) +{ + return type == OFCONN_PRIMARY ? "primary" : "service"; +} + void ofproto_async_msg_free(struct ofproto_async_msg *am) { diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 83708ee51c7a..de5793dd03e4 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -2718,7 +2718,7 @@ ofp12_controller_role_to_str(enum ofp12_controller_role role) return "slave"; case OFPCR12_ROLE_NOCHANGE: default: - return "*** INVALID ROLE ***"; + return NULL; } } From patchwork Mon Oct 29 22:57:50 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 990600 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 42kVRp6xGxz9s5c for ; Tue, 30 Oct 2018 10:00:06 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 35299347E; Mon, 29 Oct 2018 22:58:26 +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 092033451 for ; Mon, 29 Oct 2018 22:58:06 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay12.mail.gandi.net (relay12.mail.gandi.net [217.70.178.232]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 7F15D75B for ; Mon, 29 Oct 2018 22:58:04 +0000 (UTC) Received: from sigabrt.benpfaff.org (unknown [208.91.3.26]) (Authenticated sender: blp@ovn.org) by relay12.mail.gandi.net (Postfix) with ESMTPSA id EB8B7200002; Mon, 29 Oct 2018 22:58:01 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Mon, 29 Oct 2018 15:57:50 -0700 Message-Id: <20181029225751.5936-5-blp@ovn.org> X-Mailer: git-send-email 2.16.1 In-Reply-To: <20181029225751.5936-1-blp@ovn.org> References: <20181029225751.5936-1-blp@ovn.org> 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 5/6] vswitchd: Allow user to configure controllers as "primary" or "service". 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 Normally it makes sense for an active connection to be primary and a passive connection to be a service connection, but I've run into a corner case where it is better for a passive connection to be a primary connection. This specific case is for use with OFtest, which expects to be a primary controller. However, it also wants to reconnect frequently, which is slow for active connections because of the backoff; by configuring a passive, primary controller, OFtest can reconnect as frequently and as quickly as it wants, making the overall test much faster. Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- ofproto/connmgr.c | 29 +++++------- ofproto/connmgr.h | 21 --------- ofproto/ofproto.c | 6 +++ ofproto/ofproto.h | 18 ++++++++ vswitchd/bridge.c | 11 +++++ vswitchd/vswitch.ovsschema | 8 +++- vswitchd/vswitch.xml | 111 ++++++++++++++++++++++----------------------- 7 files changed, 106 insertions(+), 98 deletions(-) diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c index 30d543d220e5..1674db1594a8 100644 --- a/ofproto/connmgr.c +++ b/ofproto/connmgr.c @@ -142,7 +142,7 @@ struct ofconn { static unsigned int bundle_idle_timeout = BUNDLE_IDLE_TIMEOUT_DEFAULT; -static void ofconn_create(struct ofservice *, struct rconn *, enum ofconn_type, +static void ofconn_create(struct ofservice *, struct rconn *, const struct ofproto_controller *settings) OVS_EXCLUDED(ofproto_mutex); static void ofconn_destroy(struct ofconn *) OVS_REQUIRES(ofproto_mutex); @@ -1150,7 +1150,7 @@ bundle_remove_expired(struct ofconn *ofconn, long long int now) static void ofconn_create(struct ofservice *ofservice, struct rconn *rconn, - enum ofconn_type type, const struct ofproto_controller *settings) + const struct ofproto_controller *settings) OVS_EXCLUDED(ofproto_mutex) { ovs_mutex_lock(&ofproto_mutex); @@ -1164,7 +1164,7 @@ ofconn_create(struct ofservice *ofservice, struct rconn *rconn, ovs_list_push_back(&ofservice->conns, &ofconn->ofservice_node); ofconn->rconn = rconn; - ofconn->type = type; + ofconn->type = settings->type; ofconn->band = settings->band; ofconn->role = OFPCR12_ROLE_EQUAL; @@ -1708,8 +1708,9 @@ connmgr_get_max_probe_interval(const struct connmgr *mgr) return max_probe_interval; } -/* Returns the number of seconds for which all of 'mgr's primary controllers - * have been disconnected. Returns 0 if 'mgr' has no primary controllers. */ +/* Returns the number of seconds for which all of 'mgr's active, primary + * controllers have been disconnected. Returns 0 if 'mgr' has no active, + * primary controllers. */ int connmgr_failure_duration(const struct connmgr *mgr) { @@ -1717,7 +1718,7 @@ connmgr_failure_duration(const struct connmgr *mgr) struct ofservice *ofservice; HMAP_FOR_EACH (ofservice, hmap_node, &mgr->services) { - if (ofservice->rconn) { + if (ofservice->s.type == OFCONN_PRIMARY && ofservice->rconn) { int failure_duration = rconn_failure_duration(ofservice->rconn); min_failure_duration = MIN(min_failure_duration, failure_duration); } @@ -1734,7 +1735,8 @@ connmgr_is_any_controller_connected(const struct connmgr *mgr) { struct ofservice *ofservice; HMAP_FOR_EACH (ofservice, hmap_node, &mgr->services) { - if (ofservice->rconn && rconn_is_connected(ofservice->rconn)) { + if (ofservice->s.type == OFCONN_PRIMARY + && !ovs_list_is_empty(&ofservice->conns)) { return true; } } @@ -1905,7 +1907,7 @@ ofservice_create(struct connmgr *mgr, const char *target, ofservice->connmgr = mgr; ofservice->target = xstrdup(target); ovs_list_init(&ofservice->conns); - ofservice->type = rconn ? OFCONN_PRIMARY : OFCONN_SERVICE; + ofservice->type = c->type; ofservice->rconn = rconn; ofservice->pvconn = pvconn; ofservice->s = *c; @@ -1962,7 +1964,7 @@ ofservice_run(struct ofservice *ofservice) rconn_connect_unreliably(rconn, vconn, name); free(name); - ofconn_create(ofservice, rconn, OFCONN_SERVICE, &ofservice->s); + ofconn_create(ofservice, rconn, &ofservice->s); } else if (retval != EAGAIN) { VLOG_WARN_RL(&rl, "accept failed (%s)", ovs_strerror(retval)); } @@ -1972,8 +1974,7 @@ ofservice_run(struct ofservice *ofservice) bool connected = rconn_is_connected(ofservice->rconn); bool has_ofconn = !ovs_list_is_empty(&ofservice->conns); if (connected && !has_ofconn) { - ofconn_create(ofservice, ofservice->rconn, OFCONN_PRIMARY, - &ofservice->s); + ofconn_create(ofservice, ofservice->rconn, &ofservice->s); } } } @@ -2287,12 +2288,6 @@ ofmonitor_wait(struct connmgr *mgr) ovs_mutex_unlock(&ofproto_mutex); } -const char * -ofconn_type_to_string(enum ofconn_type type) -{ - return type == OFCONN_PRIMARY ? "primary" : "service"; -} - void ofproto_async_msg_free(struct ofproto_async_msg *am) { diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h index 11c8f9a85121..46f75b3dcf2f 100644 --- a/ofproto/connmgr.h +++ b/ofproto/connmgr.h @@ -37,27 +37,6 @@ struct rule; struct simap; struct sset; -/* ofproto supports two kinds of OpenFlow connections: - * - * - "Primary" connections to ordinary OpenFlow controllers. ofproto - * maintains persistent connections to these controllers and by default - * sends them asynchronous messages such as packet-ins. - * - * - "Service" connections, e.g. from ovs-ofctl. When these connections - * drop, it is the other side's responsibility to reconnect them if - * necessary. ofproto does not send them asynchronous messages by default. - * - * Currently, active (tcp, ssl, unix) connections are always "primary" - * connections and passive (ptcp, pssl, punix) connections are always "service" - * connections. There is no inherent reason for this, but it reflects the - * common case. - */ -enum ofconn_type { - OFCONN_PRIMARY, /* An ordinary OpenFlow controller. */ - OFCONN_SERVICE /* A service connection, e.g. "ovs-ofctl". */ -}; -const char *ofconn_type_to_string(enum ofconn_type); - /* An asynchronous message that might need to be queued between threads. */ struct ofproto_async_msg { struct ovs_list list_node; /* For queuing. */ diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 222c749940ec..29a40cd539f5 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -1892,6 +1892,12 @@ ofproto_free_ofproto_controller_info(struct shash *info) connmgr_free_controller_info(info); } +const char * +ofconn_type_to_string(enum ofconn_type type) +{ + return type == OFCONN_PRIMARY ? "primary" : "service"; +} + /* Makes a deep copy of 'old' into 'port'. */ void ofproto_port_clone(struct ofproto_port *port, const struct ofproto_port *old) diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h index 595729dd61f1..50208d481862 100644 --- a/ofproto/ofproto.h +++ b/ofproto/ofproto.h @@ -207,7 +207,25 @@ enum ofproto_band { OFPROTO_OUT_OF_BAND /* Out-of-band connection to controller. */ }; +/* ofproto supports two kinds of OpenFlow connections: + * + * - "Primary" connections to ordinary OpenFlow controllers. ofproto + * maintains persistent connections to these controllers and by default + * sends them asynchronous messages such as packet-ins. + * + * - "Service" connections, e.g. from ovs-ofctl. When these connections + * drop, it is the other side's responsibility to reconnect them if + * necessary. ofproto does not send them asynchronous messages by default. + */ +enum ofconn_type { + OFCONN_PRIMARY, /* An ordinary OpenFlow controller. */ + OFCONN_SERVICE /* A service connection, e.g. "ovs-ofctl". */ +}; +const char *ofconn_type_to_string(enum ofconn_type); + +/* Configuration for an OpenFlow controller. */ struct ofproto_controller { + enum ofconn_type type; /* Primary or service controller. */ int max_backoff; /* Maximum reconnection backoff, in seconds. */ int probe_interval; /* Max idle time before probing, in seconds. */ enum ofproto_band band; /* In-band or out-of-band? */ diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index de5793dd03e4..a427b0122b49 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -46,6 +46,7 @@ #include "openvswitch/meta-flow.h" #include "openvswitch/ofp-print.h" #include "openvswitch/ofpbuf.h" +#include "openvswitch/vconn.h" #include "openvswitch/vlog.h" #include "ovs-lldp.h" #include "ovs-numa.h" @@ -3536,6 +3537,14 @@ equal_pathnames(const char *a, const char *b, size_t b_stoplen) } } +static enum ofconn_type +get_controller_ofconn_type(const char *target, const char *type) +{ + return (type + ? (!strcmp(type, "primary") ? OFCONN_PRIMARY : OFCONN_SERVICE) + : (!vconn_verify_name(target) ? OFCONN_PRIMARY : OFCONN_SERVICE)); +} + static void bridge_configure_remotes(struct bridge *br, const struct sockaddr_in *managers, size_t n_managers) @@ -3571,6 +3580,7 @@ bridge_configure_remotes(struct bridge *br, /* Add managment controller. */ struct ofproto_controller *oc = xmalloc(sizeof *oc); *oc = (struct ofproto_controller) { + .type = OFCONN_SERVICE, .probe_interval = 60, .band = OFPROTO_OUT_OF_BAND, .enable_async_msgs = true, @@ -3639,6 +3649,7 @@ bridge_configure_remotes(struct bridge *br, oc = xmalloc(sizeof *oc); *oc = (struct ofproto_controller) { + .type = get_controller_ofconn_type(c->target, c->type), .max_backoff = c->max_backoff ? *c->max_backoff / 1000 : 8, .probe_interval = (c->inactivity_probe ? *c->inactivity_probe / 1000 : 5), diff --git a/vswitchd/vswitch.ovsschema b/vswitchd/vswitch.ovsschema index 1e4e342dcb96..9be4ca54ecd3 100644 --- a/vswitchd/vswitch.ovsschema +++ b/vswitchd/vswitch.ovsschema @@ -1,6 +1,6 @@ {"name": "Open_vSwitch", - "version": "7.16.1", - "cksum": "1452282319 23860", + "version": "7.17.0", + "cksum": "2073324140 24021", "tables": { "Open_vSwitch": { "columns": { @@ -548,6 +548,10 @@ "indexes": [["id", "bridge"]]}, "Controller": { "columns": { + "type": { + "type": {"key": {"type": "string", + "enum": ["set", ["primary", "service"]]}, + "min": 0, "max": 1}}, "target": { "type": "string"}, "max_backoff": { diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml index 6d1fc1c1c7b0..08bec290ae18 100644 --- a/vswitchd/vswitch.xml +++ b/vswitchd/vswitch.xml @@ -967,9 +967,12 @@ avoid loops on such a bridge, configure secure mode or enable STP (see ).

-

When more than one controller is configured, - is considered only when none of the - configured controllers can be contacted.

+

+ The setting applies only to primary + controllers. When more than one primary controller is configured, + is considered only when none of the + configured controllers can be contacted. +

Changing when no primary controllers are configured clears the OpenFlow flow tables, group table, and meter @@ -4453,71 +4456,64 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \

An OpenFlow controller.

-

- Open vSwitch supports two kinds of OpenFlow controllers: -

- -
-
Primary controllers
-
+ +

- This is the kind of controller envisioned by the OpenFlow 1.0 - specification. Usually, a primary controller implements a network - policy by taking charge of the switch's flow table. + Open vSwitch supports two kinds of OpenFlow controllers. A bridge + may have any number of each kind:

-

- Open vSwitch initiates and maintains persistent connections to - primary controllers, retrying the connection each time it fails or - drops. The column in the - table applies to primary controllers. -

+
+
Primary controllers
+
+

+ This is the kind of controller envisioned by the OpenFlow + specifications. Usually, a primary controller implements a + network policy by taking charge of the switch's flow table. +

-

- Open vSwitch permits a bridge to have any number of primary - controllers. When multiple controllers are configured, Open - vSwitch connects to all of them simultaneously. Because - OpenFlow 1.0 does not specify how multiple controllers - coordinate in interacting with a single switch, more than - one primary controller should be specified only if the - controllers are themselves designed to coordinate with each - other. (The Nicira-defined NXT_ROLE OpenFlow - vendor extension may be useful for this.) -

-
-
Service controllers
-
-

- These kinds of OpenFlow controller connections are intended for - occasional support and maintenance use, e.g. with - ovs-ofctl. Usually a service controller connects only - briefly to inspect or modify some of a switch's state. -

+

+ The column in the table applies to primary controllers. +

-

- Open vSwitch listens for incoming connections from service - controllers. The service controllers initiate and, if necessary, - maintain the connections from their end. The column in the table does - not apply to service controllers. -

+

+ When multiple primary controllers are configured, Open vSwitch + connects to all of them simultaneously. OpenFlow provides few + facilities to allow multiple controllers to coordinate in + interacting with a single switch, so more than one primary + controller should be specified only if the controllers are + themselves designed to coordinate with each other. +

+
+
Service controllers
+
+

+ These kinds of OpenFlow controller connections are intended for + occasional support and maintenance use, e.g. with + ovs-ofctl. Usually a service controller connects + only briefly to inspect or modify some of a switch's state. +

+ +

+ The column in the table does not apply to service controllers. +

+
+

- Open vSwitch supports configuring any number of service controllers. + By default, Open vSwitch treats controllers with active connection + methods as primary controllers and those with passive connection + methods as service controllers. Set this column to the desired type + to override this default.

-
-
- -

- The determines the type of controller. -

+ -

Connection method for controller.

- The following connection methods are currently supported for primary - controllers: + The following active connection methods are currently supported:

ssl:host[:port]
@@ -4546,8 +4542,7 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \

- The following connection methods are currently supported for service - controllers: + The following passive connection methods are currently supported:

pssl:[port][:host]
From patchwork Mon Oct 29 22:57:51 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 990601 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 42kVSG57pyz9s7W for ; Tue, 30 Oct 2018 10:00:30 +1100 (AEDT) Received: from mail.linux-foundation.org (localhost [127.0.0.1]) by mail.linuxfoundation.org (Postfix) with ESMTP id 0A429348B; Mon, 29 Oct 2018 22:58:27 +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 9744C3451 for ; Mon, 29 Oct 2018 22:58:06 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay12.mail.gandi.net (relay12.mail.gandi.net [217.70.178.232]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 0EAC6735 for ; Mon, 29 Oct 2018 22:58:05 +0000 (UTC) Received: from sigabrt.benpfaff.org (unknown [208.91.3.26]) (Authenticated sender: blp@ovn.org) by relay12.mail.gandi.net (Postfix) with ESMTPSA id 90AFC200004; Mon, 29 Oct 2018 22:58:03 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Mon, 29 Oct 2018 15:57:51 -0700 Message-Id: <20181029225751.5936-6-blp@ovn.org> X-Mailer: git-send-email 2.16.1 In-Reply-To: <20181029225751.5936-1-blp@ovn.org> References: <20181029225751.5936-1-blp@ovn.org> 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 6/6] ofproto: Don't always treat passive controllers as "equal". 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 If a passive controller chooses to configure itself as a slave controller, I don't know a reason why it should be considered "equal" for some purposes. Signed-off-by: Ben Pfaff Acked-by: Justin Pettit --- ofproto/ofproto.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 29a40cd539f5..af73d660afe7 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -3411,8 +3411,7 @@ handle_set_config(struct ofconn *ofconn, const struct ofp_header *oh) static enum ofperr reject_slave_controller(struct ofconn *ofconn) { - if (ofconn_get_type(ofconn) == OFCONN_PRIMARY - && ofconn_get_role(ofconn) == OFPCR12_ROLE_SLAVE) { + if (ofconn_get_role(ofconn) == OFPCR12_ROLE_SLAVE) { return OFPERR_OFPBRC_IS_SLAVE; } else { return 0;