From patchwork Fri Dec 11 18:01:15 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Maximets X-Patchwork-Id: 1415188 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.136; helo=silver.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 silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4CszBF6R2Hz9sSf for ; Sat, 12 Dec 2020 05:01:39 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 768CC2E137; Fri, 11 Dec 2020 18:01:37 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id NP9+t4cUCkna; Fri, 11 Dec 2020 18:01:28 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by silver.osuosl.org (Postfix) with ESMTP id C4FD820762; Fri, 11 Dec 2020 18:01:28 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id AFE62C0893; Fri, 11 Dec 2020 18:01:28 +0000 (UTC) X-Original-To: ovs-dev@openvswitch.org Delivered-To: ovs-dev@lists.linuxfoundation.org Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by lists.linuxfoundation.org (Postfix) with ESMTP id B7A05C013B for ; Fri, 11 Dec 2020 18:01:26 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 97BCD2E11A for ; Fri, 11 Dec 2020 18:01:26 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id hLriIofBbBdJ for ; Fri, 11 Dec 2020 18:01:24 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay11.mail.gandi.net (relay11.mail.gandi.net [217.70.178.231]) by silver.osuosl.org (Postfix) with ESMTPS id 8CE2320762 for ; Fri, 11 Dec 2020 18:01:23 +0000 (UTC) Received: from im-t490s.redhat.com (ip-78-45-89-65.net.upcbroadband.cz [78.45.89.65]) (Authenticated sender: i.maximets@ovn.org) by relay11.mail.gandi.net (Postfix) with ESMTPSA id 8B5D8100012; Fri, 11 Dec 2020 18:01:18 +0000 (UTC) From: Ilya Maximets To: ovs-dev@openvswitch.org Date: Fri, 11 Dec 2020 19:01:15 +0100 Message-Id: <20201211180115.3829253-1-i.maximets@ovn.org> X-Mailer: git-send-email 2.25.4 MIME-Version: 1.0 Cc: Ilya Maximets Subject: [ovs-dev] [PATCH ovn v3] northd: Fix datapath swapping in logical flows. 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" Commit that introduced support for logical datapath groups removed the 'logical_datapath' column from the lflow hash. If datapath groups disabled, there will be many flows that are same except for the 'logical_datapath' column, so they will have exactly same hash. Since 'logical_datapath' is not involved into comparison inside ovn_lflow_equal(), all these flows will be considered equal. While iterating over Southbound DB records, ovn_lflow_found() will return first of many "equal" flows and, likely, not the right one. Comparison of logical datapaths will say that we need to update the logical datapath, so it will be updated with the value from the flow that we just found. Flow that we found will be removed from the hash map. Similar procedure for the next DB record will give us different "equal" flow leading to the update of the 'logical_datapath' column for the next record. And so on. This way we will swap 'logical_datapath' column for almost all logical flows. Since we're not using same lflow more than once, resulted database will still be correct, but all these unnecessary steps will generate huge database transaction if we'll have at least one new or modified logical flow, because that will cause different order in which lflows will be found in a hash map. Fix that by re-adding 'logical_datapath' column back to hash and to the check for equality. This way correct lflows could be found, so there will be no unnecessary updates. Some functions and variables renamed to better describe their meaning. Also size_t replaced with uint32_t to avoid confusion. lib/hash.c always returns uint32_t as a hash value and that is important because we will want to update current hash with the datapath value and not to re-calculate it for all lflow columns. Fixes: bfed22400675 ("northd: Add support for Logical Datapath Groups.") Reported-by: Dumitru Ceara Acked-by: Dumitru Ceara Signed-off-by: Ilya Maximets --- Version 3: * Fixed grammar and a smal style nits from Dumitru. * Added ACK from Dumitru. Version 2: * Removed unused incorrect condition from the ovn_logical_flow_hash_datapath function. It should not return 0, but should return 'hash' instead. Anyway, there is no code that uses this branch, so I just removed it to make the code cleaner: - return logical_datapath ? hash_add(hash, uuid_hash(logical_datapath)) : 0; + return hash_add(hash, uuid_hash(logical_datapath)); lib/ovn-util.c | 15 ++++- lib/ovn-util.h | 2 + northd/ovn-northd.c | 134 ++++++++++++++++++++++++++++---------------- 3 files changed, 101 insertions(+), 50 deletions(-) diff --git a/lib/ovn-util.c b/lib/ovn-util.c index f62c97e96..2136f90fe 100644 --- a/lib/ovn-util.c +++ b/lib/ovn-util.c @@ -505,8 +505,12 @@ ovn_is_known_nb_lsp_type(const char *type) uint32_t sbrec_logical_flow_hash(const struct sbrec_logical_flow *lf) { - return ovn_logical_flow_hash(lf->table_id, lf->pipeline, - lf->priority, lf->match, lf->actions); + const struct sbrec_datapath_binding *ld = lf->logical_datapath; + uint32_t hash = ovn_logical_flow_hash(lf->table_id, lf->pipeline, + lf->priority, lf->match, + lf->actions); + + return ld ? ovn_logical_flow_hash_datapath(&ld->header_.uuid, hash) : hash; } uint32_t @@ -520,6 +524,13 @@ ovn_logical_flow_hash(uint8_t table_id, const char *pipeline, return hash_string(actions, hash); } +uint32_t +ovn_logical_flow_hash_datapath(const struct uuid *logical_datapath, + uint32_t hash) +{ + return hash_add(hash, uuid_hash(logical_datapath)); +} + bool datapath_is_switch(const struct sbrec_datapath_binding *ldp) { diff --git a/lib/ovn-util.h b/lib/ovn-util.h index 1d2f7a9c5..679f47a97 100644 --- a/lib/ovn-util.h +++ b/lib/ovn-util.h @@ -105,6 +105,8 @@ uint32_t sbrec_logical_flow_hash(const struct sbrec_logical_flow *); uint32_t ovn_logical_flow_hash(uint8_t table_id, const char *pipeline, uint16_t priority, const char *match, const char *actions); +uint32_t ovn_logical_flow_hash_datapath(const struct uuid *logical_datapath, + uint32_t hash); bool datapath_is_switch(const struct sbrec_datapath_binding *); int datapath_snat_ct_zone(const struct sbrec_datapath_binding *ldp); void ovn_conn_show(struct unixctl_conn *conn, int argc OVS_UNUSED, diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 957242367..d94c735ee 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -4097,7 +4097,8 @@ ovn_igmp_group_destroy(struct hmap *igmp_groups, struct ovn_lflow { struct hmap_node hmap_node; - struct hmapx od; /* Hash map of 'struct ovn_datapath *'. */ + struct ovn_datapath *od; /* 'logical_datapath' in SB schema. */ + struct hmapx od_group; /* Hash map of 'struct ovn_datapath *'. */ enum ovn_stage stage; uint16_t priority; char *match; @@ -4109,9 +4110,9 @@ struct ovn_lflow { static void ovn_lflow_destroy(struct hmap *lflows, struct ovn_lflow *lflow); static struct ovn_lflow * ovn_lflow_find_by_lflow(const struct hmap *, const struct ovn_lflow *, - size_t hash); + uint32_t hash); -static size_t +static uint32_t ovn_lflow_hash(const struct ovn_lflow *lflow) { return ovn_logical_flow_hash(ovn_stage_get_table(lflow->stage), @@ -4132,19 +4133,21 @@ ovn_lflow_hint(const struct ovsdb_idl_row *row) static bool ovn_lflow_equal(const struct ovn_lflow *a, const struct ovn_lflow *b) { - return (a->stage == b->stage + return (a->od == b->od + && a->stage == b->stage && a->priority == b->priority && !strcmp(a->match, b->match) && !strcmp(a->actions, b->actions)); } static void -ovn_lflow_init(struct ovn_lflow *lflow, +ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od, enum ovn_stage stage, uint16_t priority, char *match, char *actions, char *stage_hint, const char *where) { - hmapx_init(&lflow->od); + hmapx_init(&lflow->od_group); + lflow->od = od; lflow->stage = stage; lflow->priority = priority; lflow->match = match; @@ -4167,10 +4170,13 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od, ovs_assert(ovn_stage_to_datapath_type(stage) == ovn_datapath_get_type(od)); struct ovn_lflow *old_lflow, *lflow; - size_t hash; + uint32_t hash; lflow = xmalloc(sizeof *lflow); - ovn_lflow_init(lflow, stage, priority, + /* While adding new logical flows we're not setting single datapath, but + * collecting a group. 'od' will be updated later for all flows with only + * one datapath in a group, so it could be hashed correctly. */ + ovn_lflow_init(lflow, NULL, stage, priority, xstrdup(match), xstrdup(actions), ovn_lflow_hint(stage_hint), where); @@ -4179,12 +4185,12 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od, old_lflow = ovn_lflow_find_by_lflow(lflow_map, lflow, hash); if (old_lflow) { ovn_lflow_destroy(NULL, lflow); - hmapx_add(&old_lflow->od, od); + hmapx_add(&old_lflow->od_group, od); return; } } - hmapx_add(&lflow->od, od); + hmapx_add(&lflow->od_group, od); hmap_insert(lflow_map, &lflow->hmap_node, hash); } @@ -4218,12 +4224,12 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od, NULL, OVS_SOURCE_LOCATOR) static struct ovn_lflow * -ovn_lflow_find(struct hmap *lflows, +ovn_lflow_find(struct hmap *lflows, struct ovn_datapath *od, enum ovn_stage stage, uint16_t priority, const char *match, const char *actions, uint32_t hash) { struct ovn_lflow target; - ovn_lflow_init(&target, stage, priority, + ovn_lflow_init(&target, od, stage, priority, CONST_CAST(char *, match), CONST_CAST(char *, actions), NULL, NULL); @@ -4237,7 +4243,7 @@ ovn_lflow_destroy(struct hmap *lflows, struct ovn_lflow *lflow) if (lflows) { hmap_remove(lflows, &lflow->hmap_node); } - hmapx_destroy(&lflow->od); + hmapx_destroy(&lflow->od_group); free(lflow->match); free(lflow->actions); free(lflow->stage_hint); @@ -4247,7 +4253,7 @@ ovn_lflow_destroy(struct hmap *lflows, struct ovn_lflow *lflow) static struct ovn_lflow * ovn_lflow_find_by_lflow(const struct hmap *lflows, - const struct ovn_lflow *target, size_t hash) + const struct ovn_lflow *target, uint32_t hash) { struct ovn_lflow *lflow; HMAP_FOR_EACH_WITH_HASH (lflow, hmap_node, hash, lflows) { @@ -11318,33 +11324,28 @@ ovn_sb_insert_logical_dp_group(struct northd_context *ctx, } static void -ovn_sb_set_lflow_logical_datapaths( +ovn_sb_set_lflow_logical_dp_group( struct northd_context *ctx, struct hmap *dp_groups, const struct sbrec_logical_flow *sbflow, - const struct hmapx *od) + const struct hmapx *od_group) { - const struct hmapx_node *node; struct ovn_dp_group *dpg; - if (hmapx_count(od) == 1) { - HMAPX_FOR_EACH (node, od) { - struct ovn_datapath *dp = node->data; - - sbrec_logical_flow_set_logical_datapath(sbflow, dp->sb); - sbrec_logical_flow_set_logical_dp_group(sbflow, NULL); - break; - } + if (!hmapx_count(od_group)) { + sbrec_logical_flow_set_logical_dp_group(sbflow, NULL); return; } - dpg = ovn_dp_group_find(dp_groups, od, hash_int(hmapx_count(od), 0)); + ovs_assert(hmapx_count(od_group) != 1); + + dpg = ovn_dp_group_find(dp_groups, od_group, + hash_int(hmapx_count(od_group), 0)); ovs_assert(dpg != NULL); if (!dpg->dp_group) { dpg->dp_group = ovn_sb_insert_logical_dp_group(ctx, &dpg->map); } - sbrec_logical_flow_set_logical_datapath(sbflow, NULL); sbrec_logical_flow_set_logical_dp_group(sbflow, dpg->dp_group); } @@ -11365,28 +11366,55 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths, /* Collecting all unique datapath groups. */ struct hmap dp_groups = HMAP_INITIALIZER(&dp_groups); + struct hmapx single_dp_lflows = HMAPX_INITIALIZER(&single_dp_lflows); struct ovn_lflow *lflow; HMAP_FOR_EACH (lflow, hmap_node, &lflows) { - uint32_t hash = hash_int(hmapx_count(&lflow->od), 0); + uint32_t hash = hash_int(hmapx_count(&lflow->od_group), 0); struct ovn_dp_group *dpg; - if (hmapx_count(&lflow->od) == 1) { + ovs_assert(hmapx_count(&lflow->od_group)); + + if (hmapx_count(&lflow->od_group) == 1) { + /* There is only one datapath, so it should be moved out of the + * group to a single 'od'. */ + const struct hmapx_node *node; + HMAPX_FOR_EACH (node, &lflow->od_group) { + lflow->od = node->data; + break; + } + hmapx_clear(&lflow->od_group); + /* Logical flow should be re-hashed later to allow lookups. */ + hmapx_add(&single_dp_lflows, lflow); continue; } - dpg = ovn_dp_group_find(&dp_groups, &lflow->od, hash); + dpg = ovn_dp_group_find(&dp_groups, &lflow->od_group, hash); if (!dpg) { dpg = xzalloc(sizeof *dpg); - hmapx_clone(&dpg->map, &lflow->od); + hmapx_clone(&dpg->map, &lflow->od_group); hmap_insert(&dp_groups, &dpg->node, hash); } } + /* Adding datapath to the flow hash for logical flows that have only one, + * so they could be found by the southbound db record. */ + const struct hmapx_node *node; + uint32_t hash; + HMAPX_FOR_EACH (node, &single_dp_lflows) { + lflow = node->data; + hash = hmap_node_hash(&lflow->hmap_node); + hmap_remove(&lflows, &lflow->hmap_node); + hash = ovn_logical_flow_hash_datapath(&lflow->od->sb->header_.uuid, + hash); + hmap_insert(&lflows, &lflow->hmap_node, hash); + } + hmapx_destroy(&single_dp_lflows); + /* Push changes to the Logical_Flow table to database. */ const struct sbrec_logical_flow *sbflow, *next_sbflow; SBREC_LOGICAL_FLOW_FOR_EACH_SAFE (sbflow, next_sbflow, ctx->ovnsb_idl) { struct sbrec_logical_dp_group *dp_group = sbflow->logical_dp_group; - struct ovn_datapath **od; + struct ovn_datapath **od, *logical_datapath_od = NULL; int n_datapaths = 0; size_t i; @@ -11403,45 +11431,52 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths, struct sbrec_datapath_binding *dp = sbflow->logical_datapath; if (dp) { - od[n_datapaths] = ovn_datapath_from_sbrec(datapaths, dp); - if (od[n_datapaths] && !ovn_datapath_is_stale(od[n_datapaths])) { - n_datapaths++; + logical_datapath_od = ovn_datapath_from_sbrec(datapaths, dp); + if (logical_datapath_od + && ovn_datapath_is_stale(logical_datapath_od)) { + logical_datapath_od = NULL; } } - if (!n_datapaths) { + if (!n_datapaths && !logical_datapath_od) { /* This lflow has no valid logical datapaths. */ sbrec_logical_flow_delete(sbflow); free(od); continue; } - enum ovn_datapath_type dp_type = od[0]->nbs ? DP_SWITCH : DP_ROUTER; enum ovn_pipeline pipeline = !strcmp(sbflow->pipeline, "ingress") ? P_IN : P_OUT; + enum ovn_datapath_type dp_type; + if (n_datapaths) { + dp_type = od[0]->nbs ? DP_SWITCH : DP_ROUTER; + } else { + dp_type = logical_datapath_od->nbs ? DP_SWITCH : DP_ROUTER; + } lflow = ovn_lflow_find( - &lflows, ovn_stage_build(dp_type, pipeline, sbflow->table_id), + &lflows, logical_datapath_od, + ovn_stage_build(dp_type, pipeline, sbflow->table_id), sbflow->priority, sbflow->match, sbflow->actions, sbflow->hash); if (lflow) { /* This is a valid lflow. Checking if the datapath group needs * updates. */ - bool update_datapaths = false; + bool update_dp_group = false; - if (n_datapaths != hmapx_count(&lflow->od)) { - update_datapaths = true; + if (n_datapaths != hmapx_count(&lflow->od_group)) { + update_dp_group = true; } else { for (i = 0; i < n_datapaths; i++) { - if (od[i] && !hmapx_contains(&lflow->od, od[i])) { - update_datapaths = true; + if (od[i] && !hmapx_contains(&lflow->od_group, od[i])) { + update_dp_group = true; break; } } } - if (update_datapaths) { - ovn_sb_set_lflow_logical_datapaths(ctx, &dp_groups, - sbflow, &lflow->od); + if (update_dp_group) { + ovn_sb_set_lflow_logical_dp_group(ctx, &dp_groups, + sbflow, &lflow->od_group); } /* This lflow updated. Not needed anymore. */ ovn_lflow_destroy(&lflows, lflow); @@ -11457,8 +11492,11 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths, uint8_t table = ovn_stage_get_table(lflow->stage); sbflow = sbrec_logical_flow_insert(ctx->ovnsb_txn); - ovn_sb_set_lflow_logical_datapaths(ctx, &dp_groups, - sbflow, &lflow->od); + if (lflow->od) { + sbrec_logical_flow_set_logical_datapath(sbflow, lflow->od->sb); + } + ovn_sb_set_lflow_logical_dp_group(ctx, &dp_groups, + sbflow, &lflow->od_group); sbrec_logical_flow_set_pipeline(sbflow, pipeline); sbrec_logical_flow_set_table_id(sbflow, table); sbrec_logical_flow_set_priority(sbflow, lflow->priority);