[ovs-dev,v1,2/3] datapath-windows: Add a global level RW lock for NAT

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

Commit Message

Anand Kumar Nov. 14, 2017, 6:48 p.m.
Currently NAT module relies on the existing conntrack lock.
This patch provides a basic lock implementation for NAT module
in conntrack.

Signed-off-by: Anand Kumar <kumaranand@vmware.com>
---
 datapath-windows/ovsext/Conntrack.c | 36 ++++++++++++++++++++++++++++++++++--
 1 file changed, 34 insertions(+), 2 deletions(-)

Comments

Alin Gabriel Serdean Nov. 28, 2017, 2:37 p.m. | #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 2/3] datapath-windows: Add a global level RW
> lock for NAT
> 
> Currently NAT module relies on the existing conntrack lock.
> This patch provides a basic lock implementation for NAT module in
conntrack.
> 
> Signed-off-by: Anand Kumar <kumaranand@vmware.com>
> ---
>  datapath-windows/ovsext/Conntrack.c | 36
> ++++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-
> windows/ovsext/Conntrack.c
> index 48d4abf..ba0dc88 100644
> --- a/datapath-windows/ovsext/Conntrack.c
> +++ b/datapath-windows/ovsext/Conntrack.c
> @@ -32,6 +32,7 @@ KSTART_ROUTINE OvsConntrackEntryCleaner;  static
> PLIST_ENTRY ovsConntrackTable;  static OVS_CT_THREAD_CTX ctThreadCtx;
> static PNDIS_RW_LOCK_EX ovsConntrackLockObj;
> +static PNDIS_RW_LOCK_EX ovsCtNatLockObj;
>  extern POVS_SWITCH_CONTEXT gOvsSwitchContext;  static LONG
> ctTotalEntries;
> 
> @@ -56,6 +57,13 @@ OvsInitConntrack(POVS_SWITCH_CONTEXT context)
>          return STATUS_INSUFFICIENT_RESOURCES;
>      }
> 
> +    ovsCtNatLockObj = NdisAllocateRWLock(context->NdisFilterHandle);
> +    if (ovsCtNatLockObj == NULL) {
> +        NdisFreeRWLock(ovsConntrackLockObj);
> +        ovsConntrackLockObj = NULL;
> +        return STATUS_INSUFFICIENT_RESOURCES;
> +    }
> +
>      /* Init the Hash Buffer */
>      ovsConntrackTable = OvsAllocateMemoryWithTag(sizeof(LIST_ENTRY)
>                                                   * CT_HASH_TABLE_SIZE, @@
-63,6 +71,8 @@
> OvsInitConntrack(POVS_SWITCH_CONTEXT context)
>      if (ovsConntrackTable == NULL) {
>          NdisFreeRWLock(ovsConntrackLockObj);
>          ovsConntrackLockObj = NULL;
> +        NdisFreeRWLock(ovsCtNatLockObj);
> +        ovsCtNatLockObj = NULL;
>          return STATUS_INSUFFICIENT_RESOURCES;
>      }
> 
> @@ -80,6 +90,9 @@ OvsInitConntrack(POVS_SWITCH_CONTEXT context)
>          NdisFreeRWLock(ovsConntrackLockObj);
>          ovsConntrackLockObj = NULL;
> 
> +        NdisFreeRWLock(ovsCtNatLockObj);
> +        ovsCtNatLockObj = NULL;
> +
>          OvsFreeMemoryWithTag(ovsConntrackTable, OVS_CT_POOL_TAG);
>          ovsConntrackTable = NULL;
> 
> @@ -109,7 +122,7 @@ OvsInitConntrack(POVS_SWITCH_CONTEXT context)
> VOID
>  OvsCleanupConntrack(VOID)
>  {
> -    LOCK_STATE_EX lockState;
> +    LOCK_STATE_EX lockState, lockStateNat;
>      NdisAcquireRWLockWrite(ovsConntrackLockObj, &lockState, 0);
>      ctThreadCtx.exit = 1;
>      KeSetEvent(&ctThreadCtx.event, 0, FALSE); @@ -129,7 +142,11 @@
> OvsCleanupConntrack(VOID)
> 
>      NdisFreeRWLock(ovsConntrackLockObj);
>      ovsConntrackLockObj = NULL;
> +    NdisAcquireRWLockWrite(ovsCtNatLockObj, &lockStateNat, 0);
>      OvsNatCleanup();
> +    NdisReleaseRWLock(ovsCtNatLockObj, &lockStateNat);
> +    NdisFreeRWLock(ovsCtNatLockObj);
> +    ovsCtNatLockObj = NULL;
>  }
> 
>  static __inline VOID
> @@ -195,15 +212,19 @@ OvsCtAddEntry(POVS_CT_ENTRY entry,
> OvsConntrackKeyLookupCtx *ctx,
>      if (natInfo == NULL) {
>          entry->natInfo.natAction = NAT_ACTION_NONE;
>      } else {
> +        LOCK_STATE_EX lockStateNat;
> +        NdisAcquireRWLockWrite(ovsCtNatLockObj, &lockStateNat, 0);
>          if (OvsIsForwardNat(natInfo->natAction)) {
>              entry->natInfo = *natInfo;
>              if (!OvsNatTranslateCtEntry(entry)) {
> +                NdisReleaseRWLock(ovsCtNatLockObj, &lockStateNat);
>                  return FALSE;
>              }
>              ctx->hash = OvsHashCtKey(&entry->key);
>          } else {
>              entry->natInfo.natAction = natInfo->natAction;
>          }
> +        NdisReleaseRWLock(ovsCtNatLockObj, &lockStateNat);
>      }
> 
>      entry->timestampStart = now;
> @@ -356,7 +377,10 @@ OvsCtEntryDelete(POVS_CT_ENTRY entry, BOOLEAN
> forceDelete)
>      }
>      if (forceDelete || OvsCtEntryExpired(entry)) {
>          if (entry->natInfo.natAction) {
> +            LOCK_STATE_EX lockStateNat;
> +            NdisAcquireRWLockWrite(ovsCtNatLockObj, &lockStateNat, 0);
>              OvsNatDeleteKey(&entry->key);
> +            NdisReleaseRWLock(ovsCtNatLockObj, &lockStateNat);
>          }
>          OvsPostCtEventEntry(entry, OVS_EVENT_CT_DELETE);
>          RemoveEntryList(&entry->link);
> @@ -558,7 +582,10 @@ OvsCtSetupLookupCtx(OvsFlowKey *flowKey,
>          return NDIS_STATUS_INVALID_PACKET;
>      }
> 
> +    LOCK_STATE_EX lockStateNat;
> +    NdisAcquireRWLockRead(ovsCtNatLockObj, &lockStateNat, 0);
>      natEntry = OvsNatLookup(&ctx->key, TRUE);
> +    NdisReleaseRWLock(ovsCtNatLockObj, &lockStateNat);
>      if (natEntry) {
>          /* Translate address first for reverse NAT */
>          ctx->key = natEntry->ctEntry->key; @@ -811,8 +838,11 @@
> OvsCtExecute_(OvsForwardingContext *fwdCtx,
>       */
>      if (natInfo->natAction != NAT_ACTION_NONE)
>      {
> +        LOCK_STATE_EX lockStateNat;
> +        NdisAcquireRWLockWrite(ovsCtNatLockObj, &lockStateNat, 0);
>          OvsNatPacket(fwdCtx, entry, entry->natInfo.natAction,
>                       key, ctx.reply);
> +        NdisReleaseRWLock(ovsCtNatLockObj, &lockStateNat);
>      }
> 
>      OvsCtSetMarkLabel(key, entry, mark, labels, &triggerUpdateEvent); @@
-
> 1050,7 +1080,7 @@ OvsCtFlush(UINT16 zone)
>      PLIST_ENTRY link, next;
>      POVS_CT_ENTRY entry;
> 
> -    LOCK_STATE_EX lockState;
> +    LOCK_STATE_EX lockState, lockStateNat;
>      NdisAcquireRWLockWrite(ovsConntrackLockObj, &lockState, 0);
> 
>      if (ctTotalEntries) {
> @@ -1064,7 +1094,9 @@ OvsCtFlush(UINT16 zone)
>          }
>      }
> 
> +    NdisAcquireRWLockWrite(ovsCtNatLockObj, &lockStateNat, 0);
>      OvsNatFlush(zone);
> +    NdisReleaseRWLock(ovsCtNatLockObj, &lockStateNat);
>      NdisReleaseRWLock(ovsConntrackLockObj, &lockState);
>      return NDIS_STATUS_SUCCESS;
>  }
> --
> 2.9.3.windows.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch

diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
index 48d4abf..ba0dc88 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -32,6 +32,7 @@  KSTART_ROUTINE OvsConntrackEntryCleaner;
 static PLIST_ENTRY ovsConntrackTable;
 static OVS_CT_THREAD_CTX ctThreadCtx;
 static PNDIS_RW_LOCK_EX ovsConntrackLockObj;
+static PNDIS_RW_LOCK_EX ovsCtNatLockObj;
 extern POVS_SWITCH_CONTEXT gOvsSwitchContext;
 static LONG ctTotalEntries;
 
@@ -56,6 +57,13 @@  OvsInitConntrack(POVS_SWITCH_CONTEXT context)
         return STATUS_INSUFFICIENT_RESOURCES;
     }
 
+    ovsCtNatLockObj = NdisAllocateRWLock(context->NdisFilterHandle);
+    if (ovsCtNatLockObj == NULL) {
+        NdisFreeRWLock(ovsConntrackLockObj);
+        ovsConntrackLockObj = NULL;
+        return STATUS_INSUFFICIENT_RESOURCES;
+    }
+
     /* Init the Hash Buffer */
     ovsConntrackTable = OvsAllocateMemoryWithTag(sizeof(LIST_ENTRY)
                                                  * CT_HASH_TABLE_SIZE,
@@ -63,6 +71,8 @@  OvsInitConntrack(POVS_SWITCH_CONTEXT context)
     if (ovsConntrackTable == NULL) {
         NdisFreeRWLock(ovsConntrackLockObj);
         ovsConntrackLockObj = NULL;
+        NdisFreeRWLock(ovsCtNatLockObj);
+        ovsCtNatLockObj = NULL;
         return STATUS_INSUFFICIENT_RESOURCES;
     }
 
@@ -80,6 +90,9 @@  OvsInitConntrack(POVS_SWITCH_CONTEXT context)
         NdisFreeRWLock(ovsConntrackLockObj);
         ovsConntrackLockObj = NULL;
 
+        NdisFreeRWLock(ovsCtNatLockObj);
+        ovsCtNatLockObj = NULL;
+
         OvsFreeMemoryWithTag(ovsConntrackTable, OVS_CT_POOL_TAG);
         ovsConntrackTable = NULL;
 
@@ -109,7 +122,7 @@  OvsInitConntrack(POVS_SWITCH_CONTEXT context)
 VOID
 OvsCleanupConntrack(VOID)
 {
-    LOCK_STATE_EX lockState;
+    LOCK_STATE_EX lockState, lockStateNat;
     NdisAcquireRWLockWrite(ovsConntrackLockObj, &lockState, 0);
     ctThreadCtx.exit = 1;
     KeSetEvent(&ctThreadCtx.event, 0, FALSE);
@@ -129,7 +142,11 @@  OvsCleanupConntrack(VOID)
 
     NdisFreeRWLock(ovsConntrackLockObj);
     ovsConntrackLockObj = NULL;
+    NdisAcquireRWLockWrite(ovsCtNatLockObj, &lockStateNat, 0);
     OvsNatCleanup();
+    NdisReleaseRWLock(ovsCtNatLockObj, &lockStateNat);
+    NdisFreeRWLock(ovsCtNatLockObj);
+    ovsCtNatLockObj = NULL;
 }
 
 static __inline VOID
@@ -195,15 +212,19 @@  OvsCtAddEntry(POVS_CT_ENTRY entry, OvsConntrackKeyLookupCtx *ctx,
     if (natInfo == NULL) {
         entry->natInfo.natAction = NAT_ACTION_NONE;
     } else {
+        LOCK_STATE_EX lockStateNat;
+        NdisAcquireRWLockWrite(ovsCtNatLockObj, &lockStateNat, 0);
         if (OvsIsForwardNat(natInfo->natAction)) {
             entry->natInfo = *natInfo;
             if (!OvsNatTranslateCtEntry(entry)) {
+                NdisReleaseRWLock(ovsCtNatLockObj, &lockStateNat);
                 return FALSE;
             }
             ctx->hash = OvsHashCtKey(&entry->key);
         } else {
             entry->natInfo.natAction = natInfo->natAction;
         }
+        NdisReleaseRWLock(ovsCtNatLockObj, &lockStateNat);
     }
 
     entry->timestampStart = now;
@@ -356,7 +377,10 @@  OvsCtEntryDelete(POVS_CT_ENTRY entry, BOOLEAN forceDelete)
     }
     if (forceDelete || OvsCtEntryExpired(entry)) {
         if (entry->natInfo.natAction) {
+            LOCK_STATE_EX lockStateNat;
+            NdisAcquireRWLockWrite(ovsCtNatLockObj, &lockStateNat, 0);
             OvsNatDeleteKey(&entry->key);
+            NdisReleaseRWLock(ovsCtNatLockObj, &lockStateNat);
         }
         OvsPostCtEventEntry(entry, OVS_EVENT_CT_DELETE);
         RemoveEntryList(&entry->link);
@@ -558,7 +582,10 @@  OvsCtSetupLookupCtx(OvsFlowKey *flowKey,
         return NDIS_STATUS_INVALID_PACKET;
     }
 
+    LOCK_STATE_EX lockStateNat;
+    NdisAcquireRWLockRead(ovsCtNatLockObj, &lockStateNat, 0);
     natEntry = OvsNatLookup(&ctx->key, TRUE);
+    NdisReleaseRWLock(ovsCtNatLockObj, &lockStateNat);
     if (natEntry) {
         /* Translate address first for reverse NAT */
         ctx->key = natEntry->ctEntry->key;
@@ -811,8 +838,11 @@  OvsCtExecute_(OvsForwardingContext *fwdCtx,
      */
     if (natInfo->natAction != NAT_ACTION_NONE)
     {
+        LOCK_STATE_EX lockStateNat;
+        NdisAcquireRWLockWrite(ovsCtNatLockObj, &lockStateNat, 0);
         OvsNatPacket(fwdCtx, entry, entry->natInfo.natAction,
                      key, ctx.reply);
+        NdisReleaseRWLock(ovsCtNatLockObj, &lockStateNat);
     }
 
     OvsCtSetMarkLabel(key, entry, mark, labels, &triggerUpdateEvent);
@@ -1050,7 +1080,7 @@  OvsCtFlush(UINT16 zone)
     PLIST_ENTRY link, next;
     POVS_CT_ENTRY entry;
 
-    LOCK_STATE_EX lockState;
+    LOCK_STATE_EX lockState, lockStateNat;
     NdisAcquireRWLockWrite(ovsConntrackLockObj, &lockState, 0);
 
     if (ctTotalEntries) {
@@ -1064,7 +1094,9 @@  OvsCtFlush(UINT16 zone)
         }
     }
 
+    NdisAcquireRWLockWrite(ovsCtNatLockObj, &lockStateNat, 0);
     OvsNatFlush(zone);
+    NdisReleaseRWLock(ovsCtNatLockObj, &lockStateNat);
     NdisReleaseRWLock(ovsConntrackLockObj, &lockState);
     return NDIS_STATUS_SUCCESS;
 }