Using cache improves performance of recomputation of lflows(by about 30%) Exising lflow cache for `matches` and `expressions` is adopted to include `actions` as well.
Co-authored-by: Felix Huettner <[email protected]> Signed-off-by: Felix Huettner <[email protected]> Signed-off-by: Ihtisham ul Haq <[email protected]> --- controller/lflow-cache.c | 12 ++++++++-- controller/lflow-cache.h | 8 +++++-- controller/lflow.c | 41 +++++++++++++++++++++------------- controller/test-lflow-cache.c | 42 +++++++++++++++++++++++++++-------- tests/ovn-lflow-cache.at | 41 ++++++++++++++++++++++++++++++++++ 5 files changed, 115 insertions(+), 29 deletions(-) diff --git a/controller/lflow-cache.c b/controller/lflow-cache.c index f723eab8a..a648d77af 100644 --- a/controller/lflow-cache.c +++ b/controller/lflow-cache.c @@ -25,7 +25,9 @@ #include "lflow-cache.h" #include "lib/uuid.h" #include "memory-trim.h" +#include "openvswitch/ofpbuf.h" #include "openvswitch/vlog.h" +#include "ovn/actions.h" #include "ovn/expr.h" VLOG_DEFINE_THIS_MODULE(lflow_cache); @@ -209,7 +211,8 @@ lflow_cache_get_stats(const struct lflow_cache *lc, struct ds *output) void lflow_cache_add_expr(struct lflow_cache *lc, const struct uuid *lflow_uuid, - struct expr *expr, size_t expr_sz) + struct expr *expr, size_t expr_sz, + struct ofpbuf *actions) { struct lflow_cache_value *lcv = lflow_cache_add__(lc, lflow_uuid, LCACHE_T_EXPR, expr_sz); @@ -220,12 +223,14 @@ lflow_cache_add_expr(struct lflow_cache *lc, const struct uuid *lflow_uuid, } COVERAGE_INC(lflow_cache_add_expr); lcv->expr = expr; + lcv->actions = actions; } void lflow_cache_add_matches(struct lflow_cache *lc, const struct uuid *lflow_uuid, uint32_t conj_id_ofs, uint32_t n_conjs, - struct hmap *matches, size_t matches_sz) + struct hmap *matches, size_t matches_sz, + struct ofpbuf *actions) { struct lflow_cache_value *lcv = lflow_cache_add__(lc, lflow_uuid, LCACHE_T_MATCHES, matches_sz); @@ -239,6 +244,7 @@ lflow_cache_add_matches(struct lflow_cache *lc, const struct uuid *lflow_uuid, lcv->expr_matches = matches; lcv->n_conjs = n_conjs; lcv->conj_id_ofs = conj_id_ofs; + lcv->actions = actions; } struct lflow_cache_value * @@ -380,11 +386,13 @@ lflow_cache_delete__(struct lflow_cache *lc, struct lflow_cache_entry *lce) case LCACHE_T_EXPR: COVERAGE_INC(lflow_cache_free_expr); expr_destroy(lce->value.expr); + ovnacts_free((*lce->value.actions).data, (*lce->value.actions).size); break; case LCACHE_T_MATCHES: COVERAGE_INC(lflow_cache_free_matches); expr_matches_destroy(lce->value.expr_matches); free(lce->value.expr_matches); + ovnacts_free((*lce->value.actions).data, (*lce->value.actions).size); break; } diff --git a/controller/lflow-cache.h b/controller/lflow-cache.h index 300a56077..9d542beec 100644 --- a/controller/lflow-cache.h +++ b/controller/lflow-cache.h @@ -50,6 +50,8 @@ struct lflow_cache_value { uint32_t n_conjs; uint32_t conj_id_ofs; + struct ofpbuf *actions; + union { struct hmap *expr_matches; struct expr *expr; @@ -66,11 +68,13 @@ bool lflow_cache_is_enabled(const struct lflow_cache *); void lflow_cache_get_stats(const struct lflow_cache *, struct ds *output); void lflow_cache_add_expr(struct lflow_cache *, const struct uuid *lflow_uuid, - struct expr *expr, size_t expr_sz); + struct expr *expr, size_t expr_sz, + struct ofpbuf *actions); void lflow_cache_add_matches(struct lflow_cache *, const struct uuid *lflow_uuid, uint32_t conj_id_ofs, uint32_t n_conjs, - struct hmap *matches, size_t matches_sz); + struct hmap *matches, size_t matches_sz, + struct ofpbuf *actions); struct lflow_cache_value *lflow_cache_get(struct lflow_cache *, const struct uuid *lflow_uuid); diff --git a/controller/lflow.c b/controller/lflow.c index 0b071138d..5ca676ee8 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -1078,14 +1078,23 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow, struct sset template_vars_ref = SSET_INITIALIZER(&template_vars_ref); struct expr *prereqs = NULL; - if (!lflow_parse_actions(lflow, l_ctx_in, &template_vars_ref, - &ovnacts, &prereqs)) { - ovnacts_free(ovnacts.data, ovnacts.size); - ofpbuf_uninit(&ovnacts); - store_lflow_template_refs(l_ctx_out->lflow_deps_mgr, - &template_vars_ref, lflow); - sset_destroy(&template_vars_ref); - return; + struct lflow_cache_value *lcv = + lflow_cache_get(l_ctx_out->lflow_cache, &lflow->header_.uuid); + enum lflow_cache_type lcv_type = + lcv ? lcv->type : LCACHE_T_NONE; + + if (lcv_type != LCACHE_T_NONE) { + ovnacts = *ofpbuf_clone(lcv->actions); + } else { + if (!lflow_parse_actions(lflow, l_ctx_in, &template_vars_ref, + &ovnacts, &prereqs)) { + ovnacts_free(ovnacts.data, ovnacts.size); + ofpbuf_uninit(&ovnacts); + store_lflow_template_refs(l_ctx_out->lflow_deps_mgr, + &template_vars_ref, lflow); + sset_destroy(&template_vars_ref); + return; + } } struct lookup_port_aux aux = { @@ -1105,11 +1114,6 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow, .deps_mgr = l_ctx_out->lflow_deps_mgr, }; - struct lflow_cache_value *lcv = - lflow_cache_get(l_ctx_out->lflow_cache, &lflow->header_.uuid); - enum lflow_cache_type lcv_type = - lcv ? lcv->type : LCACHE_T_NONE; - struct expr *cached_expr = NULL, *expr = NULL; struct hmap *matches = NULL; size_t matches_size = 0; @@ -1211,17 +1215,20 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow, case LCACHE_T_NONE: /* Cache new entry if caching is enabled. */ if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) { + struct ofpbuf *lcv_actions = ofpbuf_clone(&ovnacts); if (cached_expr && !objdep_mgr_contains_obj(l_ctx_out->lflow_deps_mgr, &lflow->header_.uuid)) { lflow_cache_add_matches(l_ctx_out->lflow_cache, &lflow->header_.uuid, start_conj_id, - n_conjs, matches, matches_size); + n_conjs, matches, matches_size, + lcv_actions); matches = NULL; } else if (cached_expr) { lflow_cache_add_expr(l_ctx_out->lflow_cache, &lflow->header_.uuid, - cached_expr, expr_size(cached_expr)); + cached_expr, expr_size(cached_expr), + lcv_actions); cached_expr = NULL; } } @@ -1236,7 +1243,9 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow, done: expr_destroy(prereqs); - ovnacts_free(ovnacts.data, ovnacts.size); + if (!lflow_cache_is_enabled(l_ctx_out->lflow_cache)) { + ovnacts_free(ovnacts.data, ovnacts.size); + } ofpbuf_uninit(&ovnacts); expr_destroy(expr); expr_destroy(cached_expr); diff --git a/controller/test-lflow-cache.c b/controller/test-lflow-cache.c index 7ce999360..03e216290 100644 --- a/controller/test-lflow-cache.c +++ b/controller/test-lflow-cache.c @@ -16,6 +16,8 @@ #include <config.h> #include "lib/uuid.h" +#include "openvswitch/ofpbuf.h" +#include "ovn/actions.h" #include "ovn/expr.h" #include "tests/ovstest.h" #include "tests/test-utils.h" @@ -39,22 +41,26 @@ test_lflow_cache_add__(struct lflow_cache *lc, const char *op_type, const struct uuid *lflow_uuid, unsigned int conj_id_ofs, unsigned int n_conjs, - struct expr *e) + struct expr *e, struct ofpbuf *a) { + struct ds ovnacts_s = DS_EMPTY_INITIALIZER; + ovnacts_format(a->data, a->size, &ovnacts_s); + printf("ADD %s:\n", op_type); printf(" conj-id-ofs: %u\n", conj_id_ofs); printf(" n_conjs: %u\n", n_conjs); + printf(" action: %s\n", ds_cstr(&ovnacts_s)); if (!strcmp(op_type, "expr")) { lflow_cache_add_expr(lc, lflow_uuid, expr_clone(e), - TEST_LFLOW_CACHE_VALUE_SIZE); + TEST_LFLOW_CACHE_VALUE_SIZE, ofpbuf_clone(a)); } else if (!strcmp(op_type, "matches")) { struct hmap *matches = xmalloc(sizeof *matches); ovs_assert(expr_to_matches(e, NULL, NULL, matches) == 0); ovs_assert(hmap_count(matches) == 1); lflow_cache_add_matches(lc, lflow_uuid, conj_id_ofs, n_conjs, matches, - TEST_LFLOW_CACHE_VALUE_SIZE); + TEST_LFLOW_CACHE_VALUE_SIZE, ofpbuf_clone(a)); } else { OVS_NOT_REACHED(); } @@ -72,8 +78,12 @@ test_lflow_cache_lookup__(struct lflow_cache *lc, return; } + struct ds ovnacts_s = DS_EMPTY_INITIALIZER; + ovnacts_format(lcv->actions->data, lcv->actions->size, &ovnacts_s); + printf(" conj_id_ofs: %"PRIu32"\n", lcv->conj_id_ofs); printf(" n_conjs: %"PRIu32"\n", lcv->n_conjs); + printf(" action: %s\n", ds_cstr(&ovnacts_s)); switch (lcv->type) { case LCACHE_T_EXPR: printf(" type: expr\n"); @@ -110,6 +120,13 @@ test_lflow_cache_operations(struct ovs_cmdl_context *ctx) { struct lflow_cache *lc = lflow_cache_create(); struct expr *e = expr_create_boolean(true); + struct ofpbuf *a = ofpbuf_new(0); + struct ovnact_next *next = ovnact_put_NEXT(a); + next->pipeline = 1; + next->ltable = 2; + next->src_pipeline = OVNACT_P_INGRESS; + next->src_ltable = 3; + bool enabled = !strcmp(ctx->argv[1], "true"); struct uuid *lflow_uuids = NULL; size_t n_allocated_lflow_uuids = 0; @@ -160,7 +177,7 @@ test_lflow_cache_operations(struct ovs_cmdl_context *ctx) uuid_generate(lflow_uuid); test_lflow_cache_add__(lc, op_type, lflow_uuid, conj_id_ofs, - n_conjs, e); + n_conjs, e, a); test_lflow_cache_lookup__(lc, lflow_uuid); } else if (!strcmp(op, "add-del")) { const char *op_type = test_read_value(ctx, shift++, "op_type"); @@ -183,7 +200,7 @@ test_lflow_cache_operations(struct ovs_cmdl_context *ctx) struct uuid lflow_uuid; uuid_generate(&lflow_uuid); test_lflow_cache_add__(lc, op_type, &lflow_uuid, conj_id_ofs, - n_conjs, e); + n_conjs, e, a); test_lflow_cache_lookup__(lc, &lflow_uuid); test_lflow_cache_delete__(lc, &lflow_uuid); test_lflow_cache_lookup__(lc, &lflow_uuid); @@ -264,16 +281,23 @@ test_lflow_cache_negative(struct ovs_cmdl_context *ctx OVS_UNUSED) for (size_t i = 0; i < ARRAY_SIZE(lcs); i++) { struct expr *e = expr_create_boolean(true); + struct ofpbuf *a = ofpbuf_new(0); + struct ovnact_next *next = ovnact_put_NEXT(a); + next->pipeline = 1; + next->ltable = 2; + next->src_pipeline = OVNACT_P_INGRESS; + next->src_ltable = 3; + struct hmap *matches = xmalloc(sizeof *matches); ovs_assert(expr_to_matches(e, NULL, NULL, matches) == 0); ovs_assert(hmap_count(matches) == 1); - lflow_cache_add_expr(lcs[i], NULL, NULL, 0); - lflow_cache_add_expr(lcs[i], NULL, e, expr_size(e)); - lflow_cache_add_matches(lcs[i], NULL, 0, 0, NULL, 0); + lflow_cache_add_expr(lcs[i], NULL, NULL, 0, NULL); + lflow_cache_add_expr(lcs[i], NULL, e, expr_size(e), a); + lflow_cache_add_matches(lcs[i], NULL, 0, 0, NULL, 0, NULL); lflow_cache_add_matches(lcs[i], NULL, 0, 0, matches, - TEST_LFLOW_CACHE_VALUE_SIZE); + TEST_LFLOW_CACHE_VALUE_SIZE, a); lflow_cache_destroy(lcs[i]); } } diff --git a/tests/ovn-lflow-cache.at b/tests/ovn-lflow-cache.at index 593d3eaac..b1f79cb72 100644 --- a/tests/ovn-lflow-cache.at +++ b/tests/ovn-lflow-cache.at @@ -19,9 +19,11 @@ trim count : 0 ADD expr: conj-id-ofs: 2 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 0 n_conjs: 0 + action: next(pipeline=egress, table=2); type: expr Enabled: true high-watermark : 1 @@ -32,9 +34,11 @@ trim count : 0 ADD matches: conj-id-ofs: 3 n_conjs: 2 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 3 n_conjs: 2 + action: next(pipeline=egress, table=2); type: matches Enabled: true high-watermark : 2 @@ -61,9 +65,11 @@ trim count : 0 ADD expr: conj-id-ofs: 2 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 0 n_conjs: 0 + action: next(pipeline=egress, table=2); type: expr DELETE LOOKUP: @@ -77,9 +83,11 @@ trim count : 0 ADD matches: conj-id-ofs: 3 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 3 n_conjs: 1 + action: next(pipeline=egress, table=2); type: matches DELETE LOOKUP: @@ -109,6 +117,7 @@ trim count : 0 ADD expr: conj-id-ofs: 2 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: not found Enabled: false @@ -120,6 +129,7 @@ trim count : 0 ADD matches: conj-id-ofs: 3 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: not found Enabled: false @@ -154,9 +164,11 @@ trim count : 0 ADD expr: conj-id-ofs: 2 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 0 n_conjs: 0 + action: next(pipeline=egress, table=2); type: expr Enabled: true high-watermark : 1 @@ -167,9 +179,11 @@ trim count : 0 ADD matches: conj-id-ofs: 3 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 3 n_conjs: 1 + action: next(pipeline=egress, table=2); type: matches Enabled: true high-watermark : 2 @@ -188,6 +202,7 @@ trim count : 1 ADD expr: conj-id-ofs: 5 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: not found Enabled: false @@ -199,6 +214,7 @@ trim count : 1 ADD matches: conj-id-ofs: 6 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: not found Enabled: false @@ -217,9 +233,11 @@ trim count : 1 ADD expr: conj-id-ofs: 8 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 0 n_conjs: 0 + action: next(pipeline=egress, table=2); type: expr Enabled: true high-watermark : 1 @@ -230,9 +248,11 @@ trim count : 1 ADD matches: conj-id-ofs: 9 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 9 n_conjs: 1 + action: next(pipeline=egress, table=2); type: matches Enabled: true high-watermark : 2 @@ -273,9 +293,11 @@ trim count : 0 ADD expr: conj-id-ofs: 2 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 0 n_conjs: 0 + action: next(pipeline=egress, table=2); type: expr Enabled: true high-watermark : 1 @@ -286,9 +308,11 @@ trim count : 0 ADD matches: conj-id-ofs: 3 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 3 n_conjs: 1 + action: next(pipeline=egress, table=2); type: matches Enabled: true high-watermark : 2 @@ -309,9 +333,11 @@ trim count : 1 ADD expr: conj-id-ofs: 5 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 0 n_conjs: 0 + action: next(pipeline=egress, table=2); type: expr Enabled: true high-watermark : 1 @@ -322,9 +348,11 @@ trim count : 1 ADD matches: conj-id-ofs: 6 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 6 n_conjs: 1 + action: next(pipeline=egress, table=2); type: matches dnl dnl Cache is full but we can evict the expr entry because we're adding @@ -339,6 +367,7 @@ trim count : 1 ADD expr: conj-id-ofs: 7 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: not found dnl @@ -365,6 +394,7 @@ trim count : 2 ADD expr: conj-id-ofs: 9 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: not found dnl @@ -380,6 +410,7 @@ trim count : 2 ADD matches: conj-id-ofs: 10 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: not found dnl @@ -428,9 +459,11 @@ trim count : 0 ADD expr: conj-id-ofs: 1 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 0 n_conjs: 0 + action: next(pipeline=egress, table=2); type: expr Enabled: true high-watermark : 1 @@ -441,9 +474,11 @@ trim count : 0 ADD expr: conj-id-ofs: 2 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 0 n_conjs: 0 + action: next(pipeline=egress, table=2); type: expr Enabled: true high-watermark : 2 @@ -454,9 +489,11 @@ trim count : 0 ADD expr: conj-id-ofs: 3 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 0 n_conjs: 0 + action: next(pipeline=egress, table=2); type: expr Enabled: true high-watermark : 3 @@ -467,9 +504,11 @@ trim count : 0 ADD expr: conj-id-ofs: 4 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 0 n_conjs: 0 + action: next(pipeline=egress, table=2); type: expr Enabled: true high-watermark : 4 @@ -480,9 +519,11 @@ trim count : 0 ADD expr: conj-id-ofs: 5 n_conjs: 1 + action: next(pipeline=egress, table=2); LOOKUP: conj_id_ofs: 0 n_conjs: 0 + action: next(pipeline=egress, table=2); type: expr Enabled: true high-watermark : 5 -- 2.34.1 Diese E Mail enthält möglicherweise vertrauliche Inhalte und ist nur für die Verwertung durch den vorgesehenen Empfänger bestimmt. Sollten Sie nicht der vorgesehene Empfänger sein, setzen Sie den Absender bitte unverzüglich in Kenntnis und löschen diese E Mail. Hinweise zum Datenschutz finden Sie hier<https://www.datenschutz.schwarz>. This e-mail may contain confidential content and is intended only for the specified recipient/s. If you are not the intended recipient, please inform the sender immediately and delete this e-mail. Information on data protection can be found here<https://www.datenschutz.schwarz>. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
