On Wed, May 24, 2023 at 2:12 AM Ihtisham ul Haq via dev <
[email protected]> wrote:
>
> 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]>
Thanks Ihtisham and Felix for the improvement. I haven't fully reviewed it
yet, but here are two questions:
1. Could you share some details of your performance test and the
performance gains of this patch?
2. Did you consider the cases when actions cannot be cached? In other
words, actions that may change during recompute. For example, when template
is used in actions, if a template var value changes, the actions may
change. We can't simply cache such actions and reuse them. Not sure if
there are other examples, but we will need to be careful and examine
possible scenarios .
Regards,
Han
> ---
> 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
