northd: Skip matching on ct flags for stateless configurations.

If no load balancers or "allow-related" ACLs are configured on a logical
switch, no packets will be sent to conntrack in the logical switch
pipeline and ACL flows in tables ls_in/out_acl will not match on
conntrack state.  In this case there's no need to try to set ACL hints
in tables ls_in/out_acl_hint.

Furthermore, setting the hints translates to always generating flows
that match on ct.state.  Depending on the underlying hardware such flows
may not be offloadable inducing a hit in performance even when no
conntrack recirculations are required.

To avoid iterating through all configured ACLs and load balancers
multiple times, we now store two new fields in the 'ovn_datapath'
structure:
- has_stateful_acl
- has_lb_vip

Also, rename the 'has_lb_vip()' and 'has_stateful_acl()' functions,
prefixing them with 'ls_' to match other helper function names.

Fixes: 209ea46 ("ovn-northd: Reduce number of flows generated for stateful ACLs.")
Reported-by: Haresh Khandelwal <hakhande@redhat.com>
Reported-at: https://bugzilla.redhat.com/1927211
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Mark Michelson <mmichels@redhat.com>
Acked-by: Mark Michelson <mmichels@redhat.com>

northd/ovn-northd.8.xml

@@ -415,7 +415,8 @@
     <p>
       This table consists of logical flows that set hints
       (<code>reg0</code> bits) to be used in the next stage, in the ACL
-      processing table. Multiple hints can be set for the same packet.
+      processing table, if stateful ACLs or load balancers are configured.
+      Multiple hints can be set for the same packet.
       The possible hints are:
     </p>
     <ul>

northd/ovn-northd.c

@@ -597,6 +597,8 @@ struct ovn_datapath {
     struct hmap port_tnlids;
     uint32_t port_key_hint;
 
+    bool has_stateful_acl;
+    bool has_lb_vip;
     bool has_unknown;
 
     /* IPAM data. */
@@ -635,6 +637,9 @@ struct ovn_datapath {
     struct hmap nb_pgs;
 };
 
+static bool ls_has_stateful_acl(struct ovn_datapath *od);
+static bool ls_has_lb_vip(struct ovn_datapath *od);
+
 /* Contains a NAT entry with the external addresses pre-parsed. */
 struct ovn_nat {
     const struct nbrec_nat *nb;
@@ -4635,7 +4640,7 @@ ovn_ls_port_group_destroy(struct hmap *nb_pgs)
 }
 
 static bool
-has_stateful_acl(struct ovn_datapath *od)
+ls_has_stateful_acl(struct ovn_datapath *od)
 {
     for (size_t i = 0; i < od->nbs->n_acls; i++) {
         struct nbrec_acl *acl = od->nbs->acls[i];
@@ -4814,8 +4819,6 @@ skip_port_from_conntrack(struct ovn_datapath *od, struct ovn_port *op,
 static void
 build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
 {
-    bool has_stateful = has_stateful_acl(od);
-
     /* Ingress and Egress Pre-ACL Table (Priority 0): Packets are
      * allowed by default. */
     ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 0, "1", "next;");
@@ -4830,7 +4833,7 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
     /* If there are any stateful ACL rules in this datapath, we must
      * send all IP packets through the conntrack action, which handles
      * defragmentation, in order to match L4 headers. */
-    if (has_stateful) {
+    if (od->has_stateful_acl) {
         for (size_t i = 0; i < od->n_router_ports; i++) {
             skip_port_from_conntrack(od, od->router_ports[i],
                                      S_SWITCH_IN_PRE_ACL, S_SWITCH_OUT_PRE_ACL,
@@ -4933,7 +4936,7 @@ build_empty_lb_event_flow(struct ovn_datapath *od, struct hmap *lflows,
 }
 
 static bool
-has_lb_vip(struct ovn_datapath *od)
+ls_has_lb_vip(struct ovn_datapath *od)
 {
     for (int i = 0; i < od->nbs->n_load_balancer; i++) {
         struct nbrec_load_balancer *nb_lb = od->nbs->load_balancer[i];
@@ -5076,6 +5079,13 @@ build_acl_hints(struct ovn_datapath *od, struct hmap *lflows)
     for (size_t i = 0; i < ARRAY_SIZE(stages); i++) {
         enum ovn_stage stage = stages[i];
 
+        /* In any case, advance to the next stage. */
+        ovn_lflow_add(lflows, od, stage, 0, "1", "next;");
+
+        if (!od->has_stateful_acl && !od->has_lb_vip) {
+            continue;
+        }
+
         /* New, not already established connections, may hit either allow
          * or drop ACLs. For allow ACLs, the connection must also be committed
          * to conntrack so we set REGBIT_ACL_HINT_ALLOW_NEW.
@@ -5136,9 +5146,6 @@ build_acl_hints(struct ovn_datapath *od, struct hmap *lflows)
         ovn_lflow_add(lflows, od, stage, 1, "ct.est && ct_label.blocked == 0",
                       REGBIT_ACL_HINT_BLOCK " = 1; "
                       "next;");
-
-        /* In any case, advance to the next stage. */
-        ovn_lflow_add(lflows, od, stage, 0, "1", "next;");
     }
 }
 
@@ -5470,7 +5477,7 @@ static void
 build_acls(struct ovn_datapath *od, struct hmap *lflows,
            struct hmap *port_groups, const struct shash *meter_groups)
 {
-    bool has_stateful = (has_stateful_acl(od) || has_lb_vip(od));
+    bool has_stateful = od->has_stateful_acl || od->has_lb_vip;
 
     /* Ingress and Egress ACL Table (Priority 0): Packets are allowed by
      * default.  A related rule at priority 1 is added below if there
@@ -5739,7 +5746,7 @@ build_lb(struct ovn_datapath *od, struct hmap *lflows)
         }
     }
 
-    if (has_lb_vip(od)) {
+    if (od->has_lb_vip) {
         /* Ingress and Egress LB Table (Priority 65534).
          *
          * Send established traffic through conntrack for just NAT. */
@@ -5860,7 +5867,7 @@ build_lb_hairpin(struct ovn_datapath *od, struct hmap *lflows)
     ovn_lflow_add(lflows, od, S_SWITCH_IN_NAT_HAIRPIN, 0, "1", "next;");
     ovn_lflow_add(lflows, od, S_SWITCH_IN_HAIRPIN, 0, "1", "next;");
 
-    if (has_lb_vip(od)) {
+    if (od->has_lb_vip) {
         /* Check if the packet needs to be hairpinned. */
         ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_PRE_HAIRPIN, 100,
                                 "ip && ct.trk && ct.dnat",
@@ -6597,7 +6604,10 @@ build_lswitch_lflows_pre_acl_and_acl(struct ovn_datapath *od,
                                      struct shash *meter_groups,
                                      struct hmap *lbs)
 {
-   if (od->nbs) {
+    if (od->nbs) {
+        od->has_stateful_acl = ls_has_stateful_acl(od);
+        od->has_lb_vip = ls_has_lb_vip(od);
+
         build_pre_acls(od, lflows);
         build_pre_lb(od, lflows, meter_groups, lbs);
         build_pre_stateful(od, lflows);

tests/ovn-northd.at

@@ -1912,6 +1912,83 @@ check_meter_by_name NOT meter_me__${acl1} meter_me__${acl2}
 
 AT_CLEANUP
 
+AT_SETUP([ovn -- ACL skip hints for stateless config])
+AT_KEYWORDS([acl])
+ovn_start
+
+check ovn-nbctl --wait=sb \
+    -- ls-add ls \
+    -- lsp-add ls lsp \
+    -- acl-add ls from-lport 1 "ip" allow \
+    -- acl-add ls to-lport 1 "ip" allow
+
+AS_BOX([Check no match on ct_state with stateless ACLs])
+AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e ls_in_acl -e ls_out_acl | grep 'ct\.' | sort], [0], [dnl
+])
+
+AS_BOX([Check match ct_state with stateful ACLs])
+check ovn-nbctl --wait=sb \
+    -- acl-add ls from-lport 2 "udp" allow-related \
+    -- acl-add ls to-lport 2 "udp" allow-related
+AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e ls_in_acl -e ls_out_acl | grep 'ct\.' | sort], [0], [dnl
+  table=4 (ls_out_acl_hint    ), priority=1    , match=(ct.est && ct_label.blocked == 0), action=(reg0[[10]] = 1; next;)
+  table=4 (ls_out_acl_hint    ), priority=2    , match=(ct.est && ct_label.blocked == 1), action=(reg0[[9]] = 1; next;)
+  table=4 (ls_out_acl_hint    ), priority=3    , match=(!ct.est), action=(reg0[[9]] = 1; next;)
+  table=4 (ls_out_acl_hint    ), priority=4    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 0), action=(reg0[[8]] = 1; reg0[[10]] = 1; next;)
+  table=4 (ls_out_acl_hint    ), priority=5    , match=(!ct.trk), action=(reg0[[8]] = 1; reg0[[9]] = 1; next;)
+  table=4 (ls_out_acl_hint    ), priority=6    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)
+  table=4 (ls_out_acl_hint    ), priority=7    , match=(ct.new && !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)
+  table=5 (ls_out_acl         ), priority=1    , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)
+  table=5 (ls_out_acl         ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
+  table=5 (ls_out_acl         ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
+  table=5 (ls_out_acl         ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+  table=6 (ls_in_acl_hint     ), priority=1    , match=(ct.est && ct_label.blocked == 0), action=(reg0[[10]] = 1; next;)
+  table=6 (ls_in_acl_hint     ), priority=2    , match=(ct.est && ct_label.blocked == 1), action=(reg0[[9]] = 1; next;)
+  table=6 (ls_in_acl_hint     ), priority=3    , match=(!ct.est), action=(reg0[[9]] = 1; next;)
+  table=6 (ls_in_acl_hint     ), priority=4    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 0), action=(reg0[[8]] = 1; reg0[[10]] = 1; next;)
+  table=6 (ls_in_acl_hint     ), priority=5    , match=(!ct.trk), action=(reg0[[8]] = 1; reg0[[9]] = 1; next;)
+  table=6 (ls_in_acl_hint     ), priority=6    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)
+  table=6 (ls_in_acl_hint     ), priority=7    , match=(ct.new && !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)
+  table=7 (ls_in_acl          ), priority=1    , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)
+  table=7 (ls_in_acl          ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
+  table=7 (ls_in_acl          ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
+  table=7 (ls_in_acl          ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+])
+
+AS_BOX([Check match ct_state with load balancer])
+check ovn-nbctl --wait=sb \
+    -- acl-del ls from-lport 2 "udp" \
+    -- acl-del ls to-lport 2 "udp" \
+    -- lb-add lb "10.0.0.1" "10.0.0.2" \
+    -- ls-lb-add ls lb
+
+AT_CHECK([ovn-sbctl lflow-list ls | grep -e ls_in_acl_hint -e ls_out_acl_hint -e ls_in_acl -e ls_out_acl | grep 'ct\.' | sort], [0], [dnl
+  table=4 (ls_out_acl_hint    ), priority=1    , match=(ct.est && ct_label.blocked == 0), action=(reg0[[10]] = 1; next;)
+  table=4 (ls_out_acl_hint    ), priority=2    , match=(ct.est && ct_label.blocked == 1), action=(reg0[[9]] = 1; next;)
+  table=4 (ls_out_acl_hint    ), priority=3    , match=(!ct.est), action=(reg0[[9]] = 1; next;)
+  table=4 (ls_out_acl_hint    ), priority=4    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 0), action=(reg0[[8]] = 1; reg0[[10]] = 1; next;)
+  table=4 (ls_out_acl_hint    ), priority=5    , match=(!ct.trk), action=(reg0[[8]] = 1; reg0[[9]] = 1; next;)
+  table=4 (ls_out_acl_hint    ), priority=6    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)
+  table=4 (ls_out_acl_hint    ), priority=7    , match=(ct.new && !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)
+  table=5 (ls_out_acl         ), priority=1    , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)
+  table=5 (ls_out_acl         ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
+  table=5 (ls_out_acl         ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
+  table=5 (ls_out_acl         ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+  table=6 (ls_in_acl_hint     ), priority=1    , match=(ct.est && ct_label.blocked == 0), action=(reg0[[10]] = 1; next;)
+  table=6 (ls_in_acl_hint     ), priority=2    , match=(ct.est && ct_label.blocked == 1), action=(reg0[[9]] = 1; next;)
+  table=6 (ls_in_acl_hint     ), priority=3    , match=(!ct.est), action=(reg0[[9]] = 1; next;)
+  table=6 (ls_in_acl_hint     ), priority=4    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 0), action=(reg0[[8]] = 1; reg0[[10]] = 1; next;)
+  table=6 (ls_in_acl_hint     ), priority=5    , match=(!ct.trk), action=(reg0[[8]] = 1; reg0[[9]] = 1; next;)
+  table=6 (ls_in_acl_hint     ), priority=6    , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)
+  table=6 (ls_in_acl_hint     ), priority=7    , match=(ct.new && !ct.est), action=(reg0[[7]] = 1; reg0[[9]] = 1; next;)
+  table=7 (ls_in_acl          ), priority=1    , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[[1]] = 1; next;)
+  table=7 (ls_in_acl          ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
+  table=7 (ls_in_acl          ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
+  table=7 (ls_in_acl          ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+])
+
+AT_CLEANUP
+
 AT_SETUP([datapath requested-tnl-key])
 AT_KEYWORDS([requested tnl tunnel key keys])
 ovn_start