| Message ID | 20200225155001.393465-1-numans@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn] ovn-controller: Revert lflow expr caching | expand |
*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); >
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 > >
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);