| Message ID | 20160831173834.GU16586@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
Ben Pfaff <blp@ovn.org> wrote on 08/31/2016 12:38:34 PM: > From: Ben Pfaff <blp@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 08/31/2016 12:38 PM > Subject: Re: [ovs-dev,v2,3/4] Unpersist data structures for address sets. > > On Wed, Aug 31, 2016 at 03:22:45PM +0000, Ryan Moats wrote: > > 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. > > > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > > I think that this was still doing a lot more work than necessary. The > "struct address_set"s were being created and destroyed but not used for > anything in between. > > Here's my proposal. Comments? > > --8<--------------------------cut here-------------------------->8-- > > From: Ryan Moats <rmoats@us.ibm.com> > Date: Wed, 31 Aug 2016 15:22:45 +0000 > Subject: [PATCH] Unpersist data structures for address sets. > > 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. > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > Signed-off-by: Ben Pfaff <blp@ovn.org> > --- > ovn/controller/lflow.c | 176 ++++++ > +------------------------------------------ > ovn/lib/expr.c | 1 + > 2 files changed, 25 insertions(+), 152 deletions(-) > > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c > index a9c3137..efac5b3 100644 > --- a/ovn/controller/lflow.c > +++ b/ovn/controller/lflow.c > @@ -38,159 +38,24 @@ 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 > - * copy of sets still in their string form here to make it easier to compare > - * with the current values in the OVN_Southbound database. */ > -struct address_set { > - char **addresses; > - 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; > } > > +/* Iterate address sets in the southbound database. Create and update the > + * corresponding symtab entries as necessary. */ > static void > -address_set_destroy(struct address_set *addr_set) > -{ > - size_t i; > - for (i = 0; i < addr_set->n_addresses; i++) { > - free(addr_set->addresses[i]); > - } > - if (addr_set->n_addresses) { > - free(addr_set->addresses); > - } > - free(addr_set); > -} > +update_address_sets(struct controller_ctx *ctx, > + struct shash *expr_address_sets_p) > > -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); > + const struct sbrec_address_set *as; > + SBREC_ADDRESS_SET_FOR_EACH (as, ctx->ovnsb_idl) { > + expr_macros_add(expr_address_sets_p, as->name, > + (const char *const *) as->addresses, > as->n_addresses); > } > - > - /* Iterate address sets in the southbound database. Create andupdate 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; > - } > - } else { > - /* This address set is not yet in the symtab, so add it. */ > - create_set = true; > - } > - > - 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); > - } > - > - sset_destroy(&cur_addr_set_names); > } > > struct lookup_port_aux { > @@ -209,7 +74,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 +114,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 +139,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 +157,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 +267,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); > @@ -554,10 +422,16 @@ 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 expr_address_sets = SHASH_INITIALIZER (&expr_address_sets); > + > + update_address_sets(ctx, &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); > + > + expr_macros_destroy(&expr_address_sets); > + shash_destroy(&expr_address_sets); > } > > void > @@ -565,6 +439,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); > } > } > > -- > 2.1.3 > Assuming that it compiles and all the unit test cases pass - roll with it!! Ryan
"dev" <dev-bounces@openvswitch.org> wrote on 08/31/2016 12:53:27 PM: > From: Ryan Moats/Omaha/IBM@IBMUS > To: Ben Pfaff <blp@ovn.org> > Cc: dev@openvswitch.org > Date: 08/31/2016 12:53 PM > Subject: Re: [ovs-dev] [ovs-dev, v2, 3/4] Unpersist data structures > for address sets. > Sent by: "dev" <dev-bounces@openvswitch.org> > > > Ben Pfaff <blp@ovn.org> wrote on 08/31/2016 12:38:34 PM: > > > From: Ben Pfaff <blp@ovn.org> > > To: Ryan Moats/Omaha/IBM@IBMUS > > Cc: dev@openvswitch.org > > Date: 08/31/2016 12:38 PM > > Subject: Re: [ovs-dev,v2,3/4] Unpersist data structures for address sets. > > > > On Wed, Aug 31, 2016 at 03:22:45PM +0000, Ryan Moats wrote: > > > 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. > > > > > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > > > > I think that this was still doing a lot more work than necessary. The > > "struct address_set"s were being created and destroyed but not used for > > anything in between. > > > > Here's my proposal. Comments? > > > > --8<--------------------------cut here-------------------------->8-- > > > > From: Ryan Moats <rmoats@us.ibm.com> > > Date: Wed, 31 Aug 2016 15:22:45 +0000 > > Subject: [PATCH] Unpersist data structures for address sets. > > > > 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. > > > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > --- > > ovn/controller/lflow.c | 176 ++++++ > > +------------------------------------------ > > ovn/lib/expr.c | 1 + > > 2 files changed, 25 insertions(+), 152 deletions(-) > > > > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c > > index a9c3137..efac5b3 100644 > > --- a/ovn/controller/lflow.c > > +++ b/ovn/controller/lflow.c > > @@ -38,159 +38,24 @@ 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 > > - * copy of sets still in their string form here to make it easier to > compare > > - * with the current values in the OVN_Southbound database. */ > > -struct address_set { > > - char **addresses; > > - 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; > > } > > > > +/* Iterate address sets in the southbound database. Create and update > the > > + * corresponding symtab entries as necessary. */ > > static void > > -address_set_destroy(struct address_set *addr_set) > > -{ > > - size_t i; > > - for (i = 0; i < addr_set->n_addresses; i++) { > > - free(addr_set->addresses[i]); > > - } > > - if (addr_set->n_addresses) { > > - free(addr_set->addresses); > > - } > > - free(addr_set); > > -} > > +update_address_sets(struct controller_ctx *ctx, > > + struct shash *expr_address_sets_p) > > > > -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); > > + const struct sbrec_address_set *as; > > + SBREC_ADDRESS_SET_FOR_EACH (as, ctx->ovnsb_idl) { > > + expr_macros_add(expr_address_sets_p, as->name, > > + (const char *const *) as->addresses, > > as->n_addresses); > > } > > - > > - /* Iterate address sets in the southbound database. Create > andupdate 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; > > - } > > - } else { > > - /* This address set is not yet in the symtab, so add it. */ > > - create_set = true; > > - } > > - > > - 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); > > - } > > - > > - sset_destroy(&cur_addr_set_names); > > } > > > > struct lookup_port_aux { > > @@ -209,7 +74,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 +114,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 +139,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 +157,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 +267,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); > > @@ -554,10 +422,16 @@ 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 expr_address_sets = SHASH_INITIALIZER > (&expr_address_sets); > > + > > + update_address_sets(ctx, &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); > > + > > + expr_macros_destroy(&expr_address_sets); > > + shash_destroy(&expr_address_sets); > > } > > > > void > > @@ -565,6 +439,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); > > } > > } > > > > -- > > 2.1.3 > > > > Assuming that it compiles and all the unit test cases pass - roll with it!! Er, let me say that more formally... Assuming that it compiles and all the unit test cases pass... Acked-by: Ryan Moats <rmoats@us.ibm.com> [Can I do that :) ]
On Wed, Aug 31, 2016 at 01:51:51PM -0500, Ryan Moats wrote: > > > "dev" <dev-bounces@openvswitch.org> wrote on 08/31/2016 12:53:27 PM: > > > From: Ryan Moats/Omaha/IBM@IBMUS > > To: Ben Pfaff <blp@ovn.org> > > Cc: dev@openvswitch.org > > Date: 08/31/2016 12:53 PM > > Subject: Re: [ovs-dev] [ovs-dev, v2, 3/4] Unpersist data structures > > for address sets. > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > Ben Pfaff <blp@ovn.org> wrote on 08/31/2016 12:38:34 PM: > > > > > From: Ben Pfaff <blp@ovn.org> > > > To: Ryan Moats/Omaha/IBM@IBMUS > > > Cc: dev@openvswitch.org > > > Date: 08/31/2016 12:38 PM > > > Subject: Re: [ovs-dev,v2,3/4] Unpersist data structures for address > sets. > > > > > > On Wed, Aug 31, 2016 at 03:22:45PM +0000, Ryan Moats wrote: > > > > 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. > > > > > > > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > > > > > > I think that this was still doing a lot more work than necessary. The > > > "struct address_set"s were being created and destroyed but not used for > > > anything in between. > > > > > > Here's my proposal. Comments? > > > > > > --8<--------------------------cut here-------------------------->8-- > > > > > > From: Ryan Moats <rmoats@us.ibm.com> > > > Date: Wed, 31 Aug 2016 15:22:45 +0000 > > > Subject: [PATCH] Unpersist data structures for address sets. > > > > > > 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. > > > > > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > > --- > > > ovn/controller/lflow.c | 176 ++++++ > > > +------------------------------------------ > > > ovn/lib/expr.c | 1 + > > > 2 files changed, 25 insertions(+), 152 deletions(-) > > > > > > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c > > > index a9c3137..efac5b3 100644 > > > --- a/ovn/controller/lflow.c > > > +++ b/ovn/controller/lflow.c > > > @@ -38,159 +38,24 @@ 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 > > > - * copy of sets still in their string form here to make it easier to > > compare > > > - * with the current values in the OVN_Southbound database. */ > > > -struct address_set { > > > - char **addresses; > > > - 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; > > > } > > > > > > +/* Iterate address sets in the southbound database. Create and update > > the > > > + * corresponding symtab entries as necessary. */ > > > static void > > > -address_set_destroy(struct address_set *addr_set) > > > -{ > > > - size_t i; > > > - for (i = 0; i < addr_set->n_addresses; i++) { > > > - free(addr_set->addresses[i]); > > > - } > > > - if (addr_set->n_addresses) { > > > - free(addr_set->addresses); > > > - } > > > - free(addr_set); > > > -} > > > +update_address_sets(struct controller_ctx *ctx, > > > + struct shash *expr_address_sets_p) > > > > > > -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); > > > + const struct sbrec_address_set *as; > > > + SBREC_ADDRESS_SET_FOR_EACH (as, ctx->ovnsb_idl) { > > > + expr_macros_add(expr_address_sets_p, as->name, > > > + (const char *const *) as->addresses, > > > as->n_addresses); > > > } > > > - > > > - /* Iterate address sets in the southbound database. Create > > andupdate 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; > > > - } > > > - } else { > > > - /* This address set is not yet in the symtab, so add it. > */ > > > - create_set = true; > > > - } > > > - > > > - 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); > > > - } > > > - > > > - sset_destroy(&cur_addr_set_names); > > > } > > > > > > struct lookup_port_aux { > > > @@ -209,7 +74,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 +114,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 +139,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 +157,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 +267,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); > > > @@ -554,10 +422,16 @@ 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 expr_address_sets = SHASH_INITIALIZER > > (&expr_address_sets); > > > + > > > + update_address_sets(ctx, &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); > > > + > > > + expr_macros_destroy(&expr_address_sets); > > > + shash_destroy(&expr_address_sets); > > > } > > > > > > void > > > @@ -565,6 +439,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); > > > } > > > } > > > > > > -- > > > 2.1.3 > > > > > > > Assuming that it compiles and all the unit test cases pass - roll with > it!! > > Er, let me say that more formally... > > Assuming that it compiles and all the unit test cases pass... > > Acked-by: Ryan Moats <rmoats@us.ibm.com> > > [Can I do that :) ] Thanks, I applied this to master and branch-2.6. I took the additional liberty of splitting the memory leak fix into a separate patch for clarity.
Ben Pfaff <blp@ovn.org> wrote on 08/31/2016 02:45:36 PM: > From: Ben Pfaff <blp@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 08/31/2016 02:45 PM > Subject: Re: [ovs-dev] [ovs-dev, v2,3/4] Unpersist data structures > for address sets. > > On Wed, Aug 31, 2016 at 01:51:51PM -0500, Ryan Moats wrote: > > > > > > "dev" <dev-bounces@openvswitch.org> wrote on 08/31/2016 12:53:27 PM: > > > > > From: Ryan Moats/Omaha/IBM@IBMUS > > > To: Ben Pfaff <blp@ovn.org> > > > Cc: dev@openvswitch.org > > > Date: 08/31/2016 12:53 PM > > > Subject: Re: [ovs-dev] [ovs-dev, v2, 3/4] Unpersist data structures > > > for address sets. > > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > > > > > > Ben Pfaff <blp@ovn.org> wrote on 08/31/2016 12:38:34 PM: > > > > > > > From: Ben Pfaff <blp@ovn.org> > > > > To: Ryan Moats/Omaha/IBM@IBMUS > > > > Cc: dev@openvswitch.org > > > > Date: 08/31/2016 12:38 PM > > > > Subject: Re: [ovs-dev,v2,3/4] Unpersist data structures for address > > sets. > > > > > > > > On Wed, Aug 31, 2016 at 03:22:45PM +0000, Ryan Moats wrote: > > > > > 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. > > > > > > > > > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > > > > > > > > I think that this was still doing a lot more work than necessary. The > > > > "struct address_set"s were being created and destroyed but not used for > > > > anything in between. > > > > > > > > Here's my proposal. Comments? > > > > > > > > --8<--------------------------cut here--------------------------> 8-- > > > > > > > > From: Ryan Moats <rmoats@us.ibm.com> > > > > Date: Wed, 31 Aug 2016 15:22:45 +0000 > > > > Subject: [PATCH] Unpersist data structures for address sets. > > > > > > > > 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. > > > > > > > > Signed-off-by: Ryan Moats <rmoats@us.ibm.com> > > > > Signed-off-by: Ben Pfaff <blp@ovn.org> > > > > --- > > > > ovn/controller/lflow.c | 176 ++++++ > > > > +------------------------------------------ > > > > ovn/lib/expr.c | 1 + > > > > 2 files changed, 25 insertions(+), 152 deletions(-) > > > > > > > > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c > > > > index a9c3137..efac5b3 100644 > > > > --- a/ovn/controller/lflow.c > > > > +++ b/ovn/controller/lflow.c > > > > @@ -38,159 +38,24 @@ 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 > > > > - * copy of sets still in their string form here to make it easier to > > > compare > > > > - * with the current values in the OVN_Southbound database. */ > > > > -struct address_set { > > > > - char **addresses; > > > > - 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; > > > > } > > > > > > > > +/* Iterate address sets in the southbound database. Create and update > > > the > > > > + * corresponding symtab entries as necessary. */ > > > > static void > > > > -address_set_destroy(struct address_set *addr_set) > > > > -{ > > > > - size_t i; > > > > - for (i = 0; i < addr_set->n_addresses; i++) { > > > > - free(addr_set->addresses[i]); > > > > - } > > > > - if (addr_set->n_addresses) { > > > > - free(addr_set->addresses); > > > > - } > > > > - free(addr_set); > > > > -} > > > > +update_address_sets(struct controller_ctx *ctx, > > > > + struct shash *expr_address_sets_p) > > > > > > > > -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); > > > > + const struct sbrec_address_set *as; > > > > + SBREC_ADDRESS_SET_FOR_EACH (as, ctx->ovnsb_idl) { > > > > + expr_macros_add(expr_address_sets_p, as->name, > > > > + (const char *const *) as->addresses, > > > > as->n_addresses); > > > > } > > > > - > > > > - /* Iterate address sets in the southbound database. Create > > > andupdate 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; > > > > - } > > > > - } else { > > > > - /* This address set is not yet in the symtab, so add it. > > */ > > > > - create_set = true; > > > > - } > > > > - > > > > - 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); > > > > - } > > > > - > > > > - sset_destroy(&cur_addr_set_names); > > > > } > > > > > > > > struct lookup_port_aux { > > > > @@ -209,7 +74,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 +114,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 +139,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 +157,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 +267,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); > > > > @@ -554,10 +422,16 @@ 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 expr_address_sets = SHASH_INITIALIZER > > > (&expr_address_sets); > > > > + > > > > + update_address_sets(ctx, &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); > > > > + > > > > + expr_macros_destroy(&expr_address_sets); > > > > + shash_destroy(&expr_address_sets); > > > > } > > > > > > > > void > > > > @@ -565,6 +439,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); > > > > } > > > > } > > > > > > > > -- > > > > 2.1.3 > > > > > > > > > > Assuming that it compiles and all the unit test cases pass - roll with > > it!! > > > > Er, let me say that more formally... > > > > Assuming that it compiles and all the unit test cases pass... > > > > Acked-by: Ryan Moats <rmoats@us.ibm.com> > > > > [Can I do that :) ] > > Thanks, I applied this to master and branch-2.6. > > I took the additional liberty of splitting the memory leak fix into a > separate patch for clarity. > That's fine.
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c index a9c3137..efac5b3 100644 --- a/ovn/controller/lflow.c +++ b/ovn/controller/lflow.c @@ -38,159 +38,24 @@ 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 - * copy of sets still in their string form here to make it easier to compare - * with the current values in the OVN_Southbound database. */ -struct address_set { - char **addresses; - 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; } +/* Iterate address sets in the southbound database. Create and update the + * corresponding symtab entries as necessary. */ static void -address_set_destroy(struct address_set *addr_set) -{ - size_t i; - for (i = 0; i < addr_set->n_addresses; i++) { - free(addr_set->addresses[i]); - } - if (addr_set->n_addresses) { - free(addr_set->addresses); - } - free(addr_set); -} +update_address_sets(struct controller_ctx *ctx, + struct shash *expr_address_sets_p) -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); + const struct sbrec_address_set *as; + SBREC_ADDRESS_SET_FOR_EACH (as, ctx->ovnsb_idl) { + expr_macros_add(expr_address_sets_p, as->name, + (const char *const *) as->addresses, as->n_addresses); } - - /* 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; - } - } else { - /* This address set is not yet in the symtab, so add it. */ - create_set = true; - } - - 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); - } - - sset_destroy(&cur_addr_set_names); } struct lookup_port_aux { @@ -209,7 +74,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 +114,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 +139,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 +157,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 +267,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); @@ -554,10 +422,16 @@ 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 expr_address_sets = SHASH_INITIALIZER(&expr_address_sets); + + update_address_sets(ctx, &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); + + expr_macros_destroy(&expr_address_sets); + shash_destroy(&expr_address_sets); } void @@ -565,6 +439,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); } }