From patchwork Thu Feb 2 12:35:29 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Maximets X-Patchwork-Id: 1736287 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::136; helo=smtp3.osuosl.org; envelope-from=ovs-dev-bounces@openvswitch.org; receiver=) Received: from smtp3.osuosl.org (smtp3.osuosl.org [IPv6:2605:bc80:3010::136]) (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 4P6ytn1mzJz23hh for ; Thu, 2 Feb 2023 23:35:45 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by smtp3.osuosl.org (Postfix) with ESMTP id 66A1861181; Thu, 2 Feb 2023 12:35:43 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 66A1861181 X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp3.osuosl.org ([127.0.0.1]) by localhost (smtp3.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 4Zco5fynBFPo; Thu, 2 Feb 2023 12:35:41 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by smtp3.osuosl.org (Postfix) with ESMTPS id 262FE61122; Thu, 2 Feb 2023 12:35:40 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp3.osuosl.org 262FE61122 Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id F1F58C0077; Thu, 2 Feb 2023 12:35:39 +0000 (UTC) X-Original-To: ovs-dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from smtp4.osuosl.org (smtp4.osuosl.org [IPv6:2605:bc80:3010::137]) by lists.linuxfoundation.org (Postfix) with ESMTP id 945E5C0032 for ; Thu, 2 Feb 2023 12:35:37 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by smtp4.osuosl.org (Postfix) with ESMTP id 61A5D41C4F for ; Thu, 2 Feb 2023 12:35:37 +0000 (UTC) DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 61A5D41C4F X-Virus-Scanned: amavisd-new at osuosl.org Received: from smtp4.osuosl.org ([127.0.0.1]) by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id UtmZwB6qp8Z5 for ; Thu, 2 Feb 2023 12:35:35 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.8.0 DKIM-Filter: OpenDKIM Filter v2.11.0 smtp4.osuosl.org 2F39441C3C Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [IPv6:2001:4b98:dc4:8::222]) by smtp4.osuosl.org (Postfix) with ESMTPS id 2F39441C3C for ; Thu, 2 Feb 2023 12:35:34 +0000 (UTC) Received: (Authenticated sender: i.maximets@ovn.org) by mail.gandi.net (Postfix) with ESMTPSA id 912E040004; Thu, 2 Feb 2023 12:35:32 +0000 (UTC) From: Ilya Maximets To: ovs-dev@openvswitch.org Date: Thu, 2 Feb 2023 13:35:29 +0100 Message-Id: <20230202123530.945140-2-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 1/2] northd: Use larger hash bucket locks instead of dp group ones. 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" Datapath group locks are taken very frequently while generating logical flows. Every time one new datapath is added to the group, northd takes a mutex, updates a bitmap and unlocks the mutex. The chances for contention on a datapath group lock are very low, but even without any contention these constant lock/unlock operations may take more than 20% of CPU cycles. We have another type of locks in northd - hash bucket locks which protect portions of the lflows hash map. The same locks can be used to protect datapath groups of logical flows in that hash map. As long as we're holding the lock for a portion of a hash map where this lflow is located, it's safe to modify the logical flow itself. Splitting out a pair of lflow_hash_lock/unlock() functions that can be used to lock a particular lflow hash bucket. Once the hash is calculated, we can now lock it, then preform an initial lflow creation and all the datapath group updates, and then unlock. This eliminates most of the lock/unlock operations while still maintaining a very low chance of contention. Testing with ovn-heater and 4 threads shows 10-20% performance improvement depending on a weight of lflow node processing in a particular test scenario. Hash bucket locks are conditional, so the thread safety analysis is disabled to avoid spurious warnings [1]. Also, since the order of taking two locks depends on a "random" hash value, no two locks can be nested, otherwise we'll risk facing ABBA deadlocks. For that reason, some of the loops are split in two. CoPP meters do not affect the hash value, so they do not affect the locking scheme. [1] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-conditionally-held-locks Signed-off-by: Ilya Maximets --- northd/northd.c | 174 ++++++++++++++++++++++++++++-------------------- 1 file changed, 101 insertions(+), 73 deletions(-) diff --git a/northd/northd.c b/northd/northd.c index 0944a7b56..3fdb8730d 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -4997,7 +4997,6 @@ struct ovn_lflow { struct hmap_node hmap_node; struct ovn_datapath *od; /* 'logical_datapath' in SB schema. */ - struct ovs_mutex dpg_lock; /* Lock guarding access to 'dpg_bitmap' */ unsigned long *dpg_bitmap; /* Bitmap of all datapaths by their 'index'.*/ enum ovn_stage stage; uint16_t priority; @@ -5064,29 +5063,6 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od, lflow->ctrl_meter = ctrl_meter; lflow->dpg = NULL; lflow->where = where; - if (parallelization_state != STATE_NULL) { - ovs_mutex_init(&lflow->dpg_lock); - } -} - -static bool -ovn_dp_group_add_with_reference(struct ovn_lflow *lflow_ref, - struct ovn_datapath *od) - OVS_NO_THREAD_SAFETY_ANALYSIS -{ - if (!lflow_ref) { - return false; - } - - if (parallelization_state == STATE_USE_PARALLELIZATION) { - ovs_mutex_lock(&lflow_ref->dpg_lock); - bitmap_set1(lflow_ref->dpg_bitmap, od->index); - ovs_mutex_unlock(&lflow_ref->dpg_lock); - } else { - bitmap_set1(lflow_ref->dpg_bitmap, od->index); - } - - return true; } /* The lflow_hash_lock is a mutex array that protects updates to the shared @@ -5127,6 +5103,30 @@ lflow_hash_lock_destroy(void) lflow_hash_lock_initialized = false; } +static struct ovs_mutex * +lflow_hash_lock(const struct hmap *lflow_map, uint32_t hash) + OVS_NO_THREAD_SAFETY_ANALYSIS +{ + struct ovs_mutex *hash_lock = NULL; + + if (parallelization_state == STATE_USE_PARALLELIZATION) { + hash_lock = + &lflow_hash_locks[hash & lflow_map->mask & LFLOW_HASH_LOCK_MASK]; + ovs_mutex_lock(hash_lock); + } + return hash_lock; +} + +static void +lflow_hash_unlock(struct ovs_mutex *hash_lock) + OVS_NO_THREAD_SAFETY_ANALYSIS +{ + if (hash_lock) { + ovs_mutex_unlock(hash_lock); + } +} + + /* This thread-local var is used for parallel lflow building when dp-groups is * enabled. It maintains the number of lflows inserted by the current thread to * the shared lflow hmap in the current iteration. It is needed because the @@ -5139,6 +5139,22 @@ lflow_hash_lock_destroy(void) * */ static thread_local size_t thread_lflow_counter = 0; +/* Adds an OVN datapath to a datapath group of existing logical flow. + * Version to use when hash bucket locking is NOT required or the corresponding + * hash lock is already taken. */ +static bool +ovn_dp_group_add_with_reference(struct ovn_lflow *lflow_ref, + struct ovn_datapath *od) + OVS_NO_THREAD_SAFETY_ANALYSIS +{ + if (!lflow_ref) { + return false; + } + + bitmap_set1(lflow_ref->dpg_bitmap, od->index); + return true; +} + /* Adds a row with the specified contents to the Logical_Flow table. * Version to use when hash bucket locking is NOT required. */ @@ -5180,28 +5196,8 @@ do_ovn_lflow_add(struct hmap *lflow_map, struct ovn_datapath *od, } /* Adds a row with the specified contents to the Logical_Flow table. - * Version to use when hash bucket locking IS required. - */ -static struct ovn_lflow * -do_ovn_lflow_add_pd(struct hmap *lflow_map, struct ovn_datapath *od, - uint32_t hash, enum ovn_stage stage, uint16_t priority, - const char *match, const char *actions, - const char *io_port, - const struct ovsdb_idl_row *stage_hint, - const char *where, const char *ctrl_meter) -{ - - struct ovn_lflow *lflow; - struct ovs_mutex *hash_lock = - &lflow_hash_locks[hash & lflow_map->mask & LFLOW_HASH_LOCK_MASK]; - - ovs_mutex_lock(hash_lock); - lflow = do_ovn_lflow_add(lflow_map, od, hash, stage, priority, match, - actions, io_port, stage_hint, where, ctrl_meter); - ovs_mutex_unlock(hash_lock); - return lflow; -} - + * Version to use when hash is pre-computed and a hash bucket is already + * locked if necessary. */ static struct ovn_lflow * ovn_lflow_add_at_with_hash(struct hmap *lflow_map, struct ovn_datapath *od, enum ovn_stage stage, uint16_t priority, @@ -5213,14 +5209,9 @@ ovn_lflow_add_at_with_hash(struct hmap *lflow_map, struct ovn_datapath *od, struct ovn_lflow *lflow; ovs_assert(ovn_stage_to_datapath_type(stage) == ovn_datapath_get_type(od)); - if (parallelization_state == STATE_USE_PARALLELIZATION) { - lflow = do_ovn_lflow_add_pd(lflow_map, od, hash, stage, priority, - match, actions, io_port, stage_hint, where, - ctrl_meter); - } else { - lflow = do_ovn_lflow_add(lflow_map, od, hash, stage, priority, match, - actions, io_port, stage_hint, where, ctrl_meter); - } + + lflow = do_ovn_lflow_add(lflow_map, od, hash, stage, priority, match, + actions, io_port, stage_hint, where, ctrl_meter); return lflow; } @@ -5232,14 +5223,18 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od, const char *ctrl_meter, const struct ovsdb_idl_row *stage_hint, const char *where) { + struct ovs_mutex *hash_lock; uint32_t hash; hash = ovn_logical_flow_hash(ovn_stage_get_table(stage), ovn_stage_get_pipeline(stage), priority, match, actions); + + hash_lock = lflow_hash_lock(lflow_map, hash); ovn_lflow_add_at_with_hash(lflow_map, od, stage, priority, match, actions, io_port, ctrl_meter, stage_hint, where, hash); + lflow_hash_unlock(hash_lock); } static void @@ -5313,9 +5308,6 @@ static void ovn_lflow_destroy(struct hmap *lflows, struct ovn_lflow *lflow) { if (lflow) { - if (parallelization_state != STATE_NULL) { - ovs_mutex_destroy(&lflow->dpg_lock); - } if (lflows) { hmap_remove(lflows, &lflow->hmap_node); } @@ -7055,6 +7047,7 @@ build_lb_rules_pre_stateful(struct hmap *lflows, struct ovn_northd_lb *lb, ovn_stage_get_table(S_SWITCH_IN_PRE_STATEFUL), ovn_stage_get_pipeline(S_SWITCH_IN_PRE_STATEFUL), 120, ds_cstr(match), ds_cstr(action)); + struct ovs_mutex *hash_lock = lflow_hash_lock(lflows, hash); for (size_t j = 0; j < lb->n_nb_ls; j++) { struct ovn_datapath *od = lb->nb_ls[j]; @@ -7067,6 +7060,7 @@ build_lb_rules_pre_stateful(struct hmap *lflows, struct ovn_northd_lb *lb, OVS_SOURCE_LOCATOR, hash); } } + lflow_hash_unlock(hash_lock); } } @@ -7125,6 +7119,7 @@ build_lb_affinity_lr_flows(struct hmap *lflows, struct ovn_northd_lb *lb, ovn_stage_get_table(S_ROUTER_IN_LB_AFF_CHECK), ovn_stage_get_pipeline(S_ROUTER_IN_LB_AFF_CHECK), 100, new_lb_match, aff_check); + struct ovs_mutex *hash_lock = lflow_hash_lock(lflows, hash_aff_check); for (size_t i = 0; i < n_dplist; i++) { if (!ovn_dp_group_add_with_reference(lflow_ref_aff_check, dplist[i])) { @@ -7134,6 +7129,7 @@ build_lb_affinity_lr_flows(struct hmap *lflows, struct ovn_northd_lb *lb, OVS_SOURCE_LOCATOR, hash_aff_check); } } + lflow_hash_unlock(hash_lock); struct ds aff_action = DS_EMPTY_INITIALIZER; struct ds aff_action_learn = DS_EMPTY_INITIALIZER; @@ -7235,12 +7231,8 @@ build_lb_affinity_lr_flows(struct hmap *lflows, struct ovn_northd_lb *lb, ovn_stage_get_table(S_ROUTER_IN_LB_AFF_LEARN), ovn_stage_get_pipeline(S_ROUTER_IN_LB_AFF_LEARN), 100, ds_cstr(&aff_match_learn), ds_cstr(&aff_action_learn)); - - struct ovn_lflow *lflow_ref_aff_lb = NULL; - uint32_t hash_aff_lb = ovn_logical_flow_hash( - ovn_stage_get_table(S_ROUTER_IN_DNAT), - ovn_stage_get_pipeline(S_ROUTER_IN_DNAT), - 150, ds_cstr(&aff_match), ds_cstr(&aff_action)); + struct ovs_mutex *hash_lock_learn = lflow_hash_lock(lflows, + hash_aff_learn); for (size_t j = 0; j < n_dplist; j++) { /* Forward to OFTABLE_CHK_LB_AFFINITY table to store flow tuple. */ @@ -7252,6 +7244,17 @@ build_lb_affinity_lr_flows(struct hmap *lflows, struct ovn_northd_lb *lb, NULL, NULL, &lb->nlb->header_, OVS_SOURCE_LOCATOR, hash_aff_learn); } + } + lflow_hash_unlock(hash_lock_learn); + + struct ovn_lflow *lflow_ref_aff_lb = NULL; + uint32_t hash_aff_lb = ovn_logical_flow_hash( + ovn_stage_get_table(S_ROUTER_IN_DNAT), + ovn_stage_get_pipeline(S_ROUTER_IN_DNAT), + 150, ds_cstr(&aff_match), ds_cstr(&aff_action)); + struct ovs_mutex *hash_lock_lb = lflow_hash_lock(lflows, hash_aff_lb); + + for (size_t j = 0; j < n_dplist; j++) { /* Use already selected backend within affinity timeslot. */ if (!ovn_dp_group_add_with_reference(lflow_ref_aff_lb, dplist[j])) { @@ -7262,6 +7265,7 @@ build_lb_affinity_lr_flows(struct hmap *lflows, struct ovn_northd_lb *lb, hash_aff_lb); } } + lflow_hash_unlock(hash_lock_lb); ds_truncate(&aff_action, aff_action_len); ds_truncate(&aff_action_learn, aff_action_learn_len); @@ -7348,6 +7352,7 @@ build_lb_affinity_ls_flows(struct hmap *lflows, struct ovn_northd_lb *lb, ovn_stage_get_table(S_SWITCH_IN_LB_AFF_CHECK), ovn_stage_get_pipeline(S_SWITCH_IN_LB_AFF_CHECK), 100, ds_cstr(&new_lb_match), aff_check); + struct ovs_mutex *hash_lock = lflow_hash_lock(lflows, hash_aff_check); for (size_t i = 0; i < lb->n_nb_ls; i++) { if (!ovn_dp_group_add_with_reference(lflow_ref_aff_check, @@ -7358,6 +7363,7 @@ build_lb_affinity_ls_flows(struct hmap *lflows, struct ovn_northd_lb *lb, &lb->nlb->header_, OVS_SOURCE_LOCATOR, hash_aff_check); } } + lflow_hash_unlock(hash_lock); ds_destroy(&new_lb_match); struct ds aff_action = DS_EMPTY_INITIALIZER; @@ -7448,12 +7454,8 @@ build_lb_affinity_ls_flows(struct hmap *lflows, struct ovn_northd_lb *lb, ovn_stage_get_table(S_SWITCH_IN_LB_AFF_LEARN), ovn_stage_get_pipeline(S_SWITCH_IN_LB_AFF_LEARN), 100, ds_cstr(&aff_match_learn), ds_cstr(&aff_action_learn)); - - struct ovn_lflow *lflow_ref_aff_lb = NULL; - uint32_t hash_aff_lb = ovn_logical_flow_hash( - ovn_stage_get_table(S_SWITCH_IN_LB), - ovn_stage_get_pipeline(S_SWITCH_IN_LB), - 150, ds_cstr(&aff_match), ds_cstr(&aff_action)); + struct ovs_mutex *hash_lock_learn = lflow_hash_lock(lflows, + hash_aff_learn); for (size_t j = 0; j < lb->n_nb_ls; j++) { /* Forward to OFTABLE_CHK_LB_AFFINITY table to store flow tuple. */ @@ -7465,6 +7467,17 @@ build_lb_affinity_ls_flows(struct hmap *lflows, struct ovn_northd_lb *lb, NULL, NULL, &lb->nlb->header_, OVS_SOURCE_LOCATOR, hash_aff_learn); } + } + lflow_hash_unlock(hash_lock_learn); + + struct ovn_lflow *lflow_ref_aff_lb = NULL; + uint32_t hash_aff_lb = ovn_logical_flow_hash( + ovn_stage_get_table(S_SWITCH_IN_LB), + ovn_stage_get_pipeline(S_SWITCH_IN_LB), + 150, ds_cstr(&aff_match), ds_cstr(&aff_action)); + struct ovs_mutex *hash_lock_lb = lflow_hash_lock(lflows, hash_aff_lb); + + for (size_t j = 0; j < lb->n_nb_ls; j++) { /* Use already selected backend within affinity timeslot. */ if (!ovn_dp_group_add_with_reference(lflow_ref_aff_lb, lb->nb_ls[j])) { @@ -7475,6 +7488,7 @@ build_lb_affinity_ls_flows(struct hmap *lflows, struct ovn_northd_lb *lb, hash_aff_lb); } } + lflow_hash_unlock(hash_lock_lb); ds_truncate(&aff_action, aff_action_len); ds_truncate(&aff_action_learn, aff_action_learn_len); @@ -7546,6 +7560,7 @@ build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb, bool ct_lb_mark, ovn_stage_get_table(S_SWITCH_IN_LB), ovn_stage_get_pipeline(S_SWITCH_IN_LB), priority, ds_cstr(match), ds_cstr(action)); + struct ovs_mutex *hash_lock = lflow_hash_lock(lflows, hash); for (size_t j = 0; j < lb->n_nb_ls; j++) { struct ovn_datapath *od = lb->nb_ls[j]; @@ -7563,6 +7578,7 @@ build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb, bool ct_lb_mark, lflow_ref = meter ? NULL : lflow; } } + lflow_hash_unlock(hash_lock); } } @@ -10482,10 +10498,7 @@ build_gw_lrouter_nat_flows_for_lb(struct ovn_northd_lb *lb, ovn_stage_get_table(S_ROUTER_IN_DNAT), ovn_stage_get_pipeline(S_ROUTER_IN_DNAT), prio, new_match, new_action); - uint32_t hash_est = ovn_logical_flow_hash( - ovn_stage_get_table(S_ROUTER_IN_DNAT), - ovn_stage_get_pipeline(S_ROUTER_IN_DNAT), - prio, est_match, est_action); + struct ovs_mutex *hash_lock_new = lflow_hash_lock(lflows, hash_new); for (size_t i = 0; i < n_dplist; i++) { struct ovn_datapath *od = dplist[i]; @@ -10501,6 +10514,17 @@ build_gw_lrouter_nat_flows_for_lb(struct ovn_northd_lb *lb, hash_new); lflow_ref_new = meter ? NULL : lflow; } + } + lflow_hash_unlock(hash_lock_new); + + uint32_t hash_est = ovn_logical_flow_hash( + ovn_stage_get_table(S_ROUTER_IN_DNAT), + ovn_stage_get_pipeline(S_ROUTER_IN_DNAT), + prio, est_match, est_action); + struct ovs_mutex *hash_lock_est = lflow_hash_lock(lflows, hash_est); + + for (size_t i = 0; i < n_dplist; i++) { + struct ovn_datapath *od = dplist[i]; if (!ovn_dp_group_add_with_reference(lflow_ref_est, od)) { lflow_ref_est = ovn_lflow_add_at_with_hash(lflows, od, @@ -10509,6 +10533,7 @@ build_gw_lrouter_nat_flows_for_lb(struct ovn_northd_lb *lb, OVS_SOURCE_LOCATOR, hash_est); } } + lflow_hash_unlock(hash_lock_est); } static void @@ -10921,6 +10946,8 @@ build_lrouter_defrag_flows_for_lb(struct ovn_northd_lb *lb, ovn_stage_get_table(S_ROUTER_IN_DEFRAG), ovn_stage_get_pipeline(S_ROUTER_IN_DEFRAG), prio, ds_cstr(match), ds_cstr(&defrag_actions)); + struct ovs_mutex *hash_lock = lflow_hash_lock(lflows, hash); + for (size_t j = 0; j < lb->n_nb_lr; j++) { struct ovn_datapath *od = lb->nb_lr[j]; if (ovn_dp_group_add_with_reference(lflow_ref, od)) { @@ -10932,6 +10959,7 @@ build_lrouter_defrag_flows_for_lb(struct ovn_northd_lb *lb, NULL, NULL, &lb->nlb->header_, OVS_SOURCE_LOCATOR, hash); } + lflow_hash_unlock(hash_lock); } ds_destroy(&defrag_actions); } 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;