From patchwork Thu Feb 2 12:35:30 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Maximets X-Patchwork-Id: 1736288 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org (client-ip=2605:bc80:3010::138; helo=smtp1.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp1.osuosl.org (smtp1.osuosl.org [IPv6:2605:bc80:3010::138]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4P6ytp75Fsz23hh for ; Thu, 2 Feb 2023 23:35:46 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp1.osuosl.org (Postfix) with ESMTP id DE15381F99; Thu, 2 Feb 2023 12:35:44 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org DE15381F99 X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp1.osuosl.org ([127.0.0.1]) by localhost (smtp1.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id epMvFuILDlkN; Thu, 2 Feb 2023 12:35:43 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp1.osuosl.org (Postfix) with ESMTPS id 3659E81FB2; Thu, 2 Feb 2023 12:35:42 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp1.osuosl.org 3659E81FB2 Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id DEDC6C0077; Thu, 2 Feb 2023 12:35:41 +0000 (UTC) X-Original-To: ovs-dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp2.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by lists.linuxfoundation.org (Postfix) with ESMTP id E6D2BC0032 for ; Thu, 2 Feb 2023 12:35:38 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp2.osuosl.org (Postfix) with ESMTP id B1A8A403F8 for ; Thu, 2 Feb 2023 12:35:38 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org B1A8A403F8 X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp2.osuosl.org ([127.0.0.1]) by localhost (smtp2.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id PJK1j4pkduQI for ; Thu, 2 Feb 2023 12:35:37 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp2.osuosl.org 7568E40C5E Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [IPv6:2001:4b98:dc4:8::222]) by smtp2.osuosl.org (Postfix) with ESMTPS id 7568E40C5E for ; Thu, 2 Feb 2023 12:35:37 +0000 (UTC) Received: (Authenticated sender: i.maximets@ovn.org) by mail.gandi.net (Postfix) with ESMTPSA id 2A5A140010; Thu, 2 Feb 2023 12:35:35 +0000 (UTC) From: Ilya Maximets To: ovs-dev@openvswitch.org Date: Thu, 2 Feb 2023 13:35:30 +0100 Message-Id: <20230202123530.945140-3-i.maximets@ovn.org> X-Mailer: git-send-email 2.39.1 In-Reply-To: <20230202123530.945140-1-i.maximets@ovn.org> References: <20230202123530.945140-1-i.maximets@ovn.org> MIME-Version: 1.0 Cc: Dumitru Ceara , Ilya Maximets Subject: [ovs-dev] [PATCH ovn 2/2] northd: Add thread safety analysis to lflow hash locks. 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" Even though it's not possible to enable a full thread safety analysis due to conditional mutex taking and unpredictable order [1], we can add most of the functionality with a fake mutex. It can detect nesting of hash locks, incorrect locking sequences and some other issues. More details are given in a form of a comment in the code to give this fake mutex more visibility. The fake mutex is declared as extern to not actually use any memory or trigger 'unused' warnings. No variables or structure fields are marked as GUARDED, because we still need a lockless access once the parallel part is over. [1] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-conditionally-held-locks Signed-off-by: Ilya Maximets --- northd/northd.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/northd/northd.c b/northd/northd.c index 3fdb8730d..6e4fcd08b 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -5103,8 +5103,28 @@ lflow_hash_lock_destroy(void) lflow_hash_lock_initialized = false; } +/* Full thread safety analysis is not possible with hash locks, because + * they are taken conditionally based on the 'parallelization_state' and + * a flow hash. Also, the order in which two hash locks are taken is not + * predictable during the static analysis. + * + * Since the order of taking two locks depends on a random hash, to avoid + * ABBA deadlocks, no two hash locks can be nested. In that sense an array + * of hash locks is similar to a single mutex. + * + * Using a fake mutex to partially simulate thread safety restrictions, as + * if it were actually a single mutex. + * + * OVS_NO_THREAD_SAFETY_ANALYSIS below allows us to ignore conditional + * nature of the lock. Unlike other attributes, it applies to the + * implementation and not to the interface. So, we can define a function + * that acquires the lock without analysing the way it does that. + */ +extern struct ovs_mutex fake_hash_mutex; + static struct ovs_mutex * lflow_hash_lock(const struct hmap *lflow_map, uint32_t hash) + OVS_ACQUIRES(fake_hash_mutex) OVS_NO_THREAD_SAFETY_ANALYSIS { struct ovs_mutex *hash_lock = NULL; @@ -5119,6 +5139,7 @@ lflow_hash_lock(const struct hmap *lflow_map, uint32_t hash) static void lflow_hash_unlock(struct ovs_mutex *hash_lock) + OVS_RELEASES(fake_hash_mutex) OVS_NO_THREAD_SAFETY_ANALYSIS { if (hash_lock) { @@ -5145,7 +5166,7 @@ static thread_local size_t thread_lflow_counter = 0; static bool ovn_dp_group_add_with_reference(struct ovn_lflow *lflow_ref, struct ovn_datapath *od) - OVS_NO_THREAD_SAFETY_ANALYSIS + OVS_REQUIRES(fake_hash_mutex) { if (!lflow_ref) { return false; @@ -5164,6 +5185,7 @@ do_ovn_lflow_add(struct hmap *lflow_map, struct ovn_datapath *od, const char *match, const char *actions, const char *io_port, const struct ovsdb_idl_row *stage_hint, const char *where, const char *ctrl_meter) + OVS_REQUIRES(fake_hash_mutex) { struct ovn_lflow *old_lflow; @@ -5205,6 +5227,7 @@ ovn_lflow_add_at_with_hash(struct hmap *lflow_map, struct ovn_datapath *od, const char *io_port, const char *ctrl_meter, const struct ovsdb_idl_row *stage_hint, const char *where, uint32_t hash) + OVS_REQUIRES(fake_hash_mutex) { struct ovn_lflow *lflow; @@ -5222,6 +5245,7 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od, const char *match, const char *actions, const char *io_port, const char *ctrl_meter, const struct ovsdb_idl_row *stage_hint, const char *where) + OVS_EXCLUDED(fake_hash_mutex) { struct ovs_mutex *hash_lock; uint32_t hash;