On Wed, Sep 9, 2020 at 1:32 AM Mark Michelson <[email protected]> wrote: > > I know this is weird because I ACKed this exact patch once in the past, > but I found a small issue. See below: > > On 9/2/20 2:46 PM, [email protected] wrote: > > From: Numan Siddique <[email protected]> > > > > This patch was committed earlier [1] and then reverted [2]. > > > > A new function is added - expr_evaluate_condition() which evaluates > > the condition expression - is_chassis_resident. expr_simply() will > > no longer evaluates this condition. > > > > An upcoming commit needs this in order to cache the logical flow expressions > > so that every lflow_*() function which calls consider_logical_flow() doesn't > > need to convert the logical flow match to an expression. Instead the expr > > tree > > for the logical flow is cached. Since we can't cache the is_chassis_resident > > condition, it is separated out. > > > > [1] - 99e3a145927("expr: Evaluate the condition expression in a separate > > step.") > > [2] - 065bcf46218("ovn-controller: Revert lflow expr caching") > > > > Signed-off-by: Numan Siddique <[email protected]> > > --- > > 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..63df5611ec 100644 > > --- a/controller/lflow.c > > +++ b/controller/lflow.c > > @@ -684,8 +684,9 @@ 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_normalize(expr); > > expr_normalize() has the following in it: > > /* Should not hit expression type condition, since expr_normalize > is > > > * only called after expr_simplify, which resolves all conditions. > */ > > > case EXPR_T_CONDITION: > > > > default: > > > > OVS_NOT_REACHED(); > > > > } > > expr_normalize() assumes that expr_simplify has evaluated all > conditions, but now that is not the case. As it turns out, as long as > is_chassis_resident() is part of an AND or OR expression, then things > still work out ok since expr_normalize() is not called recursively. > > However, if for some reason we ever decided to create an expression that > was just a bare is_chassis_resident("blah"), then this would abort. > > Even though I'm doubtful we would ever create an expression that was > just is_chassis_resident("blah"), I think expr_normalize() should be > fixed just in case. If nothing else, change the comment since it is > incorrect now.
Thanks Mark for the review and comments. Excellent catch. I found it a bit hard to allow EXPR_T_CONDITION in expr_normalize(). I think it could complicate the processing. Hence I changed the approach a bit in v2. For logical flows having is_chassis_resident() match, we cache the simplified expr tree now. So after retrieving the simplified expr tree from the cache, we clone the expr, evaluate it and then normalize it. This would definitely cost a bit more. But I think this is better to guarantee accuracy. We can revisit later if we want to allow EXPR_T_CONDITION in expr_normalize(). I think for now it should be ok since we cache the expr matches for the majority of logical flows. Please take a look at v2 - https://patchwork.ozlabs.org/project/ovn/list/?series=200461 https://mail.openvswitch.org/pipermail/ovs-dev/2020-September/374704.html Thanks Numan > > > + expr = expr_evaluate_condition(expr, is_chassis_resident_cb, > > &cond_aux); > > uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, &aux, > > &matches); > > expr_destroy(expr); > > 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); > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
