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