diff mbox series

[ovs-dev] datapath-windows: Add an upper limit to conntrack entries

Message ID 20170828235613.231276-1-vsairam@vmware.com
State Superseded
Headers show
Series [ovs-dev] datapath-windows: Add an upper limit to conntrack entries | expand

Commit Message

Sairam Venugopal Aug. 28, 2017, 11:56 p.m. UTC 
The current implementation lacked an upper bound of number of entries in
the system. Set the size to ~2M (2^21) for the time being.

Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
---
 datapath-windows/ovsext/Conntrack.c | 6 ++++++
 datapath-windows/ovsext/Conntrack.h | 1 +
 2 files changed, 7 insertions(+)

Comments

Anand Kumar Aug. 29, 2017, 6:33 p.m. UTC | #1 
Hi Sairam,

Thanks for the patch. Please find my comment inline.

Regards,
Anand Kumar

On 8/28/17, 4:56 PM, "ovs-dev-bounces@openvswitch.org on behalf of Sairam Venugopal" <ovs-dev-bounces@openvswitch.org on behalf of vsairam@vmware.com> wrote:

    The current implementation lacked an upper bound of number of entries in
    the system. Set the size to ~2M (2^21) for the time being.
    
    Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
    ---
     datapath-windows/ovsext/Conntrack.c | 6 ++++++
     datapath-windows/ovsext/Conntrack.h | 1 +
     2 files changed, 7 insertions(+)
    
    diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
    index ce8c1c8..30de806 100644
    --- a/datapath-windows/ovsext/Conntrack.c
    +++ b/datapath-windows/ovsext/Conntrack.c
    @@ -722,6 +722,12 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx,
             entry = NULL;
         }
     
    +    if (!entry && commit && ctTotalEntries >= CT_MAX_ENTRIES) {
    +        /* Don't proceed with processing if the max limit has been hit */
    +        NdisReleaseRWLock(ovsConntrackLockObj, &lockState);
    +        return NDIS_STATUS_RESOURCES;
    +    }
    +
  [AK]: Can we add this check inside OvsCtEntryCreate() function, as entry can also be created in OvsProcessConntrackEntry()
  https://github.com/openvswitch/ovs/blob/master/datapath-windows/ovsext/Conntrack.c#L613 

         if (!entry) {
             /* If no matching entry was found, create one and add New state */
             entry = OvsCtEntryCreate(fwdCtx, key->ipKey.nwProto,
    diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-windows/ovsext/Conntrack.h
    index bca7d90..be5f34d 100644
    --- a/datapath-windows/ovsext/Conntrack.h
    +++ b/datapath-windows/ovsext/Conntrack.h
    @@ -131,6 +131,7 @@ typedef struct OvsConntrackKeyLookupCtx {
         BOOLEAN         related;
     } OvsConntrackKeyLookupCtx;
     
    +#define CT_MAX_ENTRIES 1 << 21
     #define CT_HASH_TABLE_SIZE ((UINT32)1 << 10)
     #define CT_HASH_TABLE_MASK (CT_HASH_TABLE_SIZE - 1)
     #define CT_INTERVAL_SEC 10000000LL //1s
    -- 
    2.9.0.windows.1
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Q5z9tBe-nAOpE7LIHSPV8uy5-437agMXvkeHHMkR8Us&m=EAviB6UFEys3jRhv7hULyGkP2OygltyfdtEaNlvOWDY&s=co2lpZtEG_VyCk3aIxQFVq4L0rnKR5iKtU2rsu8jFxc&e=
Sairam Venugopal Aug. 29, 2017, 7:12 p.m. UTC | #2 
Hi Anand,

OvsProcessConntrackEntry ensures that the old entry is deleted before creating a new one.
So it would prevent newer entries from being created or the limit being hit. 

I added in the function before processing to exit out quickly only if there was no matching entry and commit was specified.

Hope this answers your question.

Thanks,
Sairam 



On 8/29/17, 11:33 AM, "Anand Kumar" <kumaranand@vmware.com> wrote:

>Hi Sairam,
>
>Thanks for the patch. Please find my comment inline.
>
>Regards,
>Anand Kumar
>
>On 8/28/17, 4:56 PM, "ovs-dev-bounces@openvswitch.org on behalf of Sairam Venugopal" <ovs-dev-bounces@openvswitch.org on behalf of vsairam@vmware.com> wrote:
>
>    The current implementation lacked an upper bound of number of entries in
>    the system. Set the size to ~2M (2^21) for the time being.
>    
>    Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
>    ---
>     datapath-windows/ovsext/Conntrack.c | 6 ++++++
>     datapath-windows/ovsext/Conntrack.h | 1 +
>     2 files changed, 7 insertions(+)
>    
>    diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
>    index ce8c1c8..30de806 100644
>    --- a/datapath-windows/ovsext/Conntrack.c
>    +++ b/datapath-windows/ovsext/Conntrack.c
>    @@ -722,6 +722,12 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx,
>             entry = NULL;
>         }
>     
>    +    if (!entry && commit && ctTotalEntries >= CT_MAX_ENTRIES) {
>    +        /* Don't proceed with processing if the max limit has been hit */
>    +        NdisReleaseRWLock(ovsConntrackLockObj, &lockState);
>    +        return NDIS_STATUS_RESOURCES;
>    +    }
>    +
>  [AK]: Can we add this check inside OvsCtEntryCreate() function, as entry can also be created in OvsProcessConntrackEntry()
>  https://github.com/openvswitch/ovs/blob/master/datapath-windows/ovsext/Conntrack.c#L613 
>
>         if (!entry) {
>             /* If no matching entry was found, create one and add New state */
>             entry = OvsCtEntryCreate(fwdCtx, key->ipKey.nwProto,
>    diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-windows/ovsext/Conntrack.h
>    index bca7d90..be5f34d 100644
>    --- a/datapath-windows/ovsext/Conntrack.h
>    +++ b/datapath-windows/ovsext/Conntrack.h
>    @@ -131,6 +131,7 @@ typedef struct OvsConntrackKeyLookupCtx {
>         BOOLEAN         related;
>     } OvsConntrackKeyLookupCtx;
>     
>    +#define CT_MAX_ENTRIES 1 << 21
>     #define CT_HASH_TABLE_SIZE ((UINT32)1 << 10)
>     #define CT_HASH_TABLE_MASK (CT_HASH_TABLE_SIZE - 1)
>     #define CT_INTERVAL_SEC 10000000LL //1s
>    -- 
>    2.9.0.windows.1
>    
>    _______________________________________________
>    dev mailing list
>    dev@openvswitch.org
>    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=Q5z9tBe-nAOpE7LIHSPV8uy5-437agMXvkeHHMkR8Us&m=EAviB6UFEys3jRhv7hULyGkP2OygltyfdtEaNlvOWDY&s=co2lpZtEG_VyCk3aIxQFVq4L0rnKR5iKtU2rsu8jFxc&e= 
>    
>
Sairam Venugopal Aug. 29, 2017, 7:28 p.m. UTC | #3 
Please check the comments inline.

Thanks,
Sairam




On 8/28/17, 8:55 PM, "Shashank Ram" <rams@vmware.com> wrote:

>

>

>

>________________________________________

>From: ovs-dev-bounces@openvswitch.org <ovs-dev-bounces@openvswitch.org> on behalf of Sairam Venugopal <vsairam@vmware.com>

>Sent: Monday, August 28, 2017 4:56 PM

>To: dev@openvswitch.org

>Subject: [ovs-dev] [PATCH] datapath-windows: Add an upper limit to conntrack    entries

>

>The current implementation lacked an upper bound of number of entries in

>the system. Set the size to ~2M (2^21) for the time being.

>

>>> Any reason for choosing this arbitrarily?


Not arbitrary - I chose something that we currently use in default OVS Linux implementation.

>

>Signed-off-by: Sairam Venugopal <vsairam@vmware.com>

>---

> datapath-windows/ovsext/Conntrack.c | 6 ++++++

> datapath-windows/ovsext/Conntrack.h | 1 +

> 2 files changed, 7 insertions(+)

>

>diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c

>index ce8c1c8..30de806 100644

>--- a/datapath-windows/ovsext/Conntrack.c

>+++ b/datapath-windows/ovsext/Conntrack.c

>@@ -722,6 +722,12 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx,

>         entry = NULL;

>     }

>

>+    if (!entry && commit && ctTotalEntries >= CT_MAX_ENTRIES) {

>+        /* Don't proceed with processing if the max limit has been hit */

>+        NdisReleaseRWLock(ovsConntrackLockObj, &lockState);

>>> Add a error log here to facilitate easier debugging


It’s already caught and logged as part of the caller. 

>

>+        return NDIS_STATUS_RESOURCES;

>+    }

>+

>>> It makes more sense to put the check in OvsCtEntryCreate() since there are multiple callers to that function.


I just answered this in Anand’s review comments. But the idea is to skip complete processing if the limit is hit.
OvsCtEntryCreate is called only by OvsProcessContrack apart from the current function. ProcessContrack makes it a point 

To delete and then create the entry.

>

>     if (!entry) {

>         /* If no matching entry was found, create one and add New state */

>         entry = OvsCtEntryCreate(fwdCtx, key->ipKey.nwProto,

>diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-windows/ovsext/Conntrack.h

>index bca7d90..be5f34d 100644

>--- a/datapath-windows/ovsext/Conntrack.h

>+++ b/datapath-windows/ovsext/Conntrack.h

>@@ -131,6 +131,7 @@ typedef struct OvsConntrackKeyLookupCtx {

>     BOOLEAN         related;

> } OvsConntrackKeyLookupCtx;

>

>+#define CT_MAX_ENTRIES 1 << 21

>>> Any reason this value is not defined directly?


Yes, to maintain consistency with the other usages.

>

> #define CT_HASH_TABLE_SIZE ((UINT32)1 << 10)

> #define CT_HASH_TABLE_MASK (CT_HASH_TABLE_SIZE - 1)

> #define CT_INTERVAL_SEC 10000000LL //1s

>--

>2.9.0.windows.1

>

>_______________________________________________

>
Shashank Ram Aug. 30, 2017, 5:26 p.m. UTC | #4 
From: ovs-dev-bounces@openvswitch.org <ovs-dev-bounces@openvswitch.org> on behalf of Sairam Venugopal <vsairam@vmware.com>
Sent: Monday, August 28, 2017 4:56 PM
To: dev@openvswitch.org
Subject: [ovs-dev] [PATCH] datapath-windows: Add an upper limit to conntrack    entries

The current implementation lacked an upper bound of number of entries in
the system. Set the size to ~2M (2^21) for the time being.

Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
diff mbox series

Patch

diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c
index ce8c1c8..30de806 100644
--- a/datapath-windows/ovsext/Conntrack.c
+++ b/datapath-windows/ovsext/Conntrack.c
@@ -722,6 +722,12 @@  OvsCtExecute_(OvsForwardingContext *fwdCtx,
         entry = NULL;
     }
 
+    if (!entry && commit && ctTotalEntries >= CT_MAX_ENTRIES) {
+        /* Don't proceed with processing if the max limit has been hit */
+        NdisReleaseRWLock(ovsConntrackLockObj, &lockState);
+        return NDIS_STATUS_RESOURCES;
+    }
+
     if (!entry) {
         /* If no matching entry was found, create one and add New state */
         entry = OvsCtEntryCreate(fwdCtx, key->ipKey.nwProto,
diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-windows/ovsext/Conntrack.h
index bca7d90..be5f34d 100644
--- a/datapath-windows/ovsext/Conntrack.h
+++ b/datapath-windows/ovsext/Conntrack.h
@@ -131,6 +131,7 @@  typedef struct OvsConntrackKeyLookupCtx {
     BOOLEAN         related;
 } OvsConntrackKeyLookupCtx;
 
+#define CT_MAX_ENTRIES 1 << 21
 #define CT_HASH_TABLE_SIZE ((UINT32)1 << 10)
 #define CT_HASH_TABLE_MASK (CT_HASH_TABLE_SIZE - 1)
 #define CT_INTERVAL_SEC 10000000LL //1s