From patchwork Tue Aug 30 22:24:31 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ryan Moats X-Patchwork-Id: 664306 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (archives.nicira.com [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id 3sP3504gMlz9s65 for ; Wed, 31 Aug 2016 08:25:56 +1000 (AEST) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id DD0A51088E; Tue, 30 Aug 2016 15:25:55 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx3v3.cudamail.com (mx3.cudamail.com [64.34.241.5]) by archives.nicira.com (Postfix) with ESMTPS id 333F21088E for ; Tue, 30 Aug 2016 15:25:54 -0700 (PDT) Received: from bar6.cudamail.com (localhost [127.0.0.1]) by mx3v3.cudamail.com (Postfix) with ESMTPS id B8E7B162375 for ; Tue, 30 Aug 2016 16:25:53 -0600 (MDT) X-ASG-Debug-ID: 1472595952-0b3237762d0dfa0001-byXFYA Received: from mx3-pf1.cudamail.com ([192.168.14.2]) by bar6.cudamail.com with ESMTP id zsmPtUhuYPuGkShp (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Tue, 30 Aug 2016 16:25:52 -0600 (MDT) X-Barracuda-Envelope-From: stack@tombstone-01.cloud.svl.ibm.com X-Barracuda-RBL-Trusted-Forwarder: 192.168.14.2 Received: from unknown (HELO mx0a-001b2d01.pphosted.com) (148.163.158.5) by mx3-pf1.cudamail.com with ESMTPS (AES256-SHA encrypted); 30 Aug 2016 22:25:51 -0000 Received-SPF: none (mx3-pf1.cudamail.com: domain at tombstone-01.cloud.svl.ibm.com does not designate permitted sender hosts) X-Barracuda-Apparent-Source-IP: 148.163.158.5 X-Barracuda-RBL-IP: 148.163.158.5 Received: from pps.filterd (m0098414.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.11/8.16.0.11) with SMTP id u7UMO8B9103963 for ; Tue, 30 Aug 2016 18:25:50 -0400 Received: from e32.co.us.ibm.com (e32.co.us.ibm.com [32.97.110.150]) by mx0b-001b2d01.pphosted.com with ESMTP id 255368wabp-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Tue, 30 Aug 2016 18:25:50 -0400 Received: from localhost by e32.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 30 Aug 2016 16:25:49 -0600 Received: from d03dlp01.boulder.ibm.com (9.17.202.177) by e32.co.us.ibm.com (192.168.1.132) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 30 Aug 2016 16:25:46 -0600 X-IBM-Helo: d03dlp01.boulder.ibm.com X-IBM-MailFrom: stack@tombstone-01.cloud.svl.ibm.com Received: from b01cxnp23032.gho.pok.ibm.com (b01cxnp23032.gho.pok.ibm.com [9.57.198.27]) by d03dlp01.boulder.ibm.com (Postfix) with ESMTP id 0875F1FF0046; Tue, 30 Aug 2016 16:25:27 -0600 (MDT) Received: from b01ledav005.gho.pok.ibm.com (b01ledav005.gho.pok.ibm.com [9.57.199.110]) by b01cxnp23032.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u7UMPotx16777524; Tue, 30 Aug 2016 22:25:50 GMT Received: from b01ledav005.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 43FE7AE03B; Tue, 30 Aug 2016 18:25:45 -0400 (EDT) Received: from localhost (unknown [9.30.183.40]) by b01ledav005.gho.pok.ibm.com (Postfix) with SMTP id CBB26AE03C; Tue, 30 Aug 2016 18:25:44 -0400 (EDT) Received: by localhost (Postfix, from userid 1000) id 3451262F71; Tue, 30 Aug 2016 22:25:44 +0000 (UTC) X-CudaMail-Envelope-Sender: stack@tombstone-01.cloud.svl.ibm.com From: Ryan Moats To: dev@openvswitch.org X-CudaMail-MID: CM-V1-829056050 X-CudaMail-DTE: 083016 X-CudaMail-Originating-IP: 148.163.158.5 Date: Tue, 30 Aug 2016 22:24:31 +0000 X-ASG-Orig-Subj: [##CM-V1-829056050##][PATCH 3/4] Unpersist data structures for address sets. X-Mailer: git-send-email 2.7.4 In-Reply-To: <1472595872-22887-1-git-send-email-rmoats@us.ibm.com> References: <1472595872-22887-1-git-send-email-rmoats@us.ibm.com> X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16083022-0004-0000-0000-000010414C3A X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00005679; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000184; SDB=6.00751801; UDB=6.00355374; IPR=6.00524607; BA=6.00004681; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00012522; XFM=3.00000011; UTC=2016-08-30 22:25:47 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16083022-0005-0000-0000-000078716F3A Message-Id: <1472595872-22887-4-git-send-email-rmoats@us.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2016-08-30_09:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=3 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1604210000 definitions=main-1608300214 X-GBUdb-Analysis: 0, 148.163.158.5, Ugly c=0.442133 p=-0.260274 Source Normal X-MessageSniffer-Rules: 0-0-0-23228-c X-Barracuda-Connect: UNKNOWN[192.168.14.2] X-Barracuda-Start-Time: 1472595952 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 X-Barracuda-Spam-Score: 1.10 X-Barracuda-Spam-Status: No, SCORE=1.10 using global scores of TAG_LEVEL=3.5 QUARANTINE_LEVEL=1000.0 KILL_LEVEL=4.0 tests=BSF_RULE7568M, BSF_SC5_MJ1963, RDNS_NONE X-Barracuda-Spam-Report: Code version 3.2, rules version 3.2.3.32471 Rule breakdown below pts rule name description ---- ---------------------- -------------------------------------------------- 0.50 BSF_RULE7568M Custom Rule 7568M 0.10 RDNS_NONE Delivered to trusted network by a host with no rDNS 0.50 BSF_SC5_MJ1963 Custom Rule MJ1963 Subject: [ovs-dev] [PATCH 3/4] Unpersist data structures for address sets. X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dev-bounces@openvswitch.org Sender: "dev" With the removal of incremental processing, it is no longer necessary to persist the data structures for storing address sets. Simplify things by removing this complexity. Side effect: fixed a memory leak in expr_macros_destroy that is evidenced by this change. This commit depends on f5d916cb ("ovn-controller: Back out incremental processing"). Signed-off-by: Ryan Moats --- ovn/controller/lflow.c | 166 ++++++++++++------------------------------------- ovn/lib/expr.c | 1 + 2 files changed, 42 insertions(+), 125 deletions(-) diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c index dc69047..5713c46 100644 --- a/ovn/controller/lflow.c +++ b/ovn/controller/lflow.c @@ -38,14 +38,10 @@ VLOG_DEFINE_THIS_MODULE(lflow); /* Contains "struct expr_symbol"s for fields supported by OVN lflows. */ static struct shash symtab; -/* Contains an internal expr datastructure that represents an address set. */ -static struct shash expr_address_sets; - void lflow_init(void) { ovn_init_symtab(&symtab); - shash_init(&expr_address_sets); } /* Details of an address set currently in address_sets. We keep a cached @@ -56,54 +52,6 @@ struct address_set { size_t n_addresses; }; -/* struct address_set instances for address sets currently in the symtab, - * hashed on the address set name. */ -static struct shash local_address_sets = SHASH_INITIALIZER(&local_address_sets); - -static int -addr_cmp(const void *p1, const void *p2) -{ - const char *s1 = p1; - const char *s2 = p2; - return strcmp(s1, s2); -} - -/* Return true if the address sets match, false otherwise. */ -static bool -address_sets_match(const struct address_set *addr_set, - const struct sbrec_address_set *addr_set_rec) -{ - char **addrs1; - char **addrs2; - - if (addr_set->n_addresses != addr_set_rec->n_addresses) { - return false; - } - size_t n_addresses = addr_set->n_addresses; - - addrs1 = xmemdup(addr_set->addresses, - n_addresses * sizeof addr_set->addresses[0]); - addrs2 = xmemdup(addr_set_rec->addresses, - n_addresses * sizeof addr_set_rec->addresses[0]); - - qsort(addrs1, n_addresses, sizeof *addrs1, addr_cmp); - qsort(addrs2, n_addresses, sizeof *addrs2, addr_cmp); - - bool res = true; - size_t i; - for (i = 0; i < n_addresses; i++) { - if (strcmp(addrs1[i], addrs2[i])) { - res = false; - break; - } - } - - free(addrs1); - free(addrs2); - - return res; -} - static void address_set_destroy(struct address_set *addr_set) { @@ -118,79 +66,34 @@ address_set_destroy(struct address_set *addr_set) } static void -update_address_sets(struct controller_ctx *ctx) -{ - /* Remember the names of all address sets currently in expr_address_sets - * so we can detect address sets that have been deleted. */ - struct sset cur_addr_set_names = SSET_INITIALIZER(&cur_addr_set_names); - - struct shash_node *node; - SHASH_FOR_EACH (node, &local_address_sets) { - sset_add(&cur_addr_set_names, node->name); - } +update_address_sets(struct controller_ctx *ctx, + struct shash *local_address_sets_p, + struct shash *expr_address_sets_p) +{ /* Iterate address sets in the southbound database. Create and update the * corresponding symtab entries as necessary. */ const struct sbrec_address_set *addr_set_rec; SBREC_ADDRESS_SET_FOR_EACH (addr_set_rec, ctx->ovnsb_idl) { - struct address_set *addr_set = - shash_find_data(&local_address_sets, addr_set_rec->name); - - bool create_set = false; - if (addr_set) { - /* This address set has already been added. We must determine - * if the symtab entry needs to be updated due to a change. */ - sset_find_and_delete(&cur_addr_set_names, addr_set_rec->name); - if (!address_sets_match(addr_set, addr_set_rec)) { - shash_find_and_delete(&local_address_sets, addr_set_rec->name); - expr_macros_remove(&expr_address_sets, addr_set_rec->name); - address_set_destroy(addr_set); - addr_set = NULL; - create_set = true; + /* Create a symbol that resolves to the full set of addresses. + * Store it in address_sets to remember that we created this + * symbol. */ + struct address_set *addr_set = xzalloc(sizeof *addr_set); + addr_set->n_addresses = addr_set_rec->n_addresses; + if (addr_set_rec->n_addresses) { + addr_set->addresses = xmalloc(addr_set_rec->n_addresses + * sizeof addr_set->addresses[0]); + size_t i; + for (i = 0; i < addr_set_rec->n_addresses; i++) { + addr_set->addresses[i] = xstrdup(addr_set_rec->addresses[i]); } - } else { - /* This address set is not yet in the symtab, so add it. */ - create_set = true; } + shash_add(local_address_sets_p, addr_set_rec->name, addr_set); - if (create_set) { - /* The address set is either new or has changed. Create a symbol - * that resolves to the full set of addresses. Store it in - * address_sets to remember that we created this symbol. */ - addr_set = xzalloc(sizeof *addr_set); - addr_set->n_addresses = addr_set_rec->n_addresses; - if (addr_set_rec->n_addresses) { - addr_set->addresses = xmalloc(addr_set_rec->n_addresses - * sizeof addr_set->addresses[0]); - size_t i; - for (i = 0; i < addr_set_rec->n_addresses; i++) { - addr_set->addresses[i] = xstrdup(addr_set_rec->addresses[i]); - } - } - shash_add(&local_address_sets, addr_set_rec->name, addr_set); - - expr_macros_add(&expr_address_sets, addr_set_rec->name, - (const char * const *) addr_set->addresses, - addr_set->n_addresses); - } - } - - /* Anything remaining in cur_addr_set_names refers to an address set that - * has been deleted from the southbound database. We should delete - * the corresponding symtab entry. */ - const char *cur_node, *next_node; - SSET_FOR_EACH_SAFE (cur_node, next_node, &cur_addr_set_names) { - expr_macros_remove(&expr_address_sets, cur_node); - - struct address_set *addr_set - = shash_find_and_delete(&local_address_sets, cur_node); - address_set_destroy(addr_set); - - struct sset_node *sset_node = SSET_NODE_FROM_NAME(cur_node); - sset_delete(&cur_addr_set_names, sset_node); + expr_macros_add(expr_address_sets_p, addr_set_rec->name, + (const char * const *) addr_set->addresses, + addr_set->n_addresses); } - - sset_destroy(&cur_addr_set_names); } struct lookup_port_aux { @@ -209,7 +112,8 @@ static void consider_logical_flow(const struct lport_index *lports, struct hmap *dhcp_opts_p, struct hmap *dhcpv6_opts_p, uint32_t *conj_id_ofs_p, - struct hmap *flow_table); + struct hmap *flow_table, + struct shash *expr_address_sets_p); static bool lookup_port_cb(const void *aux_, const char *port_name, unsigned int *portp) @@ -248,7 +152,8 @@ add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports, const struct hmap *patched_datapaths, struct group_table *group_table, const struct simap *ct_zones, - struct hmap *flow_table) + struct hmap *flow_table, + struct shash *expr_address_sets_p) { uint32_t conj_id_ofs = 1; const struct sbrec_logical_flow *lflow; @@ -272,7 +177,7 @@ add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports, consider_logical_flow(lports, mcgroups, lflow, local_datapaths, patched_datapaths, group_table, ct_zones, &dhcp_opts, &dhcpv6_opts, &conj_id_ofs, - flow_table); + flow_table, expr_address_sets_p); } dhcp_opts_destroy(&dhcp_opts); @@ -290,7 +195,8 @@ consider_logical_flow(const struct lport_index *lports, struct hmap *dhcp_opts_p, struct hmap *dhcpv6_opts_p, uint32_t *conj_id_ofs_p, - struct hmap *flow_table) + struct hmap *flow_table, + struct shash *expr_address_sets_p) { /* Determine translation of logical table IDs to physical table IDs. */ bool ingress = !strcmp(lflow->pipeline, "ingress"); @@ -399,7 +305,7 @@ consider_logical_flow(const struct lport_index *lports, struct expr *expr; expr = expr_parse_string(lflow->match, &symtab, - &expr_address_sets, &error); + expr_address_sets_p, &error); if (!error) { if (prereqs) { expr = expr_combine(EXPR_T_AND, expr, prereqs); @@ -555,10 +461,22 @@ lflow_run(struct controller_ctx *ctx, const struct lport_index *lports, const struct simap *ct_zones, struct hmap *flow_table) { - update_address_sets(ctx); + struct shash local_address_sets = SHASH_INITIALIZER(&local_address_sets); + struct shash expr_address_sets = SHASH_INITIALIZER(&expr_address_sets); + + update_address_sets(ctx, &local_address_sets, &expr_address_sets); add_logical_flows(ctx, lports, mcgroups, local_datapaths, - patched_datapaths, group_table, ct_zones, flow_table); + patched_datapaths, group_table, ct_zones, flow_table, + &expr_address_sets); add_neighbor_flows(ctx, lports, flow_table); + + struct shash_node *node; + SHASH_FOR_EACH (node, &local_address_sets) { + address_set_destroy(node->data); + } + shash_destroy(&local_address_sets); + expr_macros_destroy(&expr_address_sets); + shash_destroy(&expr_address_sets); } void @@ -566,6 +484,4 @@ lflow_destroy(void) { expr_symtab_destroy(&symtab); shash_destroy(&symtab); - expr_macros_destroy(&expr_address_sets); - shash_destroy(&expr_address_sets); } diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c index e0a14ec..4ae6b0b 100644 --- a/ovn/lib/expr.c +++ b/ovn/lib/expr.c @@ -964,6 +964,7 @@ expr_macros_destroy(struct shash *macros) shash_delete(macros, node); expr_constant_set_destroy(cs); + free(cs); } }