_______________________________________
>From: [email protected] <[email protected]> on
>behalf of Sairam Venugopal <[email protected]>
>Sent: Monday, August 28, 2017 4:56 PM
>To: [email protected]
>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 <[email protected]>
>---
> 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
>--
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
