On Wed, Aug 31, 2016 at 01:51:51PM -0500, Ryan Moats wrote:
>
>
> "dev" <dev-boun...@openvswitch.org> wrote on 08/31/2016 12:53:27 PM:
>
> > From: Ryan Moats/Omaha/IBM@IBMUS
> > To: Ben Pfaff <b...@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-boun...@openvswitch.org>
> >
> >
> > Ben Pfaff <b...@ovn.org> wrote on 08/31/2016 12:38:34 PM:
> >
> > > From: Ben Pfaff <b...@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 <rmo...@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 <rmo...@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 <rmo...@us.ibm.com>
> > > Signed-off-by: Ben Pfaff <b...@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 <rmo...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
