Re: [ovs-dev] [PATCH v7 2/4] datapath-windows: Add NAT module in conntrack

2017-05-23 Thread Alin Serdean
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

2017-05-19 Thread Alin Serdean


> -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

2017-05-17 Thread Sairam Venugopal
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

2017-05-16 Thread Yin Lin
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

2017-05-16 Thread Sairam Venugopal
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

2017-05-09 Thread Yin Lin
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);
+}
+}
+}
+}
+
+/*
+