On Fri, Feb 28, 2020, 3:13 AM Mark Michelson <[email protected]> wrote:
> *sigh* > > Acked-by: Mark Michelson <[email protected]> > Thanks. I applied this patch to master and branch-20.03 Thanks Numan > :( > > On 2/25/20 10:50 AM, [email protected] wrote: > > From: Numan Siddique <[email protected]> > > > > 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 <[email protected]> > > Signed-off-by: Numan Siddique <[email protected]> > > --- > > 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
