On 1/25/21 8:08 PM, [email protected] wrote:
From: Numan Siddique <[email protected]>

When the below ACL is added -
ovn-nbctl acl-add ls1 to-lport 3
   '(ip4.src==10.0.0.1 || ip4.src==10.0.0.2) &&
    (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow

ovn-controller installs the below OF flows

table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 
actions=conjunction(2,1/2)
table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 
actions=conjunction(2,1/2)
table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 
actions=conjunction(2,2/2)
table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 
actions=conjunction(2,2/2)
table=45, priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)

When a full recompute is triggered, ovn-controller deletes the last
OF flow with the match conj_id=2 and adds the below OF flow

table=45, priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)

For subsequent recomputes, the conj_id keeps increasing by 1.

This disrupts the traffic which matches on conjuction action flows.

This patch fixes this issue.

Fixes: 1213bc8270("ovn-controller: Cache logical flow expr matches.")
Suggested-by: Dumitru Ceara <[email protected]>
Signed-off-by: Numan Siddique <[email protected]>
---
  controller/lflow.c | 46 ++++++++++++++++++++++++++++++----------------
  tests/ovn.at       | 28 ++++++++++++++++++++++++++++
  2 files changed, 58 insertions(+), 16 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index c02585b1e..7bb3cdaeb 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -668,9 +668,8 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t n_conjs)
  static void
  add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
                            const struct sbrec_datapath_binding *dp,
-                          struct hmap *matches, size_t conj_id_ofs,
-                          uint8_t ptable, uint8_t output_ptable,
-                          struct ofpbuf *ovnacts,
+                          struct hmap *matches, uint8_t ptable,
+                          uint8_t output_ptable, struct ofpbuf *ovnacts,
                            bool ingress, struct lflow_ctx_in *l_ctx_in,
                            struct lflow_ctx_out *l_ctx_out)
  {
@@ -708,9 +707,6 @@ add_matches_to_flow_table(const struct sbrec_logical_flow 
*lflow,
      struct expr_match *m;
      HMAP_FOR_EACH (m, hmap_node, matches) {
          match_set_metadata(&m->match, htonll(dp->tunnel_key));
-        if (m->match.wc.masks.conj_id) {
-            m->match.flow.conj_id += conj_id_ofs;
-        }
          if (datapath_is_switch(dp)) {
              unsigned int reg_index
                  = (ingress ? MFF_LOG_INPORT : MFF_LOG_OUTPORT) - MFF_REG0;
@@ -744,7 +740,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow 
*lflow,
                  struct ofpact_conjunction *dst;
dst = ofpact_put_CONJUNCTION(&conj);
-                dst->id = src->id + conj_id_ofs;
+                dst->id = src->id;
                  dst->clause = src->clause;
                  dst->n_clauses = src->n_clauses;
              }
@@ -815,6 +811,22 @@ convert_match_to_expr(const struct sbrec_logical_flow 
*lflow,
      return expr_simplify(e);
  }
+static void
+prepare_expr_matches(struct hmap *matches, uint32_t conj_id_ofs)

I think this fits better as a function in expr.c because it only updates internals of the expression matches, maybe renamed to expr_matches_prepare().

+{
+    struct expr_match *m;
+    HMAP_FOR_EACH (m, hmap_node, matches) {
+        if (m->match.wc.masks.conj_id) {
+            m->match.flow.conj_id += conj_id_ofs;
+        }
+
+        for (int i = 0; i < m->n; i++) {

Nit: s/int i/size_t i/

With these small issues addressed:

Acked-by: Dumitru Ceara <[email protected]>

Thanks,
Dumitru

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to