[ovs-dev] [PATCH v10 6/8] ovn: avoid snat recirc only on gateway routers

Mickey Spiegel mickeys.dev at gmail.com
Tue Jan 17 09:45:07 UTC 2017


Currently, for performance reasons on gateway routers, ct_snat
that does not specify an IP address does not immediately trigger
recirculation.  On gateway routers, ct_snat that does not specify
an IP address happens in the UNSNAT pipeline stage, which is
followed by the DNAT pipeline stage that triggers recirculation
for all packets.  This DNAT pipeline stage recirculation takes
care of the recirculation needs of UNSNAT as well as other cases
such as UNDNAT.

On distributed routers, UNDNAT is handled in the egress pipeline
stage, separately from DNAT in the ingress pipeline stages.  The
DNAT pipeline stage only triggers recirculation for some packets.
Due to this difference in design, UNSNAT needs to trigger its own
recirculation.

This patch restricts the logic that avoids recirculation for
ct_snat, so that it only applies to datapaths representing
gateway routers.

Signed-off-by: Mickey Spiegel <mickeys.dev at gmail.com>
---
 include/ovn/actions.h  |  3 +++
 ovn/controller/lflow.c | 10 ++++++++++
 ovn/lib/actions.c      | 15 +++++++++------
 ovn/ovn-sb.xml         | 23 +++++++++++++++++++----
 tests/ovn.at           |  2 +-
 5 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 5f9d203..810b901 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -418,6 +418,9 @@ struct ovnact_encode_params {
     /* 'true' if the flow is for a switch. */
     bool is_switch;
 
+    /* 'true' if the flow is for a gateway router. */
+    bool is_gateway_router;
+
     /* A map from a port name to its connection tracking zone. */
     const struct simap *ct_zones;
 
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index c41368f..b011109 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -107,6 +107,15 @@ is_switch(const struct sbrec_datapath_binding *ldp)
 
 }
 
+static bool
+is_gateway_router(const struct sbrec_datapath_binding *ldp,
+                  const struct hmap *local_datapaths)
+{
+    struct local_datapath *ld =
+        get_local_datapath(local_datapaths, ldp->tunnel_key);
+    return ld ? ld->has_local_l3gateway : false;
+}
+
 /* Adds the logical flows from the Logical_Flow table to flow tables. */
 static void
 add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports,
@@ -220,6 +229,7 @@ consider_logical_flow(const struct lport_index *lports,
         .lookup_port = lookup_port_cb,
         .aux = &aux,
         .is_switch = is_switch(ldp),
+        .is_gateway_router = is_gateway_router(ldp, local_datapaths),
         .ct_zones = ct_zones,
         .group_table = group_table,
 
diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c
index dda675b..5c57ab7 100644
--- a/ovn/lib/actions.c
+++ b/ovn/lib/actions.c
@@ -803,12 +803,15 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
     ct = ofpacts->header;
     if (cn->ip) {
         ct->flags |= NX_CT_F_COMMIT;
-    } else if (snat) {
-        /* XXX: For performance reasons, we try to prevent additional
-         * recirculations.  So far, ct_snat which is used in a gateway router
-         * does not need a recirculation. ct_snat(IP) does need a
-         * recirculation.  Should we consider a method to let the actions
-         * specify whether an action needs recirculation if there more use
+    } else if (snat && ep->is_gateway_router) {
+        /* For performance reasons, we try to prevent additional
+         * recirculations.  ct_snat which is used in a gateway router
+         * does not need a recirculation.  ct_snat(IP) does need a
+         * recirculation.  ct_snat in a distributed router needs
+         * recirculation regardless of whether an IP address is
+         * specified.
+         * XXX Should we consider a method to let the actions specify
+         * whether an action needs recirculation if there are more use
          * cases?. */
         ct->recirc_table = NX_CT_RECIRC_NONE;
     }
diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
index a7c29c3..8fe0e2b 100644
--- a/ovn/ovn-sb.xml
+++ b/ovn/ovn-sb.xml
@@ -1122,11 +1122,26 @@
         <dd>
           <p>
             <code>ct_snat</code> sends the packet through the SNAT zone to
-            unSNAT any packet that was SNATed in the opposite direction.  If
-            the packet needs to be sent to the next tables, then it should be
-            followed by a <code>next;</code> action.  The next tables will not
-            see the changes in the packet caused by the connection tracker.
+            unSNAT any packet that was SNATed in the opposite direction.  The
+            behavior on gateway routers differs from the behavior on a
+            distributed router:
           </p>
+          <ul>
+            <li>
+              On a gateway router, if the packet needs to be sent to the next
+              tables, then it should be followed by a <code>next;</code>
+              action.  The next tables will not see the changes in the packet
+              caused by the connection tracker.
+            </li>
+            <li>
+              On a distributed router, if the connection tracker finds a
+              connection that was SNATed in the opposite direction, then the
+              destination IP address of the packet is UNSNATed.  The packet is
+              automatically sent to the next tables as if followed by the
+              <code>next;</code> action.  The next tables will see the changes
+              in the packet caused by the connection tracker.
+            </li>
+          </ul>
           <p>
             <code>ct_snat(<var>IP</var>)</code> sends the packet through the
             SNAT zone to change the source IP address of the packet to
diff --git a/tests/ovn.at b/tests/ovn.at
index 1c3183f..694a38d 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -852,7 +852,7 @@ ct_dnat();
 
 # ct_snat
 ct_snat;
-    encodes as ct(zone=NXM_NX_REG12[0..15],nat)
+    encodes as ct(table=27,zone=NXM_NX_REG12[0..15],nat)
     has prereqs ip
 ct_snat(192.168.1.2);
     encodes as ct(commit,table=27,zone=NXM_NX_REG12[0..15],nat(src=192.168.1.2))
-- 
1.9.1



More information about the dev mailing list