"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 :) ]
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
