| Message ID | 20180607185237.3408-1-kumaranand@vmware.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,v2] datapath-windows: Optimize conntrack performance | expand |
On Thu, Jun 7, 2018, 11:52 AM Anand Kumar <kumaranand@vmware.com> wrote: > - Move conntrack lock out of NAT module > - Use spinlock instead of read/write lock for conntrack entry. > - Update 'ctTotalRelatedEntries' using interlocked functions > - Refactor conntrack code to make it more readable. > > Testing: > Evaluated TCP performance using iperf3. > > Before optimization: > Native: 6.0Gbps > OVS: 5.1-5.75Gbps > OVS with conntrack enabled: 3.9-4.0Gbps > > After optimization: > Native: 6.0Gbps > OVS: 5.1-5.75Gbps > OVS with conntrack enabled:: 4.2-4.4Gbps > > Tested by loading/unloading driver with driver verifier enabled > > Signed-off-by: Anand Kumar <kumaranand@vmware.com> > --- > v1->v2: Update commit message > --- > datapath-windows/ovsext/Conntrack-nat.c | 7 +- > datapath-windows/ovsext/Conntrack-related.c | 17 ++-- > datapath-windows/ovsext/Conntrack.c | 115 > ++++++++++++---------------- > datapath-windows/ovsext/Conntrack.h | 2 +- > 4 files changed, 59 insertions(+), 82 deletions(-) > > diff --git a/datapath-windows/ovsext/Conntrack-nat.c > b/datapath-windows/ovsext/Conntrack-nat.c > index 316c946..da1814f 100644 > --- a/datapath-windows/ovsext/Conntrack-nat.c > +++ b/datapath-windows/ovsext/Conntrack-nat.c > @@ -167,16 +167,13 @@ OvsNatPacket(OvsForwardingContext *ovsFwdCtx, > { > UINT32 natFlag; > const struct ct_endpoint* endpoint; > - LOCK_STATE_EX lockState; > - /* XXX: Move conntrack locks out of NAT after implementing lock in > NAT. */ > - NdisAcquireRWLockRead(entry->lock, &lockState, 0); > + > /* When it is NAT, only entry->rev_key contains NATTED address; > When it is unNAT, only entry->key contains the UNNATTED address;*/ > const OVS_CT_KEY *ctKey = reverse ? &entry->key : &entry->rev_key; > BOOLEAN isSrcNat; > > if (!(natAction & (NAT_ACTION_SRC | NAT_ACTION_DST))) { > - NdisReleaseRWLock(entry->lock, &lockState); > return; > } > isSrcNat = (((natAction & NAT_ACTION_SRC) && !reverse) || > @@ -206,7 +203,6 @@ OvsNatPacket(OvsForwardingContext *ovsFwdCtx, > } > } else if (ctKey->dl_type == htons(ETH_TYPE_IPV6)){ > // XXX: IPv6 packet not supported yet. > - NdisReleaseRWLock(entry->lock, &lockState); > return; > } > if (natAction & (NAT_ACTION_SRC_PORT | NAT_ACTION_DST_PORT)) { > @@ -220,7 +216,6 @@ OvsNatPacket(OvsForwardingContext *ovsFwdCtx, > } > } > } > - NdisReleaseRWLock(entry->lock, &lockState); > } > > > diff --git a/datapath-windows/ovsext/Conntrack-related.c > b/datapath-windows/ovsext/Conntrack-related.c > index ec4b536..00eac67 100644 > --- a/datapath-windows/ovsext/Conntrack-related.c > +++ b/datapath-windows/ovsext/Conntrack-related.c > @@ -18,7 +18,7 @@ > #include "Jhash.h" > > static PLIST_ENTRY ovsCtRelatedTable; /* Holds related entries */ > -static UINT64 ctTotalRelatedEntries; > +static LONG ctTotalRelatedEntries; > Please change to ULONG and cast it using PLONG where necessary. static OVS_CT_THREAD_CTX ctRelThreadCtx; > static PNDIS_RW_LOCK_EX ovsCtRelatedLockObj; > extern POVS_SWITCH_CONTEXT gOvsSwitchContext; > @@ -75,13 +75,11 @@ OvsCtRelatedLookup(OVS_CT_KEY key, UINT64 currentTime) > POVS_CT_REL_ENTRY entry; > LOCK_STATE_EX lockState; > > - NdisAcquireRWLockRead(ovsCtRelatedLockObj, &lockState, 0); > - > if (!ctTotalRelatedEntries) { > - NdisReleaseRWLock(ovsCtRelatedLockObj, &lockState); > return NULL; > } > > + NdisAcquireRWLockRead(ovsCtRelatedLockObj, &lockState, 0); > for (int i = 0; i < CT_HASH_TABLE_SIZE; i++) { > /* XXX - Scan the table based on the hash instead */ > LIST_FORALL_SAFE(&ovsCtRelatedTable[i], link, next) { > @@ -103,7 +101,7 @@ OvsCtRelatedEntryDelete(POVS_CT_REL_ENTRY entry) > { > RemoveEntryList(&entry->link); > OvsFreeMemoryWithTag(entry, OVS_CT_POOL_TAG); > - ctTotalRelatedEntries--; > + InterlockedDecrement((LONG volatile*)&ctTotalRelatedEntries); > Mind wrapping this using NdisInterlockedxxx function? } > > NDIS_STATUS > @@ -139,7 +137,7 @@ OvsCtRelatedEntryCreate(UINT8 ipProto, > NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0); > InsertHeadList(&ovsCtRelatedTable[hash & CT_HASH_TABLE_MASK], > &entry->link); > - ctTotalRelatedEntries++; > + InterlockedIncrement((LONG volatile *)&ctTotalRelatedEntries); > NdisReleaseRWLock(ovsCtRelatedLockObj, &lockState); > > return NDIS_STATUS_SUCCESS; > @@ -150,11 +148,10 @@ OvsCtRelatedFlush() > { > PLIST_ENTRY link, next; > POVS_CT_REL_ENTRY entry; > - > LOCK_STATE_EX lockState; > - NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0); > > if (ctTotalRelatedEntries) { > + NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0); > for (int i = 0; i < CT_HASH_TABLE_SIZE; i++) { > LIST_FORALL_SAFE(&ovsCtRelatedTable[i], link, next) { > entry = CONTAINING_RECORD(link, OVS_CT_REL_ENTRY, link); > @@ -189,9 +186,8 @@ OvsCtRelatedEntryCleaner(PVOID data) > /* Lock has been freed by 'OvsCleanupCtRelated()' */ > break; > } > - NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0); > + > if (context->exit) { > - NdisReleaseRWLock(ovsCtRelatedLockObj, &lockState); > break; > } > > @@ -201,6 +197,7 @@ OvsCtRelatedEntryCleaner(PVOID data) > threadSleepTimeout = currentTime + CT_CLEANUP_INTERVAL; > > if (ctTotalRelatedEntries) { > + NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0); > for (int i = 0; i < CT_HASH_TABLE_SIZE; i++) { > LIST_FORALL_SAFE(&ovsCtRelatedTable[i], link, next) { > entry = CONTAINING_RECORD(link, OVS_CT_REL_ENTRY, > link); > diff --git a/datapath-windows/ovsext/Conntrack.c > b/datapath-windows/ovsext/Conntrack.c > index add1491..c5aa7c5 100644 > --- a/datapath-windows/ovsext/Conntrack.c > +++ b/datapath-windows/ovsext/Conntrack.c > @@ -208,7 +208,6 @@ OvsPostCtEventEntry(POVS_CT_ENTRY entry, UINT8 type) > { > OVS_CT_EVENT_ENTRY ctEventEntry = {0}; > NdisMoveMemory(&ctEventEntry.entry, entry, sizeof(OVS_CT_ENTRY)); > - ctEventEntry.entry.lock = NULL; > ctEventEntry.entry.parent = NULL; > ctEventEntry.type = type; > OvsPostCtEvent(&ctEventEntry); > @@ -217,8 +216,7 @@ OvsPostCtEventEntry(POVS_CT_ENTRY entry, UINT8 type) > static __inline VOID > OvsCtIncrementCounters(POVS_CT_ENTRY entry, BOOLEAN reply, > PNET_BUFFER_LIST nbl) > { > - LOCK_STATE_EX lockState; > - NdisAcquireRWLockWrite(entry->lock, &lockState, 0); > + NdisAcquireSpinLock(&(entry->lock)); > if (reply) { > entry->rev_key.byteCount+= OvsPacketLenNBL(nbl); > entry->rev_key.packetCount++; > @@ -226,11 +224,11 @@ OvsCtIncrementCounters(POVS_CT_ENTRY entry, BOOLEAN > reply, PNET_BUFFER_LIST nbl) > entry->key.byteCount += OvsPacketLenNBL(nbl); > entry->key.packetCount++; > } > - NdisReleaseRWLock(entry->lock, &lockState); > + NdisReleaseSpinLock(&(entry->lock)); > } > > static __inline BOOLEAN > -OvsCtAddEntry(POVS_SWITCH_CONTEXT switchContext, POVS_CT_ENTRY entry, > +OvsCtAddEntry(POVS_CT_ENTRY entry, > OvsConntrackKeyLookupCtx *ctx, > PNAT_ACTION_INFO natInfo, UINT64 now) > { > @@ -261,11 +259,7 @@ OvsCtAddEntry(POVS_SWITCH_CONTEXT switchContext, > POVS_CT_ENTRY entry, > } > > entry->timestampStart = now; > - entry->lock = NdisAllocateRWLock(switchContext->NdisFilterHandle); > - if (entry->lock == NULL) { > - OVS_LOG_ERROR("NdisAllocateRWLock failed : Insufficient > resources"); > - return FALSE; > - } > + NdisAllocateSpinLock(&(entry->lock)); > UINT32 bucketIdx = ctx->hash & CT_HASH_TABLE_MASK; > entry->bucketLockRef = ovsCtBucketLock[bucketIdx]; > NdisAcquireRWLockWrite(ovsCtBucketLock[bucketIdx], &lockState, 0); > @@ -351,8 +345,7 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx, > if (state != OVS_CS_F_INVALID && commit) { > if (entry) { > entry->parent = parentEntry; > - if (OvsCtAddEntry(fwdCtx->switchContext, entry, ctx, > - natInfo, currentTime)) { > + if (OvsCtAddEntry(entry, ctx, natInfo, currentTime)) { > *entryCreated = TRUE; > } else { > /* Unable to add entry to the list */ > @@ -382,8 +375,7 @@ OvsCtUpdateEntry(OVS_CT_ENTRY* entry, > UINT64 now) > { > CT_UPDATE_RES status; > - LOCK_STATE_EX lockState; > - NdisAcquireRWLockWrite(entry->lock, &lockState, 0); > + NdisAcquireSpinLock(&(entry->lock)); > switch (ipProto) { > case IPPROTO_TCP: > { > @@ -407,7 +399,7 @@ OvsCtUpdateEntry(OVS_CT_ENTRY* entry, > status = CT_UPDATE_INVALID; > break; > } > - NdisReleaseRWLock(entry->lock, &lockState); > + NdisReleaseSpinLock(&(entry->lock)); > return status; > } > > @@ -431,8 +423,7 @@ OvsCtEntryDelete(POVS_CT_ENTRY entry, BOOLEAN > forceDelete) > if (entry == NULL) { > return; > } > - LOCK_STATE_EX lockState; > - NdisAcquireRWLockWrite(entry->lock, &lockState, 0); > + NdisAcquireSpinLock(&(entry->lock)); > if (forceDelete || OvsCtEntryExpired(entry)) { > if (entry->natInfo.natAction) { > LOCK_STATE_EX lockStateNat; > @@ -442,13 +433,13 @@ OvsCtEntryDelete(POVS_CT_ENTRY entry, BOOLEAN > forceDelete) > } > OvsPostCtEventEntry(entry, OVS_EVENT_CT_DELETE); > RemoveEntryList(&entry->link); > - NdisReleaseRWLock(entry->lock, &lockState); > - NdisFreeRWLock(entry->lock); > + NdisReleaseSpinLock(&(entry->lock)); > + NdisFreeSpinLock(&(entry->lock)); > OvsFreeMemoryWithTag(entry, OVS_CT_POOL_TAG); > InterlockedDecrement((LONG volatile*)&ctTotalEntries); > return; > } > - NdisReleaseRWLock(entry->lock, &lockState); > + NdisReleaseSpinLock(&(entry->lock)); > } > > static __inline NDIS_STATUS > @@ -499,7 +490,7 @@ OvsCtLookup(OvsConntrackKeyLookupCtx *ctx) > POVS_CT_ENTRY entry; > BOOLEAN reply = FALSE; > POVS_CT_ENTRY found = NULL; > - LOCK_STATE_EX lockState, lockStateTable; > + LOCK_STATE_EX lockStateTable; > UINT32 bucketIdx; > /* Reverse NAT must be performed before OvsCtLookup, so here > * we simply need to flip the src and dst in key and compare > @@ -516,7 +507,7 @@ OvsCtLookup(OvsConntrackKeyLookupCtx *ctx) > NdisAcquireRWLockRead(ovsCtBucketLock[bucketIdx], &lockStateTable, 0); > LIST_FORALL(&ovsConntrackTable[bucketIdx], link) { > entry = CONTAINING_RECORD(link, OVS_CT_ENTRY, link); > - NdisAcquireRWLockRead(entry->lock, &lockState, 0); > + NdisAcquireSpinLock(&(entry->lock)); > if (OvsCtKeyAreSame(ctx->key, entry->key)) { > found = entry; > reply = FALSE; > @@ -533,10 +524,10 @@ OvsCtLookup(OvsConntrackKeyLookupCtx *ctx) > } else { > ctx->reply = reply; > } > - NdisReleaseRWLock(entry->lock, &lockState); > + NdisReleaseSpinLock(&(entry->lock)); > break; > } > - NdisReleaseRWLock(entry->lock, &lockState); > + NdisReleaseSpinLock(&(entry->lock)); > } > > NdisReleaseRWLock(ovsCtBucketLock[bucketIdx], &lockStateTable); > @@ -689,7 +680,7 @@ OvsProcessConntrackEntry(OvsForwardingContext *fwdCtx, > POVS_CT_ENTRY entry = ctx->entry; > UINT32 state = 0; > PNET_BUFFER_LIST curNbl = fwdCtx->curNbl; > - LOCK_STATE_EX lockState, lockStateTable; > + LOCK_STATE_EX lockStateTable; > PNDIS_RW_LOCK_EX bucketLockRef = NULL; > *entryCreated = FALSE; > > @@ -730,7 +721,7 @@ OvsProcessConntrackEntry(OvsForwardingContext *fwdCtx, > } > } > if (entry) { > - NdisAcquireRWLockRead(entry->lock, &lockState, 0); > + NdisAcquireSpinLock(&(entry->lock)); > if (key->ipKey.nwProto == IPPROTO_TCP) { > /* Update the related bit if there is a parent */ > if (entry->parent) { > @@ -748,7 +739,7 @@ OvsProcessConntrackEntry(OvsForwardingContext *fwdCtx, > /* Copy mark and label from entry into flowKey. If actions specify > different mark and label, update the flowKey. */ > OvsCtUpdateFlowKey(key, state, zone, entry->mark, &entry->labels); > - NdisReleaseRWLock(entry->lock, &lockState); > + NdisReleaseSpinLock(&(entry->lock)); > } else { > OvsCtUpdateFlowKey(key, state, zone, 0, NULL); > } > @@ -802,8 +793,6 @@ OvsCtSetMarkLabel(OvsFlowKey *key, > MD_LABELS *labels, > BOOLEAN *triggerUpdateEvent) > { > - LOCK_STATE_EX lockState; > - NdisAcquireRWLockWrite(entry->lock, &lockState, 0); > if (mark) { > OvsConntrackSetMark(key, entry, mark->value, mark->mask, > triggerUpdateEvent); > @@ -813,7 +802,6 @@ OvsCtSetMarkLabel(OvsFlowKey *key, > OvsConntrackSetLabels(key, entry, &labels->value, &labels->mask, > triggerUpdateEvent); > } > - NdisReleaseRWLock(entry->lock, &lockState); > } > > /* > @@ -854,10 +842,11 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, > { > NDIS_STATUS status = NDIS_STATUS_SUCCESS; > BOOLEAN triggerUpdateEvent = FALSE; > + BOOLEAN entryCreated = FALSE; > POVS_CT_ENTRY entry = NULL; > PNET_BUFFER_LIST curNbl = fwdCtx->curNbl; > OvsConntrackKeyLookupCtx ctx = { 0 }; > - LOCK_STATE_EX lockState, lockStateTable; > + LOCK_STATE_EX lockStateTable; > UINT64 currentTime; > NdisGetCurrentSystemTime((LARGE_INTEGER *) ¤tTime); > > @@ -866,10 +855,9 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, > > /* Lookup Conntrack entries for a matching entry */ > entry = OvsCtLookup(&ctx); > - BOOLEAN entryCreated = FALSE; > > /* Delete entry in reverse direction if 'force' is specified */ > - if (entry && force && ctx.reply) { > + if (force && ctx.reply && entry) { > PNDIS_RW_LOCK_EX bucketLockRef = entry->bucketLockRef; > NdisAcquireRWLockWrite(bucketLockRef, &lockStateTable, 0); > OvsCtEntryDelete(entry, TRUE); > @@ -877,34 +865,33 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, > entry = NULL; > } > > - if (!entry && commit && ctTotalEntries >= CT_MAX_ENTRIES) { > - /* Don't proceed with processing if the max limit has been hit. > - * This blocks only new entries from being created and doesn't > - * affect existing connections. > + if (entry) { > + /* Increment stats for the entry if it wasn't tracked previously > or > + * if they are on different zones > */ > - OVS_LOG_ERROR("Conntrack Limit hit: %lu", ctTotalEntries); > - return NDIS_STATUS_RESOURCES; > - } > - > - /* Increment stats for the entry if it wasn't tracked previously or > - * if they are on different zones > - */ > - if (entry && (entry->key.zone != key->ct.zone || > - (!(key->ct.state & OVS_CS_F_TRACKED)))) { > - OvsCtIncrementCounters(entry, ctx.reply, curNbl); > - } > + if ((entry->key.zone != key->ct.zone || > + (!(key->ct.state & OVS_CS_F_TRACKED)))) { > + OvsCtIncrementCounters(entry, ctx.reply, curNbl); > + } > + /* Process the entry and update CT flags */ > + entry = OvsProcessConntrackEntry(fwdCtx, layers->l4Offset, &ctx, > key, > + zone, natInfo, commit, > currentTime, > + &entryCreated); > > - if (!entry) { > + } else { > + if (commit && ctTotalEntries >= CT_MAX_ENTRIES) { > + /* Don't proceed with processing if the max limit has been > hit. > + * This blocks only new entries from being created and doesn't > + * affect existing connections. > + */ > + OVS_LOG_ERROR("Conntrack Limit hit: %lu", ctTotalEntries); > + return NDIS_STATUS_RESOURCES; > + } > /* If no matching entry was found, create one and add New state */ > entry = OvsCtEntryCreate(fwdCtx, key->ipKey.nwProto, > layers->l4Offset, &ctx, > key, natInfo, commit, currentTime, > &entryCreated); > - } else { > - /* Process the entry and update CT flags */ > - entry = OvsProcessConntrackEntry(fwdCtx, layers->l4Offset, &ctx, > key, > - zone, natInfo, commit, > currentTime, > - &entryCreated); > } > > if (entry == NULL) { > @@ -918,6 +905,7 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, > * NAT_ACTION_REVERSE, yet entry->natInfo is NAT_ACTION_SRC or > * NAT_ACTION_DST without NAT_ACTION_REVERSE > */ > + NdisAcquireSpinLock(&(entry->lock)); > if (natInfo->natAction != NAT_ACTION_NONE) > { > LOCK_STATE_EX lockStateNat; > @@ -939,15 +927,14 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, > OVS_LOG_ERROR("Error while parsing the FTP packet"); > } > } > - NdisAcquireRWLockRead(entry->lock, &lockState, 0); > + > /* Add original tuple information to flow Key */ > if (entry->key.dl_type == ntohs(ETH_TYPE_IPV4)) { > if (entry->parent != NULL) { > POVS_CT_ENTRY parent = entry->parent; > - LOCK_STATE_EX lockStateParent; > - NdisAcquireRWLockRead(parent->lock, &lockStateParent, 0); > + NdisAcquireSpinLock(&(parent->lock)); > OvsCtUpdateTuple(key, &parent->key); > - NdisReleaseRWLock(parent->lock, &lockStateParent); > + NdisReleaseSpinLock(&(parent->lock)); > } else { > OvsCtUpdateTuple(key, &entry->key); > } > @@ -955,13 +942,11 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, > > if (entryCreated) { > OvsPostCtEventEntry(entry, OVS_EVENT_CT_NEW); > - } > - if (postUpdateEvent && !entryCreated && triggerUpdateEvent) { > + } else if (postUpdateEvent && triggerUpdateEvent) { > OvsPostCtEventEntry(entry, OVS_EVENT_CT_UPDATE); > } > > - NdisReleaseRWLock(entry->lock, &lockState); > - > + NdisReleaseSpinLock(&(entry->lock)); > return status; > } > > @@ -1739,7 +1724,7 @@ OvsCtDumpCmdHandler(POVS_USER_PARAMS_CONTEXT > usrParamsCtx, > UINT32 i = CT_HASH_TABLE_SIZE; > UINT32 outIndex = 0; > > - LOCK_STATE_EX lockState, lockStateTable; > + LOCK_STATE_EX lockStateTable; > if (ctTotalEntries) { > for (i = inBucket; i < CT_HASH_TABLE_SIZE; i++) { > PLIST_ENTRY head, link; > @@ -1756,7 +1741,7 @@ OvsCtDumpCmdHandler(POVS_USER_PARAMS_CONTEXT > usrParamsCtx, > */ > if (outIndex >= inIndex) { > entry = CONTAINING_RECORD(link, OVS_CT_ENTRY, link); > - NdisAcquireRWLockRead(entry->lock, &lockState, 0); > + NdisAcquireSpinLock(&(entry->lock)); > rc = OvsCreateNlMsgFromCtEntry(entry, > > usrParamsCtx->outputBuffer, > > usrParamsCtx->outputLength, > @@ -1765,7 +1750,7 @@ OvsCtDumpCmdHandler(POVS_USER_PARAMS_CONTEXT > usrParamsCtx, > msgIn->nlMsg.nlmsgPid, > > msgIn->nfGenMsg.version, > 0); > - NdisReleaseRWLock(entry->lock, &lockState); > + NdisReleaseSpinLock(&(entry->lock)); > if (rc != NDIS_STATUS_SUCCESS) { > NdisReleaseRWLock(ovsCtBucketLock[i], > &lockStateTable); > return STATUS_UNSUCCESSFUL; > diff --git a/datapath-windows/ovsext/Conntrack.h > b/datapath-windows/ovsext/Conntrack.h > index 3be309e..7dc92a1 100644 > --- a/datapath-windows/ovsext/Conntrack.h > +++ b/datapath-windows/ovsext/Conntrack.h > @@ -101,7 +101,7 @@ typedef struct _NAT_ACTION_INFO { > typedef struct OVS_CT_ENTRY { > /* Reference to ovsCtBucketLock of ovsConntrackTable.*/ > PNDIS_RW_LOCK_EX bucketLockRef; > - PNDIS_RW_LOCK_EX lock; /* Protects OVS_CT_ENTRY. */ > + NDIS_SPIN_LOCK lock; /* Protects OVS_CT_ENTRY. */ > OVS_CT_KEY key; > OVS_CT_KEY rev_key; > UINT64 expiration; > -- > 2.9.3.windows.1 > > _______________________________________________ > Could you also consider calculating the dispatch level and appropriately use the corresponding Spin lock routines in functions where the locks are acquired and released repeatedly? This will reduce the burden on NDIS for determining the dispatch at every call and improve the performance. >
Hi Shashank, Thanks for the review. I will address your comments and send out a V3. Regards, Anand Kumar From: Shashank Ram <shashank08@gmail.com> Date: Friday, June 8, 2018 at 10:11 AM To: Anand Kumar <kumaranand@vmware.com> Cc: "dev@openvswitch.org" <dev@openvswitch.org> Subject: Re: [ovs-dev] [PATCH v2] datapath-windows: Optimize conntrack performance On Thu, Jun 7, 2018, 11:52 AM Anand Kumar <kumaranand@vmware.com<mailto:kumaranand@vmware.com>> wrote: - Move conntrack lock out of NAT module - Use spinlock instead of read/write lock for conntrack entry. - Update 'ctTotalRelatedEntries' using interlocked functions - Refactor conntrack code to make it more readable. Testing: Evaluated TCP performance using iperf3. Before optimization: Native: 6.0Gbps OVS: 5.1-5.75Gbps OVS with conntrack enabled: 3.9-4.0Gbps After optimization: Native: 6.0Gbps OVS: 5.1-5.75Gbps OVS with conntrack enabled:: 4.2-4.4Gbps Tested by loading/unloading driver with driver verifier enabled Signed-off-by: Anand Kumar <kumaranand@vmware.com<mailto:kumaranand@vmware.com>> --- v1->v2: Update commit message --- datapath-windows/ovsext/Conntrack-nat.c | 7 +- datapath-windows/ovsext/Conntrack-related.c | 17 ++-- datapath-windows/ovsext/Conntrack.c | 115 ++++++++++++---------------- datapath-windows/ovsext/Conntrack.h | 2 +- 4 files changed, 59 insertions(+), 82 deletions(-) diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-windows/ovsext/Conntrack-nat.c index 316c946..da1814f 100644 --- a/datapath-windows/ovsext/Conntrack-nat.c +++ b/datapath-windows/ovsext/Conntrack-nat.c @@ -167,16 +167,13 @@ OvsNatPacket(OvsForwardingContext *ovsFwdCtx, { UINT32 natFlag; const struct ct_endpoint* endpoint; - LOCK_STATE_EX lockState; - /* XXX: Move conntrack locks out of NAT after implementing lock in NAT. */ - NdisAcquireRWLockRead(entry->lock, &lockState, 0); + /* When it is NAT, only entry->rev_key contains NATTED address; When it is unNAT, only entry->key contains the UNNATTED address;*/ const OVS_CT_KEY *ctKey = reverse ? &entry->key : &entry->rev_key; BOOLEAN isSrcNat; if (!(natAction & (NAT_ACTION_SRC | NAT_ACTION_DST))) { - NdisReleaseRWLock(entry->lock, &lockState); return; } isSrcNat = (((natAction & NAT_ACTION_SRC) && !reverse) || @@ -206,7 +203,6 @@ OvsNatPacket(OvsForwardingContext *ovsFwdCtx, } } else if (ctKey->dl_type == htons(ETH_TYPE_IPV6)){ // XXX: IPv6 packet not supported yet. - NdisReleaseRWLock(entry->lock, &lockState); return; } if (natAction & (NAT_ACTION_SRC_PORT | NAT_ACTION_DST_PORT)) { @@ -220,7 +216,6 @@ OvsNatPacket(OvsForwardingContext *ovsFwdCtx, } } } - NdisReleaseRWLock(entry->lock, &lockState); } diff --git a/datapath-windows/ovsext/Conntrack-related.c b/datapath-windows/ovsext/Conntrack-related.c index ec4b536..00eac67 100644 --- a/datapath-windows/ovsext/Conntrack-related.c +++ b/datapath-windows/ovsext/Conntrack-related.c @@ -18,7 +18,7 @@ #include "Jhash.h" static PLIST_ENTRY ovsCtRelatedTable; /* Holds related entries */ -static UINT64 ctTotalRelatedEntries; +static LONG ctTotalRelatedEntries; Please change to ULONG and cast it using PLONG where necessary. static OVS_CT_THREAD_CTX ctRelThreadCtx; static PNDIS_RW_LOCK_EX ovsCtRelatedLockObj; extern POVS_SWITCH_CONTEXT gOvsSwitchContext; @@ -75,13 +75,11 @@ OvsCtRelatedLookup(OVS_CT_KEY key, UINT64 currentTime) POVS_CT_REL_ENTRY entry; LOCK_STATE_EX lockState; - NdisAcquireRWLockRead(ovsCtRelatedLockObj, &lockState, 0); - if (!ctTotalRelatedEntries) { - NdisReleaseRWLock(ovsCtRelatedLockObj, &lockState); return NULL; } + NdisAcquireRWLockRead(ovsCtRelatedLockObj, &lockState, 0); for (int i = 0; i < CT_HASH_TABLE_SIZE; i++) { /* XXX - Scan the table based on the hash instead */ LIST_FORALL_SAFE(&ovsCtRelatedTable[i], link, next) { @@ -103,7 +101,7 @@ OvsCtRelatedEntryDelete(POVS_CT_REL_ENTRY entry) { RemoveEntryList(&entry->link); OvsFreeMemoryWithTag(entry, OVS_CT_POOL_TAG); - ctTotalRelatedEntries--; + InterlockedDecrement((LONG volatile*)&ctTotalRelatedEntries); Mind wrapping this using NdisInterlockedxxx function? } NDIS_STATUS @@ -139,7 +137,7 @@ OvsCtRelatedEntryCreate(UINT8 ipProto, NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0); InsertHeadList(&ovsCtRelatedTable[hash & CT_HASH_TABLE_MASK], &entry->link); - ctTotalRelatedEntries++; + InterlockedIncrement((LONG volatile *)&ctTotalRelatedEntries); NdisReleaseRWLock(ovsCtRelatedLockObj, &lockState); return NDIS_STATUS_SUCCESS; @@ -150,11 +148,10 @@ OvsCtRelatedFlush() { PLIST_ENTRY link, next; POVS_CT_REL_ENTRY entry; - LOCK_STATE_EX lockState; - NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0); if (ctTotalRelatedEntries) { + NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0); for (int i = 0; i < CT_HASH_TABLE_SIZE; i++) { LIST_FORALL_SAFE(&ovsCtRelatedTable[i], link, next) { entry = CONTAINING_RECORD(link, OVS_CT_REL_ENTRY, link); @@ -189,9 +186,8 @@ OvsCtRelatedEntryCleaner(PVOID data) /* Lock has been freed by 'OvsCleanupCtRelated()' */ break; } - NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0); + if (context->exit) { - NdisReleaseRWLock(ovsCtRelatedLockObj, &lockState); break; } @@ -201,6 +197,7 @@ OvsCtRelatedEntryCleaner(PVOID data) threadSleepTimeout = currentTime + CT_CLEANUP_INTERVAL; if (ctTotalRelatedEntries) { + NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0); for (int i = 0; i < CT_HASH_TABLE_SIZE; i++) { LIST_FORALL_SAFE(&ovsCtRelatedTable[i], link, next) { entry = CONTAINING_RECORD(link, OVS_CT_REL_ENTRY, link); diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c index add1491..c5aa7c5 100644 --- a/datapath-windows/ovsext/Conntrack.c +++ b/datapath-windows/ovsext/Conntrack.c @@ -208,7 +208,6 @@ OvsPostCtEventEntry(POVS_CT_ENTRY entry, UINT8 type) { OVS_CT_EVENT_ENTRY ctEventEntry = {0}; NdisMoveMemory(&ctEventEntry.entry, entry, sizeof(OVS_CT_ENTRY)); - ctEventEntry.entry.lock = NULL; ctEventEntry.entry.parent = NULL; ctEventEntry.type = type; OvsPostCtEvent(&ctEventEntry); @@ -217,8 +216,7 @@ OvsPostCtEventEntry(POVS_CT_ENTRY entry, UINT8 type) static __inline VOID OvsCtIncrementCounters(POVS_CT_ENTRY entry, BOOLEAN reply, PNET_BUFFER_LIST nbl) { - LOCK_STATE_EX lockState; - NdisAcquireRWLockWrite(entry->lock, &lockState, 0); + NdisAcquireSpinLock(&(entry->lock)); if (reply) { entry->rev_key.byteCount+= OvsPacketLenNBL(nbl); entry->rev_key.packetCount++; @@ -226,11 +224,11 @@ OvsCtIncrementCounters(POVS_CT_ENTRY entry, BOOLEAN reply, PNET_BUFFER_LIST nbl) entry->key.byteCount += OvsPacketLenNBL(nbl); entry->key.packetCount++; } - NdisReleaseRWLock(entry->lock, &lockState); + NdisReleaseSpinLock(&(entry->lock)); } static __inline BOOLEAN -OvsCtAddEntry(POVS_SWITCH_CONTEXT switchContext, POVS_CT_ENTRY entry, +OvsCtAddEntry(POVS_CT_ENTRY entry, OvsConntrackKeyLookupCtx *ctx, PNAT_ACTION_INFO natInfo, UINT64 now) { @@ -261,11 +259,7 @@ OvsCtAddEntry(POVS_SWITCH_CONTEXT switchContext, POVS_CT_ENTRY entry, } entry->timestampStart = now; - entry->lock = NdisAllocateRWLock(switchContext->NdisFilterHandle); - if (entry->lock == NULL) { - OVS_LOG_ERROR("NdisAllocateRWLock failed : Insufficient resources"); - return FALSE; - } + NdisAllocateSpinLock(&(entry->lock)); UINT32 bucketIdx = ctx->hash & CT_HASH_TABLE_MASK; entry->bucketLockRef = ovsCtBucketLock[bucketIdx]; NdisAcquireRWLockWrite(ovsCtBucketLock[bucketIdx], &lockState, 0); @@ -351,8 +345,7 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx, if (state != OVS_CS_F_INVALID && commit) { if (entry) { entry->parent = parentEntry; - if (OvsCtAddEntry(fwdCtx->switchContext, entry, ctx, - natInfo, currentTime)) { + if (OvsCtAddEntry(entry, ctx, natInfo, currentTime)) { *entryCreated = TRUE; } else { /* Unable to add entry to the list */ @@ -382,8 +375,7 @@ OvsCtUpdateEntry(OVS_CT_ENTRY* entry, UINT64 now) { CT_UPDATE_RES status; - LOCK_STATE_EX lockState; - NdisAcquireRWLockWrite(entry->lock, &lockState, 0); + NdisAcquireSpinLock(&(entry->lock)); switch (ipProto) { case IPPROTO_TCP: { @@ -407,7 +399,7 @@ OvsCtUpdateEntry(OVS_CT_ENTRY* entry, status = CT_UPDATE_INVALID; break; } - NdisReleaseRWLock(entry->lock, &lockState); + NdisReleaseSpinLock(&(entry->lock)); return status; } @@ -431,8 +423,7 @@ OvsCtEntryDelete(POVS_CT_ENTRY entry, BOOLEAN forceDelete) if (entry == NULL) { return; } - LOCK_STATE_EX lockState; - NdisAcquireRWLockWrite(entry->lock, &lockState, 0); + NdisAcquireSpinLock(&(entry->lock)); if (forceDelete || OvsCtEntryExpired(entry)) { if (entry->natInfo.natAction) { LOCK_STATE_EX lockStateNat; @@ -442,13 +433,13 @@ OvsCtEntryDelete(POVS_CT_ENTRY entry, BOOLEAN forceDelete) } OvsPostCtEventEntry(entry, OVS_EVENT_CT_DELETE); RemoveEntryList(&entry->link); - NdisReleaseRWLock(entry->lock, &lockState); - NdisFreeRWLock(entry->lock); + NdisReleaseSpinLock(&(entry->lock)); + NdisFreeSpinLock(&(entry->lock)); OvsFreeMemoryWithTag(entry, OVS_CT_POOL_TAG); InterlockedDecrement((LONG volatile*)&ctTotalEntries); return; } - NdisReleaseRWLock(entry->lock, &lockState); + NdisReleaseSpinLock(&(entry->lock)); } static __inline NDIS_STATUS @@ -499,7 +490,7 @@ OvsCtLookup(OvsConntrackKeyLookupCtx *ctx) POVS_CT_ENTRY entry; BOOLEAN reply = FALSE; POVS_CT_ENTRY found = NULL; - LOCK_STATE_EX lockState, lockStateTable; + LOCK_STATE_EX lockStateTable; UINT32 bucketIdx; /* Reverse NAT must be performed before OvsCtLookup, so here * we simply need to flip the src and dst in key and compare @@ -516,7 +507,7 @@ OvsCtLookup(OvsConntrackKeyLookupCtx *ctx) NdisAcquireRWLockRead(ovsCtBucketLock[bucketIdx], &lockStateTable, 0); LIST_FORALL(&ovsConntrackTable[bucketIdx], link) { entry = CONTAINING_RECORD(link, OVS_CT_ENTRY, link); - NdisAcquireRWLockRead(entry->lock, &lockState, 0); + NdisAcquireSpinLock(&(entry->lock)); if (OvsCtKeyAreSame(ctx->key, entry->key)) { found = entry; reply = FALSE; @@ -533,10 +524,10 @@ OvsCtLookup(OvsConntrackKeyLookupCtx *ctx) } else { ctx->reply = reply; } - NdisReleaseRWLock(entry->lock, &lockState); + NdisReleaseSpinLock(&(entry->lock)); break; } - NdisReleaseRWLock(entry->lock, &lockState); + NdisReleaseSpinLock(&(entry->lock)); } NdisReleaseRWLock(ovsCtBucketLock[bucketIdx], &lockStateTable); @@ -689,7 +680,7 @@ OvsProcessConntrackEntry(OvsForwardingContext *fwdCtx, POVS_CT_ENTRY entry = ctx->entry; UINT32 state = 0; PNET_BUFFER_LIST curNbl = fwdCtx->curNbl; - LOCK_STATE_EX lockState, lockStateTable; + LOCK_STATE_EX lockStateTable; PNDIS_RW_LOCK_EX bucketLockRef = NULL; *entryCreated = FALSE; @@ -730,7 +721,7 @@ OvsProcessConntrackEntry(OvsForwardingContext *fwdCtx, } } if (entry) { - NdisAcquireRWLockRead(entry->lock, &lockState, 0); + NdisAcquireSpinLock(&(entry->lock)); if (key->ipKey.nwProto == IPPROTO_TCP) { /* Update the related bit if there is a parent */ if (entry->parent) { @@ -748,7 +739,7 @@ OvsProcessConntrackEntry(OvsForwardingContext *fwdCtx, /* Copy mark and label from entry into flowKey. If actions specify different mark and label, update the flowKey. */ OvsCtUpdateFlowKey(key, state, zone, entry->mark, &entry->labels); - NdisReleaseRWLock(entry->lock, &lockState); + NdisReleaseSpinLock(&(entry->lock)); } else { OvsCtUpdateFlowKey(key, state, zone, 0, NULL); } @@ -802,8 +793,6 @@ OvsCtSetMarkLabel(OvsFlowKey *key, MD_LABELS *labels, BOOLEAN *triggerUpdateEvent) { - LOCK_STATE_EX lockState; - NdisAcquireRWLockWrite(entry->lock, &lockState, 0); if (mark) { OvsConntrackSetMark(key, entry, mark->value, mark->mask, triggerUpdateEvent); @@ -813,7 +802,6 @@ OvsCtSetMarkLabel(OvsFlowKey *key, OvsConntrackSetLabels(key, entry, &labels->value, &labels->mask, triggerUpdateEvent); } - NdisReleaseRWLock(entry->lock, &lockState); } /* @@ -854,10 +842,11 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, { NDIS_STATUS status = NDIS_STATUS_SUCCESS; BOOLEAN triggerUpdateEvent = FALSE; + BOOLEAN entryCreated = FALSE; POVS_CT_ENTRY entry = NULL; PNET_BUFFER_LIST curNbl = fwdCtx->curNbl; OvsConntrackKeyLookupCtx ctx = { 0 }; - LOCK_STATE_EX lockState, lockStateTable; + LOCK_STATE_EX lockStateTable; UINT64 currentTime; NdisGetCurrentSystemTime((LARGE_INTEGER *) ¤tTime); @@ -866,10 +855,9 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, /* Lookup Conntrack entries for a matching entry */ entry = OvsCtLookup(&ctx); - BOOLEAN entryCreated = FALSE; /* Delete entry in reverse direction if 'force' is specified */ - if (entry && force && ctx.reply) { + if (force && ctx.reply && entry) { PNDIS_RW_LOCK_EX bucketLockRef = entry->bucketLockRef; NdisAcquireRWLockWrite(bucketLockRef, &lockStateTable, 0); OvsCtEntryDelete(entry, TRUE); @@ -877,34 +865,33 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, entry = NULL; } - if (!entry && commit && ctTotalEntries >= CT_MAX_ENTRIES) { - /* Don't proceed with processing if the max limit has been hit. - * This blocks only new entries from being created and doesn't - * affect existing connections. + if (entry) { + /* Increment stats for the entry if it wasn't tracked previously or + * if they are on different zones */ - OVS_LOG_ERROR("Conntrack Limit hit: %lu", ctTotalEntries); - return NDIS_STATUS_RESOURCES; - } - - /* Increment stats for the entry if it wasn't tracked previously or - * if they are on different zones - */ - if (entry && (entry->key.zone != key->ct.zone || - (!(key->ct.state & OVS_CS_F_TRACKED)))) { - OvsCtIncrementCounters(entry, ctx.reply, curNbl); - } + if ((entry->key.zone != key->ct.zone || + (!(key->ct.state & OVS_CS_F_TRACKED)))) { + OvsCtIncrementCounters(entry, ctx.reply, curNbl); + } + /* Process the entry and update CT flags */ + entry = OvsProcessConntrackEntry(fwdCtx, layers->l4Offset, &ctx, key, + zone, natInfo, commit, currentTime, + &entryCreated); - if (!entry) { + } else { + if (commit && ctTotalEntries >= CT_MAX_ENTRIES) { + /* Don't proceed with processing if the max limit has been hit. + * This blocks only new entries from being created and doesn't + * affect existing connections. + */ + OVS_LOG_ERROR("Conntrack Limit hit: %lu", ctTotalEntries); + return NDIS_STATUS_RESOURCES; + } /* If no matching entry was found, create one and add New state */ entry = OvsCtEntryCreate(fwdCtx, key->ipKey.nwProto, layers->l4Offset, &ctx, key, natInfo, commit, currentTime, &entryCreated); - } else { - /* Process the entry and update CT flags */ - entry = OvsProcessConntrackEntry(fwdCtx, layers->l4Offset, &ctx, key, - zone, natInfo, commit, currentTime, - &entryCreated); } if (entry == NULL) { @@ -918,6 +905,7 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, * NAT_ACTION_REVERSE, yet entry->natInfo is NAT_ACTION_SRC or * NAT_ACTION_DST without NAT_ACTION_REVERSE */ + NdisAcquireSpinLock(&(entry->lock)); if (natInfo->natAction != NAT_ACTION_NONE) { LOCK_STATE_EX lockStateNat; @@ -939,15 +927,14 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, OVS_LOG_ERROR("Error while parsing the FTP packet"); } } - NdisAcquireRWLockRead(entry->lock, &lockState, 0); + /* Add original tuple information to flow Key */ if (entry->key.dl_type == ntohs(ETH_TYPE_IPV4)) { if (entry->parent != NULL) { POVS_CT_ENTRY parent = entry->parent; - LOCK_STATE_EX lockStateParent; - NdisAcquireRWLockRead(parent->lock, &lockStateParent, 0); + NdisAcquireSpinLock(&(parent->lock)); OvsCtUpdateTuple(key, &parent->key); - NdisReleaseRWLock(parent->lock, &lockStateParent); + NdisReleaseSpinLock(&(parent->lock)); } else { OvsCtUpdateTuple(key, &entry->key); } @@ -955,13 +942,11 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, if (entryCreated) { OvsPostCtEventEntry(entry, OVS_EVENT_CT_NEW); - } - if (postUpdateEvent && !entryCreated && triggerUpdateEvent) { + } else if (postUpdateEvent && triggerUpdateEvent) { OvsPostCtEventEntry(entry, OVS_EVENT_CT_UPDATE); } - NdisReleaseRWLock(entry->lock, &lockState); - + NdisReleaseSpinLock(&(entry->lock)); return status; } @@ -1739,7 +1724,7 @@ OvsCtDumpCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, UINT32 i = CT_HASH_TABLE_SIZE; UINT32 outIndex = 0; - LOCK_STATE_EX lockState, lockStateTable; + LOCK_STATE_EX lockStateTable; if (ctTotalEntries) { for (i = inBucket; i < CT_HASH_TABLE_SIZE; i++) { PLIST_ENTRY head, link; @@ -1756,7 +1741,7 @@ OvsCtDumpCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, */ if (outIndex >= inIndex) { entry = CONTAINING_RECORD(link, OVS_CT_ENTRY, link); - NdisAcquireRWLockRead(entry->lock, &lockState, 0); + NdisAcquireSpinLock(&(entry->lock)); rc = OvsCreateNlMsgFromCtEntry(entry, usrParamsCtx->outputBuffer, usrParamsCtx->outputLength, @@ -1765,7 +1750,7 @@ OvsCtDumpCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, msgIn->nlMsg.nlmsgPid, msgIn->nfGenMsg.version, 0); - NdisReleaseRWLock(entry->lock, &lockState); + NdisReleaseSpinLock(&(entry->lock)); if (rc != NDIS_STATUS_SUCCESS) { NdisReleaseRWLock(ovsCtBucketLock[i], &lockStateTable); return STATUS_UNSUCCESSFUL; diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-windows/ovsext/Conntrack.h index 3be309e..7dc92a1 100644 --- a/datapath-windows/ovsext/Conntrack.h +++ b/datapath-windows/ovsext/Conntrack.h @@ -101,7 +101,7 @@ typedef struct _NAT_ACTION_INFO { typedef struct OVS_CT_ENTRY { /* Reference to ovsCtBucketLock of ovsConntrackTable.*/ PNDIS_RW_LOCK_EX bucketLockRef; - PNDIS_RW_LOCK_EX lock; /* Protects OVS_CT_ENTRY. */ + NDIS_SPIN_LOCK lock; /* Protects OVS_CT_ENTRY. */ OVS_CT_KEY key; OVS_CT_KEY rev_key; UINT64 expiration; -- 2.9.3.windows.1
diff --git a/datapath-windows/ovsext/Conntrack-nat.c b/datapath-windows/ovsext/Conntrack-nat.c index 316c946..da1814f 100644 --- a/datapath-windows/ovsext/Conntrack-nat.c +++ b/datapath-windows/ovsext/Conntrack-nat.c @@ -167,16 +167,13 @@ OvsNatPacket(OvsForwardingContext *ovsFwdCtx, { UINT32 natFlag; const struct ct_endpoint* endpoint; - LOCK_STATE_EX lockState; - /* XXX: Move conntrack locks out of NAT after implementing lock in NAT. */ - NdisAcquireRWLockRead(entry->lock, &lockState, 0); + /* When it is NAT, only entry->rev_key contains NATTED address; When it is unNAT, only entry->key contains the UNNATTED address;*/ const OVS_CT_KEY *ctKey = reverse ? &entry->key : &entry->rev_key; BOOLEAN isSrcNat; if (!(natAction & (NAT_ACTION_SRC | NAT_ACTION_DST))) { - NdisReleaseRWLock(entry->lock, &lockState); return; } isSrcNat = (((natAction & NAT_ACTION_SRC) && !reverse) || @@ -206,7 +203,6 @@ OvsNatPacket(OvsForwardingContext *ovsFwdCtx, } } else if (ctKey->dl_type == htons(ETH_TYPE_IPV6)){ // XXX: IPv6 packet not supported yet. - NdisReleaseRWLock(entry->lock, &lockState); return; } if (natAction & (NAT_ACTION_SRC_PORT | NAT_ACTION_DST_PORT)) { @@ -220,7 +216,6 @@ OvsNatPacket(OvsForwardingContext *ovsFwdCtx, } } } - NdisReleaseRWLock(entry->lock, &lockState); } diff --git a/datapath-windows/ovsext/Conntrack-related.c b/datapath-windows/ovsext/Conntrack-related.c index ec4b536..00eac67 100644 --- a/datapath-windows/ovsext/Conntrack-related.c +++ b/datapath-windows/ovsext/Conntrack-related.c @@ -18,7 +18,7 @@ #include "Jhash.h" static PLIST_ENTRY ovsCtRelatedTable; /* Holds related entries */ -static UINT64 ctTotalRelatedEntries; +static LONG ctTotalRelatedEntries; static OVS_CT_THREAD_CTX ctRelThreadCtx; static PNDIS_RW_LOCK_EX ovsCtRelatedLockObj; extern POVS_SWITCH_CONTEXT gOvsSwitchContext; @@ -75,13 +75,11 @@ OvsCtRelatedLookup(OVS_CT_KEY key, UINT64 currentTime) POVS_CT_REL_ENTRY entry; LOCK_STATE_EX lockState; - NdisAcquireRWLockRead(ovsCtRelatedLockObj, &lockState, 0); - if (!ctTotalRelatedEntries) { - NdisReleaseRWLock(ovsCtRelatedLockObj, &lockState); return NULL; } + NdisAcquireRWLockRead(ovsCtRelatedLockObj, &lockState, 0); for (int i = 0; i < CT_HASH_TABLE_SIZE; i++) { /* XXX - Scan the table based on the hash instead */ LIST_FORALL_SAFE(&ovsCtRelatedTable[i], link, next) { @@ -103,7 +101,7 @@ OvsCtRelatedEntryDelete(POVS_CT_REL_ENTRY entry) { RemoveEntryList(&entry->link); OvsFreeMemoryWithTag(entry, OVS_CT_POOL_TAG); - ctTotalRelatedEntries--; + InterlockedDecrement((LONG volatile*)&ctTotalRelatedEntries); } NDIS_STATUS @@ -139,7 +137,7 @@ OvsCtRelatedEntryCreate(UINT8 ipProto, NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0); InsertHeadList(&ovsCtRelatedTable[hash & CT_HASH_TABLE_MASK], &entry->link); - ctTotalRelatedEntries++; + InterlockedIncrement((LONG volatile *)&ctTotalRelatedEntries); NdisReleaseRWLock(ovsCtRelatedLockObj, &lockState); return NDIS_STATUS_SUCCESS; @@ -150,11 +148,10 @@ OvsCtRelatedFlush() { PLIST_ENTRY link, next; POVS_CT_REL_ENTRY entry; - LOCK_STATE_EX lockState; - NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0); if (ctTotalRelatedEntries) { + NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0); for (int i = 0; i < CT_HASH_TABLE_SIZE; i++) { LIST_FORALL_SAFE(&ovsCtRelatedTable[i], link, next) { entry = CONTAINING_RECORD(link, OVS_CT_REL_ENTRY, link); @@ -189,9 +186,8 @@ OvsCtRelatedEntryCleaner(PVOID data) /* Lock has been freed by 'OvsCleanupCtRelated()' */ break; } - NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0); + if (context->exit) { - NdisReleaseRWLock(ovsCtRelatedLockObj, &lockState); break; } @@ -201,6 +197,7 @@ OvsCtRelatedEntryCleaner(PVOID data) threadSleepTimeout = currentTime + CT_CLEANUP_INTERVAL; if (ctTotalRelatedEntries) { + NdisAcquireRWLockWrite(ovsCtRelatedLockObj, &lockState, 0); for (int i = 0; i < CT_HASH_TABLE_SIZE; i++) { LIST_FORALL_SAFE(&ovsCtRelatedTable[i], link, next) { entry = CONTAINING_RECORD(link, OVS_CT_REL_ENTRY, link); diff --git a/datapath-windows/ovsext/Conntrack.c b/datapath-windows/ovsext/Conntrack.c index add1491..c5aa7c5 100644 --- a/datapath-windows/ovsext/Conntrack.c +++ b/datapath-windows/ovsext/Conntrack.c @@ -208,7 +208,6 @@ OvsPostCtEventEntry(POVS_CT_ENTRY entry, UINT8 type) { OVS_CT_EVENT_ENTRY ctEventEntry = {0}; NdisMoveMemory(&ctEventEntry.entry, entry, sizeof(OVS_CT_ENTRY)); - ctEventEntry.entry.lock = NULL; ctEventEntry.entry.parent = NULL; ctEventEntry.type = type; OvsPostCtEvent(&ctEventEntry); @@ -217,8 +216,7 @@ OvsPostCtEventEntry(POVS_CT_ENTRY entry, UINT8 type) static __inline VOID OvsCtIncrementCounters(POVS_CT_ENTRY entry, BOOLEAN reply, PNET_BUFFER_LIST nbl) { - LOCK_STATE_EX lockState; - NdisAcquireRWLockWrite(entry->lock, &lockState, 0); + NdisAcquireSpinLock(&(entry->lock)); if (reply) { entry->rev_key.byteCount+= OvsPacketLenNBL(nbl); entry->rev_key.packetCount++; @@ -226,11 +224,11 @@ OvsCtIncrementCounters(POVS_CT_ENTRY entry, BOOLEAN reply, PNET_BUFFER_LIST nbl) entry->key.byteCount += OvsPacketLenNBL(nbl); entry->key.packetCount++; } - NdisReleaseRWLock(entry->lock, &lockState); + NdisReleaseSpinLock(&(entry->lock)); } static __inline BOOLEAN -OvsCtAddEntry(POVS_SWITCH_CONTEXT switchContext, POVS_CT_ENTRY entry, +OvsCtAddEntry(POVS_CT_ENTRY entry, OvsConntrackKeyLookupCtx *ctx, PNAT_ACTION_INFO natInfo, UINT64 now) { @@ -261,11 +259,7 @@ OvsCtAddEntry(POVS_SWITCH_CONTEXT switchContext, POVS_CT_ENTRY entry, } entry->timestampStart = now; - entry->lock = NdisAllocateRWLock(switchContext->NdisFilterHandle); - if (entry->lock == NULL) { - OVS_LOG_ERROR("NdisAllocateRWLock failed : Insufficient resources"); - return FALSE; - } + NdisAllocateSpinLock(&(entry->lock)); UINT32 bucketIdx = ctx->hash & CT_HASH_TABLE_MASK; entry->bucketLockRef = ovsCtBucketLock[bucketIdx]; NdisAcquireRWLockWrite(ovsCtBucketLock[bucketIdx], &lockState, 0); @@ -351,8 +345,7 @@ OvsCtEntryCreate(OvsForwardingContext *fwdCtx, if (state != OVS_CS_F_INVALID && commit) { if (entry) { entry->parent = parentEntry; - if (OvsCtAddEntry(fwdCtx->switchContext, entry, ctx, - natInfo, currentTime)) { + if (OvsCtAddEntry(entry, ctx, natInfo, currentTime)) { *entryCreated = TRUE; } else { /* Unable to add entry to the list */ @@ -382,8 +375,7 @@ OvsCtUpdateEntry(OVS_CT_ENTRY* entry, UINT64 now) { CT_UPDATE_RES status; - LOCK_STATE_EX lockState; - NdisAcquireRWLockWrite(entry->lock, &lockState, 0); + NdisAcquireSpinLock(&(entry->lock)); switch (ipProto) { case IPPROTO_TCP: { @@ -407,7 +399,7 @@ OvsCtUpdateEntry(OVS_CT_ENTRY* entry, status = CT_UPDATE_INVALID; break; } - NdisReleaseRWLock(entry->lock, &lockState); + NdisReleaseSpinLock(&(entry->lock)); return status; } @@ -431,8 +423,7 @@ OvsCtEntryDelete(POVS_CT_ENTRY entry, BOOLEAN forceDelete) if (entry == NULL) { return; } - LOCK_STATE_EX lockState; - NdisAcquireRWLockWrite(entry->lock, &lockState, 0); + NdisAcquireSpinLock(&(entry->lock)); if (forceDelete || OvsCtEntryExpired(entry)) { if (entry->natInfo.natAction) { LOCK_STATE_EX lockStateNat; @@ -442,13 +433,13 @@ OvsCtEntryDelete(POVS_CT_ENTRY entry, BOOLEAN forceDelete) } OvsPostCtEventEntry(entry, OVS_EVENT_CT_DELETE); RemoveEntryList(&entry->link); - NdisReleaseRWLock(entry->lock, &lockState); - NdisFreeRWLock(entry->lock); + NdisReleaseSpinLock(&(entry->lock)); + NdisFreeSpinLock(&(entry->lock)); OvsFreeMemoryWithTag(entry, OVS_CT_POOL_TAG); InterlockedDecrement((LONG volatile*)&ctTotalEntries); return; } - NdisReleaseRWLock(entry->lock, &lockState); + NdisReleaseSpinLock(&(entry->lock)); } static __inline NDIS_STATUS @@ -499,7 +490,7 @@ OvsCtLookup(OvsConntrackKeyLookupCtx *ctx) POVS_CT_ENTRY entry; BOOLEAN reply = FALSE; POVS_CT_ENTRY found = NULL; - LOCK_STATE_EX lockState, lockStateTable; + LOCK_STATE_EX lockStateTable; UINT32 bucketIdx; /* Reverse NAT must be performed before OvsCtLookup, so here * we simply need to flip the src and dst in key and compare @@ -516,7 +507,7 @@ OvsCtLookup(OvsConntrackKeyLookupCtx *ctx) NdisAcquireRWLockRead(ovsCtBucketLock[bucketIdx], &lockStateTable, 0); LIST_FORALL(&ovsConntrackTable[bucketIdx], link) { entry = CONTAINING_RECORD(link, OVS_CT_ENTRY, link); - NdisAcquireRWLockRead(entry->lock, &lockState, 0); + NdisAcquireSpinLock(&(entry->lock)); if (OvsCtKeyAreSame(ctx->key, entry->key)) { found = entry; reply = FALSE; @@ -533,10 +524,10 @@ OvsCtLookup(OvsConntrackKeyLookupCtx *ctx) } else { ctx->reply = reply; } - NdisReleaseRWLock(entry->lock, &lockState); + NdisReleaseSpinLock(&(entry->lock)); break; } - NdisReleaseRWLock(entry->lock, &lockState); + NdisReleaseSpinLock(&(entry->lock)); } NdisReleaseRWLock(ovsCtBucketLock[bucketIdx], &lockStateTable); @@ -689,7 +680,7 @@ OvsProcessConntrackEntry(OvsForwardingContext *fwdCtx, POVS_CT_ENTRY entry = ctx->entry; UINT32 state = 0; PNET_BUFFER_LIST curNbl = fwdCtx->curNbl; - LOCK_STATE_EX lockState, lockStateTable; + LOCK_STATE_EX lockStateTable; PNDIS_RW_LOCK_EX bucketLockRef = NULL; *entryCreated = FALSE; @@ -730,7 +721,7 @@ OvsProcessConntrackEntry(OvsForwardingContext *fwdCtx, } } if (entry) { - NdisAcquireRWLockRead(entry->lock, &lockState, 0); + NdisAcquireSpinLock(&(entry->lock)); if (key->ipKey.nwProto == IPPROTO_TCP) { /* Update the related bit if there is a parent */ if (entry->parent) { @@ -748,7 +739,7 @@ OvsProcessConntrackEntry(OvsForwardingContext *fwdCtx, /* Copy mark and label from entry into flowKey. If actions specify different mark and label, update the flowKey. */ OvsCtUpdateFlowKey(key, state, zone, entry->mark, &entry->labels); - NdisReleaseRWLock(entry->lock, &lockState); + NdisReleaseSpinLock(&(entry->lock)); } else { OvsCtUpdateFlowKey(key, state, zone, 0, NULL); } @@ -802,8 +793,6 @@ OvsCtSetMarkLabel(OvsFlowKey *key, MD_LABELS *labels, BOOLEAN *triggerUpdateEvent) { - LOCK_STATE_EX lockState; - NdisAcquireRWLockWrite(entry->lock, &lockState, 0); if (mark) { OvsConntrackSetMark(key, entry, mark->value, mark->mask, triggerUpdateEvent); @@ -813,7 +802,6 @@ OvsCtSetMarkLabel(OvsFlowKey *key, OvsConntrackSetLabels(key, entry, &labels->value, &labels->mask, triggerUpdateEvent); } - NdisReleaseRWLock(entry->lock, &lockState); } /* @@ -854,10 +842,11 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, { NDIS_STATUS status = NDIS_STATUS_SUCCESS; BOOLEAN triggerUpdateEvent = FALSE; + BOOLEAN entryCreated = FALSE; POVS_CT_ENTRY entry = NULL; PNET_BUFFER_LIST curNbl = fwdCtx->curNbl; OvsConntrackKeyLookupCtx ctx = { 0 }; - LOCK_STATE_EX lockState, lockStateTable; + LOCK_STATE_EX lockStateTable; UINT64 currentTime; NdisGetCurrentSystemTime((LARGE_INTEGER *) ¤tTime); @@ -866,10 +855,9 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, /* Lookup Conntrack entries for a matching entry */ entry = OvsCtLookup(&ctx); - BOOLEAN entryCreated = FALSE; /* Delete entry in reverse direction if 'force' is specified */ - if (entry && force && ctx.reply) { + if (force && ctx.reply && entry) { PNDIS_RW_LOCK_EX bucketLockRef = entry->bucketLockRef; NdisAcquireRWLockWrite(bucketLockRef, &lockStateTable, 0); OvsCtEntryDelete(entry, TRUE); @@ -877,34 +865,33 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, entry = NULL; } - if (!entry && commit && ctTotalEntries >= CT_MAX_ENTRIES) { - /* Don't proceed with processing if the max limit has been hit. - * This blocks only new entries from being created and doesn't - * affect existing connections. + if (entry) { + /* Increment stats for the entry if it wasn't tracked previously or + * if they are on different zones */ - OVS_LOG_ERROR("Conntrack Limit hit: %lu", ctTotalEntries); - return NDIS_STATUS_RESOURCES; - } - - /* Increment stats for the entry if it wasn't tracked previously or - * if they are on different zones - */ - if (entry && (entry->key.zone != key->ct.zone || - (!(key->ct.state & OVS_CS_F_TRACKED)))) { - OvsCtIncrementCounters(entry, ctx.reply, curNbl); - } + if ((entry->key.zone != key->ct.zone || + (!(key->ct.state & OVS_CS_F_TRACKED)))) { + OvsCtIncrementCounters(entry, ctx.reply, curNbl); + } + /* Process the entry and update CT flags */ + entry = OvsProcessConntrackEntry(fwdCtx, layers->l4Offset, &ctx, key, + zone, natInfo, commit, currentTime, + &entryCreated); - if (!entry) { + } else { + if (commit && ctTotalEntries >= CT_MAX_ENTRIES) { + /* Don't proceed with processing if the max limit has been hit. + * This blocks only new entries from being created and doesn't + * affect existing connections. + */ + OVS_LOG_ERROR("Conntrack Limit hit: %lu", ctTotalEntries); + return NDIS_STATUS_RESOURCES; + } /* If no matching entry was found, create one and add New state */ entry = OvsCtEntryCreate(fwdCtx, key->ipKey.nwProto, layers->l4Offset, &ctx, key, natInfo, commit, currentTime, &entryCreated); - } else { - /* Process the entry and update CT flags */ - entry = OvsProcessConntrackEntry(fwdCtx, layers->l4Offset, &ctx, key, - zone, natInfo, commit, currentTime, - &entryCreated); } if (entry == NULL) { @@ -918,6 +905,7 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, * NAT_ACTION_REVERSE, yet entry->natInfo is NAT_ACTION_SRC or * NAT_ACTION_DST without NAT_ACTION_REVERSE */ + NdisAcquireSpinLock(&(entry->lock)); if (natInfo->natAction != NAT_ACTION_NONE) { LOCK_STATE_EX lockStateNat; @@ -939,15 +927,14 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, OVS_LOG_ERROR("Error while parsing the FTP packet"); } } - NdisAcquireRWLockRead(entry->lock, &lockState, 0); + /* Add original tuple information to flow Key */ if (entry->key.dl_type == ntohs(ETH_TYPE_IPV4)) { if (entry->parent != NULL) { POVS_CT_ENTRY parent = entry->parent; - LOCK_STATE_EX lockStateParent; - NdisAcquireRWLockRead(parent->lock, &lockStateParent, 0); + NdisAcquireSpinLock(&(parent->lock)); OvsCtUpdateTuple(key, &parent->key); - NdisReleaseRWLock(parent->lock, &lockStateParent); + NdisReleaseSpinLock(&(parent->lock)); } else { OvsCtUpdateTuple(key, &entry->key); } @@ -955,13 +942,11 @@ OvsCtExecute_(OvsForwardingContext *fwdCtx, if (entryCreated) { OvsPostCtEventEntry(entry, OVS_EVENT_CT_NEW); - } - if (postUpdateEvent && !entryCreated && triggerUpdateEvent) { + } else if (postUpdateEvent && triggerUpdateEvent) { OvsPostCtEventEntry(entry, OVS_EVENT_CT_UPDATE); } - NdisReleaseRWLock(entry->lock, &lockState); - + NdisReleaseSpinLock(&(entry->lock)); return status; } @@ -1739,7 +1724,7 @@ OvsCtDumpCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, UINT32 i = CT_HASH_TABLE_SIZE; UINT32 outIndex = 0; - LOCK_STATE_EX lockState, lockStateTable; + LOCK_STATE_EX lockStateTable; if (ctTotalEntries) { for (i = inBucket; i < CT_HASH_TABLE_SIZE; i++) { PLIST_ENTRY head, link; @@ -1756,7 +1741,7 @@ OvsCtDumpCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, */ if (outIndex >= inIndex) { entry = CONTAINING_RECORD(link, OVS_CT_ENTRY, link); - NdisAcquireRWLockRead(entry->lock, &lockState, 0); + NdisAcquireSpinLock(&(entry->lock)); rc = OvsCreateNlMsgFromCtEntry(entry, usrParamsCtx->outputBuffer, usrParamsCtx->outputLength, @@ -1765,7 +1750,7 @@ OvsCtDumpCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx, msgIn->nlMsg.nlmsgPid, msgIn->nfGenMsg.version, 0); - NdisReleaseRWLock(entry->lock, &lockState); + NdisReleaseSpinLock(&(entry->lock)); if (rc != NDIS_STATUS_SUCCESS) { NdisReleaseRWLock(ovsCtBucketLock[i], &lockStateTable); return STATUS_UNSUCCESSFUL; diff --git a/datapath-windows/ovsext/Conntrack.h b/datapath-windows/ovsext/Conntrack.h index 3be309e..7dc92a1 100644 --- a/datapath-windows/ovsext/Conntrack.h +++ b/datapath-windows/ovsext/Conntrack.h @@ -101,7 +101,7 @@ typedef struct _NAT_ACTION_INFO { typedef struct OVS_CT_ENTRY { /* Reference to ovsCtBucketLock of ovsConntrackTable.*/ PNDIS_RW_LOCK_EX bucketLockRef; - PNDIS_RW_LOCK_EX lock; /* Protects OVS_CT_ENTRY. */ + NDIS_SPIN_LOCK lock; /* Protects OVS_CT_ENTRY. */ OVS_CT_KEY key; OVS_CT_KEY rev_key; UINT64 expiration;
- Move conntrack lock out of NAT module - Use spinlock instead of read/write lock for conntrack entry. - Update 'ctTotalRelatedEntries' using interlocked functions - Refactor conntrack code to make it more readable. Testing: Evaluated TCP performance using iperf3. Before optimization: Native: 6.0Gbps OVS: 5.1-5.75Gbps OVS with conntrack enabled: 3.9-4.0Gbps After optimization: Native: 6.0Gbps OVS: 5.1-5.75Gbps OVS with conntrack enabled:: 4.2-4.4Gbps Tested by loading/unloading driver with driver verifier enabled Signed-off-by: Anand Kumar <kumaranand@vmware.com> --- v1->v2: Update commit message --- datapath-windows/ovsext/Conntrack-nat.c | 7 +- datapath-windows/ovsext/Conntrack-related.c | 17 ++-- datapath-windows/ovsext/Conntrack.c | 115 ++++++++++++---------------- datapath-windows/ovsext/Conntrack.h | 2 +- 4 files changed, 59 insertions(+), 82 deletions(-)