Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/1528347/?format=api
{ "id": 1528347, "url": "http://patchwork.ozlabs.org/api/patches/1528347/?format=api", "web_url": "http://patchwork.ozlabs.org/project/ovn/patch/20210915124340.1765-3-anton.ivanov@cambridgegreys.com/", "project": { "id": 68, "url": "http://patchwork.ozlabs.org/api/projects/68/?format=api", "name": "Open Virtual Network development", "link_name": "ovn", "list_id": "ovs-dev.openvswitch.org", "list_email": "ovs-dev@openvswitch.org", "web_url": "http://openvswitch.org/", "scm_url": "", "webscm_url": "", "list_archive_url": "", "list_archive_url_format": "", "commit_url_format": "" }, "msgid": "<20210915124340.1765-3-anton.ivanov@cambridgegreys.com>", "list_archive_url": null, "date": "2021-09-15T12:43:40", "name": "[ovs-dev,v8,3/3] northd: Restore parallel build with dp_groups", "commit_ref": null, "pull_url": null, "state": "accepted", "archived": false, "hash": "b313480120f67e38f7e973c2d636f1a46236f507", "submitter": { "id": 71996, "url": "http://patchwork.ozlabs.org/api/people/71996/?format=api", "name": "Anton Ivanov", "email": "anton.ivanov@cambridgegreys.com" }, "delegate": null, "mbox": "http://patchwork.ozlabs.org/project/ovn/patch/20210915124340.1765-3-anton.ivanov@cambridgegreys.com/mbox/", "series": [ { "id": 262448, "url": "http://patchwork.ozlabs.org/api/series/262448/?format=api", "web_url": "http://patchwork.ozlabs.org/project/ovn/list/?series=262448", "date": "2021-09-15T12:43:38", "name": "[ovs-dev,v8,1/3] northd: Disable parallel processing for logical_dp_groups", "version": 8, "mbox": "http://patchwork.ozlabs.org/series/262448/mbox/" } ], "comments": "http://patchwork.ozlabs.org/api/patches/1528347/comments/", "check": "fail", "checks": "http://patchwork.ozlabs.org/api/patches/1528347/checks/", "tags": {}, "related": [], "headers": { "Return-Path": "<ovs-dev-bounces@openvswitch.org>", "X-Original-To": [ "incoming@patchwork.ozlabs.org", "ovs-dev@openvswitch.org" ], "Delivered-To": [ "patchwork-incoming@ozlabs.org", "ovs-dev@lists.linuxfoundation.org" ], "Authentication-Results": "ozlabs.org;\n spf=pass (sender SPF authorized) smtp.mailfrom=openvswitch.org\n (client-ip=140.211.166.137; helo=smtp4.osuosl.org;\n envelope-from=ovs-dev-bounces@openvswitch.org; receiver=<UNKNOWN>)", "Received": [ "from smtp4.osuosl.org (smtp4.osuosl.org [140.211.166.137])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\t key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest\n SHA256)\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 4H8g0p0Qdvz9ssD\n\tfor <incoming@patchwork.ozlabs.org>; Wed, 15 Sep 2021 22:45:13 +1000 (AEST)", "from localhost (localhost [127.0.0.1])\n\tby smtp4.osuosl.org (Postfix) with ESMTP id 6D50F406A5;\n\tWed, 15 Sep 2021 12:45:10 +0000 (UTC)", "from smtp4.osuosl.org ([127.0.0.1])\n\tby localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n\twith ESMTP id xH2VdDNrwGwF; Wed, 15 Sep 2021 12:45:09 +0000 (UTC)", "from lists.linuxfoundation.org (lf-lists.osuosl.org\n [IPv6:2605:bc80:3010:104::8cd3:938])\n\tby smtp4.osuosl.org (Postfix) with ESMTPS id 6AE0C406A3;\n\tWed, 15 Sep 2021 12:45:08 +0000 (UTC)", "from lf-lists.osuosl.org (localhost [127.0.0.1])\n\tby lists.linuxfoundation.org (Postfix) with ESMTP id 3A11EC0022;\n\tWed, 15 Sep 2021 12:45:08 +0000 (UTC)", "from smtp4.osuosl.org (smtp4.osuosl.org [IPv6:2605:bc80:3010::137])\n by lists.linuxfoundation.org (Postfix) with ESMTP id 1A165C0011\n for <ovs-dev@openvswitch.org>; Wed, 15 Sep 2021 12:45:06 +0000 (UTC)", "from localhost (localhost [127.0.0.1])\n by smtp4.osuosl.org (Postfix) with ESMTP id 57EE240641\n for <ovs-dev@openvswitch.org>; Wed, 15 Sep 2021 12:43:54 +0000 (UTC)", "from smtp4.osuosl.org ([127.0.0.1])\n by localhost (smtp4.osuosl.org [127.0.0.1]) (amavisd-new, port 10024)\n with ESMTP id ECbT4GS-4glI for <ovs-dev@openvswitch.org>;\n Wed, 15 Sep 2021 12:43:53 +0000 (UTC)", "from www.kot-begemot.co.uk (ivanoab7.miniserver.com [37.128.132.42])\n by smtp4.osuosl.org (Postfix) with ESMTPS id 38C8340597\n for <ovs-dev@openvswitch.org>; Wed, 15 Sep 2021 12:43:51 +0000 (UTC)", "from tun252.jain.kot-begemot.co.uk ([192.168.18.6]\n helo=jain.kot-begemot.co.uk)\n by www.kot-begemot.co.uk with esmtps\n (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256)\n (Exim 4.92) (envelope-from <anton.ivanov@cambridgegreys.com>)\n id 1mQUGT-0000Vd-C7; Wed, 15 Sep 2021 12:43:49 +0000", "from jain.kot-begemot.co.uk ([192.168.3.3])\n by jain.kot-begemot.co.uk with esmtp (Exim 4.92)\n (envelope-from <anton.ivanov@cambridgegreys.com>)\n id 1mQUGQ-0000TO-56; Wed, 15 Sep 2021 13:43:48 +0100" ], "X-Virus-Scanned": [ "amavisd-new at osuosl.org", "amavisd-new at osuosl.org" ], "X-Greylist": "from auto-whitelisted by SQLgrey-1.8.0", "From": "anton.ivanov@cambridgegreys.com", "To": "ovs-dev@openvswitch.org", "Date": "Wed, 15 Sep 2021 13:43:40 +0100", "Message-Id": "<20210915124340.1765-3-anton.ivanov@cambridgegreys.com>", "X-Mailer": "git-send-email 2.20.1", "In-Reply-To": "<20210915124340.1765-1-anton.ivanov@cambridgegreys.com>", "References": "<20210915124340.1765-1-anton.ivanov@cambridgegreys.com>", "MIME-Version": "1.0", "X-Clacks-Overhead": "GNU Terry Pratchett", "Cc": "i.maximets@ovn.org, Anton Ivanov <anton.ivanov@cambridgegreys.com>", "Subject": "[ovs-dev] [OVN Patch v8 3/3] northd: Restore parallel build with\n\tdp_groups", "X-BeenThere": "ovs-dev@openvswitch.org", "X-Mailman-Version": "2.1.15", "Precedence": "list", "List-Id": "<ovs-dev.openvswitch.org>", "List-Unsubscribe": "<https://mail.openvswitch.org/mailman/options/ovs-dev>,\n <mailto:ovs-dev-request@openvswitch.org?subject=unsubscribe>", "List-Archive": "<http://mail.openvswitch.org/pipermail/ovs-dev/>", "List-Post": "<mailto:ovs-dev@openvswitch.org>", "List-Help": "<mailto:ovs-dev-request@openvswitch.org?subject=help>", "List-Subscribe": "<https://mail.openvswitch.org/mailman/listinfo/ovs-dev>,\n <mailto:ovs-dev-request@openvswitch.org?subject=subscribe>", "Content-Type": "text/plain; charset=\"us-ascii\"", "Content-Transfer-Encoding": "7bit", "Errors-To": "ovs-dev-bounces@openvswitch.org", "Sender": "\"dev\" <ovs-dev-bounces@openvswitch.org>" }, "content": "From: Anton Ivanov <anton.ivanov@cambridgegreys.com>\n\nRestore parallel build with dp groups using rwlock instead\nof per row locking as an underlying mechanism.\n\nThis provides improvement ~ 10% end-to-end on ovn-heater\nunder virutalization despite awakening some qemu gremlin\nwhich makes qemu climb to silly CPU usage. The gain on\nbare metal is likely to be higher.\n\nSigned-off-by: Anton Ivanov <anton.ivanov@cambridgegreys.com>\n---\n northd/ovn-northd.c | 150 +++++++++++++++++++++++++++++++++++++-------\n 1 file changed, 127 insertions(+), 23 deletions(-)", "diff": "diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c\nindex ed231510e..34e6ad1a9 100644\n--- a/northd/ovn-northd.c\n+++ b/northd/ovn-northd.c\n@@ -59,6 +59,7 @@\n #include \"unixctl.h\"\n #include \"util.h\"\n #include \"uuid.h\"\n+#include \"ovs-thread.h\"\n #include \"openvswitch/vlog.h\"\n \n VLOG_DEFINE_THIS_MODULE(ovn_northd);\n@@ -4294,6 +4295,7 @@ struct ovn_lflow {\n struct hmap_node hmap_node;\n \n struct ovn_datapath *od; /* 'logical_datapath' in SB schema. */\n+ struct ovs_mutex odg_lock; /* Lock guarding access to od_group */\n struct hmapx od_group; /* Hash map of 'struct ovn_datapath *'. */\n enum ovn_stage stage;\n uint16_t priority;\n@@ -4335,6 +4337,11 @@ ovn_lflow_equal(const struct ovn_lflow *a, const struct ovn_datapath *od,\n && !strcmp(a->actions, actions)\n && nullable_string_is_equal(a->ctrl_meter, ctrl_meter));\n }\n+/* If this option is 'true' northd will combine logical flows that differ by\n+ * logical datapath only by creating a datapath group. */\n+static bool use_logical_dp_groups = false;\n+static bool use_parallel_build = true;\n+\n \n static void\n ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od,\n@@ -4353,24 +4360,56 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od,\n lflow->ctrl_meter = ctrl_meter;\n lflow->dpg = NULL;\n lflow->where = where;\n+ if (use_parallel_build && use_logical_dp_groups) {\n+ ovs_mutex_init(&lflow->odg_lock);\n+ }\n }\n \n-/* If this option is 'true' northd will combine logical flows that differ by\n- * logical datapath only by creating a datapath group. */\n-static bool use_logical_dp_groups = false;\n-static bool use_parallel_build = true;\n+ /* Adds a row with the specified contents to the Logical_Flow table.\n+ * Version to use with dp_groups + parallel - when locking is required.\n+ *\n+ * Assumptions:\n+ *\n+ * 1. A large proportion of the operations are lookups (reads).\n+ * 2. RW operations are a small proportion of overall adds.\n+ * 3. Most RW ops are not flow adds, but changes to the\n+ * od groups.\n+ *\n+ * Principles of operation:\n+ * 1. All accesses to the flow table are protected by a rwlock.\n+ * 2. By default, everyone grabs a rd lock so that multiple threads\n+ * can do lookups simultaneously.\n+ * 3. If a change to the lflow is needed, the rd lock is released and\n+ * a wr lock is acquired instead (the fact that POSIX does not have an\n+ * \"upgrade\" on locks is a major pain, but there is nothing we can do\n+ * - it's not available).\n+ * 4. WR lock operations in rd/wr locking have LOWER priority than RD.\n+ * That is by design and spec. So the code after a request for WR lock\n+ * may wait for a considerable amount of time until it is given a\n+ * change to run. That means that another thread may get there in the\n+ * meantime and change the data. Hence all wr operations MUST be coded\n+ * to ensure that they are not vulnerable to \"someone pulled this from\n+ * under my feet\". Re- reads, checks for presense, etc.\n+ * 5. Operations on the actual od_group hash map are protected by\n+ * per-flow locks. There is no need for these to be rd, mutex is more\n+ * appropriate. They are low contention as each protects only its flow\n+ * and only during modification which happen while holding a rd lock on\n+ * the flow table.\n+ */\n \n-static struct hashrow_locks lflow_locks;\n+static struct ovs_rwlock flowtable_lock;\n \n /* Adds a row with the specified contents to the Logical_Flow table.\n- * Version to use when locking is required.\n+ * Version to use when locking is NOT required.\n */\n+\n static struct ovn_lflow *\n do_ovn_lflow_add(struct hmap *lflow_map, struct ovn_datapath *od,\n uint32_t hash, enum ovn_stage stage, uint16_t priority,\n const char *match, const char *actions, const char *io_port,\n const struct ovsdb_idl_row *stage_hint,\n const char *where, const char *ctrl_meter)\n+ OVS_NO_THREAD_SAFETY_ANALYSIS\n {\n \n struct ovn_lflow *old_lflow;\n@@ -4403,6 +4442,59 @@ do_ovn_lflow_add(struct hmap *lflow_map, struct ovn_datapath *od,\n return lflow;\n }\n \n+/* Adds a row with the specified contents to the Logical_Flow table.\n+ * Version to use when locking is IS required.\n+ */\n+\n+static struct ovn_lflow *\n+do_ovn_lflow_add_pd(struct hmap *lflow_map, struct ovn_datapath *od,\n+ uint32_t hash, enum ovn_stage stage, uint16_t priority,\n+ const char *match, const char *actions,\n+ const char *io_port,\n+ const struct ovsdb_idl_row *stage_hint,\n+ const char *where, const char *ctrl_meter)\n+ OVS_NO_THREAD_SAFETY_ANALYSIS\n+{\n+\n+ struct ovn_lflow *old_lflow;\n+ struct ovn_lflow *lflow;\n+\n+ /* Fast Path - try to amend an existing flow without asking\n+ * for WR access to the whole flow table. Locking the actual\n+ * hmapx for the particular flow's odg is low overhead as its\n+ * contention is much lower.\n+ */\n+\n+ ovs_rwlock_rdlock(&flowtable_lock);\n+ old_lflow = ovn_lflow_find(lflow_map, NULL, stage, priority, match,\n+ actions, ctrl_meter, hash);\n+ if (old_lflow) {\n+ ovs_mutex_lock(&old_lflow->odg_lock);\n+ hmapx_add(&old_lflow->od_group, od);\n+ ovs_mutex_unlock(&old_lflow->odg_lock);\n+ }\n+ ovs_rwlock_unlock(&flowtable_lock);\n+\n+ if (old_lflow) {\n+ return old_lflow;\n+ }\n+\n+ ovs_rwlock_wrlock(&flowtable_lock);\n+\n+ /* We need to rerun the \"if in flowtable\" steps, because someone\n+ * could have inserted it while we were waiting to acquire an\n+ * wr lock. As we are now holding a wr lock on it nobody else is\n+ * in the * \"fast\" portion of the code which is protected by the\n+ * rwlock.\n+ */\n+ lflow = do_ovn_lflow_add(lflow_map, od, hash, stage, priority, match,\n+ actions, io_port, stage_hint, where,\n+ ctrl_meter);\n+ ovs_rwlock_unlock(&flowtable_lock);\n+ return lflow;\n+}\n+\n+\n static struct ovn_lflow *\n ovn_lflow_add_at_with_hash(struct hmap *lflow_map, struct ovn_datapath *od,\n enum ovn_stage stage, uint16_t priority,\n@@ -4415,11 +4507,9 @@ ovn_lflow_add_at_with_hash(struct hmap *lflow_map, struct ovn_datapath *od,\n \n ovs_assert(ovn_stage_to_datapath_type(stage) == ovn_datapath_get_type(od));\n if (use_logical_dp_groups && use_parallel_build) {\n- lock_hash_row(&lflow_locks, hash);\n- lflow = do_ovn_lflow_add(lflow_map, od, hash, stage, priority, match,\n- actions, io_port, stage_hint, where,\n- ctrl_meter);\n- unlock_hash_row(&lflow_locks, hash);\n+ lflow = do_ovn_lflow_add_pd(lflow_map, od, hash, stage, priority,\n+ match, actions, io_port, stage_hint, where,\n+ ctrl_meter);\n } else {\n lflow = do_ovn_lflow_add(lflow_map, od, hash, stage, priority, match,\n actions, io_port, stage_hint, where, ctrl_meter);\n@@ -4447,17 +4537,17 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od,\n \n static bool\n ovn_dp_group_add_with_reference(struct ovn_lflow *lflow_ref,\n- struct ovn_datapath *od,\n- uint32_t hash)\n+ struct ovn_datapath *od)\n+ OVS_NO_THREAD_SAFETY_ANALYSIS\n {\n if (!use_logical_dp_groups || !lflow_ref) {\n return false;\n }\n \n- if (use_parallel_build) {\n- lock_hash_row(&lflow_locks, hash);\n+ if (use_parallel_build & use_logical_dp_groups) {\n+ ovs_mutex_lock(&lflow_ref->odg_lock);\n hmapx_add(&lflow_ref->od_group, od);\n- unlock_hash_row(&lflow_locks, hash);\n+ ovs_mutex_unlock(&lflow_ref->odg_lock);\n } else {\n hmapx_add(&lflow_ref->od_group, od);\n }\n@@ -6423,7 +6513,7 @@ build_lb_rules(struct hmap *lflows, struct ovn_northd_lb *lb,\n if (reject) {\n meter = copp_meter_get(COPP_REJECT, od->nbs->copp,\n meter_groups);\n- } else if (ovn_dp_group_add_with_reference(lflow_ref, od, hash)) {\n+ } else if (ovn_dp_group_add_with_reference(lflow_ref, od)) {\n continue;\n }\n lflow_ref = ovn_lflow_add_at_with_hash(lflows, od,\n@@ -9476,7 +9566,7 @@ build_lrouter_defrag_flows_for_lb(struct ovn_northd_lb *lb,\n ds_cstr(match), ds_cstr(&defrag_actions));\n for (size_t j = 0; j < lb->n_nb_lr; j++) {\n struct ovn_datapath *od = lb->nb_lr[j];\n- if (ovn_dp_group_add_with_reference(lflow_ref, od, hash)) {\n+ if (ovn_dp_group_add_with_reference(lflow_ref, od)) {\n continue;\n }\n lflow_ref = ovn_lflow_add_at_with_hash(lflows, od,\n@@ -9540,7 +9630,7 @@ build_lflows_for_unreachable_vips(struct ovn_northd_lb *lb,\n continue;\n }\n \n- if (ovn_dp_group_add_with_reference(lflow_ref, peer->od, hash)) {\n+ if (ovn_dp_group_add_with_reference(lflow_ref, peer->od)) {\n continue;\n }\n lflow_ref = ovn_lflow_add_at_with_hash(lflows, peer->od,\n@@ -12974,7 +13064,7 @@ build_lswitch_and_lrouter_flows(struct hmap *datapaths, struct hmap *ports,\n }\n }\n \n- if (use_parallel_build && (!use_logical_dp_groups)) {\n+ if (use_parallel_build) {\n struct hmap *lflow_segs;\n struct lswitch_flow_build_info *lsiv;\n int index;\n@@ -13156,6 +13246,8 @@ ovn_sb_set_lflow_logical_dp_group(\n }\n \n static ssize_t max_seen_lflow_size = 128;\n+static bool needs_parallel_init = true;\n+static bool reset_parallel = false;\n \n /* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB database,\n * constructing their contents based on the OVN_NB database. */\n@@ -13169,9 +13261,22 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths,\n {\n struct hmap lflows;\n \n+ if (reset_parallel) {\n+ /* Parallel build was disabled before, we need to\n+ * re-enable it. */\n+ use_parallel_build = true;\n+ reset_parallel = false;\n+ }\n+\n fast_hmap_size_for(&lflows, max_seen_lflow_size);\n- if (use_parallel_build) {\n- update_hashrow_locks(&lflows, &lflow_locks);\n+ if (use_parallel_build && use_logical_dp_groups &&\n+ needs_parallel_init) {\n+ ovs_rwlock_init(&flowtable_lock);\n+ needs_parallel_init = false;\n+ /* Disable parallel build on first run with dp_groups\n+ * to determine the correct sizing of hashes. */\n+ use_parallel_build = false;\n+ reset_parallel = true;\n }\n build_lswitch_and_lrouter_flows(datapaths, ports,\n port_groups, &lflows, mcgroups,\n@@ -15167,7 +15272,6 @@ main(int argc, char *argv[])\n \n daemonize_complete();\n \n- init_hash_row_locks(&lflow_locks);\n use_parallel_build = can_parallelize_hashes(false);\n \n /* We want to detect (almost) all changes to the ovn-nb db. */\n", "prefixes": [ "ovs-dev", "v8", "3/3" ] }