On Wed, Sep 9, 2020 at 7:09 PM Mark Michelson <mmich...@redhat.com> wrote:
>
> For the whole series:
>
> Acked-by: Mark Michelson <mmich...@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.
>
Thanks Mark for the review and Acks.
I applied the series to master after addressing the above comments.
I also did the below changes in patch 4
*****
diff --git a/controller/lflow.c b/controller/lflow.c
index 1195fdb84b..e785866d19 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -985,6 +985,8 @@ consider_logical_flow(const struct
sbrec_logical_flow *lflow,
add_matches_to_flow_table(lflow, matches, lc->conj_id_ofs,
ptable, output_ptable, &ovnacts, ingress,
l_ctx_in, l_ctx_out);
+ ovnacts_free(ovnacts.data, ovnacts.size);
+ ofpbuf_uninit(&ovnacts);
if (!is_cr_cond_present && lc->type == LCACHE_T_EXPR) {
/* If 'is_chassis_resident' match is not present, then cache
*****
I had missed freeing the ovnacts ofpbuffer in one place.
Thanks
Numan
> On 9/9/20 6:09 AM, num...@ovn.org wrote:
> > From: Numan Siddique <num...@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 <num...@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);
> >
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
