[ovs-dev] [PATCH ovn v2 3/4] expr: Evaluate the condition expression in a separate step.

Mark Michelson mmichels at redhat.com
Wed Sep 9 13:38:53 UTC 2020


For the whole series:

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

However, there are a few nits for this particular patch that I think 
should be addressed before committing:

1) The commit message says expr_simply() instead of expr_simplify()
2) The comment above expr_normalize() in expr.c needs to be updated to 
mention that 'expr' must have already been simplified and had conditions 
evaluated.
3) In expr_normalize(), the comment above the EXPR_T_CONDITION case 
needs to be altered to say that expr_evaluate_condition resolves 
conditions instead of expr_simplify.

These are all simple enough that you can just update these and then merge.

Warning: I'm also ACK-ing Han's patch series that adds some lflow-level 
caching (but his patch series caches ovn_flows). So there is a chance 
that whoever loses this merge race will have some extra work to get 
their patch to apply cleanly.

On 9/9/20 6:09 AM, numans at ovn.org wrote:
> From: Numan Siddique <numans at ovn.org>
> 
> A similar patch was committed earlier [1] and then reverted [2].
> This patch is similar to [1], but not exactly same.
> 
> A new function is added - expr_evaluate_condition() which evaluates
> the condition expression - is_chassis_resident. expr_simply() will
> no longer evaluates this condition. expr_normalize() is still expected
> to be called after expr_evaluate_condition(). Otherwise it will assert
> if 'is_chassis_resident()' is not evaluated.
> 
> An upcoming commit needs this in order to cache the logical flow expressions
> for logical flows having 'is_chassis_resident()' condition in their match.
> The expr tree after expr_simplify()  for such logical flows is cached. Since
> we can't cache the is_chassis_resident condition, it is separated out.
> (For logical flows which do not have this condition in their match, the expr matches
> will be cached.)
> 
> [1] - 99e3a145927("expr: Evaluate the condition expression in a separate step.")
> [2] - 065bcf46218("ovn-controller: Revert lflow expr caching")
> 
> Signed-off-by: Numan Siddique <numans at ovn.org>
> ---
>   controller/lflow.c    |  3 ++-
>   include/ovn/expr.h    | 10 +++++----
>   lib/expr.c            | 50 +++++++++++++++++++++++++++++++++----------
>   tests/test-ovn.c      | 11 +++++-----
>   utilities/ovn-trace.c |  6 ++++--
>   5 files changed, 57 insertions(+), 23 deletions(-)
> 
> diff --git a/controller/lflow.c b/controller/lflow.c
> index c2f939f90f..c6e1586283 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -684,7 +684,8 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
>           .lflow = lflow,
>           .lfrr = l_ctx_out->lfrr
>       };
> -    expr = expr_simplify(expr, is_chassis_resident_cb, &cond_aux);
> +    expr = expr_simplify(expr);
> +    expr = expr_evaluate_condition(expr, is_chassis_resident_cb, &cond_aux);
>       expr = expr_normalize(expr);
>       uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, &aux,
>                                          &matches);
> diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> index b34fb0e813..ed7b054144 100644
> --- a/include/ovn/expr.h
> +++ b/include/ovn/expr.h
> @@ -432,10 +432,12 @@ void expr_destroy(struct expr *);
>   
>   struct expr *expr_annotate(struct expr *, const struct shash *symtab,
>                              char **errorp);
> -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_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_normalize(struct expr *);
>   
>   bool expr_honors_invariants(const struct expr *);
> diff --git a/lib/expr.c b/lib/expr.c
> index 6fb96757ad..bff48dfd20 100644
> --- a/lib/expr.c
> +++ b/lib/expr.c
> @@ -2063,10 +2063,10 @@ expr_simplify_relational(struct expr *expr)
>   
>   /* Resolves condition and replaces the expression with a boolean. */
>   static struct expr *
> -expr_simplify_condition(struct expr *expr,
> -                        bool (*is_chassis_resident)(const void *c_aux,
> +expr_evaluate_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;
>   
> @@ -2084,13 +2084,41 @@ expr_simplify_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,
> -              bool (*is_chassis_resident)(const void *c_aux,
> -                                        const char *port_name),
> -              const void *c_aux)
> +expr_simplify(struct expr *expr)
>   {
>       struct expr *sub, *next;
>   
> @@ -2105,8 +2133,7 @@ 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, is_chassis_resident, c_aux));
> +            expr_insert_andor(expr, next, expr_simplify(sub));
>           }
>           return expr_fix(expr);
>   
> @@ -2114,7 +2141,7 @@ expr_simplify(struct expr *expr,
>           return expr;
>   
>       case EXPR_T_CONDITION:
> -        return expr_simplify_condition(expr, is_chassis_resident, c_aux);
> +        return expr;
>       }
>       OVS_NOT_REACHED();
>   }
> @@ -3397,7 +3424,8 @@ expr_parse_microflow__(struct lexer *lexer,
>       struct ds annotated = DS_EMPTY_INITIALIZER;
>       expr_format(e, &annotated);
>   
> -    e = expr_simplify(e, microflow_is_chassis_resident_cb, NULL);
> +    e = expr_simplify(e);
> +    e = expr_evaluate_condition(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 ba628288bb..544fee4c87 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -312,8 +312,9 @@ test_parse_expr__(int steps)
>           }
>           if (!error) {
>               if (steps > 1) {
> -                expr = expr_simplify(expr, is_chassis_resident_cb,
> -                                     &ports);
> +                expr = expr_simplify(expr);
> +                expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
> +                                               &ports);
>               }
>               if (steps > 2) {
>                   expr = expr_normalize(expr);
> @@ -914,9 +915,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),
> -                                     tree_shape_is_chassis_resident_cb,
> -                                     NULL);
> +            modified = expr_simplify(expr_clone(expr));
> +            modified = expr_evaluate_condition(
> +                expr_clone(modified), 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 50a32b7149..39839cb709 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -931,8 +931,10 @@ read_flows(void)
>               continue;
>           }
>           if (match) {
> -            match = expr_simplify(match, ovntrace_is_chassis_resident,
> -                                  NULL);
> +            match = expr_simplify(match);
> +            match = expr_evaluate_condition(match,
> +                                            ovntrace_is_chassis_resident,
> +                                            NULL);
>           }
>   
>           struct ovntrace_flow *flow = xzalloc(sizeof *flow);
> 



More information about the dev mailing list