[ovs-dev,ovn] ovn-controller: Revert lflow expr caching
diff mbox series

Message ID 20200225155001.393465-1-numans@ovn.org
State Accepted
Headers show
Series
  • [ovs-dev,ovn] ovn-controller: Revert lflow expr caching
Related show

Commit Message

Numan Siddique Feb. 25, 2020, 3:50 p.m. UTC
From: Numan Siddique <numans@ovn.org>

This patch reverts the below patches which added lflow expr caching
supported and follow up patches which fixed few issues.

With the present lflow expr caching we still have issues with logical
flows referencing port groups/address sets. If a port group/address set
change happens along with the port binding change in the same transaction,
ovn-controller may trigger full recompute and the lflow expr cache storing
the address sets and port groups would be invalid resulting in wrong
OF flows.

This patch reverts the below patches [1] which added lflow expr caching
supported and follow up patches which fixed few issues for now. These
patches will be submitted again addressing all the issues and the support
to periodically clear the expr cache which is missing now.

Reverts 99e3a145927("expr: Evaluate the condition expression in a separate step.")
        8795bec737b("ovn-controller: Cache logical flow expr tree for each lflow.)
        672508f6368("ovn-controller: Fix memory issues due to lflow expr caching.)
        06ccb8d1dff("Save the addr set and port groups in lflow expr cache")

Reported-by: Jakub Libosvar <jlibosva@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 controller/lflow.c          | 216 +++++++-----------------------------
 controller/ovn-controller.c |   6 -
 include/ovn/expr.h          |  10 +-
 lib/expr.c                  |  50 ++-------
 tests/test-ovn.c            |  11 +-
 utilities/ovn-trace.c       |   6 +-
 6 files changed, 63 insertions(+), 236 deletions(-)

Comments

Mark Michelson Feb. 27, 2020, 9:43 p.m. UTC | #1
*sigh*

Acked-by: Mark Michelson <mmichels@redhat.com>

:(

On 2/25/20 10:50 AM, numans@ovn.org wrote:
> From: Numan Siddique <numans@ovn.org>
> 
> This patch reverts the below patches which added lflow expr caching
> supported and follow up patches which fixed few issues.
> 
> With the present lflow expr caching we still have issues with logical
> flows referencing port groups/address sets. If a port group/address set
> change happens along with the port binding change in the same transaction,
> ovn-controller may trigger full recompute and the lflow expr cache storing
> the address sets and port groups would be invalid resulting in wrong
> OF flows.
> 
> This patch reverts the below patches [1] which added lflow expr caching
> supported and follow up patches which fixed few issues for now. These
> patches will be submitted again addressing all the issues and the support
> to periodically clear the expr cache which is missing now.
> 
> Reverts 99e3a145927("expr: Evaluate the condition expression in a separate step.")
>          8795bec737b("ovn-controller: Cache logical flow expr tree for each lflow.)
>          672508f6368("ovn-controller: Fix memory issues due to lflow expr caching.)
>          06ccb8d1dff("Save the addr set and port groups in lflow expr cache")
> 
> Reported-by: Jakub Libosvar <jlibosva@redhat.com>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
>   controller/lflow.c          | 216 +++++++-----------------------------
>   controller/ovn-controller.c |   6 -
>   include/ovn/expr.h          |  10 +-
>   lib/expr.c                  |  50 ++-------
>   tests/test-ovn.c            |  11 +-
>   utilities/ovn-trace.c       |   6 +-
>   6 files changed, 63 insertions(+), 236 deletions(-)
> 
> diff --git a/controller/lflow.c b/controller/lflow.c
> index 55e6b8b0a..ee11fc617 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -66,7 +66,6 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
>                         struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
>                         struct hmap *nd_ra_opts,
>                         struct controller_event_options *controller_event_opts,
> -                      bool delete_expr_from_cache,
>                         struct lflow_ctx_in *l_ctx_in,
>                         struct lflow_ctx_out *l_ctx_out);
>   
> @@ -256,75 +255,6 @@ lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr,
>       free(lfrn);
>   }
>   
> -/* Represents expr tree for the lflow. We don't store the
> - * match in this structure because -
> - *   - Whenever a flow match or action needs to be modified,
> - *     ovn-northd deletes the existing flow in the logical_flow
> - *     table and adds a new one.
> - *  We may want to revisit this and store match incase the behavior
> - *  of ovn-northd changes.
> - */
> -struct lflow_expr {
> -    struct hmap_node node;
> -    struct uuid lflow_uuid; /* key */
> -    struct expr *expr;
> -    struct sset addr_sets_ref;
> -    struct sset port_groups_ref;
> -};
> -
> -static void
> -lflow_expr_add(struct hmap *lflow_expr_cache,
> -               const struct sbrec_logical_flow *lflow,
> -               struct expr *lflow_expr, struct sset *addr_sets_ref,
> -               struct sset *port_groups_ref)
> -{
> -    struct lflow_expr *le = xmalloc(sizeof *le);
> -    le->lflow_uuid = lflow->header_.uuid;
> -    le->expr = lflow_expr;
> -    sset_clone(&le->addr_sets_ref, addr_sets_ref);
> -    sset_clone(&le->port_groups_ref, port_groups_ref);
> -    hmap_insert(lflow_expr_cache, &le->node, uuid_hash(&le->lflow_uuid));
> -}
> -
> -static struct lflow_expr *
> -lflow_expr_get(struct hmap *lflow_expr_cache,
> -               const struct sbrec_logical_flow *lflow)
> -{
> -    struct lflow_expr *le;
> -    size_t hash = uuid_hash(&lflow->header_.uuid);
> -    HMAP_FOR_EACH_WITH_HASH (le, node, hash, lflow_expr_cache) {
> -        if (uuid_equals(&le->lflow_uuid, &lflow->header_.uuid)) {
> -            return le;
> -        }
> -    }
> -
> -    return NULL;
> -}
> -
> -static void
> -lflow_expr_delete(struct hmap *lflow_expr_cache, struct lflow_expr *le)
> -{
> -    hmap_remove(lflow_expr_cache, &le->node);
> -    expr_destroy(le->expr);
> -    sset_destroy(&le->addr_sets_ref);
> -    sset_destroy(&le->port_groups_ref);
> -    free(le);
> -}
> -
> -void
> -lflow_expr_destroy(struct hmap *lflow_expr_cache)
> -{
> -    struct lflow_expr *le;
> -    HMAP_FOR_EACH_POP (le, node, lflow_expr_cache) {
> -        expr_destroy(le->expr);
> -        sset_destroy(&le->addr_sets_ref);
> -        sset_destroy(&le->port_groups_ref);
> -        free(le);
> -    }
> -
> -    hmap_destroy(lflow_expr_cache);
> -}
> -
>   /* Adds the logical flows from the Logical_Flow table to flow tables. */
>   static void
>   add_logical_flows(struct lflow_ctx_in *l_ctx_in,
> @@ -357,7 +287,7 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in,
>   
>       SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow, l_ctx_in->logical_flow_table) {
>           if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> -                                   &nd_ra_opts, &controller_event_opts, false,
> +                                   &nd_ra_opts, &controller_event_opts,
>                                      l_ctx_in, l_ctx_out)) {
>               static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
>               VLOG_ERR_RL(&rl, "Conjunction id overflow when processing lflow "
> @@ -411,11 +341,6 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in,
>               /* Delete entries from lflow resource reference. */
>               lflow_resource_destroy_lflow(l_ctx_out->lfrr,
>                                            &lflow->header_.uuid);
> -            struct lflow_expr *le =
> -                lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow);
> -            if (le) {
> -                lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
> -            }
>           }
>       }
>   
> @@ -441,7 +366,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in,
>                        UUID_ARGS(&lflow->header_.uuid));
>               if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
>                                          &nd_ra_opts, &controller_event_opts,
> -                                       true, l_ctx_in, l_ctx_out)) {
> +                                       l_ctx_in, l_ctx_out)) {
>                   ret = false;
>                   break;
>               }
> @@ -526,7 +451,7 @@ lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
>   
>           if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
>                                      &nd_ra_opts, &controller_event_opts,
> -                                   true, l_ctx_in, l_ctx_out)) {
> +                                   l_ctx_in, l_ctx_out)) {
>               ret = false;
>               break;
>           }
> @@ -557,31 +482,11 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs)
>       return true;
>   }
>   
> -static void
> -lflow_update_resource_refs(const struct sbrec_logical_flow *lflow,
> -                           struct sset *addr_sets_ref,
> -                           struct sset *port_groups_ref,
> -                           struct lflow_resource_ref *lfrr)
> -{
> -    const char *addr_set_name;
> -    SSET_FOR_EACH (addr_set_name, addr_sets_ref) {
> -        lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name,
> -                           &lflow->header_.uuid);
> -    }
> -
> -    const char *port_group_name;
> -    SSET_FOR_EACH (port_group_name, port_groups_ref) {
> -        lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name,
> -                           &lflow->header_.uuid);
> -    }
> -}
> -
>   static bool
>   consider_logical_flow(const struct sbrec_logical_flow *lflow,
>                         struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
>                         struct hmap *nd_ra_opts,
>                         struct controller_event_options *controller_event_opts,
> -                      bool delete_expr_from_cache,
>                         struct lflow_ctx_in *l_ctx_in,
>                         struct lflow_ctx_out *l_ctx_out)
>   {
> @@ -641,83 +546,59 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
>   
>       /* Translate OVN match into table of OpenFlow matches. */
>       struct hmap matches;
> -    struct expr *expr = NULL;
> +    struct expr *expr;
>   
> -    struct lflow_expr *le = lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow);
> -    if (le) {
> -        if (delete_expr_from_cache) {
> -            lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
> -            le = NULL;
> -        } else {
> -            expr = le->expr;
> -        }
> +    struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
> +    struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
> +    expr = expr_parse_string(lflow->match, &symtab, l_ctx_in->addr_sets,
> +                             l_ctx_in->port_groups,
> +                             &addr_sets_ref, &port_groups_ref, &error);
> +    const char *addr_set_name;
> +    SSET_FOR_EACH (addr_set_name, &addr_sets_ref) {
> +        lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET, addr_set_name,
> +                           &lflow->header_.uuid);
>       }
> +    const char *port_group_name;
> +    SSET_FOR_EACH (port_group_name, &port_groups_ref) {
> +        lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP,
> +                           port_group_name, &lflow->header_.uuid);
> +    }
> +    sset_destroy(&addr_sets_ref);
> +    sset_destroy(&port_groups_ref);
>   
> -    if (!expr) {
> -        struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
> -        struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
> -
> -        expr = expr_parse_string(lflow->match, &symtab, l_ctx_in->addr_sets,
> -                                 l_ctx_in->port_groups, &addr_sets_ref,
> -                                 &port_groups_ref, &error);
> -        /* Add the address set and port groups (if any) to the lflow
> -         * references. */
> -        lflow_update_resource_refs(lflow, &addr_sets_ref, &port_groups_ref,
> -                                   l_ctx_out->lfrr);
> -
> -        if (!error) {
> -            if (prereqs) {
> -                expr = expr_combine(EXPR_T_AND, expr, prereqs);
> -                prereqs = NULL;
> -            }
> -            expr = expr_annotate(expr, &symtab, &error);
> +    if (!error) {
> +        if (prereqs) {
> +            expr = expr_combine(EXPR_T_AND, expr, prereqs);
> +            prereqs = NULL;
>           }
> -        if (error) {
> -            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> -            VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s",
> -                        lflow->match, error);
> -            expr_destroy(prereqs);
> -            free(error);
> -            ovnacts_free(ovnacts.data, ovnacts.size);
> -            ofpbuf_uninit(&ovnacts);
> -            sset_destroy(&addr_sets_ref);
> -            sset_destroy(&port_groups_ref);
> -            return true;
> -        }
> -
> -        expr = expr_simplify(expr);
> -        expr = expr_normalize(expr);
> -
> -        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr,
> -                       &addr_sets_ref, &port_groups_ref);
> -        sset_destroy(&addr_sets_ref);
> -        sset_destroy(&port_groups_ref);
> -    } else {
> +        expr = expr_annotate(expr, &symtab, &error);
> +    }
> +    if (error) {
> +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> +        VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s",
> +                     lflow->match, error);
>           expr_destroy(prereqs);
> -        /* Add the cached address set and port groups (if any) to the lflow
> -         * references. */
> -        lflow_update_resource_refs(lflow, &le->addr_sets_ref,
> -                                   &le->port_groups_ref, l_ctx_out->lfrr);
> +        free(error);
> +        ovnacts_free(ovnacts.data, ovnacts.size);
> +        ofpbuf_uninit(&ovnacts);
> +        return true;
>       }
>   
> -    struct condition_aux cond_aux = {
> -        .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
> -        .chassis = l_ctx_in->chassis,
> -        .active_tunnels = l_ctx_in->active_tunnels,
> -    };
> -
> -    expr = expr_clone(expr);
> -    expr = expr_evaluate_condition(expr, is_chassis_resident_cb, &cond_aux);
> -
>       struct lookup_port_aux aux = {
>           .sbrec_multicast_group_by_name_datapath
>               = l_ctx_in->sbrec_multicast_group_by_name_datapath,
>           .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
>           .dp = lflow->logical_datapath
>       };
> +    struct condition_aux cond_aux = {
> +        .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
> +        .chassis = l_ctx_in->chassis,
> +        .active_tunnels = l_ctx_in->active_tunnels,
> +    };
> +    expr = expr_simplify(expr, is_chassis_resident_cb, &cond_aux);
> +    expr = expr_normalize(expr);
>       uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, &aux,
>                                          &matches);
> -
>       expr_destroy(expr);
>   
>       if (hmap_is_empty(&matches)) {
> @@ -954,21 +835,6 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out)
>   {
>       COVERAGE_INC(lflow_run);
>   
> -    /* when lflow_run is called, it's possible that some of the logical flows
> -     * are deleted. We need to delete the lflow expr cache for these lflows,
> -     * otherwise, they will not be deleted at all. */
> -    const struct sbrec_logical_flow *lflow;
> -    SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow,
> -                                               l_ctx_in->logical_flow_table) {
> -        if (sbrec_logical_flow_is_deleted(lflow)) {
> -            struct lflow_expr *le =
> -                lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow);
> -            if (le) {
> -                lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
> -            }
> -        }
> -    }
> -
>       add_logical_flows(l_ctx_in, l_ctx_out);
>       add_neighbor_flows(l_ctx_in->sbrec_port_binding_by_name,
>                          l_ctx_in->mac_binding_table, l_ctx_in->local_datapaths,
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index cacaaa578..303e5708d 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1240,9 +1240,6 @@ struct ed_type_flow_output {
>       uint32_t conj_id_ofs;
>       /* lflow resource cross reference */
>       struct lflow_resource_ref lflow_resource_ref;
> -
> -    /* Cache of lflow expr tree. */
> -    struct hmap lflow_expr_cache;
>   };
>   
>   static void init_physical_ctx(struct engine_node *node,
> @@ -1380,7 +1377,6 @@ static void init_lflow_ctx(struct engine_node *node,
>       l_ctx_out->group_table = &fo->group_table;
>       l_ctx_out->meter_table = &fo->meter_table;
>       l_ctx_out->lfrr = &fo->lflow_resource_ref;
> -    l_ctx_out->lflow_expr_cache = &fo->lflow_expr_cache;
>       l_ctx_out->conj_id_ofs = &fo->conj_id_ofs;
>   }
>   
> @@ -1395,7 +1391,6 @@ en_flow_output_init(struct engine_node *node OVS_UNUSED,
>       ovn_extend_table_init(&data->meter_table);
>       data->conj_id_ofs = 1;
>       lflow_resource_init(&data->lflow_resource_ref);
> -    hmap_init(&data->lflow_expr_cache);
>       return data;
>   }
>   
> @@ -1407,7 +1402,6 @@ en_flow_output_cleanup(void *data)
>       ovn_extend_table_destroy(&flow_output_data->group_table);
>       ovn_extend_table_destroy(&flow_output_data->meter_table);
>       lflow_resource_destroy(&flow_output_data->lflow_resource_ref);
> -    lflow_expr_destroy(&flow_output_data->lflow_expr_cache);
>   }
>   
>   static void
> diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> index 00a021236..21bf51c22 100644
> --- a/include/ovn/expr.h
> +++ b/include/ovn/expr.h
> @@ -404,12 +404,10 @@ void expr_destroy(struct expr *);
>   
>   struct expr *expr_annotate(struct expr *, const struct shash *symtab,
>                              char **errorp);
> -struct expr *expr_simplify(struct expr *);
> -struct expr *expr_evaluate_condition(
> -    struct expr *,
> -    bool (*is_chassis_resident)(const void *c_aux,
> -                                const char *port_name),
> -    const void *c_aux);
> +struct expr *expr_simplify(struct expr *,
> +                           bool (*is_chassis_resident)(const void *c_aux,
> +                                                       const char *port_name),
> +                           const void *c_aux);
>   struct expr *expr_normalize(struct expr *);
>   
>   bool expr_honors_invariants(const struct expr *);
> diff --git a/lib/expr.c b/lib/expr.c
> index 12557e3ca..78646a1af 100644
> --- a/lib/expr.c
> +++ b/lib/expr.c
> @@ -2033,10 +2033,10 @@ expr_simplify_relational(struct expr *expr)
>   
>   /* Resolves condition and replaces the expression with a boolean. */
>   static struct expr *
> -expr_evaluate_condition__(struct expr *expr,
> -                          bool (*is_chassis_resident)(const void *c_aux,
> +expr_simplify_condition(struct expr *expr,
> +                        bool (*is_chassis_resident)(const void *c_aux,
>                                                       const char *port_name),
> -                          const void *c_aux)
> +                        const void *c_aux)
>   {
>       bool result;
>   
> @@ -2054,41 +2054,13 @@ expr_evaluate_condition__(struct expr *expr,
>       return expr_create_boolean(result);
>   }
>   
> -struct expr *
> -expr_evaluate_condition(struct expr *expr,
> -                        bool (*is_chassis_resident)(const void *c_aux,
> -                                                    const char *port_name),
> -                        const void *c_aux)
> -{
> -    struct expr *sub, *next;
> -
> -    switch (expr->type) {
> -    case EXPR_T_AND:
> -    case EXPR_T_OR:
> -         LIST_FOR_EACH_SAFE (sub, next, node, &expr->andor) {
> -            ovs_list_remove(&sub->node);
> -            struct expr *e = expr_evaluate_condition(sub, is_chassis_resident,
> -                                                     c_aux);
> -            e = expr_fix(e);
> -            expr_insert_andor(expr, next, e);
> -        }
> -        return expr_fix(expr);
> -
> -    case EXPR_T_CONDITION:
> -        return expr_evaluate_condition__(expr, is_chassis_resident, c_aux);
> -
> -    case EXPR_T_CMP:
> -    case EXPR_T_BOOLEAN:
> -        return expr;
> -    }
> -
> -    OVS_NOT_REACHED();
> -}
> -
>   /* Takes ownership of 'expr' and returns an equivalent expression whose
>    * EXPR_T_CMP nodes use only tests for equality (EXPR_R_EQ). */
>   struct expr *
> -expr_simplify(struct expr *expr)
> +expr_simplify(struct expr *expr,
> +              bool (*is_chassis_resident)(const void *c_aux,
> +                                        const char *port_name),
> +              const void *c_aux)
>   {
>       struct expr *sub, *next;
>   
> @@ -2103,7 +2075,8 @@ expr_simplify(struct expr *expr)
>       case EXPR_T_OR:
>           LIST_FOR_EACH_SAFE (sub, next, node, &expr->andor) {
>               ovs_list_remove(&sub->node);
> -            expr_insert_andor(expr, next, expr_simplify(sub));
> +            expr_insert_andor(expr, next,
> +                              expr_simplify(sub, is_chassis_resident, c_aux));
>           }
>           return expr_fix(expr);
>   
> @@ -2111,7 +2084,7 @@ expr_simplify(struct expr *expr)
>           return expr;
>   
>       case EXPR_T_CONDITION:
> -        return expr;
> +        return expr_simplify_condition(expr, is_chassis_resident, c_aux);
>       }
>       OVS_NOT_REACHED();
>   }
> @@ -3393,8 +3366,7 @@ expr_parse_microflow__(struct lexer *lexer,
>       struct ds annotated = DS_EMPTY_INITIALIZER;
>       expr_format(e, &annotated);
>   
> -    e = expr_simplify(e);
> -    e = expr_evaluate_condition(e, microflow_is_chassis_resident_cb, NULL);
> +    e = expr_simplify(e, microflow_is_chassis_resident_cb, NULL);
>       e = expr_normalize(e);
>   
>       struct match m = MATCH_CATCHALL_INITIALIZER;
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index c607a8f89..11697ebd0 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -308,9 +308,8 @@ test_parse_expr__(int steps)
>           }
>           if (!error) {
>               if (steps > 1) {
> -                expr = expr_simplify(expr);
> -                expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
> -                                               &ports);
> +                expr = expr_simplify(expr, is_chassis_resident_cb,
> +                                     &ports);
>               }
>               if (steps > 2) {
>                   expr = expr_normalize(expr);
> @@ -911,9 +910,9 @@ test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab,
>                   exit(EXIT_FAILURE);
>               }
>           } else if (operation >= OP_SIMPLIFY) {
> -            modified = expr_simplify(expr_clone(expr));
> -            modified = expr_evaluate_condition(
> -                modified, tree_shape_is_chassis_resident_cb, NULL);
> +            modified = expr_simplify(expr_clone(expr),
> +                                     tree_shape_is_chassis_resident_cb,
> +                                     NULL);
>               ovs_assert(expr_honors_invariants(modified));
>   
>               if (operation >= OP_NORMALIZE) {
> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> index 7279452ee..84e5f2b5c 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -926,10 +926,8 @@ read_flows(void)
>               continue;
>           }
>           if (match) {
> -            match = expr_simplify(match);
> -            match = expr_evaluate_condition(match,
> -                                            ovntrace_is_chassis_resident,
> -                                            NULL);
> +            match = expr_simplify(match, ovntrace_is_chassis_resident,
> +                                  NULL);
>           }
>   
>           struct ovntrace_flow *flow = xzalloc(sizeof *flow);
>
Numan Siddique Feb. 28, 2020, 12:38 p.m. UTC | #2
On Fri, Feb 28, 2020, 3:13 AM Mark Michelson <mmichels@redhat.com> wrote:

> *sigh*
>
> Acked-by: Mark Michelson <mmichels@redhat.com>
>

Thanks. I applied this patch to master and branch-20.03

Thanks
Numan


> :(
>
> On 2/25/20 10:50 AM, numans@ovn.org wrote:
> > From: Numan Siddique <numans@ovn.org>
> >
> > This patch reverts the below patches which added lflow expr caching
> > supported and follow up patches which fixed few issues.
> >
> > With the present lflow expr caching we still have issues with logical
> > flows referencing port groups/address sets. If a port group/address set
> > change happens along with the port binding change in the same
> transaction,
> > ovn-controller may trigger full recompute and the lflow expr cache
> storing
> > the address sets and port groups would be invalid resulting in wrong
> > OF flows.
> >
> > This patch reverts the below patches [1] which added lflow expr caching
> > supported and follow up patches which fixed few issues for now. These
> > patches will be submitted again addressing all the issues and the support
> > to periodically clear the expr cache which is missing now.
> >
> > Reverts 99e3a145927("expr: Evaluate the condition expression in a
> separate step.")
> >          8795bec737b("ovn-controller: Cache logical flow expr tree for
> each lflow.)
> >          672508f6368("ovn-controller: Fix memory issues due to lflow
> expr caching.)
> >          06ccb8d1dff("Save the addr set and port groups in lflow expr
> cache")
> >
> > Reported-by: Jakub Libosvar <jlibosva@redhat.com>
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
> >   controller/lflow.c          | 216 +++++++-----------------------------
> >   controller/ovn-controller.c |   6 -
> >   include/ovn/expr.h          |  10 +-
> >   lib/expr.c                  |  50 ++-------
> >   tests/test-ovn.c            |  11 +-
> >   utilities/ovn-trace.c       |   6 +-
> >   6 files changed, 63 insertions(+), 236 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index 55e6b8b0a..ee11fc617 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -66,7 +66,6 @@ consider_logical_flow(const struct sbrec_logical_flow
> *lflow,
> >                         struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
> >                         struct hmap *nd_ra_opts,
> >                         struct controller_event_options
> *controller_event_opts,
> > -                      bool delete_expr_from_cache,
> >                         struct lflow_ctx_in *l_ctx_in,
> >                         struct lflow_ctx_out *l_ctx_out);
> >
> > @@ -256,75 +255,6 @@ lflow_resource_destroy_lflow(struct
> lflow_resource_ref *lfrr,
> >       free(lfrn);
> >   }
> >
> > -/* Represents expr tree for the lflow. We don't store the
> > - * match in this structure because -
> > - *   - Whenever a flow match or action needs to be modified,
> > - *     ovn-northd deletes the existing flow in the logical_flow
> > - *     table and adds a new one.
> > - *  We may want to revisit this and store match incase the behavior
> > - *  of ovn-northd changes.
> > - */
> > -struct lflow_expr {
> > -    struct hmap_node node;
> > -    struct uuid lflow_uuid; /* key */
> > -    struct expr *expr;
> > -    struct sset addr_sets_ref;
> > -    struct sset port_groups_ref;
> > -};
> > -
> > -static void
> > -lflow_expr_add(struct hmap *lflow_expr_cache,
> > -               const struct sbrec_logical_flow *lflow,
> > -               struct expr *lflow_expr, struct sset *addr_sets_ref,
> > -               struct sset *port_groups_ref)
> > -{
> > -    struct lflow_expr *le = xmalloc(sizeof *le);
> > -    le->lflow_uuid = lflow->header_.uuid;
> > -    le->expr = lflow_expr;
> > -    sset_clone(&le->addr_sets_ref, addr_sets_ref);
> > -    sset_clone(&le->port_groups_ref, port_groups_ref);
> > -    hmap_insert(lflow_expr_cache, &le->node,
> uuid_hash(&le->lflow_uuid));
> > -}
> > -
> > -static struct lflow_expr *
> > -lflow_expr_get(struct hmap *lflow_expr_cache,
> > -               const struct sbrec_logical_flow *lflow)
> > -{
> > -    struct lflow_expr *le;
> > -    size_t hash = uuid_hash(&lflow->header_.uuid);
> > -    HMAP_FOR_EACH_WITH_HASH (le, node, hash, lflow_expr_cache) {
> > -        if (uuid_equals(&le->lflow_uuid, &lflow->header_.uuid)) {
> > -            return le;
> > -        }
> > -    }
> > -
> > -    return NULL;
> > -}
> > -
> > -static void
> > -lflow_expr_delete(struct hmap *lflow_expr_cache, struct lflow_expr *le)
> > -{
> > -    hmap_remove(lflow_expr_cache, &le->node);
> > -    expr_destroy(le->expr);
> > -    sset_destroy(&le->addr_sets_ref);
> > -    sset_destroy(&le->port_groups_ref);
> > -    free(le);
> > -}
> > -
> > -void
> > -lflow_expr_destroy(struct hmap *lflow_expr_cache)
> > -{
> > -    struct lflow_expr *le;
> > -    HMAP_FOR_EACH_POP (le, node, lflow_expr_cache) {
> > -        expr_destroy(le->expr);
> > -        sset_destroy(&le->addr_sets_ref);
> > -        sset_destroy(&le->port_groups_ref);
> > -        free(le);
> > -    }
> > -
> > -    hmap_destroy(lflow_expr_cache);
> > -}
> > -
> >   /* Adds the logical flows from the Logical_Flow table to flow tables.
> */
> >   static void
> >   add_logical_flows(struct lflow_ctx_in *l_ctx_in,
> > @@ -357,7 +287,7 @@ add_logical_flows(struct lflow_ctx_in *l_ctx_in,
> >
> >       SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow,
> l_ctx_in->logical_flow_table) {
> >           if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> > -                                   &nd_ra_opts, &controller_event_opts,
> false,
> > +                                   &nd_ra_opts, &controller_event_opts,
> >                                      l_ctx_in, l_ctx_out)) {
> >               static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
> 5);
> >               VLOG_ERR_RL(&rl, "Conjunction id overflow when processing
> lflow "
> > @@ -411,11 +341,6 @@ lflow_handle_changed_flows(struct lflow_ctx_in
> *l_ctx_in,
> >               /* Delete entries from lflow resource reference. */
> >               lflow_resource_destroy_lflow(l_ctx_out->lfrr,
> >                                            &lflow->header_.uuid);
> > -            struct lflow_expr *le =
> > -                lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow);
> > -            if (le) {
> > -                lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
> > -            }
> >           }
> >       }
> >
> > @@ -441,7 +366,7 @@ lflow_handle_changed_flows(struct lflow_ctx_in
> *l_ctx_in,
> >                        UUID_ARGS(&lflow->header_.uuid));
> >               if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> >                                          &nd_ra_opts,
> &controller_event_opts,
> > -                                       true, l_ctx_in, l_ctx_out)) {
> > +                                       l_ctx_in, l_ctx_out)) {
> >                   ret = false;
> >                   break;
> >               }
> > @@ -526,7 +451,7 @@ lflow_handle_changed_ref(enum ref_type ref_type,
> const char *ref_name,
> >
> >           if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
> >                                      &nd_ra_opts, &controller_event_opts,
> > -                                   true, l_ctx_in, l_ctx_out)) {
> > +                                   l_ctx_in, l_ctx_out)) {
> >               ret = false;
> >               break;
> >           }
> > @@ -557,31 +482,11 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t
> n_conjs)
> >       return true;
> >   }
> >
> > -static void
> > -lflow_update_resource_refs(const struct sbrec_logical_flow *lflow,
> > -                           struct sset *addr_sets_ref,
> > -                           struct sset *port_groups_ref,
> > -                           struct lflow_resource_ref *lfrr)
> > -{
> > -    const char *addr_set_name;
> > -    SSET_FOR_EACH (addr_set_name, addr_sets_ref) {
> > -        lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name,
> > -                           &lflow->header_.uuid);
> > -    }
> > -
> > -    const char *port_group_name;
> > -    SSET_FOR_EACH (port_group_name, port_groups_ref) {
> > -        lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name,
> > -                           &lflow->header_.uuid);
> > -    }
> > -}
> > -
> >   static bool
> >   consider_logical_flow(const struct sbrec_logical_flow *lflow,
> >                         struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
> >                         struct hmap *nd_ra_opts,
> >                         struct controller_event_options
> *controller_event_opts,
> > -                      bool delete_expr_from_cache,
> >                         struct lflow_ctx_in *l_ctx_in,
> >                         struct lflow_ctx_out *l_ctx_out)
> >   {
> > @@ -641,83 +546,59 @@ consider_logical_flow(const struct
> sbrec_logical_flow *lflow,
> >
> >       /* Translate OVN match into table of OpenFlow matches. */
> >       struct hmap matches;
> > -    struct expr *expr = NULL;
> > +    struct expr *expr;
> >
> > -    struct lflow_expr *le = lflow_expr_get(l_ctx_out->lflow_expr_cache,
> lflow);
> > -    if (le) {
> > -        if (delete_expr_from_cache) {
> > -            lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
> > -            le = NULL;
> > -        } else {
> > -            expr = le->expr;
> > -        }
> > +    struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
> > +    struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
> > +    expr = expr_parse_string(lflow->match, &symtab, l_ctx_in->addr_sets,
> > +                             l_ctx_in->port_groups,
> > +                             &addr_sets_ref, &port_groups_ref, &error);
> > +    const char *addr_set_name;
> > +    SSET_FOR_EACH (addr_set_name, &addr_sets_ref) {
> > +        lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET,
> addr_set_name,
> > +                           &lflow->header_.uuid);
> >       }
> > +    const char *port_group_name;
> > +    SSET_FOR_EACH (port_group_name, &port_groups_ref) {
> > +        lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP,
> > +                           port_group_name, &lflow->header_.uuid);
> > +    }
> > +    sset_destroy(&addr_sets_ref);
> > +    sset_destroy(&port_groups_ref);
> >
> > -    if (!expr) {
> > -        struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
> > -        struct sset port_groups_ref =
> SSET_INITIALIZER(&port_groups_ref);
> > -
> > -        expr = expr_parse_string(lflow->match, &symtab,
> l_ctx_in->addr_sets,
> > -                                 l_ctx_in->port_groups, &addr_sets_ref,
> > -                                 &port_groups_ref, &error);
> > -        /* Add the address set and port groups (if any) to the lflow
> > -         * references. */
> > -        lflow_update_resource_refs(lflow, &addr_sets_ref,
> &port_groups_ref,
> > -                                   l_ctx_out->lfrr);
> > -
> > -        if (!error) {
> > -            if (prereqs) {
> > -                expr = expr_combine(EXPR_T_AND, expr, prereqs);
> > -                prereqs = NULL;
> > -            }
> > -            expr = expr_annotate(expr, &symtab, &error);
> > +    if (!error) {
> > +        if (prereqs) {
> > +            expr = expr_combine(EXPR_T_AND, expr, prereqs);
> > +            prereqs = NULL;
> >           }
> > -        if (error) {
> > -            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1,
> 1);
> > -            VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s",
> > -                        lflow->match, error);
> > -            expr_destroy(prereqs);
> > -            free(error);
> > -            ovnacts_free(ovnacts.data, ovnacts.size);
> > -            ofpbuf_uninit(&ovnacts);
> > -            sset_destroy(&addr_sets_ref);
> > -            sset_destroy(&port_groups_ref);
> > -            return true;
> > -        }
> > -
> > -        expr = expr_simplify(expr);
> > -        expr = expr_normalize(expr);
> > -
> > -        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr,
> > -                       &addr_sets_ref, &port_groups_ref);
> > -        sset_destroy(&addr_sets_ref);
> > -        sset_destroy(&port_groups_ref);
> > -    } else {
> > +        expr = expr_annotate(expr, &symtab, &error);
> > +    }
> > +    if (error) {
> > +        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> > +        VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s",
> > +                     lflow->match, error);
> >           expr_destroy(prereqs);
> > -        /* Add the cached address set and port groups (if any) to the
> lflow
> > -         * references. */
> > -        lflow_update_resource_refs(lflow, &le->addr_sets_ref,
> > -                                   &le->port_groups_ref,
> l_ctx_out->lfrr);
> > +        free(error);
> > +        ovnacts_free(ovnacts.data, ovnacts.size);
> > +        ofpbuf_uninit(&ovnacts);
> > +        return true;
> >       }
> >
> > -    struct condition_aux cond_aux = {
> > -        .sbrec_port_binding_by_name =
> l_ctx_in->sbrec_port_binding_by_name,
> > -        .chassis = l_ctx_in->chassis,
> > -        .active_tunnels = l_ctx_in->active_tunnels,
> > -    };
> > -
> > -    expr = expr_clone(expr);
> > -    expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
> &cond_aux);
> > -
> >       struct lookup_port_aux aux = {
> >           .sbrec_multicast_group_by_name_datapath
> >               = l_ctx_in->sbrec_multicast_group_by_name_datapath,
> >           .sbrec_port_binding_by_name =
> l_ctx_in->sbrec_port_binding_by_name,
> >           .dp = lflow->logical_datapath
> >       };
> > +    struct condition_aux cond_aux = {
> > +        .sbrec_port_binding_by_name =
> l_ctx_in->sbrec_port_binding_by_name,
> > +        .chassis = l_ctx_in->chassis,
> > +        .active_tunnels = l_ctx_in->active_tunnels,
> > +    };
> > +    expr = expr_simplify(expr, is_chassis_resident_cb, &cond_aux);
> > +    expr = expr_normalize(expr);
> >       uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, &aux,
> >                                          &matches);
> > -
> >       expr_destroy(expr);
> >
> >       if (hmap_is_empty(&matches)) {
> > @@ -954,21 +835,6 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct
> lflow_ctx_out *l_ctx_out)
> >   {
> >       COVERAGE_INC(lflow_run);
> >
> > -    /* when lflow_run is called, it's possible that some of the logical
> flows
> > -     * are deleted. We need to delete the lflow expr cache for these
> lflows,
> > -     * otherwise, they will not be deleted at all. */
> > -    const struct sbrec_logical_flow *lflow;
> > -    SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow,
> > -
>  l_ctx_in->logical_flow_table) {
> > -        if (sbrec_logical_flow_is_deleted(lflow)) {
> > -            struct lflow_expr *le =
> > -                lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow);
> > -            if (le) {
> > -                lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
> > -            }
> > -        }
> > -    }
> > -
> >       add_logical_flows(l_ctx_in, l_ctx_out);
> >       add_neighbor_flows(l_ctx_in->sbrec_port_binding_by_name,
> >                          l_ctx_in->mac_binding_table,
> l_ctx_in->local_datapaths,
> > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> > index cacaaa578..303e5708d 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -1240,9 +1240,6 @@ struct ed_type_flow_output {
> >       uint32_t conj_id_ofs;
> >       /* lflow resource cross reference */
> >       struct lflow_resource_ref lflow_resource_ref;
> > -
> > -    /* Cache of lflow expr tree. */
> > -    struct hmap lflow_expr_cache;
> >   };
> >
> >   static void init_physical_ctx(struct engine_node *node,
> > @@ -1380,7 +1377,6 @@ static void init_lflow_ctx(struct engine_node
> *node,
> >       l_ctx_out->group_table = &fo->group_table;
> >       l_ctx_out->meter_table = &fo->meter_table;
> >       l_ctx_out->lfrr = &fo->lflow_resource_ref;
> > -    l_ctx_out->lflow_expr_cache = &fo->lflow_expr_cache;
> >       l_ctx_out->conj_id_ofs = &fo->conj_id_ofs;
> >   }
> >
> > @@ -1395,7 +1391,6 @@ en_flow_output_init(struct engine_node *node
> OVS_UNUSED,
> >       ovn_extend_table_init(&data->meter_table);
> >       data->conj_id_ofs = 1;
> >       lflow_resource_init(&data->lflow_resource_ref);
> > -    hmap_init(&data->lflow_expr_cache);
> >       return data;
> >   }
> >
> > @@ -1407,7 +1402,6 @@ en_flow_output_cleanup(void *data)
> >       ovn_extend_table_destroy(&flow_output_data->group_table);
> >       ovn_extend_table_destroy(&flow_output_data->meter_table);
> >       lflow_resource_destroy(&flow_output_data->lflow_resource_ref);
> > -    lflow_expr_destroy(&flow_output_data->lflow_expr_cache);
> >   }
> >
> >   static void
> > diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> > index 00a021236..21bf51c22 100644
> > --- a/include/ovn/expr.h
> > +++ b/include/ovn/expr.h
> > @@ -404,12 +404,10 @@ void expr_destroy(struct expr *);
> >
> >   struct expr *expr_annotate(struct expr *, const struct shash *symtab,
> >                              char **errorp);
> > -struct expr *expr_simplify(struct expr *);
> > -struct expr *expr_evaluate_condition(
> > -    struct expr *,
> > -    bool (*is_chassis_resident)(const void *c_aux,
> > -                                const char *port_name),
> > -    const void *c_aux);
> > +struct expr *expr_simplify(struct expr *,
> > +                           bool (*is_chassis_resident)(const void
> *c_aux,
> > +                                                       const char
> *port_name),
> > +                           const void *c_aux);
> >   struct expr *expr_normalize(struct expr *);
> >
> >   bool expr_honors_invariants(const struct expr *);
> > diff --git a/lib/expr.c b/lib/expr.c
> > index 12557e3ca..78646a1af 100644
> > --- a/lib/expr.c
> > +++ b/lib/expr.c
> > @@ -2033,10 +2033,10 @@ expr_simplify_relational(struct expr *expr)
> >
> >   /* Resolves condition and replaces the expression with a boolean. */
> >   static struct expr *
> > -expr_evaluate_condition__(struct expr *expr,
> > -                          bool (*is_chassis_resident)(const void *c_aux,
> > +expr_simplify_condition(struct expr *expr,
> > +                        bool (*is_chassis_resident)(const void *c_aux,
> >                                                       const char
> *port_name),
> > -                          const void *c_aux)
> > +                        const void *c_aux)
> >   {
> >       bool result;
> >
> > @@ -2054,41 +2054,13 @@ expr_evaluate_condition__(struct expr *expr,
> >       return expr_create_boolean(result);
> >   }
> >
> > -struct expr *
> > -expr_evaluate_condition(struct expr *expr,
> > -                        bool (*is_chassis_resident)(const void *c_aux,
> > -                                                    const char
> *port_name),
> > -                        const void *c_aux)
> > -{
> > -    struct expr *sub, *next;
> > -
> > -    switch (expr->type) {
> > -    case EXPR_T_AND:
> > -    case EXPR_T_OR:
> > -         LIST_FOR_EACH_SAFE (sub, next, node, &expr->andor) {
> > -            ovs_list_remove(&sub->node);
> > -            struct expr *e = expr_evaluate_condition(sub,
> is_chassis_resident,
> > -                                                     c_aux);
> > -            e = expr_fix(e);
> > -            expr_insert_andor(expr, next, e);
> > -        }
> > -        return expr_fix(expr);
> > -
> > -    case EXPR_T_CONDITION:
> > -        return expr_evaluate_condition__(expr, is_chassis_resident,
> c_aux);
> > -
> > -    case EXPR_T_CMP:
> > -    case EXPR_T_BOOLEAN:
> > -        return expr;
> > -    }
> > -
> > -    OVS_NOT_REACHED();
> > -}
> > -
> >   /* Takes ownership of 'expr' and returns an equivalent expression whose
> >    * EXPR_T_CMP nodes use only tests for equality (EXPR_R_EQ). */
> >   struct expr *
> > -expr_simplify(struct expr *expr)
> > +expr_simplify(struct expr *expr,
> > +              bool (*is_chassis_resident)(const void *c_aux,
> > +                                        const char *port_name),
> > +              const void *c_aux)
> >   {
> >       struct expr *sub, *next;
> >
> > @@ -2103,7 +2075,8 @@ expr_simplify(struct expr *expr)
> >       case EXPR_T_OR:
> >           LIST_FOR_EACH_SAFE (sub, next, node, &expr->andor) {
> >               ovs_list_remove(&sub->node);
> > -            expr_insert_andor(expr, next, expr_simplify(sub));
> > +            expr_insert_andor(expr, next,
> > +                              expr_simplify(sub, is_chassis_resident,
> c_aux));
> >           }
> >           return expr_fix(expr);
> >
> > @@ -2111,7 +2084,7 @@ expr_simplify(struct expr *expr)
> >           return expr;
> >
> >       case EXPR_T_CONDITION:
> > -        return expr;
> > +        return expr_simplify_condition(expr, is_chassis_resident,
> c_aux);
> >       }
> >       OVS_NOT_REACHED();
> >   }
> > @@ -3393,8 +3366,7 @@ expr_parse_microflow__(struct lexer *lexer,
> >       struct ds annotated = DS_EMPTY_INITIALIZER;
> >       expr_format(e, &annotated);
> >
> > -    e = expr_simplify(e);
> > -    e = expr_evaluate_condition(e, microflow_is_chassis_resident_cb,
> NULL);
> > +    e = expr_simplify(e, microflow_is_chassis_resident_cb, NULL);
> >       e = expr_normalize(e);
> >
> >       struct match m = MATCH_CATCHALL_INITIALIZER;
> > diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> > index c607a8f89..11697ebd0 100644
> > --- a/tests/test-ovn.c
> > +++ b/tests/test-ovn.c
> > @@ -308,9 +308,8 @@ test_parse_expr__(int steps)
> >           }
> >           if (!error) {
> >               if (steps > 1) {
> > -                expr = expr_simplify(expr);
> > -                expr = expr_evaluate_condition(expr,
> is_chassis_resident_cb,
> > -                                               &ports);
> > +                expr = expr_simplify(expr, is_chassis_resident_cb,
> > +                                     &ports);
> >               }
> >               if (steps > 2) {
> >                   expr = expr_normalize(expr);
> > @@ -911,9 +910,9 @@ test_tree_shape_exhaustively(struct expr *expr,
> struct shash *symtab,
> >                   exit(EXIT_FAILURE);
> >               }
> >           } else if (operation >= OP_SIMPLIFY) {
> > -            modified = expr_simplify(expr_clone(expr));
> > -            modified = expr_evaluate_condition(
> > -                modified, tree_shape_is_chassis_resident_cb, NULL);
> > +            modified = expr_simplify(expr_clone(expr),
> > +                                     tree_shape_is_chassis_resident_cb,
> > +                                     NULL);
> >               ovs_assert(expr_honors_invariants(modified));
> >
> >               if (operation >= OP_NORMALIZE) {
> > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> > index 7279452ee..84e5f2b5c 100644
> > --- a/utilities/ovn-trace.c
> > +++ b/utilities/ovn-trace.c
> > @@ -926,10 +926,8 @@ read_flows(void)
> >               continue;
> >           }
> >           if (match) {
> > -            match = expr_simplify(match);
> > -            match = expr_evaluate_condition(match,
> > -
> ovntrace_is_chassis_resident,
> > -                                            NULL);
> > +            match = expr_simplify(match, ovntrace_is_chassis_resident,
> > +                                  NULL);
> >           }
> >
> >           struct ovntrace_flow *flow = xzalloc(sizeof *flow);
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>

Patch
diff mbox series

diff --git a/controller/lflow.c b/controller/lflow.c
index 55e6b8b0a..ee11fc617 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -66,7 +66,6 @@  consider_logical_flow(const struct sbrec_logical_flow *lflow,
                       struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
                       struct hmap *nd_ra_opts,
                       struct controller_event_options *controller_event_opts,
-                      bool delete_expr_from_cache,
                       struct lflow_ctx_in *l_ctx_in,
                       struct lflow_ctx_out *l_ctx_out);
 
@@ -256,75 +255,6 @@  lflow_resource_destroy_lflow(struct lflow_resource_ref *lfrr,
     free(lfrn);
 }
 
-/* Represents expr tree for the lflow. We don't store the
- * match in this structure because -
- *   - Whenever a flow match or action needs to be modified,
- *     ovn-northd deletes the existing flow in the logical_flow
- *     table and adds a new one.
- *  We may want to revisit this and store match incase the behavior
- *  of ovn-northd changes.
- */
-struct lflow_expr {
-    struct hmap_node node;
-    struct uuid lflow_uuid; /* key */
-    struct expr *expr;
-    struct sset addr_sets_ref;
-    struct sset port_groups_ref;
-};
-
-static void
-lflow_expr_add(struct hmap *lflow_expr_cache,
-               const struct sbrec_logical_flow *lflow,
-               struct expr *lflow_expr, struct sset *addr_sets_ref,
-               struct sset *port_groups_ref)
-{
-    struct lflow_expr *le = xmalloc(sizeof *le);
-    le->lflow_uuid = lflow->header_.uuid;
-    le->expr = lflow_expr;
-    sset_clone(&le->addr_sets_ref, addr_sets_ref);
-    sset_clone(&le->port_groups_ref, port_groups_ref);
-    hmap_insert(lflow_expr_cache, &le->node, uuid_hash(&le->lflow_uuid));
-}
-
-static struct lflow_expr *
-lflow_expr_get(struct hmap *lflow_expr_cache,
-               const struct sbrec_logical_flow *lflow)
-{
-    struct lflow_expr *le;
-    size_t hash = uuid_hash(&lflow->header_.uuid);
-    HMAP_FOR_EACH_WITH_HASH (le, node, hash, lflow_expr_cache) {
-        if (uuid_equals(&le->lflow_uuid, &lflow->header_.uuid)) {
-            return le;
-        }
-    }
-
-    return NULL;
-}
-
-static void
-lflow_expr_delete(struct hmap *lflow_expr_cache, struct lflow_expr *le)
-{
-    hmap_remove(lflow_expr_cache, &le->node);
-    expr_destroy(le->expr);
-    sset_destroy(&le->addr_sets_ref);
-    sset_destroy(&le->port_groups_ref);
-    free(le);
-}
-
-void
-lflow_expr_destroy(struct hmap *lflow_expr_cache)
-{
-    struct lflow_expr *le;
-    HMAP_FOR_EACH_POP (le, node, lflow_expr_cache) {
-        expr_destroy(le->expr);
-        sset_destroy(&le->addr_sets_ref);
-        sset_destroy(&le->port_groups_ref);
-        free(le);
-    }
-
-    hmap_destroy(lflow_expr_cache);
-}
-
 /* Adds the logical flows from the Logical_Flow table to flow tables. */
 static void
 add_logical_flows(struct lflow_ctx_in *l_ctx_in,
@@ -357,7 +287,7 @@  add_logical_flows(struct lflow_ctx_in *l_ctx_in,
 
     SBREC_LOGICAL_FLOW_TABLE_FOR_EACH (lflow, l_ctx_in->logical_flow_table) {
         if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
-                                   &nd_ra_opts, &controller_event_opts, false,
+                                   &nd_ra_opts, &controller_event_opts,
                                    l_ctx_in, l_ctx_out)) {
             static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
             VLOG_ERR_RL(&rl, "Conjunction id overflow when processing lflow "
@@ -411,11 +341,6 @@  lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in,
             /* Delete entries from lflow resource reference. */
             lflow_resource_destroy_lflow(l_ctx_out->lfrr,
                                          &lflow->header_.uuid);
-            struct lflow_expr *le =
-                lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow);
-            if (le) {
-                lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
-            }
         }
     }
 
@@ -441,7 +366,7 @@  lflow_handle_changed_flows(struct lflow_ctx_in *l_ctx_in,
                      UUID_ARGS(&lflow->header_.uuid));
             if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
                                        &nd_ra_opts, &controller_event_opts,
-                                       true, l_ctx_in, l_ctx_out)) {
+                                       l_ctx_in, l_ctx_out)) {
                 ret = false;
                 break;
             }
@@ -526,7 +451,7 @@  lflow_handle_changed_ref(enum ref_type ref_type, const char *ref_name,
 
         if (!consider_logical_flow(lflow, &dhcp_opts, &dhcpv6_opts,
                                    &nd_ra_opts, &controller_event_opts,
-                                   true, l_ctx_in, l_ctx_out)) {
+                                   l_ctx_in, l_ctx_out)) {
             ret = false;
             break;
         }
@@ -557,31 +482,11 @@  update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs)
     return true;
 }
 
-static void
-lflow_update_resource_refs(const struct sbrec_logical_flow *lflow,
-                           struct sset *addr_sets_ref,
-                           struct sset *port_groups_ref,
-                           struct lflow_resource_ref *lfrr)
-{
-    const char *addr_set_name;
-    SSET_FOR_EACH (addr_set_name, addr_sets_ref) {
-        lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name,
-                           &lflow->header_.uuid);
-    }
-
-    const char *port_group_name;
-    SSET_FOR_EACH (port_group_name, port_groups_ref) {
-        lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name,
-                           &lflow->header_.uuid);
-    }
-}
-
 static bool
 consider_logical_flow(const struct sbrec_logical_flow *lflow,
                       struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
                       struct hmap *nd_ra_opts,
                       struct controller_event_options *controller_event_opts,
-                      bool delete_expr_from_cache,
                       struct lflow_ctx_in *l_ctx_in,
                       struct lflow_ctx_out *l_ctx_out)
 {
@@ -641,83 +546,59 @@  consider_logical_flow(const struct sbrec_logical_flow *lflow,
 
     /* Translate OVN match into table of OpenFlow matches. */
     struct hmap matches;
-    struct expr *expr = NULL;
+    struct expr *expr;
 
-    struct lflow_expr *le = lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow);
-    if (le) {
-        if (delete_expr_from_cache) {
-            lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
-            le = NULL;
-        } else {
-            expr = le->expr;
-        }
+    struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
+    struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
+    expr = expr_parse_string(lflow->match, &symtab, l_ctx_in->addr_sets,
+                             l_ctx_in->port_groups,
+                             &addr_sets_ref, &port_groups_ref, &error);
+    const char *addr_set_name;
+    SSET_FOR_EACH (addr_set_name, &addr_sets_ref) {
+        lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET, addr_set_name,
+                           &lflow->header_.uuid);
     }
+    const char *port_group_name;
+    SSET_FOR_EACH (port_group_name, &port_groups_ref) {
+        lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP,
+                           port_group_name, &lflow->header_.uuid);
+    }
+    sset_destroy(&addr_sets_ref);
+    sset_destroy(&port_groups_ref);
 
-    if (!expr) {
-        struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
-        struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
-
-        expr = expr_parse_string(lflow->match, &symtab, l_ctx_in->addr_sets,
-                                 l_ctx_in->port_groups, &addr_sets_ref,
-                                 &port_groups_ref, &error);
-        /* Add the address set and port groups (if any) to the lflow
-         * references. */
-        lflow_update_resource_refs(lflow, &addr_sets_ref, &port_groups_ref,
-                                   l_ctx_out->lfrr);
-
-        if (!error) {
-            if (prereqs) {
-                expr = expr_combine(EXPR_T_AND, expr, prereqs);
-                prereqs = NULL;
-            }
-            expr = expr_annotate(expr, &symtab, &error);
+    if (!error) {
+        if (prereqs) {
+            expr = expr_combine(EXPR_T_AND, expr, prereqs);
+            prereqs = NULL;
         }
-        if (error) {
-            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
-            VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s",
-                        lflow->match, error);
-            expr_destroy(prereqs);
-            free(error);
-            ovnacts_free(ovnacts.data, ovnacts.size);
-            ofpbuf_uninit(&ovnacts);
-            sset_destroy(&addr_sets_ref);
-            sset_destroy(&port_groups_ref);
-            return true;
-        }
-
-        expr = expr_simplify(expr);
-        expr = expr_normalize(expr);
-
-        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr,
-                       &addr_sets_ref, &port_groups_ref);
-        sset_destroy(&addr_sets_ref);
-        sset_destroy(&port_groups_ref);
-    } else {
+        expr = expr_annotate(expr, &symtab, &error);
+    }
+    if (error) {
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+        VLOG_WARN_RL(&rl, "error parsing match \"%s\": %s",
+                     lflow->match, error);
         expr_destroy(prereqs);
-        /* Add the cached address set and port groups (if any) to the lflow
-         * references. */
-        lflow_update_resource_refs(lflow, &le->addr_sets_ref,
-                                   &le->port_groups_ref, l_ctx_out->lfrr);
+        free(error);
+        ovnacts_free(ovnacts.data, ovnacts.size);
+        ofpbuf_uninit(&ovnacts);
+        return true;
     }
 
-    struct condition_aux cond_aux = {
-        .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
-        .chassis = l_ctx_in->chassis,
-        .active_tunnels = l_ctx_in->active_tunnels,
-    };
-
-    expr = expr_clone(expr);
-    expr = expr_evaluate_condition(expr, is_chassis_resident_cb, &cond_aux);
-
     struct lookup_port_aux aux = {
         .sbrec_multicast_group_by_name_datapath
             = l_ctx_in->sbrec_multicast_group_by_name_datapath,
         .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
         .dp = lflow->logical_datapath
     };
+    struct condition_aux cond_aux = {
+        .sbrec_port_binding_by_name = l_ctx_in->sbrec_port_binding_by_name,
+        .chassis = l_ctx_in->chassis,
+        .active_tunnels = l_ctx_in->active_tunnels,
+    };
+    expr = expr_simplify(expr, is_chassis_resident_cb, &cond_aux);
+    expr = expr_normalize(expr);
     uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, &aux,
                                        &matches);
-
     expr_destroy(expr);
 
     if (hmap_is_empty(&matches)) {
@@ -954,21 +835,6 @@  lflow_run(struct lflow_ctx_in *l_ctx_in, struct lflow_ctx_out *l_ctx_out)
 {
     COVERAGE_INC(lflow_run);
 
-    /* when lflow_run is called, it's possible that some of the logical flows
-     * are deleted. We need to delete the lflow expr cache for these lflows,
-     * otherwise, they will not be deleted at all. */
-    const struct sbrec_logical_flow *lflow;
-    SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_TRACKED (lflow,
-                                               l_ctx_in->logical_flow_table) {
-        if (sbrec_logical_flow_is_deleted(lflow)) {
-            struct lflow_expr *le =
-                lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow);
-            if (le) {
-                lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
-            }
-        }
-    }
-
     add_logical_flows(l_ctx_in, l_ctx_out);
     add_neighbor_flows(l_ctx_in->sbrec_port_binding_by_name,
                        l_ctx_in->mac_binding_table, l_ctx_in->local_datapaths,
diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
index cacaaa578..303e5708d 100644
--- a/controller/ovn-controller.c
+++ b/controller/ovn-controller.c
@@ -1240,9 +1240,6 @@  struct ed_type_flow_output {
     uint32_t conj_id_ofs;
     /* lflow resource cross reference */
     struct lflow_resource_ref lflow_resource_ref;
-
-    /* Cache of lflow expr tree. */
-    struct hmap lflow_expr_cache;
 };
 
 static void init_physical_ctx(struct engine_node *node,
@@ -1380,7 +1377,6 @@  static void init_lflow_ctx(struct engine_node *node,
     l_ctx_out->group_table = &fo->group_table;
     l_ctx_out->meter_table = &fo->meter_table;
     l_ctx_out->lfrr = &fo->lflow_resource_ref;
-    l_ctx_out->lflow_expr_cache = &fo->lflow_expr_cache;
     l_ctx_out->conj_id_ofs = &fo->conj_id_ofs;
 }
 
@@ -1395,7 +1391,6 @@  en_flow_output_init(struct engine_node *node OVS_UNUSED,
     ovn_extend_table_init(&data->meter_table);
     data->conj_id_ofs = 1;
     lflow_resource_init(&data->lflow_resource_ref);
-    hmap_init(&data->lflow_expr_cache);
     return data;
 }
 
@@ -1407,7 +1402,6 @@  en_flow_output_cleanup(void *data)
     ovn_extend_table_destroy(&flow_output_data->group_table);
     ovn_extend_table_destroy(&flow_output_data->meter_table);
     lflow_resource_destroy(&flow_output_data->lflow_resource_ref);
-    lflow_expr_destroy(&flow_output_data->lflow_expr_cache);
 }
 
 static void
diff --git a/include/ovn/expr.h b/include/ovn/expr.h
index 00a021236..21bf51c22 100644
--- a/include/ovn/expr.h
+++ b/include/ovn/expr.h
@@ -404,12 +404,10 @@  void expr_destroy(struct expr *);
 
 struct expr *expr_annotate(struct expr *, const struct shash *symtab,
                            char **errorp);
-struct expr *expr_simplify(struct expr *);
-struct expr *expr_evaluate_condition(
-    struct expr *,
-    bool (*is_chassis_resident)(const void *c_aux,
-                                const char *port_name),
-    const void *c_aux);
+struct expr *expr_simplify(struct expr *,
+                           bool (*is_chassis_resident)(const void *c_aux,
+                                                       const char *port_name),
+                           const void *c_aux);
 struct expr *expr_normalize(struct expr *);
 
 bool expr_honors_invariants(const struct expr *);
diff --git a/lib/expr.c b/lib/expr.c
index 12557e3ca..78646a1af 100644
--- a/lib/expr.c
+++ b/lib/expr.c
@@ -2033,10 +2033,10 @@  expr_simplify_relational(struct expr *expr)
 
 /* Resolves condition and replaces the expression with a boolean. */
 static struct expr *
-expr_evaluate_condition__(struct expr *expr,
-                          bool (*is_chassis_resident)(const void *c_aux,
+expr_simplify_condition(struct expr *expr,
+                        bool (*is_chassis_resident)(const void *c_aux,
                                                     const char *port_name),
-                          const void *c_aux)
+                        const void *c_aux)
 {
     bool result;
 
@@ -2054,41 +2054,13 @@  expr_evaluate_condition__(struct expr *expr,
     return expr_create_boolean(result);
 }
 
-struct expr *
-expr_evaluate_condition(struct expr *expr,
-                        bool (*is_chassis_resident)(const void *c_aux,
-                                                    const char *port_name),
-                        const void *c_aux)
-{
-    struct expr *sub, *next;
-
-    switch (expr->type) {
-    case EXPR_T_AND:
-    case EXPR_T_OR:
-         LIST_FOR_EACH_SAFE (sub, next, node, &expr->andor) {
-            ovs_list_remove(&sub->node);
-            struct expr *e = expr_evaluate_condition(sub, is_chassis_resident,
-                                                     c_aux);
-            e = expr_fix(e);
-            expr_insert_andor(expr, next, e);
-        }
-        return expr_fix(expr);
-
-    case EXPR_T_CONDITION:
-        return expr_evaluate_condition__(expr, is_chassis_resident, c_aux);
-
-    case EXPR_T_CMP:
-    case EXPR_T_BOOLEAN:
-        return expr;
-    }
-
-    OVS_NOT_REACHED();
-}
-
 /* Takes ownership of 'expr' and returns an equivalent expression whose
  * EXPR_T_CMP nodes use only tests for equality (EXPR_R_EQ). */
 struct expr *
-expr_simplify(struct expr *expr)
+expr_simplify(struct expr *expr,
+              bool (*is_chassis_resident)(const void *c_aux,
+                                        const char *port_name),
+              const void *c_aux)
 {
     struct expr *sub, *next;
 
@@ -2103,7 +2075,8 @@  expr_simplify(struct expr *expr)
     case EXPR_T_OR:
         LIST_FOR_EACH_SAFE (sub, next, node, &expr->andor) {
             ovs_list_remove(&sub->node);
-            expr_insert_andor(expr, next, expr_simplify(sub));
+            expr_insert_andor(expr, next,
+                              expr_simplify(sub, is_chassis_resident, c_aux));
         }
         return expr_fix(expr);
 
@@ -2111,7 +2084,7 @@  expr_simplify(struct expr *expr)
         return expr;
 
     case EXPR_T_CONDITION:
-        return expr;
+        return expr_simplify_condition(expr, is_chassis_resident, c_aux);
     }
     OVS_NOT_REACHED();
 }
@@ -3393,8 +3366,7 @@  expr_parse_microflow__(struct lexer *lexer,
     struct ds annotated = DS_EMPTY_INITIALIZER;
     expr_format(e, &annotated);
 
-    e = expr_simplify(e);
-    e = expr_evaluate_condition(e, microflow_is_chassis_resident_cb, NULL);
+    e = expr_simplify(e, microflow_is_chassis_resident_cb, NULL);
     e = expr_normalize(e);
 
     struct match m = MATCH_CATCHALL_INITIALIZER;
diff --git a/tests/test-ovn.c b/tests/test-ovn.c
index c607a8f89..11697ebd0 100644
--- a/tests/test-ovn.c
+++ b/tests/test-ovn.c
@@ -308,9 +308,8 @@  test_parse_expr__(int steps)
         }
         if (!error) {
             if (steps > 1) {
-                expr = expr_simplify(expr);
-                expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
-                                               &ports);
+                expr = expr_simplify(expr, is_chassis_resident_cb,
+                                     &ports);
             }
             if (steps > 2) {
                 expr = expr_normalize(expr);
@@ -911,9 +910,9 @@  test_tree_shape_exhaustively(struct expr *expr, struct shash *symtab,
                 exit(EXIT_FAILURE);
             }
         } else if (operation >= OP_SIMPLIFY) {
-            modified = expr_simplify(expr_clone(expr));
-            modified = expr_evaluate_condition(
-                modified, tree_shape_is_chassis_resident_cb, NULL);
+            modified = expr_simplify(expr_clone(expr),
+                                     tree_shape_is_chassis_resident_cb,
+                                     NULL);
             ovs_assert(expr_honors_invariants(modified));
 
             if (operation >= OP_NORMALIZE) {
diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
index 7279452ee..84e5f2b5c 100644
--- a/utilities/ovn-trace.c
+++ b/utilities/ovn-trace.c
@@ -926,10 +926,8 @@  read_flows(void)
             continue;
         }
         if (match) {
-            match = expr_simplify(match);
-            match = expr_evaluate_condition(match,
-                                            ovntrace_is_chassis_resident,
-                                            NULL);
+            match = expr_simplify(match, ovntrace_is_chassis_resident,
+                                  NULL);
         }
 
         struct ovntrace_flow *flow = xzalloc(sizeof *flow);