Re: [ovs-dev] [PATCH v7 2/4] datapath-windows: Add NAT module in conntrack
Just one small nit on this one > datapath-windows/ovsext/Conntrack-icmp.c \ > datapath-windows/ovsext/Conntrack-other.c \ > datapath-windows/ovsext/Conntrack-related.c \ > +datapath-windows/ovsext/Conntrack-nat.c \ [Alin Serdean] tab instead of 4 space > datapath-windows/ovsext/Conntrack-tcp.c \ > +datapath-windows/ovsext/Conntrack-nat.h \ [Alin Serdean] tab instead of 4 space > datapath-windows/ovsext/Conntrack.c \ > datapath-windows/ovsext/Conntrack.h \ > datapath-windows/ovsext/Datapath.c \ ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v7 2/4] datapath-windows: Add NAT module in conntrack
> -Original Message- > From: Sairam Venugopal [mailto:vsai...@vmware.com] > Sent: Wednesday, May 17, 2017 9:05 PM > To: Yin Lin <li...@vmware.com>; d...@openvswitch.org > Cc: Alin Serdean <aserd...@cloudbasesolutions.com> > Subject: Re: [ovs-dev] [PATCH v7 2/4] datapath-windows: Add NAT module > in conntrack > > Hi Yin, > > Thanks for clarifying the comments. I will ack the next version. > > @Alin - will you have sometime to review the changes? [Alin Serdean] I'll give it a shot over the weekend . > > Thanks, > Sairam > > > > > On 5/16/17, 5:11 PM, "Yin Lin" <li...@vmware.com> wrote: > > >Thanks Sai for the review! I fixed most of them and explained the remaining > ones in the inline comments. > > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v7 2/4] datapath-windows: Add NAT module in conntrack
Hi Yin, Thanks for clarifying the comments. I will ack the next version. @Alin - will you have sometime to review the changes? Thanks, Sairam On 5/16/17, 5:11 PM, "Yin Lin" <li...@vmware.com> wrote: >Thanks Sai for the review! I fixed most of them and explained the remaining >ones in the inline comments. > >-Original Message- >From: Sairam Venugopal >Sent: Tuesday, May 16, 2017 4:50 PM >To: Yin Lin <li...@vmware.com>; d...@openvswitch.org >Subject: Re: [ovs-dev] [PATCH v7 2/4] datapath-windows: Add NAT module in >conntrack > >Hi Yin, > >Thanks for the patch. Please find my comments inline. > >Thanks, >Sairam > > > > >On 5/9/17, 3:59 PM, "ovs-dev-boun...@openvswitch.org on behalf of Yin Lin" ><ovs-dev-boun...@openvswitch.org on behalf of li...@vmware.com> wrote: > >>Signed-off-by: Yin Lin <li...@vmware.com> >>--- >> datapath-windows/automake.mk| 2 + >> datapath-windows/ovsext/Conntrack-nat.c | 424 >> >> datapath-windows/ovsext/Conntrack-nat.h | 39 +++ >> 3 files changed, 465 insertions(+) >> create mode 100644 datapath-windows/ovsext/Conntrack-nat.c >> create mode 100644 datapath-windows/ovsext/Conntrack-nat.h >> >>diff --git a/datapath-windows/automake.mk b/datapath-windows/automake.mk >>index 4f7b55a..7177630 100644 >>--- a/datapath-windows/automake.mk >>+++ b/datapath-windows/automake.mk >>@@ -16,7 +16,9 @@ EXTRA_DIST += \ >> datapath-windows/ovsext/Conntrack-icmp.c \ >> datapath-windows/ovsext/Conntrack-other.c \ >> datapath-windows/ovsext/Conntrack-related.c \ >>+datapath-windows/ovsext/Conntrack-nat.c \ >> datapath-windows/ovsext/Conntrack-tcp.c \ >>+datapath-windows/ovsext/Conntrack-nat.h \ >> datapath-windows/ovsext/Conntrack.c \ >> datapath-windows/ovsext/Conntrack.h \ >> datapath-windows/ovsext/Datapath.c \ >>diff --git a/datapath-windows/ovsext/Conntrack-nat.c >>b/datapath-windows/ovsext/Conntrack-nat.c >>new file mode 100644 >>index 000..edf5f8f >>--- /dev/null >>+++ b/datapath-windows/ovsext/Conntrack-nat.c >>@@ -0,0 +1,424 @@ >>+#include "Conntrack-nat.h" >>+#include "Jhash.h" >>+ >>+PLIST_ENTRY ovsNatTable = NULL; >>+PLIST_ENTRY ovsUnNatTable = NULL; >>+ >>+/* >>+ *--- >>+ * OvsHashNatKey >>+ * Hash NAT related fields in a Conntrack key. >>+ *--- >>+ */ >>+static __inline UINT32 >>+OvsHashNatKey(const OVS_CT_KEY *key) >>+{ >>+UINT32 hash = 0; >>+#define HASH_ADD(field) \ >>+hash = OvsJhashBytes(>field, sizeof(key->field), hash) >>+ >>+HASH_ADD(src.addr.ipv4_aligned); >>+HASH_ADD(dst.addr.ipv4_aligned); >>+HASH_ADD(src.port); >>+HASH_ADD(dst.port); >>+HASH_ADD(zone); >>+#undef HASH_ADD >>+return hash; >>+} >>+ >>+/* >>+ *--- >>+ * OvsNatKeyAreSame >>+ * Compare NAT related fields in a Conntrack key. >>+ *--- >>+ */ >>+static __inline BOOLEAN >>+OvsNatKeyAreSame(const OVS_CT_KEY *key1, const OVS_CT_KEY *key2) >>+{ >>+// XXX: Compare IPv6 key as well >>+#define FIELD_COMPARE(field) \ > >[Sai]: Nit: move return statement to next line. >[Yin]: This is a MACRO and I don't want to spread it over lines. (Note that I >don't even add a semicolon at the end to make it look like a function) > >>+if (key1->field != key2->field) return FALSE >>+ >>+FIELD_COMPARE(src.addr.ipv4_aligned); >>+FIELD_COMPARE(dst.addr.ipv4_aligned); >>+FIELD_COMPARE(src.port); >>+FIELD_COMPARE(dst.port); >>+FIELD_COMPARE(zone); >>+return TRUE; >>+#undef FIELD_COMPARE >>+} >>+ >>+/* >>+ *--- > >[Sai]: Nit - OvsNatGetBucket >[Yin]: Fixed > >>+ * OvsNaGetBucket >>+ * Returns the row of NAT table that has the same hash as the given NAT >>+ * hash key. If isReverse is TRUE, returns the row of reverse NAT table >>+ * instead. >>+ *--- >>+ */ >>+static __inline PLIST_ENTRY >>+OvsNatGetBucket(const OVS_CT_KEY
Re: [ovs-dev] [PATCH v7 2/4] datapath-windows: Add NAT module in conntrack
Thanks Sai for the review! I fixed most of them and explained the remaining ones in the inline comments. -Original Message- From: Sairam Venugopal Sent: Tuesday, May 16, 2017 4:50 PM To: Yin Lin <li...@vmware.com>; d...@openvswitch.org Subject: Re: [ovs-dev] [PATCH v7 2/4] datapath-windows: Add NAT module in conntrack Hi Yin, Thanks for the patch. Please find my comments inline. Thanks, Sairam On 5/9/17, 3:59 PM, "ovs-dev-boun...@openvswitch.org on behalf of Yin Lin" <ovs-dev-boun...@openvswitch.org on behalf of li...@vmware.com> wrote: >Signed-off-by: Yin Lin <li...@vmware.com> >--- > datapath-windows/automake.mk| 2 + > datapath-windows/ovsext/Conntrack-nat.c | 424 > datapath-windows/ovsext/Conntrack-nat.h | 39 +++ > 3 files changed, 465 insertions(+) > create mode 100644 datapath-windows/ovsext/Conntrack-nat.c > create mode 100644 datapath-windows/ovsext/Conntrack-nat.h > >diff --git a/datapath-windows/automake.mk b/datapath-windows/automake.mk >index 4f7b55a..7177630 100644 >--- a/datapath-windows/automake.mk >+++ b/datapath-windows/automake.mk >@@ -16,7 +16,9 @@ EXTRA_DIST += \ > datapath-windows/ovsext/Conntrack-icmp.c \ > datapath-windows/ovsext/Conntrack-other.c \ > datapath-windows/ovsext/Conntrack-related.c \ >+datapath-windows/ovsext/Conntrack-nat.c \ > datapath-windows/ovsext/Conntrack-tcp.c \ >+datapath-windows/ovsext/Conntrack-nat.h \ > datapath-windows/ovsext/Conntrack.c \ > datapath-windows/ovsext/Conntrack.h \ > datapath-windows/ovsext/Datapath.c \ >diff --git a/datapath-windows/ovsext/Conntrack-nat.c >b/datapath-windows/ovsext/Conntrack-nat.c >new file mode 100644 >index 000..edf5f8f >--- /dev/null >+++ b/datapath-windows/ovsext/Conntrack-nat.c >@@ -0,0 +1,424 @@ >+#include "Conntrack-nat.h" >+#include "Jhash.h" >+ >+PLIST_ENTRY ovsNatTable = NULL; >+PLIST_ENTRY ovsUnNatTable = NULL; >+ >+/* >+ *--- >+ * OvsHashNatKey >+ * Hash NAT related fields in a Conntrack key. >+ *--- >+ */ >+static __inline UINT32 >+OvsHashNatKey(const OVS_CT_KEY *key) >+{ >+UINT32 hash = 0; >+#define HASH_ADD(field) \ >+hash = OvsJhashBytes(>field, sizeof(key->field), hash) >+ >+HASH_ADD(src.addr.ipv4_aligned); >+HASH_ADD(dst.addr.ipv4_aligned); >+HASH_ADD(src.port); >+HASH_ADD(dst.port); >+HASH_ADD(zone); >+#undef HASH_ADD >+return hash; >+} >+ >+/* >+ *--- >+ * OvsNatKeyAreSame >+ * Compare NAT related fields in a Conntrack key. >+ *--- >+ */ >+static __inline BOOLEAN >+OvsNatKeyAreSame(const OVS_CT_KEY *key1, const OVS_CT_KEY *key2) >+{ >+// XXX: Compare IPv6 key as well >+#define FIELD_COMPARE(field) \ [Sai]: Nit: move return statement to next line. [Yin]: This is a MACRO and I don't want to spread it over lines. (Note that I don't even add a semicolon at the end to make it look like a function) >+if (key1->field != key2->field) return FALSE >+ >+FIELD_COMPARE(src.addr.ipv4_aligned); >+FIELD_COMPARE(dst.addr.ipv4_aligned); >+FIELD_COMPARE(src.port); >+FIELD_COMPARE(dst.port); >+FIELD_COMPARE(zone); >+return TRUE; >+#undef FIELD_COMPARE >+} >+ >+/* >+ *--- [Sai]: Nit - OvsNatGetBucket [Yin]: Fixed >+ * OvsNaGetBucket >+ * Returns the row of NAT table that has the same hash as the given NAT >+ * hash key. If isReverse is TRUE, returns the row of reverse NAT table >+ * instead. >+ *--- >+ */ >+static __inline PLIST_ENTRY >+OvsNatGetBucket(const OVS_CT_KEY *key, BOOLEAN isReverse) >+{ >+uint32_t hash = OvsHashNatKey(key); >+if (isReverse) { >+return [hash & NAT_HASH_TABLE_MASK]; >+} else { >+return [hash & NAT_HASH_TABLE_MASK]; >+} >+} >+ >+/* >+ *--- >+ * OvsNatInit >+ * Initialize NAT related resources. >+ *--- >+ */ >+NTSTATUS OvsNatInit() >+{ >+ASSERT(ovsNatTable == NULL); >+ >+/* Init the Hash Buffer */ >+ovsNatTable = OvsAllocateMemoryWithTag( >+sizeof(LIST_ENTRY) * NAT
Re: [ovs-dev] [PATCH v7 2/4] datapath-windows: Add NAT module in conntrack
Hi Yin, Thanks for the patch. Please find my comments inline. Thanks, Sairam On 5/9/17, 3:59 PM, "ovs-dev-boun...@openvswitch.org on behalf of Yin Lin"wrote: >Signed-off-by: Yin Lin >--- > datapath-windows/automake.mk| 2 + > datapath-windows/ovsext/Conntrack-nat.c | 424 > datapath-windows/ovsext/Conntrack-nat.h | 39 +++ > 3 files changed, 465 insertions(+) > create mode 100644 datapath-windows/ovsext/Conntrack-nat.c > create mode 100644 datapath-windows/ovsext/Conntrack-nat.h > >diff --git a/datapath-windows/automake.mk b/datapath-windows/automake.mk >index 4f7b55a..7177630 100644 >--- a/datapath-windows/automake.mk >+++ b/datapath-windows/automake.mk >@@ -16,7 +16,9 @@ EXTRA_DIST += \ > datapath-windows/ovsext/Conntrack-icmp.c \ > datapath-windows/ovsext/Conntrack-other.c \ > datapath-windows/ovsext/Conntrack-related.c \ >+datapath-windows/ovsext/Conntrack-nat.c \ > datapath-windows/ovsext/Conntrack-tcp.c \ >+datapath-windows/ovsext/Conntrack-nat.h \ > datapath-windows/ovsext/Conntrack.c \ > datapath-windows/ovsext/Conntrack.h \ > datapath-windows/ovsext/Datapath.c \ >diff --git a/datapath-windows/ovsext/Conntrack-nat.c >b/datapath-windows/ovsext/Conntrack-nat.c >new file mode 100644 >index 000..edf5f8f >--- /dev/null >+++ b/datapath-windows/ovsext/Conntrack-nat.c >@@ -0,0 +1,424 @@ >+#include "Conntrack-nat.h" >+#include "Jhash.h" >+ >+PLIST_ENTRY ovsNatTable = NULL; >+PLIST_ENTRY ovsUnNatTable = NULL; >+ >+/* >+ *--- >+ * OvsHashNatKey >+ * Hash NAT related fields in a Conntrack key. >+ *--- >+ */ >+static __inline UINT32 >+OvsHashNatKey(const OVS_CT_KEY *key) >+{ >+UINT32 hash = 0; >+#define HASH_ADD(field) \ >+hash = OvsJhashBytes(>field, sizeof(key->field), hash) >+ >+HASH_ADD(src.addr.ipv4_aligned); >+HASH_ADD(dst.addr.ipv4_aligned); >+HASH_ADD(src.port); >+HASH_ADD(dst.port); >+HASH_ADD(zone); >+#undef HASH_ADD >+return hash; >+} >+ >+/* >+ *--- >+ * OvsNatKeyAreSame >+ * Compare NAT related fields in a Conntrack key. >+ *--- >+ */ >+static __inline BOOLEAN >+OvsNatKeyAreSame(const OVS_CT_KEY *key1, const OVS_CT_KEY *key2) >+{ >+// XXX: Compare IPv6 key as well >+#define FIELD_COMPARE(field) \ [Sai]: Nit: move return statement to next line. >+if (key1->field != key2->field) return FALSE >+ >+FIELD_COMPARE(src.addr.ipv4_aligned); >+FIELD_COMPARE(dst.addr.ipv4_aligned); >+FIELD_COMPARE(src.port); >+FIELD_COMPARE(dst.port); >+FIELD_COMPARE(zone); >+return TRUE; >+#undef FIELD_COMPARE >+} >+ >+/* >+ *--- [Sai]: Nit - OvsNatGetBucket >+ * OvsNaGetBucket >+ * Returns the row of NAT table that has the same hash as the given NAT >+ * hash key. If isReverse is TRUE, returns the row of reverse NAT table >+ * instead. >+ *--- >+ */ >+static __inline PLIST_ENTRY >+OvsNatGetBucket(const OVS_CT_KEY *key, BOOLEAN isReverse) >+{ >+uint32_t hash = OvsHashNatKey(key); >+if (isReverse) { >+return [hash & NAT_HASH_TABLE_MASK]; >+} else { >+return [hash & NAT_HASH_TABLE_MASK]; >+} >+} >+ >+/* >+ *--- >+ * OvsNatInit >+ * Initialize NAT related resources. >+ *--- >+ */ >+NTSTATUS OvsNatInit() >+{ >+ASSERT(ovsNatTable == NULL); >+ >+/* Init the Hash Buffer */ >+ovsNatTable = OvsAllocateMemoryWithTag( >+sizeof(LIST_ENTRY) * NAT_HASH_TABLE_SIZE, >+OVS_CT_POOL_TAG); >+if (ovsNatTable == NULL) { >+goto failNoMem; >+} >+ >+ovsUnNatTable = OvsAllocateMemoryWithTag( >+sizeof(LIST_ENTRY) * NAT_HASH_TABLE_SIZE, >+OVS_CT_POOL_TAG); >+if (ovsUnNatTable == NULL) { >+goto freeNatTable; >+} >+ >+for (int i = 0; i < NAT_HASH_TABLE_SIZE; i++) { >+InitializeListHead([i]); >+InitializeListHead([i]); >+} >+return STATUS_SUCCESS; >+ >+freeNatTable: >+OvsFreeMemoryWithTag(ovsNatTable, OVS_CT_POOL_TAG); >+failNoMem: >+return STATUS_INSUFFICIENT_RESOURCES; >+} >+ >+/* >+ * >+ * OvsNatFlush >+ * Flushes out all NAT entries that match the given zone. >+ * >+ */ >+VOID OvsNatFlush(UINT16 zone) >+{
[ovs-dev] [PATCH v7 2/4] datapath-windows: Add NAT module in conntrack
Signed-off-by: Yin Lin--- datapath-windows/automake.mk| 2 + datapath-windows/ovsext/Conntrack-nat.c | 424 datapath-windows/ovsext/Conntrack-nat.h | 39 +++ 3 files changed, 465 insertions(+) create mode 100644 datapath-windows/ovsext/Conntrack-nat.c create mode 100644 datapath-windows/ovsext/Conntrack-nat.h diff --git a/datapath-windows/automake.mk b/datapath-windows/automake.mk index 4f7b55a..7177630 100644 --- a/datapath-windows/automake.mk +++ b/datapath-windows/automake.mk @@ -16,7 +16,9 @@ EXTRA_DIST += \ datapath-windows/ovsext/Conntrack-icmp.c \ datapath-windows/ovsext/Conntrack-other.c \ datapath-windows/ovsext/Conntrack-related.c \ +datapath-windows/ovsext/Conntrack-nat.c \ datapath-windows/ovsext/Conntrack-tcp.c \ +datapath-windows/ovsext/Conntrack-nat.h \ datapath-windows/ovsext/Conntrack.c \ datapath-windows/ovsext/Conntrack.h \ datapath-windows/ovsext/Datapath.c \ diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-windows/ovsext/Conntrack-nat.c new file mode 100644 index 000..edf5f8f --- /dev/null +++ b/datapath-windows/ovsext/Conntrack-nat.c @@ -0,0 +1,424 @@ +#include "Conntrack-nat.h" +#include "Jhash.h" + +PLIST_ENTRY ovsNatTable = NULL; +PLIST_ENTRY ovsUnNatTable = NULL; + +/* + *--- + * OvsHashNatKey + * Hash NAT related fields in a Conntrack key. + *--- + */ +static __inline UINT32 +OvsHashNatKey(const OVS_CT_KEY *key) +{ +UINT32 hash = 0; +#define HASH_ADD(field) \ +hash = OvsJhashBytes(>field, sizeof(key->field), hash) + +HASH_ADD(src.addr.ipv4_aligned); +HASH_ADD(dst.addr.ipv4_aligned); +HASH_ADD(src.port); +HASH_ADD(dst.port); +HASH_ADD(zone); +#undef HASH_ADD +return hash; +} + +/* + *--- + * OvsNatKeyAreSame + * Compare NAT related fields in a Conntrack key. + *--- + */ +static __inline BOOLEAN +OvsNatKeyAreSame(const OVS_CT_KEY *key1, const OVS_CT_KEY *key2) +{ +// XXX: Compare IPv6 key as well +#define FIELD_COMPARE(field) \ +if (key1->field != key2->field) return FALSE + +FIELD_COMPARE(src.addr.ipv4_aligned); +FIELD_COMPARE(dst.addr.ipv4_aligned); +FIELD_COMPARE(src.port); +FIELD_COMPARE(dst.port); +FIELD_COMPARE(zone); +return TRUE; +#undef FIELD_COMPARE +} + +/* + *--- + * OvsNaGetBucket + * Returns the row of NAT table that has the same hash as the given NAT + * hash key. If isReverse is TRUE, returns the row of reverse NAT table + * instead. + *--- + */ +static __inline PLIST_ENTRY +OvsNatGetBucket(const OVS_CT_KEY *key, BOOLEAN isReverse) +{ +uint32_t hash = OvsHashNatKey(key); +if (isReverse) { +return [hash & NAT_HASH_TABLE_MASK]; +} else { +return [hash & NAT_HASH_TABLE_MASK]; +} +} + +/* + *--- + * OvsNatInit + * Initialize NAT related resources. + *--- + */ +NTSTATUS OvsNatInit() +{ +ASSERT(ovsNatTable == NULL); + +/* Init the Hash Buffer */ +ovsNatTable = OvsAllocateMemoryWithTag( +sizeof(LIST_ENTRY) * NAT_HASH_TABLE_SIZE, +OVS_CT_POOL_TAG); +if (ovsNatTable == NULL) { +goto failNoMem; +} + +ovsUnNatTable = OvsAllocateMemoryWithTag( +sizeof(LIST_ENTRY) * NAT_HASH_TABLE_SIZE, +OVS_CT_POOL_TAG); +if (ovsUnNatTable == NULL) { +goto freeNatTable; +} + +for (int i = 0; i < NAT_HASH_TABLE_SIZE; i++) { +InitializeListHead([i]); +InitializeListHead([i]); +} +return STATUS_SUCCESS; + +freeNatTable: +OvsFreeMemoryWithTag(ovsNatTable, OVS_CT_POOL_TAG); +failNoMem: +return STATUS_INSUFFICIENT_RESOURCES; +} + +/* + * + * OvsNatFlush + * Flushes out all NAT entries that match the given zone. + * + */ +VOID OvsNatFlush(UINT16 zone) +{ +PLIST_ENTRY link, next; +for (int i = 0; i < NAT_HASH_TABLE_SIZE; i++) { +LIST_FORALL_SAFE([i], link, next) { +POVS_NAT_ENTRY entry = +CONTAINING_RECORD(link, OVS_NAT_ENTRY, link); +/* zone is a non-zero value */ +if (!zone || zone == entry->key.zone) { +OvsNatDeleteEntry(entry); +} +} +} +} + +/* +