diff mbox series

[ovs-dev,v4,3/4] datapath-windows: Implement locking in conntrack NAT.

Message ID 20180618053729.2372-4-kumaranand@vmware.com
State Superseded
Headers show
Series Optimize conntrack performance | expand

Commit Message

Anand Kumar June 18, 2018, 5:37 a.m. UTC 
The 'ovsNatTable' and 'ovsUnNatTable' tables are shared
between cleanup threads and packet processing thread.
In order to protect these two tables use a spinlock.

Also introduce counters to track number of nat entries.

Signed-off-by: Anand Kumar <kumaranand@vmware.com>
---
 datapath-windows/ovsext/Conntrack-nat.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

Comments

Shashank Ram June 18, 2018, 9:36 p.m. UTC | #1 
This patch should be combined with the patch where NAT lock is removed 
from CT. Keeping this separate will cause the previous patches in this 
series to break NAT functionality.

Thanks,
Shashank


On 06/17/2018 10:37 PM, Anand Kumar wrote:
> The 'ovsNatTable' and 'ovsUnNatTable' tables are shared
> between cleanup threads and packet processing thread.
> In order to protect these two tables use a spinlock.
>
> Also introduce counters to track number of nat entries.
>
> Signed-off-by: Anand Kumar <kumaranand@vmware.com>
> ---
>   datapath-windows/ovsext/Conntrack-nat.c | 27 ++++++++++++++++++++++++++-
>   1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-windows/ovsext/Conntrack-nat.c
> index da1814f..11057e6 100644
> --- a/datapath-windows/ovsext/Conntrack-nat.c
> +++ b/datapath-windows/ovsext/Conntrack-nat.c
> @@ -3,7 +3,8 @@
>   
>   PLIST_ENTRY ovsNatTable = NULL;
>   PLIST_ENTRY ovsUnNatTable = NULL;
> -
> +static NDIS_SPIN_LOCK ovsCtNatLock;
> +static ULONG ovsNatEntries;
>   /*
>    *---------------------------------------------------------------------------
>    * OvsHashNatKey
> @@ -109,6 +110,8 @@ NTSTATUS OvsNatInit()
>           InitializeListHead(&ovsUnNatTable[i]);
>       }
>   
> +    NdisAllocateSpinLock(&ovsCtNatLock);
> +    ovsNatEntries = 0;
>       return STATUS_SUCCESS;
>   }
>   
> @@ -121,6 +124,11 @@ NTSTATUS OvsNatInit()
>   VOID OvsNatFlush(UINT16 zone)
>   {
>       PLIST_ENTRY link, next;
> +    if (!ovsNatEntries) {
> +        return;
> +    }
> +
> +    NdisAcquireSpinLock(&ovsCtNatLock);
>       for (int i = 0; i < NAT_HASH_TABLE_SIZE; i++) {
>           LIST_FORALL_SAFE(&ovsNatTable[i], link, next) {
>               POVS_NAT_ENTRY entry =
> @@ -131,6 +139,7 @@ VOID OvsNatFlush(UINT16 zone)
>               }
>           }
>       }
> +    NdisReleaseSpinLock(&ovsCtNatLock);
>   }
>   
>   /*
> @@ -144,10 +153,14 @@ VOID OvsNatCleanup()
>       if (ovsNatTable == NULL) {
>          return;
>       }
> +
> +    NdisAcquireSpinLock(&ovsCtNatLock);
>       OvsFreeMemoryWithTag(ovsNatTable, OVS_CT_POOL_TAG);
>       OvsFreeMemoryWithTag(ovsUnNatTable, OVS_CT_POOL_TAG);
>       ovsNatTable = NULL;
>       ovsUnNatTable = NULL;
> +    NdisReleaseSpinLock(&ovsCtNatLock);
> +    NdisFreeSpinLock(&ovsCtNatLock);
>   }
>   
>   /*
> @@ -250,10 +263,13 @@ static UINT32 OvsNatHashRange(const OVS_CT_ENTRY *entry, UINT32 basis)
>   VOID
>   OvsNatAddEntry(OVS_NAT_ENTRY* entry)
>   {
> +    NdisAcquireSpinLock(&ovsCtNatLock);
>       InsertHeadList(OvsNatGetBucket(&entry->key, FALSE),
>                      &entry->link);
>       InsertHeadList(OvsNatGetBucket(&entry->value, TRUE),
>                      &entry->reverseLink);
> +    NdisReleaseSpinLock(&ovsCtNatLock);
> +    NdisInterlockedIncrement((PLONG)&ovsNatEntries);
>   }
>   
>   /*
> @@ -399,21 +415,29 @@ OvsNatLookup(const OVS_CT_KEY *ctKey, BOOLEAN reverse)
>       PLIST_ENTRY link;
>       POVS_NAT_ENTRY entry;
>   
> +    if (!ovsNatEntries) {
> +        return NULL;
> +    }
> +
> +    NdisAcquireSpinLock(&ovsCtNatLock);
>       LIST_FORALL(OvsNatGetBucket(ctKey, reverse), link) {
>           if (reverse) {
>               entry = CONTAINING_RECORD(link, OVS_NAT_ENTRY, reverseLink);
>   
>               if (OvsNatKeyAreSame(ctKey, &entry->value)) {
> +                NdisReleaseSpinLock(&ovsCtNatLock);
>                   return entry;
>               }
>           } else {
>               entry = CONTAINING_RECORD(link, OVS_NAT_ENTRY, link);
>   
>               if (OvsNatKeyAreSame(ctKey, &entry->key)) {
> +                NdisReleaseSpinLock(&ovsCtNatLock);
>                   return entry;
>               }
>           }
>       }
> +    NdisReleaseSpinLock(&ovsCtNatLock);
>       return NULL;
>   }
>   
> @@ -432,6 +456,7 @@ OvsNatDeleteEntry(POVS_NAT_ENTRY entry)
>       RemoveEntryList(&entry->link);
>       RemoveEntryList(&entry->reverseLink);
>       OvsFreeMemoryWithTag(entry, OVS_CT_POOL_TAG);
> +    NdisInterlockedDecrement((PLONG)&ovsNatEntries);
>   }
>   
>   /*
Anand Kumar June 19, 2018, 12:45 a.m. UTC | #2 
Hi Shashank,

I will address this in next version of the patch.

Thanks,
Anand Kumar

On 6/18/18, 2:36 PM, "Shashank Ram" <rams@vmware.com> wrote:

    This patch should be combined with the patch where NAT lock is removed 
    from CT. Keeping this separate will cause the previous patches in this 
    series to break NAT functionality.
    
    Thanks,
    Shashank
    
    
    On 06/17/2018 10:37 PM, Anand Kumar wrote:
    > The 'ovsNatTable' and 'ovsUnNatTable' tables are shared
    > between cleanup threads and packet processing thread.
    > In order to protect these two tables use a spinlock.
    >
    > Also introduce counters to track number of nat entries.
    >
    > Signed-off-by: Anand Kumar <kumaranand@vmware.com>
    > ---
    >   datapath-windows/ovsext/Conntrack-nat.c | 27 ++++++++++++++++++++++++++-
    >   1 file changed, 26 insertions(+), 1 deletion(-)
    >
    > diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-windows/ovsext/Conntrack-nat.c
    > index da1814f..11057e6 100644
    > --- a/datapath-windows/ovsext/Conntrack-nat.c
    > +++ b/datapath-windows/ovsext/Conntrack-nat.c
    > @@ -3,7 +3,8 @@
    >   
    >   PLIST_ENTRY ovsNatTable = NULL;
    >   PLIST_ENTRY ovsUnNatTable = NULL;
    > -
    > +static NDIS_SPIN_LOCK ovsCtNatLock;
    > +static ULONG ovsNatEntries;
    >   /*
    >    *---------------------------------------------------------------------------
    >    * OvsHashNatKey
    > @@ -109,6 +110,8 @@ NTSTATUS OvsNatInit()
    >           InitializeListHead(&ovsUnNatTable[i]);
    >       }
    >   
    > +    NdisAllocateSpinLock(&ovsCtNatLock);
    > +    ovsNatEntries = 0;
    >       return STATUS_SUCCESS;
    >   }
    >   
    > @@ -121,6 +124,11 @@ NTSTATUS OvsNatInit()
    >   VOID OvsNatFlush(UINT16 zone)
    >   {
    >       PLIST_ENTRY link, next;
    > +    if (!ovsNatEntries) {
    > +        return;
    > +    }
    > +
    > +    NdisAcquireSpinLock(&ovsCtNatLock);
    >       for (int i = 0; i < NAT_HASH_TABLE_SIZE; i++) {
    >           LIST_FORALL_SAFE(&ovsNatTable[i], link, next) {
    >               POVS_NAT_ENTRY entry =
    > @@ -131,6 +139,7 @@ VOID OvsNatFlush(UINT16 zone)
    >               }
    >           }
    >       }
    > +    NdisReleaseSpinLock(&ovsCtNatLock);
    >   }
    >   
    >   /*
    > @@ -144,10 +153,14 @@ VOID OvsNatCleanup()
    >       if (ovsNatTable == NULL) {
    >          return;
    >       }
    > +
    > +    NdisAcquireSpinLock(&ovsCtNatLock);
    >       OvsFreeMemoryWithTag(ovsNatTable, OVS_CT_POOL_TAG);
    >       OvsFreeMemoryWithTag(ovsUnNatTable, OVS_CT_POOL_TAG);
    >       ovsNatTable = NULL;
    >       ovsUnNatTable = NULL;
    > +    NdisReleaseSpinLock(&ovsCtNatLock);
    > +    NdisFreeSpinLock(&ovsCtNatLock);
    >   }
    >   
    >   /*
    > @@ -250,10 +263,13 @@ static UINT32 OvsNatHashRange(const OVS_CT_ENTRY *entry, UINT32 basis)
    >   VOID
    >   OvsNatAddEntry(OVS_NAT_ENTRY* entry)
    >   {
    > +    NdisAcquireSpinLock(&ovsCtNatLock);
    >       InsertHeadList(OvsNatGetBucket(&entry->key, FALSE),
    >                      &entry->link);
    >       InsertHeadList(OvsNatGetBucket(&entry->value, TRUE),
    >                      &entry->reverseLink);
    > +    NdisReleaseSpinLock(&ovsCtNatLock);
    > +    NdisInterlockedIncrement((PLONG)&ovsNatEntries);
    >   }
    >   
    >   /*
    > @@ -399,21 +415,29 @@ OvsNatLookup(const OVS_CT_KEY *ctKey, BOOLEAN reverse)
    >       PLIST_ENTRY link;
    >       POVS_NAT_ENTRY entry;
    >   
    > +    if (!ovsNatEntries) {
    > +        return NULL;
    > +    }
    > +
    > +    NdisAcquireSpinLock(&ovsCtNatLock);
    >       LIST_FORALL(OvsNatGetBucket(ctKey, reverse), link) {
    >           if (reverse) {
    >               entry = CONTAINING_RECORD(link, OVS_NAT_ENTRY, reverseLink);
    >   
    >               if (OvsNatKeyAreSame(ctKey, &entry->value)) {
    > +                NdisReleaseSpinLock(&ovsCtNatLock);
    >                   return entry;
    >               }
    >           } else {
    >               entry = CONTAINING_RECORD(link, OVS_NAT_ENTRY, link);
    >   
    >               if (OvsNatKeyAreSame(ctKey, &entry->key)) {
    > +                NdisReleaseSpinLock(&ovsCtNatLock);
    >                   return entry;
    >               }
    >           }
    >       }
    > +    NdisReleaseSpinLock(&ovsCtNatLock);
    >       return NULL;
    >   }
    >   
    > @@ -432,6 +456,7 @@ OvsNatDeleteEntry(POVS_NAT_ENTRY entry)
    >       RemoveEntryList(&entry->link);
    >       RemoveEntryList(&entry->reverseLink);
    >       OvsFreeMemoryWithTag(entry, OVS_CT_POOL_TAG);
    > +    NdisInterlockedDecrement((PLONG)&ovsNatEntries);
    >   }
    >   
    >   /*
diff mbox series

Patch

diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-windows/ovsext/Conntrack-nat.c
index da1814f..11057e6 100644
--- a/datapath-windows/ovsext/Conntrack-nat.c
+++ b/datapath-windows/ovsext/Conntrack-nat.c
@@ -3,7 +3,8 @@ 
 
 PLIST_ENTRY ovsNatTable = NULL;
 PLIST_ENTRY ovsUnNatTable = NULL;
-
+static NDIS_SPIN_LOCK ovsCtNatLock;
+static ULONG ovsNatEntries;
 /*
  *---------------------------------------------------------------------------
  * OvsHashNatKey
@@ -109,6 +110,8 @@  NTSTATUS OvsNatInit()
         InitializeListHead(&ovsUnNatTable[i]);
     }
 
+    NdisAllocateSpinLock(&ovsCtNatLock);
+    ovsNatEntries = 0;
     return STATUS_SUCCESS;
 }
 
@@ -121,6 +124,11 @@  NTSTATUS OvsNatInit()
 VOID OvsNatFlush(UINT16 zone)
 {
     PLIST_ENTRY link, next;
+    if (!ovsNatEntries) {
+        return;
+    }
+
+    NdisAcquireSpinLock(&ovsCtNatLock);
     for (int i = 0; i < NAT_HASH_TABLE_SIZE; i++) {
         LIST_FORALL_SAFE(&ovsNatTable[i], link, next) {
             POVS_NAT_ENTRY entry =
@@ -131,6 +139,7 @@  VOID OvsNatFlush(UINT16 zone)
             }
         }
     }
+    NdisReleaseSpinLock(&ovsCtNatLock);
 }
 
 /*
@@ -144,10 +153,14 @@  VOID OvsNatCleanup()
     if (ovsNatTable == NULL) {
        return;
     }
+
+    NdisAcquireSpinLock(&ovsCtNatLock);
     OvsFreeMemoryWithTag(ovsNatTable, OVS_CT_POOL_TAG);
     OvsFreeMemoryWithTag(ovsUnNatTable, OVS_CT_POOL_TAG);
     ovsNatTable = NULL;
     ovsUnNatTable = NULL;
+    NdisReleaseSpinLock(&ovsCtNatLock);
+    NdisFreeSpinLock(&ovsCtNatLock);
 }
 
 /*
@@ -250,10 +263,13 @@  static UINT32 OvsNatHashRange(const OVS_CT_ENTRY *entry, UINT32 basis)
 VOID
 OvsNatAddEntry(OVS_NAT_ENTRY* entry)
 {
+    NdisAcquireSpinLock(&ovsCtNatLock);
     InsertHeadList(OvsNatGetBucket(&entry->key, FALSE),
                    &entry->link);
     InsertHeadList(OvsNatGetBucket(&entry->value, TRUE),
                    &entry->reverseLink);
+    NdisReleaseSpinLock(&ovsCtNatLock);
+    NdisInterlockedIncrement((PLONG)&ovsNatEntries);
 }
 
 /*
@@ -399,21 +415,29 @@  OvsNatLookup(const OVS_CT_KEY *ctKey, BOOLEAN reverse)
     PLIST_ENTRY link;
     POVS_NAT_ENTRY entry;
 
+    if (!ovsNatEntries) {
+        return NULL;
+    }
+
+    NdisAcquireSpinLock(&ovsCtNatLock);
     LIST_FORALL(OvsNatGetBucket(ctKey, reverse), link) {
         if (reverse) {
             entry = CONTAINING_RECORD(link, OVS_NAT_ENTRY, reverseLink);
 
             if (OvsNatKeyAreSame(ctKey, &entry->value)) {
+                NdisReleaseSpinLock(&ovsCtNatLock);
                 return entry;
             }
         } else {
             entry = CONTAINING_RECORD(link, OVS_NAT_ENTRY, link);
 
             if (OvsNatKeyAreSame(ctKey, &entry->key)) {
+                NdisReleaseSpinLock(&ovsCtNatLock);
                 return entry;
             }
         }
     }
+    NdisReleaseSpinLock(&ovsCtNatLock);
     return NULL;
 }
 
@@ -432,6 +456,7 @@  OvsNatDeleteEntry(POVS_NAT_ENTRY entry)
     RemoveEntryList(&entry->link);
     RemoveEntryList(&entry->reverseLink);
     OvsFreeMemoryWithTag(entry, OVS_CT_POOL_TAG);
+    NdisInterlockedDecrement((PLONG)&ovsNatEntries);
 }
 
 /*