diff mbox

[ovs-dev,v3,4/5] ovn-controller: Assign conntrack zones for gateway router.

Message ID CAM_3v9+ftxL0jKrj02ijJhJ=4ECEGHJgya3yYSF_bXtLC=W3Fw@mail.gmail.com
State Not Applicable
Headers show

Commit Message

Gurucharan Shetty June 3, 2016, 5:22 a.m. UTC 
On 2 June 2016 at 14:19, Ben Pfaff <blp@ovn.org> wrote:

> On Thu, May 19, 2016 at 01:02:33PM -0700, Gurucharan Shetty wrote:
> > OVS NAT currently cannot do snat and dnat in the same zone.
> > So we need two zones per gateway router.
> >
> > Signed-off-by: Gurucharan Shetty <guru@ovn.org>
>
> We're running out of registers quickly, but we're also using a full
> 32-bit register when we only need 16 bits, so there's considerable room
> to economize later if necessary.
>
I agree. I will work on atleast combining the 2 registers being used here
into one.


>
> Please update ovn-architecture(7) to mention the new connection tracking
> zones (the existing one is already mentioned).
>

I will add the following incremental.




>
> Acked-by: Ben Pfaff <blp@ovn.org>
>

Comments

Ben Pfaff June 3, 2016, 3:18 p.m. UTC | #1 
On Thu, Jun 02, 2016 at 10:22:05PM -0700, Guru Shetty wrote:
> On 2 June 2016 at 14:19, Ben Pfaff <blp@ovn.org> wrote:
> 
> > On Thu, May 19, 2016 at 01:02:33PM -0700, Gurucharan Shetty wrote:
> > > OVS NAT currently cannot do snat and dnat in the same zone.
> > > So we need two zones per gateway router.
> > >
> > > Signed-off-by: Gurucharan Shetty <guru@ovn.org>
> >
> > We're running out of registers quickly, but we're also using a full
> > 32-bit register when we only need 16 bits, so there's considerable room
> > to economize later if necessary.
> >
> I agree. I will work on atleast combining the 2 registers being used here
> into one.

It might require some new infrastructure, since I think there is some
code around oriented around using a whole MFF_* field instead of a
subfield.  If so, I don't think it's essential for this patch.

> > There are a couple of instances of
> > +            char *dnat = xasprintf(UUID_FMT"_%s",
> > +
> >  UUID_ARGS(&binding->datapath->header_.uuid),
> > +                                   "dnat");
> > +            char *snat = xasprintf(UUID_FMT"_%s",
> > +
> >  UUID_ARGS(&binding->datapath->header_.uuid),
> > +                                   "snat");
> > or similar.  Do you think it would be worth having a helper function (or
> > two) so that they are harder to get out-of-sync?
> >
> 
> How about something like the following?

Looks good to me, thanks!
Gurucharan Shetty June 3, 2016, 4:38 p.m. UTC | #2 
On 3 June 2016 at 08:18, Ben Pfaff <blp@ovn.org> wrote:

> On Thu, Jun 02, 2016 at 10:22:05PM -0700, Guru Shetty wrote:
> > On 2 June 2016 at 14:19, Ben Pfaff <blp@ovn.org> wrote:
> >
> > > On Thu, May 19, 2016 at 01:02:33PM -0700, Gurucharan Shetty wrote:
> > > > OVS NAT currently cannot do snat and dnat in the same zone.
> > > > So we need two zones per gateway router.
> > > >
> > > > Signed-off-by: Gurucharan Shetty <guru@ovn.org>
> > >
> > > We're running out of registers quickly, but we're also using a full
> > > 32-bit register when we only need 16 bits, so there's considerable room
> > > to economize later if necessary.
> > >
> > I agree. I will work on atleast combining the 2 registers being used here
> > into one.
>
> It might require some new infrastructure, since I think there is some
> code around oriented around using a whole MFF_* field instead of a
> subfield.  If so, I don't think it's essential for this patch.
>

I will leave this out from this patch.


>
> > > There are a couple of instances of
> > > +            char *dnat = xasprintf(UUID_FMT"_%s",
> > > +
> > >  UUID_ARGS(&binding->datapath->header_.uuid),
> > > +                                   "dnat");
> > > +            char *snat = xasprintf(UUID_FMT"_%s",
> > > +
> > >  UUID_ARGS(&binding->datapath->header_.uuid),
> > > +                                   "snat");
> > > or similar.  Do you think it would be worth having a helper function
> (or
> > > two) so that they are harder to get out-of-sync?
> > >
> >
> > How about something like the following?
>
> Looks good to me, thanks!
>
Thanks, I applied this.
diff mbox

Patch

diff --git a/ovn/ovn-architecture.7.xml b/ovn/ovn-architecture.7.xml
index 13acaf5..553c2e5 100644
--- a/ovn/ovn-architecture.7.xml
+++ b/ovn/ovn-architecture.7.xml
@@ -669,12 +669,22 @@ 
       </p>
     </dd>

-    <dt>conntrack zone field</dt>
+    <dt>conntrack zone field for logical ports</dt>
     <dd>
-      A field that denotes the connection tracking zone.  The value only
-      has local significance and is not meaningful between chassis.
-      This is initialized to 0 at the beginning of the logical ingress
-      pipeline.  OVN stores this in Nicira extension register number 5.
+      A field that denotes the connection tracking zone for logical ports.
+      The value only has local significance and is not meaningful between
+      chassis.  This is initialized to 0 at the beginning of the logical
+      ingress pipeline.  OVN stores this in Nicira extension register
number 5.
+    </dd>
+
+    <dt>conntrack zone fields for Gateway router</dt>
+    <dd>
+      Fields that denote the connection tracking zones for Gateway routers.
+      These values only have local significance (only on chassis that have
+      Gateway routers instantiated) and is not meaningful between
+      chassis.  OVN stores the zone information for DNATting in Nicira
+      extension register number 3 and zone information for SNATing in
Nicira
+      extension register number 4.
     </dd>


>
> There are a couple of instances of
> +            char *dnat = xasprintf(UUID_FMT"_%s",
> +
>  UUID_ARGS(&binding->datapath->header_.uuid),
> +                                   "dnat");
> +            char *snat = xasprintf(UUID_FMT"_%s",
> +
>  UUID_ARGS(&binding->datapath->header_.uuid),
> +                                   "snat");
> or similar.  Do you think it would be worth having a helper function (or
> two) so that they are harder to get out-of-sync?
>

How about something like the following?
diff --git a/ovn/controller/ovn-controller.c
b/ovn/controller/ovn-controller.c
index 3b55e6c..356a94b 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -40,6 +40,7 @@ 
 #include "openvswitch/vconn.h"
 #include "openvswitch/vlog.h"
 #include "ovn/lib/ovn-sb-idl.h"
+#include "ovn/lib/ovn-util.h"
 #include "patch.h"
 #include "physical.h"
 #include "pinctrl.h"
@@ -273,12 +274,8 @@  update_ct_zones(struct sset *lports, struct hmap
*patched_datapaths,
             continue;
         }

-        char *dnat = xasprintf(UUID_FMT"_%s",
-
 UUID_ARGS(&pd->port_binding->datapath->header_.uuid),
-                        "dnat");
-        char *snat = xasprintf(UUID_FMT"_%s",
-
UUID_ARGS(&pd->port_binding->datapath->header_.uuid),
-                       "snat");
+        char *dnat = alloc_nat_zone_key(pd->port_binding, "dnat");
+        char *snat = alloc_nat_zone_key(pd->port_binding, "snat");
         sset_add(&all_users, dnat);
         sset_add(&all_users, snat);
         free(dnat);
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index b6e752c..85528e0 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -24,6 +24,7 @@ 
 #include "openvswitch/vlog.h"
 #include "ovn-controller.h"
 #include "ovn/lib/ovn-sb-idl.h"
+#include "ovn/lib/ovn-util.h"
 #include "physical.h"
 #include "shash.h"
 #include "simap.h"
@@ -368,12 +369,8 @@  physical_run(struct controller_ctx *ctx, enum
mf_field_id mff_ovn_ge
             }

             int zone_id_dnat, zone_id_snat;
-            char *dnat = xasprintf(UUID_FMT"_%s",
-
UUID_ARGS(&binding->datapath->header_.uuid),
-                                   "dnat");
-            char *snat = xasprintf(UUID_FMT"_%s",
-
UUID_ARGS(&binding->datapath->header_.uuid),
-                                   "snat");
+            char *dnat = alloc_nat_zone_key(binding, "dnat");
+            char *snat = alloc_nat_zone_key(binding, "snat");
             zone_id_dnat = simap_get(ct_zones, dnat);
             if (zone_id_dnat) {
                 put_load(zone_id_dnat, MFF_LOG_DNAT_ZONE, 0, 32, &ofpacts);
diff --git a/ovn/lib/ovn-util.c b/ovn/lib/ovn-util.c
index abdc247..5a1dcb6 100644
--- a/ovn/lib/ovn-util.c
+++ b/ovn/lib/ovn-util.c
@@ -15,6 +15,7 @@ 
 #include <config.h>
 #include "ovn-util.h"
 #include "openvswitch/vlog.h"
+#include "ovn/lib/ovn-sb-idl.h"

 VLOG_DEFINE_THIS_MODULE(ovn_util);

@@ -102,3 +103,15 @@  extract_lport_addresses(char *address, struct
lport_addresses *laddr

     return true;
 }
+
+/* Allocates a key for NAT conntrack zone allocation for a provided
+ * 'port_binding' record and a 'type'.
+ *
+ * It is the caller's responsibility to free the allocated memory. */
+char *
+alloc_nat_zone_key(const struct sbrec_port_binding *port_binding,
+                   const char *type)
+{
+    return xasprintf(UUID_FMT"_%s",
+                     UUID_ARGS(&port_binding->datapath->header_.uuid),
type);
+}
diff --git a/ovn/lib/ovn-util.h b/ovn/lib/ovn-util.h
index 242984a..f475e6f 100644
--- a/ovn/lib/ovn-util.h
+++ b/ovn/lib/ovn-util.h
@@ -18,6 +18,8 @@ 

 #include "lib/packets.h"

+struct sbrec_port_binding;
+
 struct ipv4_netaddr {
     ovs_be32 addr;
     unsigned int plen;
@@ -40,5 +42,7 @@  bool
 extract_lport_addresses(char *address, struct lport_addresses *laddrs,
                         bool store_ipv6);

-
+char *
+alloc_nat_zone_key(const struct sbrec_port_binding *port_binding,
+                   const char *type);