Please check the comments inline.
Thanks,
Sairam
On 8/28/17, 8:55 PM, "Shashank Ram" <r...@vmware.com> wrote:
>
>
>
>________________________________________
>From: ovs-dev-boun...@openvswitch.org <ovs-dev-boun...@openvswitch.org> on
>behalf of Sairam Venugopal <vsai...@vmware.com>
>Sent: Monday, August 28, 2017 4:56 PM
>To: d...@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 <vsai...@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
>
>_______________________________________________
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
