Show a patch.

Update a patch.

Update a patch.

GET /api/patches/1528347/
Content-Type: application/json
Vary: Accept

    "id": 1528347,
    "url": "",
    "web_url": "",
    "project": {
        "id": 68,
        "url": "",
        "name": "Open Virtual Network development",
        "link_name": "ovn",
        "list_id": "",
        "list_email": "",
        "web_url": "",
        "scm_url": "",
        "webscm_url": "",
        "list_archive_url": "",
        "list_archive_url_format": "",
        "commit_url_format": ""
    "msgid": "<>",
    "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": "",
        "name": "Anton Ivanov",
        "email": ""
    "delegate": null,
    "mbox": "",
    "series": [
            "id": 262448,
            "url": "",
            "web_url": "",
            "date": "2021-09-15T12:43:38",
            "name": "[ovs-dev,v8,1/3] northd: Disable parallel processing for logical_dp_groups",
            "version": 8,
            "mbox": ""
    "comments": "",
    "check": "fail",
    "checks": "",
    "tags": {},
    "related": [],
    "headers": {
        "Return-Path": "<>",
        "X-Original-To": [
        "Delivered-To": [
        "Authentication-Results": ";\n spf=pass (sender SPF authorized)\n (client-ip=;;\n; receiver=<UNKNOWN>)",
        "Received": [
            "from ( [])\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 (Postfix) with ESMTPS id 4H8g0p0Qdvz9ssD\n\tfor <>; Wed, 15 Sep 2021 22:45:13 +1000 (AEST)",
            "from localhost (localhost [])\n\tby (Postfix) with ESMTP id 6D50F406A5;\n\tWed, 15 Sep 2021 12:45:10 +0000 (UTC)",
            "from ([])\n\tby localhost ( []) (amavisd-new, port 10024)\n\twith ESMTP id xH2VdDNrwGwF; Wed, 15 Sep 2021 12:45:09 +0000 (UTC)",
            "from (\n [IPv6:2605:bc80:3010:104::8cd3:938])\n\tby (Postfix) with ESMTPS id 6AE0C406A3;\n\tWed, 15 Sep 2021 12:45:08 +0000 (UTC)",
            "from (localhost [])\n\tby (Postfix) with ESMTP id 3A11EC0022;\n\tWed, 15 Sep 2021 12:45:08 +0000 (UTC)",
            "from ( [IPv6:2605:bc80:3010::137])\n by (Postfix) with ESMTP id 1A165C0011\n for <>; Wed, 15 Sep 2021 12:45:06 +0000 (UTC)",
            "from localhost (localhost [])\n by (Postfix) with ESMTP id 57EE240641\n for <>; Wed, 15 Sep 2021 12:43:54 +0000 (UTC)",
            "from ([])\n by localhost ( []) (amavisd-new, port 10024)\n with ESMTP id ECbT4GS-4glI for <>;\n Wed, 15 Sep 2021 12:43:53 +0000 (UTC)",
            "from ( [])\n by (Postfix) with ESMTPS id 38C8340597\n for <>; Wed, 15 Sep 2021 12:43:51 +0000 (UTC)",
            "from ([]\n\n by with esmtps\n (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256)\n (Exim 4.92) (envelope-from <>)\n id 1mQUGT-0000Vd-C7; Wed, 15 Sep 2021 12:43:49 +0000",
            "from ([])\n by with esmtp (Exim 4.92)\n (envelope-from <>)\n id 1mQUGQ-0000TO-56; Wed, 15 Sep 2021 13:43:48 +0100"
        "X-Virus-Scanned": [
            "amavisd-new at",
            "amavisd-new at"
        "X-Greylist": "from auto-whitelisted by SQLgrey-1.8.0",
        "From": "",
        "To": "",
        "Date": "Wed, 15 Sep 2021 13:43:40 +0100",
        "Message-Id": "<>",
        "X-Mailer": "git-send-email 2.20.1",
        "In-Reply-To": "<>",
        "References": "<>",
        "MIME-Version": "1.0",
        "X-Clacks-Overhead": "GNU Terry Pratchett",
        "Cc": ", Anton Ivanov <>",
        "Subject": "[ovs-dev] [OVN Patch v8 3/3] northd: Restore parallel build with\n\tdp_groups",
        "X-BeenThere": "",
        "X-Mailman-Version": "2.1.15",
        "Precedence": "list",
        "List-Id": "<>",
        "List-Unsubscribe": "<>,\n <>",
        "List-Archive": "<>",
        "List-Post": "<>",
        "List-Help": "<>",
        "List-Subscribe": "<>,\n <>",
        "Content-Type": "text/plain; charset=\"us-ascii\"",
        "Content-Transfer-Encoding": "7bit",
        "Errors-To": "",
        "Sender": "\"dev\" <>"
    "content": "From: Anton Ivanov <>\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 <>\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": [