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);
