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,