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

[Shashank Ram]

_______________________________________
>From: ovs-dev-bounces at openvswitch.org <ovs-dev-bounces at openvswitch.org> on behalf of Sairam Venugopal <vsairam at vmware.com>
>Sent: Monday, August 28, 2017 4:56 PM
>To: dev at 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 at 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.

>>>> The caller does not log that this is explicitly due to entry limit being reached. So how would you know what caused the error? Its better to add a log here if its not easy to figure out the reason this function failed.

>
>+        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.
>>>> It seems rather odd to me to define it this way for consistency when it can be directly defined. Any reason other usages to be defined this way?

>
> #define CT_HASH_TABLE_SIZE ((UINT32)1 << 10)
> #define CT_HASH_TABLE_MASK (CT_HASH_TABLE_SIZE - 1)
> #define CT_INTERVAL_SEC 10000000LL //1s
>--