diff mbox series

[ovs-dev,v1,1/3] datapath-windows: Refactor conntrack code.

Message ID 20171114184821.124-2-kumaranand@vmware.com
State Changes Requested
Delegated to: Alin Gabriel Serdean
Headers show
Series datapath-windows: New lock implementation in conntrack | expand

Commit Message

Anand Kumar Nov. 14, 2017, 6:48 p.m. UTC
Some of the functions and  code are refactored
so that new conntrack lock can be implemented

Signed-off-by: Anand Kumar <kumaranand@vmware.com>
---
 datapath-windows/ovsext/Conntrack-nat.c |  11 +--
 datapath-windows/ovsext/Conntrack.c     | 170 ++++++++++++++++++--------------
 datapath-windows/ovsext/Conntrack.h     |   4 -
 3 files changed, 101 insertions(+), 84 deletions(-)

Comments

Alin Serdean Nov. 28, 2017, 2:36 p.m. UTC | #1
Acked-by: Alin Gabriel Serdean <aserdean@ovn.org>

> -----Original Message-----
> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev-
> bounces@openvswitch.org] On Behalf Of Anand Kumar
> Sent: Tuesday, November 14, 2017 8:48 PM
> To: dev@openvswitch.org
> Subject: [ovs-dev] [PATCH v1 1/3] datapath-windows: Refactor conntrack
> code.
> 
> Some of the functions and  code are refactored so that new conntrack lock
> can be implemented
> 
> Signed-off-by: Anand Kumar <kumaranand@vmware.com>
> ---
>  datapath-windows/ovsext/Conntrack-nat.c |  11 +--
>  datapath-windows/ovsext/Conntrack.c     | 170 ++++++++++++++++++------
> --------
>  datapath-windows/ovsext/Conntrack.h     |   4 -
>  3 files changed, 101 insertions(+), 84 deletions(-)
>
diff mbox series

Patch

diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-windows/ovsext/Conntrack-nat.c
index c778f12..7975770 100644
--- a/datapath-windows/ovsext/Conntrack-nat.c
+++ b/datapath-windows/ovsext/Conntrack-nat.c
@@ -93,26 +93,23 @@  NTSTATUS OvsNatInit()
         sizeof(LIST_ENTRY) * NAT_HASH_TABLE_SIZE,
         OVS_CT_POOL_TAG);
     if (ovsNatTable == NULL) {
-        goto failNoMem;
+        return STATUS_INSUFFICIENT_RESOURCES;
     }
 
     ovsUnNatTable = OvsAllocateMemoryWithTag(
         sizeof(LIST_ENTRY) * NAT_HASH_TABLE_SIZE,
         OVS_CT_POOL_TAG);
     if (ovsUnNatTable == NULL) {
-        goto freeNatTable;
+        OvsFreeMemoryWithTag(ovsNatTable, OVS_CT_POOL_TAG);
+        return STATUS_INSUFFICIENT_RESOURCES;
     }
 
     for (int i = 0; i < NAT_HASH_TABLE_SIZE; i++) {
         InitializeListHead(&ovsNatTable[i]);
         InitializeListHead(&ovsUnNatTable[i]);
     }
-    return STATUS_SUCCESS;
 
-freeNatTable:
-    OvsFreeMemoryWithTag(ovsNatTable, OVS_CT_POOL_TAG);
-failNoMem:
-    return STATUS_INSUFFICIENT_RESOURCES;
+    return STATUS_SUCCESS;
 }
 
 /*
diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
index 3203411..48d4abf 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -33,7 +33,7 @@  static PLIST_ENTRY ovsConntrackTable;
 static OVS_CT_THREAD_CTX ctThreadCtx;
 static PNDIS_RW_LOCK_EX ovsConntrackLockObj;
 extern POVS_SWITCH_CONTEXT gOvsSwitchContext;
-static UINT64 ctTotalEntries;
+static LONG ctTotalEntries;
 
 static __inline NDIS_STATUS OvsCtFlush(UINT16 zone);
 
@@ -210,7 +210,7 @@  OvsCtAddEntry(POVS_CT_ENTRY entry, OvsConntrackKeyLookupCtx *ctx,
     InsertHeadList(&ovsConntrackTable[ctx->hash & CT_HASH_TABLE_MASK],
                    &entry->link);
 
-    ctTotalEntries++;
+    InterlockedIncrement((LONG volatile *)&ctTotalEntries);
     return TRUE;
 }
 
@@ -233,11 +233,6 @@  OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
     *entryCreated = FALSE;
     state |= OVS_CS_F_NEW;
 
-    parentEntry = OvsCtRelatedLookup(ctx->key, currentTime);
-    if (parentEntry != NULL) {
-        state |= OVS_CS_F_RELATED;
-    }
-
     switch (ipProto) {
     case IPPROTO_TCP:
     {
@@ -281,6 +276,11 @@  OvsCtEntryCreate(OvsForwardingContext *fwdCtx,
         break;
     }
 
+    parentEntry = OvsCtRelatedLookup(ctx->key, currentTime);
+    if (parentEntry != NULL && state != OVS_CS_F_INVALID) {
+        state |= OVS_CS_F_RELATED;
+    }
+
     if (state != OVS_CS_F_INVALID && commit) {
         if (entry) {
             entry->parent = parentEntry;
@@ -313,6 +313,7 @@  OvsCtUpdateEntry(OVS_CT_ENTRY* entry,
                  BOOLEAN reply,
                  UINT64 now)
 {
+    CT_UPDATE_RES status;
     switch (ipProto) {
     case IPPROTO_TCP:
     {
@@ -320,32 +321,23 @@  OvsCtUpdateEntry(OVS_CT_ENTRY* entry,
         const TCPHdr *tcp;
         tcp = OvsGetTcp(nbl, l4Offset, &tcpStorage);
         if (!tcp) {
-            return CT_UPDATE_INVALID;
+            status =  CT_UPDATE_INVALID;
+            break;
         }
-        return OvsConntrackUpdateTcpEntry(entry, tcp, nbl, reply, now);
+        status =  OvsConntrackUpdateTcpEntry(entry, tcp, nbl, reply, now);
+        break;
     }
     case IPPROTO_ICMP:
-        return OvsConntrackUpdateIcmpEntry(entry, reply, now);
+        status =  OvsConntrackUpdateIcmpEntry(entry, reply, now);
+        break;
     case IPPROTO_UDP:
-        return OvsConntrackUpdateOtherEntry(entry, reply, now);
+        status =  OvsConntrackUpdateOtherEntry(entry, reply, now);
+        break;
     default:
-        return CT_UPDATE_INVALID;
-    }
-}
-
-static __inline VOID
-OvsCtEntryDelete(POVS_CT_ENTRY entry)
-{
-    if (entry == NULL) {
-        return;
-    }
-    if (entry->natInfo.natAction) {
-        OvsNatDeleteKey(&entry->key);
+        status =  CT_UPDATE_INVALID;
+        break;
     }
-    OvsPostCtEventEntry(entry, OVS_EVENT_CT_DELETE);
-    RemoveEntryList(&entry->link);
-    OvsFreeMemoryWithTag(entry, OVS_CT_POOL_TAG);
-    ctTotalEntries--;
+    return status;
 }
 
 static __inline BOOLEAN
@@ -356,6 +348,24 @@  OvsCtEntryExpired(POVS_CT_ENTRY entry)
     return entry->expiration < currentTime;
 }
 
+static __inline VOID
+OvsCtEntryDelete(POVS_CT_ENTRY entry, BOOLEAN forceDelete)
+{
+    if (entry == NULL) {
+        return;
+    }
+    if (forceDelete || OvsCtEntryExpired(entry)) {
+        if (entry->natInfo.natAction) {
+            OvsNatDeleteKey(&entry->key);
+        }
+        OvsPostCtEventEntry(entry, OVS_EVENT_CT_DELETE);
+        RemoveEntryList(&entry->link);
+        OvsFreeMemoryWithTag(entry, OVS_CT_POOL_TAG);
+        InterlockedDecrement((LONG volatile*)&ctTotalEntries);
+        return;
+    }
+}
+
 static __inline NDIS_STATUS
 OvsDetectCtPacket(OvsForwardingContext *fwdCtx,
                   OvsFlowKey *key,
@@ -423,21 +433,20 @@  OvsCtLookup(OvsConntrackKeyLookupCtx *ctx)
         if (OvsCtKeyAreSame(ctx->key, entry->key)) {
             found = entry;
             reply = FALSE;
-            break;
         }
 
-        if (OvsCtKeyAreSame(revCtxKey, entry->key)) {
+        if (!found && OvsCtKeyAreSame(revCtxKey, entry->key)) {
             found = entry;
             reply = TRUE;
-            break;
         }
-    }
 
-    if (found) {
-        if (OvsCtEntryExpired(found)) {
-            found = NULL;
-        } else {
-            ctx->reply = reply;
+        if (found) {
+            if (OvsCtEntryExpired(found)) {
+                found = NULL;
+            } else {
+                ctx->reply = reply;
+            }
+            break;
         }
     }
 
@@ -611,7 +620,7 @@  OvsProcessConntrackEntry(OvsForwardingContext *fwdCtx,
             break;
         case CT_UPDATE_NEW:
             //Delete and update the Conntrack
-            OvsCtEntryDelete(ctx->entry);
+            OvsCtEntryDelete(ctx->entry, TRUE);
             ctx->entry = NULL;
             entry = OvsCtEntryCreate(fwdCtx, key->ipKey.nwProto, l4Offset,
                                      ctx, key, natInfo, commit, currentTime,
@@ -687,6 +696,41 @@  OvsConntrackSetLabels(OvsFlowKey *key,
                    sizeof(struct ovs_key_ct_labels));
 }
 
+static void
+OvsCtSetMarkLabel(OvsFlowKey *key,
+                       POVS_CT_ENTRY entry,
+                       MD_MARK *mark,
+                       MD_LABELS *labels,
+                       BOOLEAN *triggerUpdateEvent)
+{
+    if (mark) {
+        OvsConntrackSetMark(key, entry, mark->value, mark->mask,
+                            triggerUpdateEvent);
+    }
+
+    if (labels) {
+        OvsConntrackSetLabels(key, entry, &labels->value, &labels->mask,
+                              triggerUpdateEvent);
+    }
+}
+
+static __inline void
+OvsCtUpdateTuple(OvsFlowKey *key, OVS_CT_KEY *ctKey)
+{
+    key->ct.tuple_ipv4.ipv4_src = ctKey->src.addr.ipv4_aligned;
+    key->ct.tuple_ipv4.ipv4_dst = ctKey->dst.addr.ipv4_aligned;
+    key->ct.tuple_ipv4.ipv4_proto = ctKey->nw_proto;
+
+    /* Orig tuple Port is overloaded to take in ICMP-Type & Code */
+    /* This mimics the behavior in lib/conntrack.c*/
+    key->ct.tuple_ipv4.src_port = ctKey->nw_proto != IPPROTO_ICMP ?
+                                    ctKey->src.port :
+                                    htons(ctKey->src.icmp_type);
+    key->ct.tuple_ipv4.dst_port = ctKey->nw_proto != IPPROTO_ICMP ?
+                                    ctKey->dst.port :
+                                    htons(ctKey->src.icmp_code);
+}
+
 static __inline NDIS_STATUS
 OvsCtExecute_(OvsForwardingContext *fwdCtx,
               OvsFlowKey *key,
@@ -721,7 +765,7 @@  OvsCtExecute_(OvsForwardingContext *fwdCtx,
 
     /* Delete entry in reverse direction if 'force' is specified */
     if (entry && force && ctx.reply) {
-        OvsCtEntryDelete(entry);
+        OvsCtEntryDelete(entry, TRUE);
         entry = NULL;
     }
 
@@ -755,6 +799,9 @@  OvsCtExecute_(OvsForwardingContext *fwdCtx,
                                          &entryCreated);
     }
 
+    if (entry == NULL) {
+        return status;
+    }
     /*
      * Note that natInfo is not the same as entry->natInfo here. natInfo
      * is decided by action in the openflow rule, entry->natInfo is decided
@@ -762,23 +809,15 @@  OvsCtExecute_(OvsForwardingContext *fwdCtx,
      * NAT_ACTION_REVERSE, yet entry->natInfo is NAT_ACTION_SRC or
      * NAT_ACTION_DST without NAT_ACTION_REVERSE
      */
-    if (entry && natInfo->natAction != NAT_ACTION_NONE)
+    if (natInfo->natAction != NAT_ACTION_NONE)
     {
         OvsNatPacket(fwdCtx, entry, entry->natInfo.natAction,
                      key, ctx.reply);
     }
 
-    if (entry && mark) {
-        OvsConntrackSetMark(key, entry, mark->value, mark->mask,
-                            &triggerUpdateEvent);
-    }
-
-    if (entry && labels) {
-        OvsConntrackSetLabels(key, entry, &labels->value, &labels->mask,
-                              &triggerUpdateEvent);
-    }
+    OvsCtSetMarkLabel(key, entry, mark, labels, &triggerUpdateEvent);
 
-    if (entry && OvsDetectFtpPacket(key)) {
+    if (OvsDetectFtpPacket(key)) {
         /* FTP parser will always be loaded */
         UNREFERENCED_PARAMETER(helper);
 
@@ -790,33 +829,19 @@  OvsCtExecute_(OvsForwardingContext *fwdCtx,
     }
 
     /* Add original tuple information to flow Key */
-    if (entry && entry->key.dl_type == ntohs(ETH_TYPE_IPV4)) {
-        OVS_CT_KEY *ctKey;
+    if (entry->key.dl_type == ntohs(ETH_TYPE_IPV4)) {
         if (entry->parent != NULL) {
             POVS_CT_ENTRY parent = entry->parent;
-            ctKey = &parent->key;
+            OvsCtUpdateTuple(key, &parent->key);
         } else {
-            ctKey = &entry->key;
+            OvsCtUpdateTuple(key, &entry->key);
         }
-
-        key->ct.tuple_ipv4.ipv4_src = ctKey->src.addr.ipv4_aligned;
-        key->ct.tuple_ipv4.ipv4_dst = ctKey->dst.addr.ipv4_aligned;
-        key->ct.tuple_ipv4.ipv4_proto = ctKey->nw_proto;
-
-        /* Orig tuple Port is overloaded to take in ICMP-Type & Code */
-        /* This mimics the behavior in lib/conntrack.c*/
-        key->ct.tuple_ipv4.src_port = ctKey->nw_proto != IPPROTO_ICMP ?
-                                      ctKey->src.port :
-                                      htons(ctKey->src.icmp_type);
-        key->ct.tuple_ipv4.dst_port = ctKey->nw_proto != IPPROTO_ICMP ?
-                                      ctKey->dst.port :
-                                      htons(ctKey->src.icmp_code);
     }
 
-    if (entryCreated && entry) {
+    if (entryCreated) {
         OvsPostCtEventEntry(entry, OVS_EVENT_CT_NEW);
     }
-    if (postUpdateEvent && entry && !entryCreated && triggerUpdateEvent) {
+    if (postUpdateEvent && !entryCreated && triggerUpdateEvent) {
         OvsPostCtEventEntry(entry, OVS_EVENT_CT_UPDATE);
     }
 
@@ -1001,9 +1026,7 @@  OvsConntrackEntryCleaner(PVOID data)
             for (int i = 0; i < CT_HASH_TABLE_SIZE; i++) {
                 LIST_FORALL_SAFE(&ovsConntrackTable[i], link, next) {
                     entry = CONTAINING_RECORD(link, OVS_CT_ENTRY, link);
-                    if (entry && OvsCtEntryExpired(entry)) {
-                        OvsCtEntryDelete(entry);
-                    }
+                    OvsCtEntryDelete(entry, FALSE);
                 }
             }
         }
@@ -1031,12 +1054,12 @@  OvsCtFlush(UINT16 zone)
     NdisAcquireRWLockWrite(ovsConntrackLockObj, &lockState, 0);
 
     if (ctTotalEntries) {
-        for (int i = 0; i < CT_HASH_TABLE_SIZE; i++) {
+        for (UINT32 i = 0; i < CT_HASH_TABLE_SIZE; i++) {
             LIST_FORALL_SAFE(&ovsConntrackTable[i], link, next) {
                 entry = CONTAINING_RECORD(link, OVS_CT_ENTRY, link);
                 /* zone is a non-zero value */
                 if (!zone || zone == entry->key.zone)
-                    OvsCtEntryDelete(entry);
+                    OvsCtEntryDelete(entry, TRUE);
             }
         }
     }
@@ -1312,6 +1335,7 @@  OvsCreateNlMsgFromCtEntry(POVS_CT_ENTRY entry,
     UINT16 nlmsgFlags = NLM_F_CREATE;
     NdisGetCurrentSystemTime((LARGE_INTEGER *)&currentTime);
     UINT8 nfgenFamily = 0;
+
     if (entry->key.dl_type == htons(ETH_TYPE_IPV4)) {
         nfgenFamily = AF_INET;
     } else if (entry->key.dl_type == htons(ETH_TYPE_IPV6)) {
diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-windows/ovsext/Conntrack.h
index be5f34d..66957e8 100644
--- a/datapath-windows/ovsext/Conntrack.h
+++ b/datapath-windows/ovsext/Conntrack.h
@@ -231,8 +231,4 @@  NDIS_STATUS OvsCtHandleFtp(PNET_BUFFER_LIST curNbl,
                            BOOLEAN request);
 
 UINT32 OvsHashCtKey(const OVS_CT_KEY *key);
-BOOLEAN OvsCtKeyAreSame(OVS_CT_KEY ctxKey, OVS_CT_KEY entryKey);
-POVS_CT_ENTRY OvsCtLookup(OvsConntrackKeyLookupCtx *ctx);
-
-
 #endif /* __OVS_CONNTRACK_H_ */