[ovs-dev] [PATCH ovn 1/2] lflow-cache: Remove conjunction id cache.

Han Zhou hzhou at ovn.org
Wed Nov 3 06:46:43 UTC 2021


Conjunction id cache is useful to keep conjunction ids consistent between
iterations when match or expr cache is not available. However, the
current implementation caches the conjunction id whenever n_conjs > 0.
It assumes that when the lflow is reprocessed, the conjunction ids
starting from the saved conj_id_ofs to the conj_id_ofs + n_conjs - 1 are
always available. This would be true only if the n_conjs doesn't change.
Unfortunately, the n_conjs can change when a lflow is reprocessed, e.g.
when the members of an address-set/port-group increases from 1 to >1.
This would result in conflict conjunction ids being used by different
lflows. This patch adds a test case to cover this scenario, and the test
passes when conjunction id cache is removed.

A follow-up patch will take a different approach to keep conjunction ids
consistent.

Signed-off-by: Han Zhou <hzhou at ovn.org>
---
 controller/lflow-cache.c      |  24 +--
 controller/lflow-cache.h      |  10 +-
 controller/lflow.c            |   7 -
 controller/test-lflow-cache.c |   8 +-
 tests/ovn-lflow-cache.at      | 272 +++++++---------------------------
 tests/ovn.at                  | 222 ++++++++-------------------
 6 files changed, 122 insertions(+), 421 deletions(-)

diff --git a/controller/lflow-cache.c b/controller/lflow-cache.c
index ece39dbf0..26228c960 100644
--- a/controller/lflow-cache.c
+++ b/controller/lflow-cache.c
@@ -30,10 +30,8 @@
 VLOG_DEFINE_THIS_MODULE(lflow_cache);
 
 COVERAGE_DEFINE(lflow_cache_flush);
-COVERAGE_DEFINE(lflow_cache_add_conj_id);
 COVERAGE_DEFINE(lflow_cache_add_expr);
 COVERAGE_DEFINE(lflow_cache_add_matches);
-COVERAGE_DEFINE(lflow_cache_free_conj_id);
 COVERAGE_DEFINE(lflow_cache_free_expr);
 COVERAGE_DEFINE(lflow_cache_free_matches);
 COVERAGE_DEFINE(lflow_cache_add);
@@ -46,7 +44,6 @@ COVERAGE_DEFINE(lflow_cache_made_room);
 COVERAGE_DEFINE(lflow_cache_trim);
 
 static const char *lflow_cache_type_names[LCACHE_T_MAX] = {
-    [LCACHE_T_CONJ_ID] = "cache-conj-id",
     [LCACHE_T_EXPR]    = "cache-expr",
     [LCACHE_T_MATCHES] = "cache-matches",
 };
@@ -204,20 +201,6 @@ lflow_cache_get_stats(const struct lflow_cache *lc, struct ds *output)
                   ROUND_UP(lc->mem_usage, 1024) / 1024);
 }
 
-void
-lflow_cache_add_conj_id(struct lflow_cache *lc, const struct uuid *lflow_uuid,
-                        uint32_t conj_id_ofs)
-{
-    struct lflow_cache_value *lcv =
-        lflow_cache_add__(lc, lflow_uuid, LCACHE_T_CONJ_ID, 0);
-
-    if (!lcv) {
-        return;
-    }
-    COVERAGE_INC(lflow_cache_add_conj_id);
-    lcv->conj_id_ofs = conj_id_ofs;
-}
-
 void
 lflow_cache_add_expr(struct lflow_cache *lc, const struct uuid *lflow_uuid,
                      uint32_t conj_id_ofs, struct expr *expr, size_t expr_sz)
@@ -294,9 +277,7 @@ lflow_cache_make_room__(struct lflow_cache *lc, enum lflow_cache_type type)
 {
     /* When the cache becomes full, the rule is to prefer more "important"
      * cache entries over less "important" ones.  That is, evict entries of
-     * type LCACHE_T_CONJ_ID if there's no room to add an entry of type
-     * LCACHE_T_EXPR.  Similarly, evict entries of type LCACHE_T_CONJ_ID or
-     * LCACHE_T_EXPR if there's no room to add an entry of type
+     * type LCACHE_T_EXPR if there's no room to add an entry of type
      * LCACHE_T_MATCHES.
      */
     for (size_t i = 0; i < type; i++) {
@@ -372,9 +353,6 @@ lflow_cache_delete__(struct lflow_cache *lc, struct lflow_cache_entry *lce)
     case LCACHE_T_NONE:
         OVS_NOT_REACHED();
         break;
-    case LCACHE_T_CONJ_ID:
-        COVERAGE_INC(lflow_cache_free_conj_id);
-        break;
     case LCACHE_T_EXPR:
         COVERAGE_INC(lflow_cache_free_expr);
         expr_destroy(lce->value.expr);
diff --git a/controller/lflow-cache.h b/controller/lflow-cache.h
index 553e05f8e..d42bdeaa3 100644
--- a/controller/lflow-cache.h
+++ b/controller/lflow-cache.h
@@ -30,14 +30,11 @@ struct lflow_cache;
  *    results in conjunctive OpenvSwitch flows.
  *
  *  - Caches
- *     (1) Conjunction ID offset if the logical flow has port group/address
- *         set references.
- *     (2) expr tree if the logical flow doesn't have port group/address set
+ *     (1) expr tree if the logical flow doesn't have port group/address set
  *         references but has other references (such as lport).
- *     (3) expr matches if the logical flow doesn't have any references.
+ *     (2) expr matches if the logical flow doesn't have any references.
  */
 enum lflow_cache_type {
-    LCACHE_T_CONJ_ID, /* Only conjunction id offset is cached. */
     LCACHE_T_EXPR,    /* Expr tree of the logical flow is cached. */
     LCACHE_T_MATCHES, /* Expression matches are cached. */
     LCACHE_T_MAX,
@@ -63,9 +60,6 @@ void lflow_cache_enable(struct lflow_cache *, bool enabled, uint32_t capacity,
 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_conj_id(struct lflow_cache *,
-                             const struct uuid *lflow_uuid,
-                             uint32_t conj_id_ofs);
 void lflow_cache_add_expr(struct lflow_cache *, const struct uuid *lflow_uuid,
                           uint32_t conj_id_ofs, struct expr *expr,
                           size_t expr_sz);
diff --git a/controller/lflow.c b/controller/lflow.c
index 923d8f0a4..59f5d3a07 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -876,7 +876,6 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
     /* Get match expr, either from cache or from lflow match. */
     switch (lcv_type) {
     case LCACHE_T_NONE:
-    case LCACHE_T_CONJ_ID:
         expr = convert_match_to_expr(lflow, dp, &prereqs, l_ctx_in->addr_sets,
                                      l_ctx_in->port_groups, l_ctx_out->lfrr,
                                      &pg_addr_set_ref);
@@ -903,7 +902,6 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
     /* Normalize expression if needed. */
     switch (lcv_type) {
     case LCACHE_T_NONE:
-    case LCACHE_T_CONJ_ID:
     case LCACHE_T_EXPR:
         expr = expr_evaluate_condition(expr, is_chassis_resident_cb,
                                        &cond_aux);
@@ -916,7 +914,6 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
     /* Get matches, either from cache or from expr computed above. */
     switch (lcv_type) {
     case LCACHE_T_NONE:
-    case LCACHE_T_CONJ_ID:
     case LCACHE_T_EXPR:
         matches = xmalloc(sizeof *matches);
         n_conjs = expr_to_matches(expr, lookup_port_cb, &aux, matches);
@@ -957,13 +954,9 @@ consider_logical_flow__(const struct sbrec_logical_flow *lflow,
                                      &lflow->header_.uuid, conj_id_ofs,
                                      cached_expr, expr_size(cached_expr));
                 cached_expr = NULL;
-            } else if (n_conjs) {
-                lflow_cache_add_conj_id(l_ctx_out->lflow_cache,
-                                        &lflow->header_.uuid, conj_id_ofs);
             }
         }
         break;
-    case LCACHE_T_CONJ_ID:
     case LCACHE_T_EXPR:
         break;
     case LCACHE_T_MATCHES:
diff --git a/controller/test-lflow-cache.c b/controller/test-lflow-cache.c
index b46b1f743..95e619ee0 100644
--- a/controller/test-lflow-cache.c
+++ b/controller/test-lflow-cache.c
@@ -41,9 +41,7 @@ test_lflow_cache_add__(struct lflow_cache *lc, const char *op_type,
     printf("ADD %s:\n", op_type);
     printf("  conj-id-ofs: %u\n", conj_id_ofs);
 
-    if (!strcmp(op_type, "conj-id")) {
-        lflow_cache_add_conj_id(lc, lflow_uuid, conj_id_ofs);
-    } else if (!strcmp(op_type, "expr")) {
+    if (!strcmp(op_type, "expr")) {
         lflow_cache_add_expr(lc, lflow_uuid, conj_id_ofs, expr_clone(e),
                              TEST_LFLOW_CACHE_VALUE_SIZE);
     } else if (!strcmp(op_type, "matches")) {
@@ -71,9 +69,6 @@ test_lflow_cache_lookup__(struct lflow_cache *lc,
 
     printf("  conj_id_ofs: %"PRIu32"\n", lcv->conj_id_ofs);
     switch (lcv->type) {
-    case LCACHE_T_CONJ_ID:
-        printf("  type: conj-id\n");
-        break;
     case LCACHE_T_EXPR:
         printf("  type: expr\n");
         break;
@@ -251,7 +246,6 @@ test_lflow_cache_negative(struct ovs_cmdl_context *ctx OVS_UNUSED)
         ovs_assert(expr_to_matches(e, NULL, NULL, matches) == 0);
         ovs_assert(hmap_count(matches) == 1);
 
-        lflow_cache_add_conj_id(lcs[i], NULL, 0);
         lflow_cache_add_expr(lcs[i], NULL, 0, NULL, 0);
         lflow_cache_add_expr(lcs[i], NULL, 0, e, expr_size(e));
         lflow_cache_add_matches(lcs[i], NULL, NULL, 0);
diff --git a/tests/ovn-lflow-cache.at b/tests/ovn-lflow-cache.at
index cb4604f51..97d24d0dc 100644
--- a/tests/ovn-lflow-cache.at
+++ b/tests/ovn-lflow-cache.at
@@ -6,27 +6,13 @@ AT_BANNER([OVN unit tests - lflow-cache])
 AT_SETUP([unit test -- lflow-cache single add/lookup])
 AT_CHECK(
     [ovstest test-lflow-cache lflow_cache_operations \
-        true 3 \
-        add conj-id 1 \
+        true 2 \
         add expr 2 \
         add matches 3 | grep -v 'Mem usage (KB)'],
     [0], [dnl
 Enabled: true
 high-watermark  : 0
 total           : 0
-cache-conj-id   : 0
-cache-expr      : 0
-cache-matches   : 0
-trim count      : 0
-ADD conj-id:
-  conj-id-ofs: 1
-LOOKUP:
-  conj_id_ofs: 1
-  type: conj-id
-Enabled: true
-high-watermark  : 1
-total           : 1
-cache-conj-id   : 1
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 0
@@ -36,9 +22,8 @@ LOOKUP:
   conj_id_ofs: 2
   type: expr
 Enabled: true
-high-watermark  : 2
-total           : 2
-cache-conj-id   : 1
+high-watermark  : 1
+total           : 1
 cache-expr      : 1
 cache-matches   : 0
 trim count      : 0
@@ -48,9 +33,8 @@ LOOKUP:
   conj_id_ofs: 0
   type: matches
 Enabled: true
-high-watermark  : 3
-total           : 3
-cache-conj-id   : 1
+high-watermark  : 2
+total           : 2
 cache-expr      : 1
 cache-matches   : 1
 trim count      : 0
@@ -60,30 +44,13 @@ AT_CLEANUP
 AT_SETUP([unit test -- lflow-cache single add/lookup/del])
 AT_CHECK(
     [ovstest test-lflow-cache lflow_cache_operations \
-        true 3 \
-        add-del conj-id 1 \
+        true 2 \
         add-del expr 2 \
         add-del matches 3 | grep -v 'Mem usage (KB)'],
     [0], [dnl
 Enabled: true
 high-watermark  : 0
 total           : 0
-cache-conj-id   : 0
-cache-expr      : 0
-cache-matches   : 0
-trim count      : 0
-ADD conj-id:
-  conj-id-ofs: 1
-LOOKUP:
-  conj_id_ofs: 1
-  type: conj-id
-DELETE
-LOOKUP:
-  not found
-Enabled: true
-high-watermark  : 1
-total           : 0
-cache-conj-id   : 0
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 0
@@ -98,7 +65,6 @@ LOOKUP:
 Enabled: true
 high-watermark  : 1
 total           : 0
-cache-conj-id   : 0
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 0
@@ -113,7 +79,6 @@ LOOKUP:
 Enabled: true
 high-watermark  : 1
 total           : 0
-cache-conj-id   : 0
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 0
@@ -123,26 +88,13 @@ AT_CLEANUP
 AT_SETUP([unit test -- lflow-cache disabled single add/lookup/del])
 AT_CHECK(
     [ovstest test-lflow-cache lflow_cache_operations \
-        false 3 \
-        add conj-id 1 \
+        false 2 \
         add expr 2 \
         add matches 3 | grep -v 'Mem usage (KB)'],
     [0], [dnl
 Enabled: false
 high-watermark  : 0
 total           : 0
-cache-conj-id   : 0
-cache-expr      : 0
-cache-matches   : 0
-trim count      : 0
-ADD conj-id:
-  conj-id-ofs: 1
-LOOKUP:
-  not found
-Enabled: false
-high-watermark  : 0
-total           : 0
-cache-conj-id   : 0
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 0
@@ -153,7 +105,6 @@ LOOKUP:
 Enabled: false
 high-watermark  : 0
 total           : 0
-cache-conj-id   : 0
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 0
@@ -164,7 +115,6 @@ LOOKUP:
 Enabled: false
 high-watermark  : 0
 total           : 0
-cache-conj-id   : 0
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 0
@@ -174,16 +124,13 @@ AT_CLEANUP
 AT_SETUP([unit test -- lflow-cache disable/enable/flush])
 AT_CHECK(
     [ovstest test-lflow-cache lflow_cache_operations \
-        true 12 \
-        add conj-id 1 \
+        true 9 \
         add expr 2 \
         add matches 3 \
         disable \
-        add conj-id 4 \
         add expr 5 \
         add matches 6 \
         enable 1000 1024 \
-        add conj-id 7 \
         add expr 8 \
         add matches 9 \
         flush | grep -v 'Mem usage (KB)'],
@@ -191,19 +138,6 @@ AT_CHECK(
 Enabled: true
 high-watermark  : 0
 total           : 0
-cache-conj-id   : 0
-cache-expr      : 0
-cache-matches   : 0
-trim count      : 0
-ADD conj-id:
-  conj-id-ofs: 1
-LOOKUP:
-  conj_id_ofs: 1
-  type: conj-id
-Enabled: true
-high-watermark  : 1
-total           : 1
-cache-conj-id   : 1
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 0
@@ -213,9 +147,8 @@ LOOKUP:
   conj_id_ofs: 2
   type: expr
 Enabled: true
-high-watermark  : 2
-total           : 2
-cache-conj-id   : 1
+high-watermark  : 1
+total           : 1
 cache-expr      : 1
 cache-matches   : 0
 trim count      : 0
@@ -225,9 +158,8 @@ LOOKUP:
   conj_id_ofs: 0
   type: matches
 Enabled: true
-high-watermark  : 3
-total           : 3
-cache-conj-id   : 1
+high-watermark  : 2
+total           : 2
 cache-expr      : 1
 cache-matches   : 1
 trim count      : 0
@@ -235,22 +167,10 @@ DISABLE
 Enabled: false
 high-watermark  : 0
 total           : 0
-cache-conj-id   : 0
 cache-expr      : 0
 cache-matches   : 0
 dnl At "disable" the cache was flushed.
 trim count      : 1
-ADD conj-id:
-  conj-id-ofs: 4
-LOOKUP:
-  not found
-Enabled: false
-high-watermark  : 0
-total           : 0
-cache-conj-id   : 0
-cache-expr      : 0
-cache-matches   : 0
-trim count      : 1
 ADD expr:
   conj-id-ofs: 5
 LOOKUP:
@@ -258,7 +178,6 @@ LOOKUP:
 Enabled: false
 high-watermark  : 0
 total           : 0
-cache-conj-id   : 0
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 1
@@ -269,7 +188,6 @@ LOOKUP:
 Enabled: false
 high-watermark  : 0
 total           : 0
-cache-conj-id   : 0
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 1
@@ -277,19 +195,6 @@ ENABLE
 Enabled: true
 high-watermark  : 0
 total           : 0
-cache-conj-id   : 0
-cache-expr      : 0
-cache-matches   : 0
-trim count      : 1
-ADD conj-id:
-  conj-id-ofs: 7
-LOOKUP:
-  conj_id_ofs: 7
-  type: conj-id
-Enabled: true
-high-watermark  : 1
-total           : 1
-cache-conj-id   : 1
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 1
@@ -299,9 +204,8 @@ LOOKUP:
   conj_id_ofs: 8
   type: expr
 Enabled: true
-high-watermark  : 2
-total           : 2
-cache-conj-id   : 1
+high-watermark  : 1
+total           : 1
 cache-expr      : 1
 cache-matches   : 0
 trim count      : 1
@@ -311,9 +215,8 @@ LOOKUP:
   conj_id_ofs: 0
   type: matches
 Enabled: true
-high-watermark  : 3
-total           : 3
-cache-conj-id   : 1
+high-watermark  : 2
+total           : 2
 cache-expr      : 1
 cache-matches   : 1
 trim count      : 1
@@ -321,7 +224,6 @@ FLUSH
 Enabled: true
 high-watermark  : 0
 total           : 0
-cache-conj-id   : 0
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 2
@@ -331,36 +233,20 @@ AT_CLEANUP
 AT_SETUP([unit test -- lflow-cache set limit])
 AT_CHECK(
     [ovstest test-lflow-cache lflow_cache_operations \
-        true 12 \
-        add conj-id 1 \
+        true 9 \
         add expr 2 \
         add matches 3 \
         enable 1 1024 \
-        add conj-id 4 \
         add expr 5 \
         add matches 6 \
-        add conj-id 7 \
+        add expr 7 \
         enable 1 1 \
-        add conj-id 8 \
         add expr 9 \
         add matches 10 | grep -v 'Mem usage (KB)'],
     [0], [dnl
 Enabled: true
 high-watermark  : 0
 total           : 0
-cache-conj-id   : 0
-cache-expr      : 0
-cache-matches   : 0
-trim count      : 0
-ADD conj-id:
-  conj-id-ofs: 1
-LOOKUP:
-  conj_id_ofs: 1
-  type: conj-id
-Enabled: true
-high-watermark  : 1
-total           : 1
-cache-conj-id   : 1
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 0
@@ -370,9 +256,8 @@ LOOKUP:
   conj_id_ofs: 2
   type: expr
 Enabled: true
-high-watermark  : 2
-total           : 2
-cache-conj-id   : 1
+high-watermark  : 1
+total           : 1
 cache-expr      : 1
 cache-matches   : 0
 trim count      : 0
@@ -382,9 +267,8 @@ LOOKUP:
   conj_id_ofs: 0
   type: matches
 Enabled: true
-high-watermark  : 3
-total           : 3
-cache-conj-id   : 1
+high-watermark  : 2
+total           : 2
 cache-expr      : 1
 cache-matches   : 1
 trim count      : 0
@@ -395,19 +279,6 @@ dnl
 Enabled: true
 high-watermark  : 0
 total           : 0
-cache-conj-id   : 0
-cache-expr      : 0
-cache-matches   : 0
-trim count      : 1
-ADD conj-id:
-  conj-id-ofs: 4
-LOOKUP:
-  conj_id_ofs: 4
-  type: conj-id
-Enabled: true
-high-watermark  : 1
-total           : 1
-cache-conj-id   : 1
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 1
@@ -416,14 +287,9 @@ ADD expr:
 LOOKUP:
   conj_id_ofs: 5
   type: expr
-dnl
-dnl Cache is full but we can evict the conj-id entry because we're adding
-dnl an expr one.
-dnl
 Enabled: true
 high-watermark  : 1
 total           : 1
-cache-conj-id   : 0
 cache-expr      : 1
 cache-matches   : 0
 trim count      : 1
@@ -439,22 +305,20 @@ dnl
 Enabled: true
 high-watermark  : 1
 total           : 1
-cache-conj-id   : 0
 cache-expr      : 0
 cache-matches   : 1
 trim count      : 1
-ADD conj-id:
+ADD expr:
   conj-id-ofs: 7
 LOOKUP:
   not found
 dnl
-dnl Cache is full and we're adding a conj-id entry so we shouldn't evict
+dnl Cache is full and we're adding a expr entry so we shouldn't evict
 dnl anything else.
 dnl
 Enabled: true
 high-watermark  : 1
 total           : 1
-cache-conj-id   : 0
 cache-expr      : 0
 cache-matches   : 1
 trim count      : 1
@@ -466,19 +330,6 @@ dnl
 Enabled: true
 high-watermark  : 0
 total           : 0
-cache-conj-id   : 0
-cache-expr      : 0
-cache-matches   : 0
-trim count      : 2
-ADD conj-id:
-  conj-id-ofs: 8
-LOOKUP:
-  conj_id_ofs: 8
-  type: conj-id
-Enabled: true
-high-watermark  : 1
-total           : 1
-cache-conj-id   : 1
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 2
@@ -491,9 +342,8 @@ dnl Cache is full and we're adding a cache entry that would go over the max
 dnl memory limit so adding should fail.
 dnl
 Enabled: true
-high-watermark  : 1
-total           : 1
-cache-conj-id   : 1
+high-watermark  : 0
+total           : 0
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 2
@@ -506,9 +356,8 @@ dnl Cache is full and we're adding a cache entry that would go over the max
 dnl memory limit so adding should fail.
 dnl
 Enabled: true
-high-watermark  : 1
-total           : 1
-cache-conj-id   : 1
+high-watermark  : 0
+total           : 0
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 2
@@ -520,11 +369,11 @@ AT_CHECK(
     [ovstest test-lflow-cache lflow_cache_operations \
         true 12 \
         enable 1000 1024 trim-limit 100 trim-wmark-perc 100 \
-        add conj-id 1 \
-        add conj-id 2 \
-        add conj-id 3 \
-        add conj-id 4 \
-        add conj-id 5 \
+        add expr 1 \
+        add expr 2 \
+        add expr 3 \
+        add expr 4 \
+        add expr 5 \
         del \
         enable 1000 1024 trim-limit 0 trim-wmark-perc 100 \
         del \
@@ -535,7 +384,6 @@ AT_CHECK(
 Enabled: true
 high-watermark  : 0
 total           : 0
-cache-conj-id   : 0
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 0
@@ -543,68 +391,62 @@ ENABLE
 Enabled: true
 high-watermark  : 0
 total           : 0
-cache-conj-id   : 0
 cache-expr      : 0
 cache-matches   : 0
 trim count      : 0
-ADD conj-id:
+ADD expr:
   conj-id-ofs: 1
 LOOKUP:
   conj_id_ofs: 1
-  type: conj-id
+  type: expr
 Enabled: true
 high-watermark  : 1
 total           : 1
-cache-conj-id   : 1
-cache-expr      : 0
+cache-expr      : 1
 cache-matches   : 0
 trim count      : 0
-ADD conj-id:
+ADD expr:
   conj-id-ofs: 2
 LOOKUP:
   conj_id_ofs: 2
-  type: conj-id
+  type: expr
 Enabled: true
 high-watermark  : 2
 total           : 2
-cache-conj-id   : 2
-cache-expr      : 0
+cache-expr      : 2
 cache-matches   : 0
 trim count      : 0
-ADD conj-id:
+ADD expr:
   conj-id-ofs: 3
 LOOKUP:
   conj_id_ofs: 3
-  type: conj-id
+  type: expr
 Enabled: true
 high-watermark  : 3
 total           : 3
-cache-conj-id   : 3
-cache-expr      : 0
+cache-expr      : 3
 cache-matches   : 0
 trim count      : 0
-ADD conj-id:
+ADD expr:
   conj-id-ofs: 4
 LOOKUP:
   conj_id_ofs: 4
-  type: conj-id
+  type: expr
 Enabled: true
 high-watermark  : 4
 total           : 4
-cache-conj-id   : 4
-cache-expr      : 0
+cache-expr      : 4
 cache-matches   : 0
 trim count      : 0
-ADD conj-id:
+ADD expr:
   conj-id-ofs: 5
 LOOKUP:
   conj_id_ofs: 5
-  type: conj-id
+  type: expr
 Enabled: true
 high-watermark  : 5
 total           : 5
-cache-conj-id   : 5
-cache-expr      : 0
+cache-expr      : 5
 cache-matches   : 0
 trim count      : 0
 DELETE
@@ -614,8 +456,7 @@ dnl
 Enabled: true
 high-watermark  : 5
 total           : 4
-cache-conj-id   : 4
-cache-expr      : 0
+cache-expr      : 4
 cache-matches   : 0
 trim count      : 0
 ENABLE
@@ -626,8 +467,7 @@ dnl
 Enabled: true
 high-watermark  : 4
 total           : 4
-cache-conj-id   : 4
-cache-expr      : 0
+cache-expr      : 4
 cache-matches   : 0
 trim count      : 1
 DELETE
@@ -638,16 +478,14 @@ dnl
 Enabled: true
 high-watermark  : 3
 total           : 3
-cache-conj-id   : 3
-cache-expr      : 0
+cache-expr      : 3
 cache-matches   : 0
 trim count      : 2
 ENABLE
 Enabled: true
 high-watermark  : 3
 total           : 3
-cache-conj-id   : 3
-cache-expr      : 0
+cache-expr      : 3
 cache-matches   : 0
 trim count      : 2
 DELETE
@@ -658,8 +496,7 @@ dnl
 Enabled: true
 high-watermark  : 3
 total           : 2
-cache-conj-id   : 2
-cache-expr      : 0
+cache-expr      : 2
 cache-matches   : 0
 trim count      : 2
 dnl
@@ -670,8 +507,7 @@ DELETE
 Enabled: true
 high-watermark  : 1
 total           : 1
-cache-conj-id   : 1
-cache-expr      : 0
+cache-expr      : 1
 cache-matches   : 0
 trim count      : 3
 ])
diff --git a/tests/ovn.at b/tests/ovn.at
index 5458552d3..5d5e00957 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -15213,6 +15213,70 @@ OVN_CLEANUP([hv1])
 AT_CLEANUP
 ])
 
+# Number of conjunctions can change for the same logical flow, which should not
+# cause conflict conjunction IDs between logical flows.
+OVN_FOR_EACH_NORTHD([
+AT_SETUP([n_conjs change])
+ovn_start
+
+check ovn-nbctl ls-add ls1
+
+check ovn-nbctl lsp-add ls1 ls1-lp1 \
+-- lsp-set-addresses ls1-lp1 "f0:00:00:00:00:01" \
+-- set logical_switch_port ls1-lp1 options:requested-tnl-key=1
+
+check ovn-nbctl lsp-add ls1 ls1-lp2 \
+-- lsp-set-addresses ls1-lp2 "f0:00:00:00:00:02" \
+-- set logical_switch_port ls1-lp1 options:requested-tnl-key=2
+
+net_add n1
+sim_add hv1
+
+as hv1
+check ovs-vsctl add-br br-phys
+ovn_attach n1 br-phys 192.168.0.1
+check ovs-vsctl -- add-port br-int hv1-vif1 -- \
+    set interface hv1-vif1 external-ids:iface-id=ls1-lp1 \
+    options:tx_pcap=hv1/vif1-tx.pcap \
+    options:rxq_pcap=hv1/vif1-rx.pcap \
+    ofport-request=1
+
+check ovs-vsctl -- add-port br-int hv1-vif2 -- \
+    set interface hv1-vif2 external-ids:iface-id=ls1-lp2 \
+    options:tx_pcap=hv1/vif2-tx.pcap \
+    options:rxq_pcap=hv1/vif2-rx.pcap \
+    ofport-request=2
+
+ovn-nbctl create address_set name=as1 addresses="10.0.0.1"
+ovn-nbctl create address_set name=as2 addresses="10.0.0.11,10.0.0.12"
+ovn-nbctl pg-add pg1 ls1-lp1 ls1-lp2
+
+# The 1st ACL potentially can generate 2 conjunctions, but as1 has only 1 address,
+# so it would generate 1 conjunction for now.
+check ovn-nbctl acl-add pg1 to-lport 100 \
+    '(outport == @pg1 && ip4.src == $as1) || (outport == @pg1 && ip4.dst == $as2)' allow
+
+# The 2nd ACL generates 1 conjunction (use another conjunction ID)
+check ovn-nbctl acl-add pg1 to-lport 100 'outport == @pg1 && ip4.src == $as2' allow
+
+wait_for_ports_up
+check ovn-nbctl --wait=hv sync
+ovs-ofctl dump-flows br-int table=44
+AT_CHECK([test `ovs-ofctl dump-flows br-int table=44 | grep -c conj_id` = 2])
+
+echo -------
+# Add another address in as1, so that the 1st ACL will now generate 2 conjunctions.
+ovn-nbctl set address_set as1 addresses="10.0.0.1,10.0.0.2"
+check ovn-nbctl --wait=hv sync
+
+ovs-ofctl dump-flows br-int table=44
+# There should be 3 conjunctions in total (2 from 1st ACL + 1 from 2nd ACL)
+AT_CHECK([test `ovs-ofctl dump-flows br-int table=44 | grep -c conj_id` = 3])
+
+OVN_CLEANUP([hv1])
+AT_CLEANUP
+])
+
 # 3 hypervisors, one logical switch, 3 logical ports per hypervisor
 OVN_FOR_EACH_NORTHD([
 AT_SETUP([L2 Drop and Allow ACL w/ Stateful ACL])
@@ -24462,156 +24526,6 @@ OVN_CLEANUP([hv1],[hv2])
 AT_CLEANUP
 ])
 
-OVN_FOR_EACH_NORTHD([
-AT_SETUP([lflow cache for conjunctions])
-ovn_start
-net_add n1
-sim_add hv1
-
-as hv1
-ovs-vsctl add-br br-phys
-ovn_attach n1 br-phys 192.168.0.1
-
-check ovn-nbctl ls-add sw0
-check ovn-nbctl lsp-add sw0 sw0-p1
-check ovn-nbctl lsp-set-addresses sw0-p1 "10:14:00:00:00:03 10.0.0.3"
-check ovn-nbctl lsp-set-port-security sw0-p1 "10:14:00:00:00:03 10.0.0.3"
-
-check ovn-nbctl lsp-add sw0 sw0-p2
-check ovn-nbctl lsp-set-addresses sw0-p2 "10:14:00:00:00:04 10.0.0.4"
-check ovn-nbctl lsp-set-port-security sw0-p2 "10:14:00:00:00:04 10.0.0.4"
-
-check ovn-nbctl lsp-add sw0 sw0-p3
-check ovn-nbctl lsp-set-addresses sw0-p3 "10:14:00:00:00:05 10.0.0.5"
-check ovn-nbctl lsp-set-port-security sw0-p3 "10:14:00:00:00:05 10.0.0.5"
-
-check ovn-nbctl lsp-add sw0 sw0-p4
-check ovn-nbctl lsp-set-addresses sw0-p4 "10:14:00:00:00:06 10.0.0.6"
-check ovn-nbctl lsp-set-port-security sw0-p4 "10:14:00:00:00:06 10.0.0.6"
-
-as hv1
-ovs-vsctl -- add-port br-int hv1-vif1 -- \
-    set interface hv1-vif1 external-ids:iface-id=sw0-p1 \
-    options:tx_pcap=hv1/vif1-tx.pcap \
-    options:rxq_pcap=hv1/vif1-rx.pcap \
-    ofport-request=1
-ovs-vsctl -- add-port br-int hv1-vif2 -- \
-    set interface hv1-vif2 external-ids:iface-id=sw0-p2 \
-    options:tx_pcap=hv1/vif2-tx.pcap \
-    options:rxq_pcap=hv1/vif2-rx.pcap \
-    ofport-request=2
-ovs-vsctl -- add-port br-int hv1-vif3 -- \
-    set interface hv1-vif3 external-ids:iface-id=sw0-p3 \
-    options:tx_pcap=hv1/vif3-tx.pcap \
-    options:rxq_pcap=hv1/vif3-rx.pcap \
-    ofport-request=3
-ovs-vsctl -- add-port br-int hv1-vif4 -- \
-    set interface hv1-vif4 external-ids:iface-id=sw0-p4 \
-    options:tx_pcap=hv1/vif4-tx.pcap \
-    options:rxq_pcap=hv1/vif4-rx.pcap \
-    ofport-request=4
-
-wait_for_ports_up
-
-check ovn-nbctl pg-add pg0 sw0-p1 sw0-p2
-check ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && tcp.dst >= 80 && tcp.dst <= 82" allow
-check ovn-nbctl --wait=hv sync
-
-# wait_conj_id_count COUNT ["ID COUNT [MATCH]"]...
-#
-# Waits until COUNT flows matching against conj_id appear in the
-# table 44 on hv1's br-int bridge.  Makes the flows available in
-# "hv1flows", which will be logged on error.
-#
-# In addition, for each quoted "ID COUNT" or "ID COUNT MATCH",
-# verifies that there are COUNT flows in table 45 that match
-# aginst conj_id=ID and (if MATCH) is nonempty, match MATCH.
-wait_conj_id_count() {
-  AT_CAPTURE_FILE([hv1flows])
-  local retval
-  case $1 in
-      (0) retval=1 ;;
-      (*) retval=0 ;;
-  esac
-
-  echo "waiting for $1 conj_id flows..."
-  OVS_WAIT_FOR_OUTPUT_UNQUOTED(
-    [ovs-ofctl dump-flows br-int > hv1flows
-     grep table=44 hv1flows | grep -c conj_id],
-    [$retval], [$1
-])
-
-  shift
-  for arg; do
-    set -- $arg; id=$1 count=$2 match=$3
-    echo "checking that there are $count ${match:+$match }flows with conj_id=$id..."
-    AT_CHECK_UNQUOTED(
-      [grep table=44 hv1flows | grep "$match" | grep -c conj_id=$id],
-      [0], [$count
-])
-  done
-}
-
-AS_BOX([Add sw0-p3 to the port group pg0. The conj_id should be 2.])
-check ovn-nbctl --wait=hv pg-set-ports pg0 sw0-p1 sw0-p2 sw0-p3
-wait_conj_id_count 1 "2 1"
-
-AS_BOX([Add sw0p4 to the port group pg0. The conj_id should be 2.])
-check ovn-nbctl --wait=hv pg-set-ports pg0 sw0-p1 sw0-p2 sw0-p3 sw0-p4
-wait_conj_id_count 1 "2 1"
-
-AS_BOX([Add another ACL with conjunction.])
-check ovn-nbctl --wait=hv acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && udp.dst >= 80 && udp.dst <= 82" allow
-wait_conj_id_count 2 "2 1 tcp" "3 1 udp"
-
-AS_BOX([Delete tcp ACL.])
-check ovn-nbctl --wait=hv acl-del pg0 to-lport 1002 "outport == @pg0 && ip4 && tcp.dst >= 80 && tcp.dst <= 82"
-wait_conj_id_count 1 "3 1 udp"
-
-AS_BOX([Add back the tcp ACL.])
-check ovn-nbctl --wait=hv acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && tcp.dst >= 80 && tcp.dst <= 82" allow
-wait_conj_id_count 2 "3 1 udp" "4 1 tcp"
-AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep udp | grep -c "conj_id=3")])
-AT_CHECK([test 1 = $(as hv1 ovs-ofctl dump-flows br-int table=44 | grep tcp | grep -c "conj_id=4")])
-
-AS_BOX([Add another tcp ACL.])
-check ovn-nbctl --wait=hv acl-add pg0 to-lport 1002 "outport == @pg0 && inport == @pg0 && ip4 && tcp.dst >= 84 && tcp.dst <= 86" allow
-wait_conj_id_count 3 "3 1 udp" "4 1 tcp" "5 1 tcp"
-
-AS_BOX([Clear ACLs.])
-check ovn-nbctl --wait=hv clear port_group pg0 acls
-wait_conj_id_count 0
-
-AS_BOX([Add TCP ACL.])
-check ovn-nbctl --wait=hv acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && tcp.dst >= 80 && tcp.dst <= 82" allow
-check ovn-nbctl acl-add pg0 to-lport 1002 "outport == @pg0 && ip4 && udp.dst >= 80 && udp.dst <= 82" allow
-wait_conj_id_count 2 "6 1 tcp" "7 1 udp"
-
-AS_BOX([Flush lflow cache.])
-as hv1 ovn-appctl -t ovn-controller lflow-cache/flush
-wait_conj_id_count 2 "2 1" "3 1"
-
-AS_BOX([Disable lflow caching.])
-as hv1 ovs-vsctl set open . external_ids:ovn-enable-lflow-cache=false
-
-AS_BOX([Wait until ovn-enble-lflow-cache is processed by ovn-controller.])
-wait_row_count Chassis 1 name=hv1 other_config:ovn-enable-lflow-cache=false
-wait_conj_id_count 2 "2 1" "3 1"
-
-AS_BOX([Remove port sw0-p4 from port group.])
-check ovn-nbctl --wait=hv pg-set-ports pg0 sw0-p1 sw0-p2 sw0-p3
-wait_conj_id_count 2 "4 1" "5 1"
-
-AS_BOX([Recompute.])
-as hv1 ovn-appctl -t ovn-controller recompute
-
-wait_conj_id_count 2 "2 1" "3 1"
-
-OVN_CLEANUP([hv1])
-
-AT_CLEANUP
-])
-
 OVN_FOR_EACH_NORTHD([
 AT_SETUP([lflow cache operations])
 ovn_start
@@ -24642,31 +24556,26 @@ get_cache_count () {
 }
 
 AS_BOX([Check matches caching])
-conj_id_cnt=$(get_cache_count cache-conj-id)
 expr_cnt=$(get_cache_count cache-expr)
 matches_cnt=$(get_cache_count cache-matches)
 
 check ovn-nbctl acl-add ls1 from-lport 1 '1' drop
 check ovn-nbctl --wait=hv sync
 
-AT_CHECK([test "$conj_id_cnt" = "$(get_cache_count cache-conj-id)"], [0], [])
 AT_CHECK([test "$expr_cnt" = "$(get_cache_count cache-expr)"], [0], [])
 AT_CHECK([test "$(($matches_cnt + 1))" = "$(get_cache_count cache-matches)"], [0], [])
 
 AS_BOX([Check expr caching for is_chassis_resident() matches])
-conj_id_cnt=$(get_cache_count cache-conj-id)
 expr_cnt=$(get_cache_count cache-expr)
 matches_cnt=$(get_cache_count cache-matches)
 
 check ovn-nbctl acl-add ls1 from-lport 1 'is_chassis_resident("lsp1")' drop
 check ovn-nbctl --wait=hv sync
 
-AT_CHECK([test "$conj_id_cnt" = "$(get_cache_count cache-conj-id)"], [0], [])
 AT_CHECK([test "$(($expr_cnt + 1))" = "$(get_cache_count cache-expr)"], [0], [])
 AT_CHECK([test "$matches_cnt" = "$(get_cache_count cache-matches)"], [0], [])
 
 AS_BOX([Check conj-id caching for conjunctive port group/address set matches])
-conj_id_cnt=$(get_cache_count cache-conj-id)
 expr_cnt=$(get_cache_count cache-expr)
 matches_cnt=$(get_cache_count cache-matches)
 
@@ -24674,19 +24583,16 @@ check ovn-nbctl acl-add ls1 from-lport 1 'inport == @pg1 && outport == @pg1 && i
 check ovn-nbctl acl-add ls1 from-lport 1 'ip4.src == $as1 && ip4.dst == $as1 && is_chassis_resident("lsp1")' drop
 check ovn-nbctl --wait=hv sync
 
-AT_CHECK([test "$(($conj_id_cnt + 2))" = "$(get_cache_count cache-conj-id)"], [0], [])
 AT_CHECK([test "$expr_cnt" = "$(get_cache_count cache-expr)"], [0], [])
 AT_CHECK([test "$matches_cnt" = "$(get_cache_count cache-matches)"], [0], [])
 
 AS_BOX([Check no caching for non-conjunctive port group/address set matches])
-conj_id_cnt=$(get_cache_count cache-conj-id)
 expr_cnt=$(get_cache_count cache-expr)
 matches_cnt=$(get_cache_count cache-matches)
 
 check ovn-nbctl acl-add ls1 from-lport 1 'inport == @pg2 && outport == @pg2 && is_chassis_resident("lsp1")' drop
 check ovn-nbctl --wait=hv sync
 
-AT_CHECK([test "$conj_id_cnt" = "$(get_cache_count cache-conj-id)"], [0], [])
 AT_CHECK([test "$expr_cnt" = "$(get_cache_count cache-expr)"], [0], [])
 AT_CHECK([test "$matches_cnt" = "$(get_cache_count cache-matches)"], [0], [])
 
-- 
2.30.2



More information about the dev mailing list