From patchwork Thu Jan 9 20:49:43 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 1220646 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.133; helo=hemlock.osuosl.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 hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 47tyt30cJ5z9sPn for ; Fri, 10 Jan 2020 07:50:03 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 6986A8816F; Thu, 9 Jan 2020 20:50:01 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 16SgoKnVfAOt; Thu, 9 Jan 2020 20:49:58 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id 95FBA883DC; Thu, 9 Jan 2020 20:49:58 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id 74C3CC18DC; Thu, 9 Jan 2020 20:49:58 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) by lists.linuxfoundation.org (Postfix) with ESMTP id ECE57C18DC for ; Thu, 9 Jan 2020 20:49:56 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id D9B70865AB for ; Thu, 9 Jan 2020 20:49:56 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id D2UyEBMvVnIy for ; Thu, 9 Jan 2020 20:49:54 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net [217.70.183.197]) by fraxinus.osuosl.org (Postfix) with ESMTPS id A180B863C5 for ; Thu, 9 Jan 2020 20:49:53 +0000 (UTC) X-Originating-IP: 66.170.99.95 Received: from localhost.localdomain (unknown [66.170.99.95]) (Authenticated sender: blp@ovn.org) by relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 11A4D1C0005; Thu, 9 Jan 2020 20:49:49 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Thu, 9 Jan 2020 12:49:43 -0800 Message-Id: <20200109204944.4092346-1-blp@ovn.org> X-Mailer: git-send-email 2.23.0 MIME-Version: 1.0 Cc: Ben Pfaff Subject: [ovs-dev] [PATCH repost 1/2] ofproto-dpif-upcall: Get rid of udpif_synchronize(). X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" RCU provides the semantics we want from udpif_synchronize() and it should be much more lightweight than killing and restarting all the upcall threads. It looks like udpif_synchronize() was written before the OVS tree had RCU support, which is probably why we didn't use it here from the beginning. So we can just change udpif_synchronize() to a single ovsrcu_synchronize() call. However, udpif_synchronize() only has a single caller, which calls ovsrcu_synchronize() anyway just beforehand, via xlate_txn_commit(). So we can get rid of udpif_synchronize() entirely, which this patch does. As a side effect, this eliminates one reason why terminating OVS cleanly clears the datapath flow table. An upcoming patch will eliminate other reasons. Signed-off-by: Ben Pfaff Acked-by: Numan Siddique --- ofproto/ofproto-dpif-upcall.c | 17 ----------------- ofproto/ofproto-dpif-upcall.h | 1 - ofproto/ofproto-dpif-xlate.c | 14 +++++++++----- ofproto/ofproto-dpif.c | 4 ---- 4 files changed, 9 insertions(+), 27 deletions(-) diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 409286ab1587..03cb8a1dd486 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -644,23 +644,6 @@ udpif_set_threads(struct udpif *udpif, size_t n_handlers_, } } -/* Waits for all ongoing upcall translations to complete. This ensures that - * there are no transient references to any removed ofprotos (or other - * objects). In particular, this should be called after an ofproto is removed - * (e.g. via xlate_remove_ofproto()) but before it is destroyed. */ -void -udpif_synchronize(struct udpif *udpif) -{ - /* This is stronger than necessary. It would be sufficient to ensure - * (somehow) that each handler and revalidator thread had passed through - * its main loop once. */ - size_t n_handlers_ = udpif->n_handlers; - size_t n_revalidators_ = udpif->n_revalidators; - - udpif_stop_threads(udpif); - udpif_start_threads(udpif, n_handlers_, n_revalidators_); -} - /* Notifies 'udpif' that something changed which may render previous * xlate_actions() results invalid. */ void diff --git a/ofproto/ofproto-dpif-upcall.h b/ofproto/ofproto-dpif-upcall.h index cef1d34198d6..693107ae56c1 100644 --- a/ofproto/ofproto-dpif-upcall.h +++ b/ofproto/ofproto-dpif-upcall.h @@ -33,7 +33,6 @@ struct udpif *udpif_create(struct dpif_backer *, struct dpif *); void udpif_run(struct udpif *udpif); void udpif_set_threads(struct udpif *, size_t n_handlers, size_t n_revalidators); -void udpif_synchronize(struct udpif *); void udpif_destroy(struct udpif *); void udpif_revalidate(struct udpif *); void udpif_get_memory_usage(struct udpif *, struct simap *usage); diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 4407f9c97a9e..0b45ecf3d4da 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -1171,11 +1171,15 @@ xlate_xport_copy(struct xbridge *xbridge, struct xbundle *xbundle, * * A sample workflow: * - * xlate_txn_start(); - * ... - * edit_xlate_configuration(); - * ... - * xlate_txn_commit(); */ + * xlate_txn_start(); + * ... + * edit_xlate_configuration(); + * ... + * xlate_txn_commit(); + * + * The ovsrcu_synchronize() call here also ensures that the upcall threads + * retain no references to anything in the previous configuration. + */ void xlate_txn_commit(void) { diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index d3cb392077df..0222ec82f00a 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -1751,10 +1751,6 @@ destruct(struct ofproto *ofproto_, bool del) xlate_remove_ofproto(ofproto); xlate_txn_commit(); - /* Ensure that the upcall processing threads have no remaining references - * to the ofproto or anything in it. */ - udpif_synchronize(ofproto->backer->udpif); - hmap_remove(&all_ofproto_dpifs_by_name, &ofproto->all_ofproto_dpifs_by_name_node); hmap_remove(&all_ofproto_dpifs_by_uuid, From patchwork Thu Jan 9 20:49:44 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Pfaff X-Patchwork-Id: 1220645 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=140.211.166.133; helo=hemlock.osuosl.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 hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 47tyt01h01z9sPn for ; Fri, 10 Jan 2020 07:50:00 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 83CDF882A7; Thu, 9 Jan 2020 20:49:58 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id nUv+I0ljp5FL; Thu, 9 Jan 2020 20:49:56 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id CC41187FB2; Thu, 9 Jan 2020 20:49:56 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id C2A76C18DC; Thu, 9 Jan 2020 20:49:56 +0000 (UTC) X-Original-To: dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by lists.linuxfoundation.org (Postfix) with ESMTP id 57D73C0881 for ; Thu, 9 Jan 2020 20:49:55 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id 5318A867AC for ; Thu, 9 Jan 2020 20:49:55 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Ott6BADyQ4HD for ; Thu, 9 Jan 2020 20:49:53 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net [217.70.183.197]) by whitealder.osuosl.org (Postfix) with ESMTPS id 7E17586CC7 for ; Thu, 9 Jan 2020 20:49:53 +0000 (UTC) X-Originating-IP: 66.170.99.95 Received: from localhost.localdomain (unknown [66.170.99.95]) (Authenticated sender: blp@ovn.org) by relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 57CA01C0006; Thu, 9 Jan 2020 20:49:51 +0000 (UTC) From: Ben Pfaff To: dev@openvswitch.org Date: Thu, 9 Jan 2020 12:49:44 -0800 Message-Id: <20200109204944.4092346-2-blp@ovn.org> X-Mailer: git-send-email 2.23.0 In-Reply-To: <20200109204944.4092346-1-blp@ovn.org> References: <20200109204944.4092346-1-blp@ovn.org> MIME-Version: 1.0 Cc: Ben Pfaff Subject: [ovs-dev] [PATCH repost 2/2] ofproto: Do not delete datapath flows on exit by default. X-BeenThere: ovs-dev@openvswitch.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: ovs-dev-bounces@openvswitch.org Sender: "dev" Commit e96a5c24e853 ("upcall: Remove datapath flows when setting n-threads.") caused OVS to delete datapath flows when it exits through any graceful means. This is not necessarily desirable, especially when OVS is being stopped as part of an upgrade. This commit changes OVS so that it only removes datapath flows when requested, via "ovs-appctl exit --cleanup". I've only tested this in the OVS sandbox. Signed-off-by: Ben Pfaff Acked-by: Numan Siddique --- ofproto/ofproto-dpif-upcall.c | 26 ++++++++++++++++---------- ofproto/ofproto.c | 8 ++++---- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 03cb8a1dd486..8dfa05b71df4 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -332,7 +332,7 @@ static size_t recv_upcalls(struct handler *); static int process_upcall(struct udpif *, struct upcall *, struct ofpbuf *odp_actions, struct flow_wildcards *); static void handle_upcalls(struct udpif *, struct upcall *, size_t n_upcalls); -static void udpif_stop_threads(struct udpif *); +static void udpif_stop_threads(struct udpif *, bool delete_flows); static void udpif_start_threads(struct udpif *, size_t n_handlers, size_t n_revalidators); static void udpif_pause_revalidators(struct udpif *); @@ -483,7 +483,7 @@ udpif_run(struct udpif *udpif) void udpif_destroy(struct udpif *udpif) { - udpif_stop_threads(udpif); + udpif_stop_threads(udpif, false); dpif_register_dp_purge_cb(udpif->dpif, NULL, udpif); dpif_register_upcall_cb(udpif->dpif, NULL, udpif); @@ -504,9 +504,15 @@ udpif_destroy(struct udpif *udpif) free(udpif); } -/* Stops the handler and revalidator threads. */ +/* Stops the handler and revalidator threads. + * + * If 'delete_flows' is true, we delete ukeys and delete all flows from the + * datapath. Otherwise, we end up double-counting stats for flows that remain + * in the datapath. If 'delete_flows' is false, we skip this step. This is + * appropriate if OVS is about to exit anyway and it is desirable to let + * existing network connections continue being forwarded afterward. */ static void -udpif_stop_threads(struct udpif *udpif) +udpif_stop_threads(struct udpif *udpif, bool delete_flows) { if (udpif && (udpif->n_handlers != 0 || udpif->n_revalidators != 0)) { size_t i; @@ -526,10 +532,10 @@ udpif_stop_threads(struct udpif *udpif) dpif_disable_upcall(udpif->dpif); ovsrcu_quiesce_end(); - /* Delete ukeys, and delete all flows from the datapath to prevent - * double-counting stats. */ - for (i = 0; i < udpif->n_revalidators; i++) { - revalidator_purge(&udpif->revalidators[i]); + if (delete_flows) { + for (i = 0; i < udpif->n_revalidators; i++) { + revalidator_purge(&udpif->revalidators[i]); + } } latch_poll(&udpif->exit_latch); @@ -627,7 +633,7 @@ udpif_set_threads(struct udpif *udpif, size_t n_handlers_, if (udpif->n_handlers != n_handlers_ || udpif->n_revalidators != n_revalidators_) { - udpif_stop_threads(udpif); + udpif_stop_threads(udpif, true); } if (!udpif->handlers && !udpif->revalidators) { @@ -681,7 +687,7 @@ udpif_flush(struct udpif *udpif) size_t n_handlers_ = udpif->n_handlers; size_t n_revalidators_ = udpif->n_revalidators; - udpif_stop_threads(udpif); + udpif_stop_threads(udpif, true); dpif_flow_flush(udpif->dpif); udpif_start_threads(udpif, n_handlers_, n_revalidators_); } diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 08830d837164..5d69a4332f9f 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -1601,13 +1601,13 @@ ofproto_rule_delete(struct ofproto *ofproto, struct rule *rule) } static void -ofproto_flush__(struct ofproto *ofproto) +ofproto_flush__(struct ofproto *ofproto, bool del) OVS_EXCLUDED(ofproto_mutex) { struct oftable *table; /* This will flush all datapath flows. */ - if (ofproto->ofproto_class->flush) { + if (del && ofproto->ofproto_class->flush) { ofproto->ofproto_class->flush(ofproto); } @@ -1710,7 +1710,7 @@ ofproto_destroy(struct ofproto *p, bool del) return; } - ofproto_flush__(p); + ofproto_flush__(p, del); HMAP_FOR_EACH_SAFE (ofport, next_ofport, hmap_node, &p->ports) { ofport_destroy(ofport, del); } @@ -2288,7 +2288,7 @@ void ofproto_flush_flows(struct ofproto *ofproto) { COVERAGE_INC(ofproto_flush); - ofproto_flush__(ofproto); + ofproto_flush__(ofproto, false); connmgr_flushed(ofproto->connmgr); }