From patchwork Tue Jan 21 14:40:50 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Numan Siddique X-Patchwork-Id: 1226614 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.133; helo=hemlock.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 hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 482B7059S8z9sRf for ; Wed, 22 Jan 2020 01:41:16 +1100 (AEDT) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 0C7FC8780D; Tue, 21 Jan 2020 14:41:15 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id zz4n2MDQqGdx; Tue, 21 Jan 2020 14:41:09 +0000 (UTC) Received: from lists.linuxfoundation.org (lf-lists.osuosl.org [140.211.9.56]) by hemlock.osuosl.org (Postfix) with ESMTP id CBE7887553; Tue, 21 Jan 2020 14:41:09 +0000 (UTC) Received: from lf-lists.osuosl.org (localhost [127.0.0.1]) by lists.linuxfoundation.org (Postfix) with ESMTP id C3EC2C0176; Tue, 21 Jan 2020 14:41:09 +0000 (UTC) X-Original-To: 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 1CBC4C0174 for ; Tue, 21 Jan 2020 14:41:09 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 182B6203B1 for ; Tue, 21 Jan 2020 14:41:09 +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 13fZvIJj8HsG for ; Tue, 21 Jan 2020 14:41:03 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from relay12.mail.gandi.net (relay12.mail.gandi.net [217.70.178.232]) by silver.osuosl.org (Postfix) with ESMTPS id BBF7A204F8 for ; Tue, 21 Jan 2020 14:41:00 +0000 (UTC) Received: from nummac.local (unknown [116.75.117.25]) (Authenticated sender: numans@ovn.org) by relay12.mail.gandi.net (Postfix) with ESMTPSA id CDFBD200002; Tue, 21 Jan 2020 14:40:57 +0000 (UTC) From: numans@ovn.org To: dev@openvswitch.org Date: Tue, 21 Jan 2020 20:10:50 +0530 Message-Id: <20200121144050.245868-1-numans@ovn.org> X-Mailer: git-send-email 2.24.1 In-Reply-To: <20200121143955.245616-1-numans@ovn.org> References: <20200121143955.245616-1-numans@ovn.org> MIME-Version: 1.0 Subject: [ovs-dev] [PATCH ovn v2 2/2] ovn-controller: Cache logical flow expr tree for each lflow. 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" From: Numan Siddique This patch caches the logical flow expr tree for each logical flow. This cache is stored as an hashmap in the output_flow engine data. When a logical flow is deleted, the expr tree for that lflow is deleted in the flow_output_sb_logical_flow_handler(). Below are the details of the OVN resource in my setup No of logical switches - 49 No of logical ports - 1191 No of logical routers - 7 No of logical ports - 51 No of ACLs - 1221 No of Logical flows - 664736 On a chassis hosting 7 distributed router ports and around 1090 port bindings, the no of OVS rules was 921162. Without this patch, the function add_logical_flows() took ~15 seconds. And with this patch it took ~10 seconds. There is a good 34% improvement in logical flow processing. Acked-by: Mark Michelson Signed-off-by: Numan Siddique Acked-by: Han Zhou --- controller/lflow.c | 192 ++++++++++++++++++++++++++---------- controller/lflow.h | 9 +- controller/ovn-controller.c | 16 ++- 3 files changed, 160 insertions(+), 57 deletions(-) diff --git a/controller/lflow.c b/controller/lflow.c index 93ec53a1c..997c59662 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -75,11 +75,13 @@ static bool consider_logical_flow( const struct shash *port_groups, const struct sset *active_tunnels, const struct sset *local_lport_ids, + bool delete_expr_from_cache, struct ovn_desired_flow_table *, struct ovn_extend_table *group_table, struct ovn_extend_table *meter_table, struct lflow_resource_ref *lfrr, - uint32_t *conj_id_ofs); + uint32_t *conj_id_ofs, + struct hmap *lflow_expr_cache); static bool lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp) @@ -255,6 +257,66 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr, free(lfrn); } +/* Represents expr tree for the lflow. We don't store the + * match in this structure because - + * - Whenever a flow match or action needs to be modified, + * ovn-northd deletes the existing flow in the logical_flow + * table and adds a new one. + * We may want to revisit this and store match incase the behavior + * of ovn-northd changes. + */ +struct lflow_expr { + struct hmap_node node; + struct uuid lflow_uuid; /* key */ + struct expr *expr; +}; + +static void +lflow_expr_add(struct hmap *lflow_expr_cache, + const struct sbrec_logical_flow *lflow, + struct expr *lflow_expr) +{ + struct lflow_expr *le = xmalloc(sizeof *le); + le->lflow_uuid = lflow->header_.uuid; + le->expr = lflow_expr; + hmap_insert(lflow_expr_cache, &le->node, uuid_hash(&le->lflow_uuid)); +} + +static struct lflow_expr * +lflow_expr_get(struct hmap *lflow_expr_cache, + const struct sbrec_logical_flow *lflow) +{ + struct lflow_expr *le; + size_t hash = uuid_hash(&lflow->header_.uuid); + HMAP_FOR_EACH_WITH_HASH (le, node, hash, lflow_expr_cache) { + if (uuid_equals(&le->lflow_uuid, &lflow->header_.uuid)) { + return le; + } + } + + return NULL; +} + +static void +lflow_expr_delete(struct hmap *lflow_expr_cache, struct lflow_expr *le) +{ + hmap_remove(lflow_expr_cache, &le->node); + expr_destroy(le->expr); + free(le); +} + +void +lflow_expr_destroy(struct hmap *lflow_expr_cache) +{ + struct lflow_expr *le; + HMAP_FOR_EACH_POP (le, node, lflow_expr_cache) { + expr_destroy(le->expr); + free(le); + } + + hmap_destroy(lflow_expr_cache); +} + /* Adds the logical flows from the Logical_Flow table to flow tables. */ static void add_logical_flows( @@ -273,7 +335,8 @@ add_logical_flows( struct ovn_extend_table *group_table, struct ovn_extend_table *meter_table, struct lflow_resource_ref *lfrr, - uint32_t *conj_id_ofs) + uint32_t *conj_id_ofs, + struct hmap *lflow_expr_cache) { const struct sbrec_logical_flow *lflow; @@ -306,9 +369,9 @@ add_logical_flows( chassis, &dhcp_opts, &dhcpv6_opts, &nd_ra_opts, &controller_event_opts, addr_sets, port_groups, - active_tunnels, local_lport_ids, + active_tunnels, local_lport_ids, false, flow_table, group_table, meter_table, - lfrr, conj_id_ofs)) { + lfrr, conj_id_ofs, lflow_expr_cache)) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5); VLOG_ERR_RL(&rl, "Conjunction id overflow when processing lflow " UUID_FMT, UUID_ARGS(&lflow->header_.uuid)); @@ -338,7 +401,8 @@ lflow_handle_changed_flows( struct ovn_extend_table *group_table, struct ovn_extend_table *meter_table, struct lflow_resource_ref *lfrr, - uint32_t *conj_id_ofs) + uint32_t *conj_id_ofs, + struct hmap *lflow_expr_cache) { bool ret = true; const struct sbrec_logical_flow *lflow; @@ -373,6 +437,12 @@ lflow_handle_changed_flows( ofctrl_remove_flows(flow_table, &lflow->header_.uuid); /* Delete entries from lflow resource reference. */ lflow_resource_destroy_lflow(lfrr, &lflow->header_.uuid); + + /* Delete the expr cache for this lflow. */ + struct lflow_expr *le = lflow_expr_get(lflow_expr_cache, lflow); + if (le) { + lflow_expr_delete(lflow_expr_cache, le); + } } } @@ -399,9 +469,9 @@ lflow_handle_changed_flows( chassis, &dhcp_opts, &dhcpv6_opts, &nd_ra_opts, &controller_event_opts, addr_sets, port_groups, - active_tunnels, local_lport_ids, + active_tunnels, local_lport_ids, true, flow_table, group_table, meter_table, - lfrr, conj_id_ofs)) { + lfrr, conj_id_ofs, lflow_expr_cache)) { ret = false; break; } @@ -434,6 +504,7 @@ lflow_handle_changed_ref( struct ovn_extend_table *meter_table, struct lflow_resource_ref *lfrr, uint32_t *conj_id_ofs, + struct hmap *lflow_expr_cache, bool *changed) { struct ref_lflow_node *rlfn = ref_lflow_lookup(&lfrr->ref_lflow_table, @@ -503,9 +574,9 @@ lflow_handle_changed_ref( chassis, &dhcp_opts, &dhcpv6_opts, &nd_ra_opts, &controller_event_opts, addr_sets, port_groups, - active_tunnels, local_lport_ids, + active_tunnels, local_lport_ids, true, flow_table, group_table, meter_table, - lfrr, conj_id_ofs)) { + lfrr, conj_id_ofs, lflow_expr_cache)) { ret = false; break; } @@ -551,11 +622,13 @@ consider_logical_flow( const struct shash *port_groups, const struct sset *active_tunnels, const struct sset *local_lport_ids, + bool delete_expr_from_cache, struct ovn_desired_flow_table *flow_table, struct ovn_extend_table *group_table, struct ovn_extend_table *meter_table, struct lflow_resource_ref *lfrr, - uint32_t *conj_id_ofs) + uint32_t *conj_id_ofs, + struct hmap *lflow_expr_cache) { /* Determine translation of logical table IDs to physical table IDs. */ bool ingress = !strcmp(lflow->pipeline, "ingress"); @@ -613,59 +686,77 @@ consider_logical_flow( /* Translate OVN match into table of OpenFlow matches. */ struct hmap matches; - struct expr *expr; + struct expr *expr = NULL; struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref); struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref); - expr = expr_parse_string(lflow->match, &symtab, addr_sets, port_groups, - &addr_sets_ref, &port_groups_ref, &error); - const char *addr_set_name; - SSET_FOR_EACH (addr_set_name, &addr_sets_ref) { - lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name, - &lflow->header_.uuid); - } - const char *port_group_name; - SSET_FOR_EACH (port_group_name, &port_groups_ref) { - lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name, - &lflow->header_.uuid); - } - sset_destroy(&addr_sets_ref); - sset_destroy(&port_groups_ref); - - if (!error) { - if (prereqs) { - expr = expr_combine(EXPR_T_AND, expr, prereqs); - prereqs = NULL; + struct lflow_expr *le = lflow_expr_get(lflow_expr_cache, lflow); + if (le) { + if (delete_expr_from_cache) { + lflow_expr_delete(lflow_expr_cache, le); + } else { + expr = le->expr; } - expr = expr_annotate(expr, &symtab, &error); } - if (error) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); - VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s", - lflow->match, error); - expr_destroy(prereqs); - free(error); - ovnacts_free(ovnacts.data, ovnacts.size); - ofpbuf_uninit(&ovnacts); - return true; + + if (!expr) { + expr = expr_parse_string(lflow->match, &symtab, addr_sets, port_groups, + &addr_sets_ref, &port_groups_ref, &error); + const char *addr_set_name; + SSET_FOR_EACH (addr_set_name, &addr_sets_ref) { + lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name, + &lflow->header_.uuid); + } + const char *port_group_name; + SSET_FOR_EACH (port_group_name, &port_groups_ref) { + lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name, + &lflow->header_.uuid); + } + sset_destroy(&addr_sets_ref); + sset_destroy(&port_groups_ref); + + if (!error) { + if (prereqs) { + expr = expr_combine(EXPR_T_AND, expr, prereqs); + prereqs = NULL; + } + expr = expr_annotate(expr, &symtab, &error); + } + if (error) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s", + lflow->match, error); + expr_destroy(prereqs); + free(error); + ovnacts_free(ovnacts.data, ovnacts.size); + ofpbuf_uninit(&ovnacts); + return true; + } + + expr = expr_simplify(expr); + expr = expr_normalize(expr); + + lflow_expr_add(lflow_expr_cache, lflow, expr); } - struct lookup_port_aux aux = { - .sbrec_multicast_group_by_name_datapath - = sbrec_multicast_group_by_name_datapath, - .sbrec_port_binding_by_name = sbrec_port_binding_by_name, - .dp = lflow->logical_datapath - }; struct condition_aux cond_aux = { .sbrec_port_binding_by_name = sbrec_port_binding_by_name, .chassis = chassis, .active_tunnels = active_tunnels, }; - expr = expr_simplify(expr); - expr = expr_normalize(expr); + + expr = expr_clone(expr); expr = expr_evaluate_condition(expr, is_chassis_resident_cb, &cond_aux); + + struct lookup_port_aux aux = { + .sbrec_multicast_group_by_name_datapath + = sbrec_multicast_group_by_name_datapath, + .sbrec_port_binding_by_name = sbrec_port_binding_by_name, + .dp = lflow->logical_datapath + }; uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, &aux, &matches); + expr_destroy(expr); if (hmap_is_empty(&matches)) { @@ -907,7 +998,8 @@ lflow_run(struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath, struct ovn_extend_table *group_table, struct ovn_extend_table *meter_table, struct lflow_resource_ref *lfrr, - uint32_t *conj_id_ofs) + uint32_t *conj_id_ofs, + struct hmap *lflow_expr_cache) { COVERAGE_INC(lflow_run); @@ -916,7 +1008,7 @@ lflow_run(struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath, dhcpv6_options_table, logical_flow_table, local_datapaths, chassis, addr_sets, port_groups, active_tunnels, local_lport_ids, flow_table, group_table, - meter_table, lfrr, conj_id_ofs); + meter_table, lfrr, conj_id_ofs, lflow_expr_cache); add_neighbor_flows(sbrec_port_binding_by_name, mac_binding_table, flow_table); } diff --git a/controller/lflow.h b/controller/lflow.h index abdc55e96..a2fa087f8 100644 --- a/controller/lflow.h +++ b/controller/lflow.h @@ -132,7 +132,8 @@ void lflow_run(struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath, struct ovn_extend_table *group_table, struct ovn_extend_table *meter_table, struct lflow_resource_ref *, - uint32_t *conj_id_ofs); + uint32_t *conj_id_ofs, + struct hmap *lflow_expr_cache); bool lflow_handle_changed_flows( struct ovsdb_idl_index *sbrec_multicast_group_by_name_datapath, @@ -150,7 +151,8 @@ bool lflow_handle_changed_flows( struct ovn_extend_table *group_table, struct ovn_extend_table *meter_table, struct lflow_resource_ref *, - uint32_t *conj_id_ofs); + uint32_t *conj_id_ofs, + struct hmap *lflow_expr_cache); bool lflow_handle_changed_ref( enum ref_type, @@ -171,6 +173,7 @@ bool lflow_handle_changed_ref( struct ovn_extend_table *meter_table, struct lflow_resource_ref *, uint32_t *conj_id_ofs, + struct hmap *lflow_expr_cache, bool *changed); void lflow_handle_changed_neighbors( @@ -180,4 +183,6 @@ void lflow_handle_changed_neighbors( void lflow_destroy(void); +void lflow_expr_destroy(struct hmap *lflow_expr_cache); + #endif /* controller/lflow.h */ diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c index 17744d416..31ce1107c 100644 --- a/controller/ovn-controller.c +++ b/controller/ovn-controller.c @@ -1211,6 +1211,9 @@ struct ed_type_flow_output { uint32_t conj_id_ofs; /* lflow resource cross reference */ struct lflow_resource_ref lflow_resource_ref; + + /* Cache of lflow expr tree. */ + struct hmap lflow_expr_cache; }; static void * @@ -1224,6 +1227,7 @@ en_flow_output_init(struct engine_node *node OVS_UNUSED, ovn_extend_table_init(&data->meter_table); data->conj_id_ofs = 1; lflow_resource_init(&data->lflow_resource_ref); + hmap_init(&data->lflow_expr_cache); return data; } @@ -1235,6 +1239,7 @@ en_flow_output_cleanup(void *data) ovn_extend_table_destroy(&flow_output_data->group_table); ovn_extend_table_destroy(&flow_output_data->meter_table); lflow_resource_destroy(&flow_output_data->lflow_resource_ref); + lflow_expr_destroy(&flow_output_data->lflow_expr_cache); } static void @@ -1335,7 +1340,8 @@ en_flow_output_run(struct engine_node *node, void *data) chassis, local_datapaths, addr_sets, port_groups, active_tunnels, local_lport_ids, flow_table, group_table, meter_table, lfrr, - conj_id_ofs); + conj_id_ofs, + &fo->lflow_expr_cache); struct sbrec_multicast_group_table *multicast_group_table = (struct sbrec_multicast_group_table *)EN_OVSDB_GET( @@ -1436,7 +1442,7 @@ flow_output_sb_logical_flow_handler(struct engine_node *node, void *data) local_datapaths, chassis, addr_sets, port_groups, active_tunnels, local_lport_ids, flow_table, group_table, meter_table, lfrr, - conj_id_ofs); + conj_id_ofs, &fo->lflow_expr_cache); engine_set_node_state(node, EN_UPDATED); return handled; @@ -1721,7 +1727,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, local_datapaths, chassis, addr_sets, port_groups, active_tunnels, local_lport_ids, flow_table, group_table, meter_table, lfrr, - conj_id_ofs, &changed)) { + conj_id_ofs, &fo->lflow_expr_cache, &changed)) { return false; } if (changed) { @@ -1736,7 +1742,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, local_datapaths, chassis, addr_sets, port_groups, active_tunnels, local_lport_ids, flow_table, group_table, meter_table, lfrr, - conj_id_ofs, &changed)) { + conj_id_ofs, &fo->lflow_expr_cache, &changed)) { return false; } if (changed) { @@ -1751,7 +1757,7 @@ _flow_output_resource_ref_handler(struct engine_node *node, void *data, local_datapaths, chassis, addr_sets, port_groups, active_tunnels, local_lport_ids, flow_table, group_table, meter_table, lfrr, - conj_id_ofs, &changed)) { + conj_id_ofs, &fo->lflow_expr_cache, &changed)) { return false; } if (changed) {