diff mbox series

[ovs-dev] lflow-cache: Introduce cache for lflow actions

Message ID ZG3Dq8QPzpLAvRKo@SIT-SDELAP1003.int.lidl.net
State Superseded
Headers show
Series [ovs-dev] lflow-cache: Introduce cache for lflow actions | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Ihtisham ul Haq May 24, 2023, 7:58 a.m. UTC 
Using cache improves performance of recomputation of lflows. We see
about 30% improvments during our tests

Exising lflow cache for `matches` and `expressions` is adopted
to include `actions` as well.

Co-authored-by: Felix Huettner <felix.huettner@mail.schwarz>
Signed-off-by: Ihtisham ul Haq <ihtisham.ul_haq@mail.schwarz>
---
 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(-)

--
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>.
diff mbox series

Patch

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..ad5284870 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