[ovs-dev] [PATCH v4] datapath-windows: Removed gOvsCtrlLock global spinlock
Sorin Vinturis
svinturis at cloudbasesolutions.com
Thu Apr 23 20:27:53 UTC 2015
There is no need to use gOvsCtrlLock spinlock to guard the switch
context, as there is now the switch context's reference count used
for this purpose.
Now the gOvsCtrlLock spinlock guards only one shared resource, the
OVS_OPEN_INSTANCE global instance array.
v2: Atomically test and set if the instance is in attach process, to
protect filter attach routine against multiple calls.
v3: Changed gOvsInAttach global variable type into 'LONG volatile'.
Also removed an unrelated change from SubscribeIoctl handler.
v4: Rebased the patch.
Signed-off-by: Sorin Vinturis <svinturis at cloudbasesolutions.com>
Acked-by: Nithin Raju <nithin at vmware.com>
---
datapath-windows/ovsext/Datapath.c | 15 ---------------
datapath-windows/ovsext/Flow.c | 39 +++++++++++++-------------------------
datapath-windows/ovsext/Switch.c | 13 ++++---------
datapath-windows/ovsext/User.c | 18 +++++-------------
datapath-windows/ovsext/Vport.c | 22 ++-------------------
5 files changed, 24 insertions(+), 83 deletions(-)
diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c
index fea7d3a..1dead33 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -958,14 +958,11 @@ ValidateNetlinkCmd(UINT32 devOp,
/* Validate the DP for commands that require a DP. */
if (nlFamilyOps->cmds[i].validateDpIndex == TRUE) {
- OvsAcquireCtrlLock();
if (ovsMsg->ovsHdr.dp_ifindex !=
(INT)gOvsSwitchContext->dpNo) {
status = STATUS_INVALID_PARAMETER;
- OvsReleaseCtrlLock();
goto done;
}
- OvsReleaseCtrlLock();
}
/* Validate the PID. */
@@ -1045,7 +1042,6 @@ OvsGetPidHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
* --------------------------------------------------------------------------
* Utility function to fill up information about the datapath in a reply to
* userspace.
- * Assumes that 'gOvsCtrlLock' lock is acquired.
* --------------------------------------------------------------------------
*/
static NTSTATUS
@@ -1245,9 +1241,7 @@ HandleGetDpDump(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
NlBufInit(&nlBuf, usrParamsCtx->outputBuffer,
usrParamsCtx->outputLength);
- OvsAcquireCtrlLock();
status = OvsDpFillInfo(gOvsSwitchContext, msgIn, &nlBuf);
- OvsReleaseCtrlLock();
if (status != STATUS_SUCCESS) {
*replyLen = 0;
@@ -1334,11 +1328,9 @@ HandleDpTransactionCommon(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
NlBufInit(&nlBuf, usrParamsCtx->outputBuffer, usrParamsCtx->outputLength);
- OvsAcquireCtrlLock();
if (dpAttrs[OVS_DP_ATTR_NAME] != NULL) {
if (!OvsCompareString(NlAttrGet(dpAttrs[OVS_DP_ATTR_NAME]),
OVS_SYSTEM_DP_NAME)) {
- OvsReleaseCtrlLock();
/* Creation of new datapaths is not supported. */
if (usrParamsCtx->ovsMsg->genlMsg.cmd == OVS_DP_CMD_SET) {
@@ -1350,19 +1342,16 @@ HandleDpTransactionCommon(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
goto cleanup;
}
} else if ((UINT32)msgIn->ovsHdr.dp_ifindex != gOvsSwitchContext->dpNo) {
- OvsReleaseCtrlLock();
nlError = NL_ERROR_NODEV;
goto cleanup;
}
if (usrParamsCtx->ovsMsg->genlMsg.cmd == OVS_DP_CMD_NEW) {
- OvsReleaseCtrlLock();
nlError = NL_ERROR_EXIST;
goto cleanup;
}
status = OvsDpFillInfo(gOvsSwitchContext, msgIn, &nlBuf);
- OvsReleaseCtrlLock();
*replyLen = NlBufSize(&nlBuf);
@@ -1444,7 +1433,6 @@ MapIrpOutputBuffer(PIRP irp,
* --------------------------------------------------------------------------
* Utility function to fill up information about the state of a port in a reply
* to* userspace.
- * Assumes that 'gOvsCtrlLock' lock is acquired.
* --------------------------------------------------------------------------
*/
static NTSTATUS
@@ -1548,8 +1536,6 @@ OvsReadEventCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
NlBufInit(&nlBuf, usrParamsCtx->outputBuffer, usrParamsCtx->outputLength);
- OvsAcquireCtrlLock();
-
/* remove an event entry from the event queue */
status = OvsRemoveEventEntry(usrParamsCtx->ovsInstance, &eventEntry);
if (status != STATUS_SUCCESS) {
@@ -1565,7 +1551,6 @@ OvsReadEventCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
}
cleanup:
- OvsReleaseCtrlLock();
return status;
}
diff --git a/datapath-windows/ovsext/Flow.c b/datapath-windows/ovsext/Flow.c
index f25fe9a..6fa10a3 100644
--- a/datapath-windows/ovsext/Flow.c
+++ b/datapath-windows/ovsext/Flow.c
@@ -31,7 +31,6 @@
#pragma warning( push )
#pragma warning( disable:4127 )
-extern PNDIS_SPIN_LOCK gOvsCtrlLock;
extern POVS_SWITCH_CONTEXT gOvsSwitchContext;
extern UINT64 ovsTimeIncrementPerTick;
@@ -1995,25 +1994,23 @@ OvsDoDumpFlows(OvsFlowDumpInput *dumpInput,
BOOLEAN findNextNonEmpty = FALSE;
dpNo = dumpInput->dpNo;
- NdisAcquireSpinLock(gOvsCtrlLock);
if (gOvsSwitchContext->dpNo != dpNo) {
status = STATUS_INVALID_PARAMETER;
- goto unlock;
+ goto exit;
}
rowIndex = dumpInput->position[0];
if (rowIndex >= OVS_FLOW_TABLE_SIZE) {
dumpOutput->n = 0;
*replyLen = sizeof(*dumpOutput);
- goto unlock;
+ goto exit;
}
columnIndex = dumpInput->position[1];
datapath = &gOvsSwitchContext->datapath;
ASSERT(datapath);
- ASSERT(KeGetCurrentIrql() == DISPATCH_LEVEL);
- OvsAcquireDatapathRead(datapath, &dpLockState, TRUE);
+ OvsAcquireDatapathRead(datapath, &dpLockState, FALSE);
head = &datapath->flowTable[rowIndex];
node = head->Flink;
@@ -2062,8 +2059,7 @@ OvsDoDumpFlows(OvsFlowDumpInput *dumpInput,
dp_unlock:
OvsReleaseDatapath(datapath, &dpLockState);
-unlock:
- NdisReleaseSpinLock(gOvsCtrlLock);
+exit:
return status;
}
@@ -2124,21 +2120,18 @@ OvsPutFlowIoctl(PVOID inputBuffer,
}
dpNo = put->dpNo;
- NdisAcquireSpinLock(gOvsCtrlLock);
if (gOvsSwitchContext->dpNo != dpNo) {
status = STATUS_INVALID_PARAMETER;
- goto unlock;
+ goto exit;
}
datapath = &gOvsSwitchContext->datapath;
ASSERT(datapath);
- ASSERT(KeGetCurrentIrql() == DISPATCH_LEVEL);
- OvsAcquireDatapathWrite(datapath, &dpLockState, TRUE);
+ OvsAcquireDatapathWrite(datapath, &dpLockState, FALSE);
status = HandleFlowPut(put, datapath, stats);
OvsReleaseDatapath(datapath, &dpLockState);
-unlock:
- NdisReleaseSpinLock(gOvsCtrlLock);
+exit:
return status;
}
@@ -2306,16 +2299,14 @@ OvsGetFlowIoctl(PVOID inputBuffer,
}
dpNo = getInput->dpNo;
- NdisAcquireSpinLock(gOvsCtrlLock);
if (gOvsSwitchContext->dpNo != dpNo) {
status = STATUS_INVALID_PARAMETER;
- goto unlock;
+ goto exit;
}
datapath = &gOvsSwitchContext->datapath;
ASSERT(datapath);
- ASSERT(KeGetCurrentIrql() == DISPATCH_LEVEL);
- OvsAcquireDatapathRead(datapath, &dpLockState, TRUE);
+ OvsAcquireDatapathRead(datapath, &dpLockState, FALSE);
flow = OvsLookupFlow(datapath, &getInput->key, &hash, FALSE);
if (!flow) {
status = STATUS_INVALID_PARAMETER;
@@ -2327,8 +2318,7 @@ OvsGetFlowIoctl(PVOID inputBuffer,
dp_unlock:
OvsReleaseDatapath(datapath, &dpLockState);
-unlock:
- NdisReleaseSpinLock(gOvsCtrlLock);
+exit:
return status;
}
@@ -2339,21 +2329,18 @@ OvsFlushFlowIoctl(UINT32 dpNo)
OVS_DATAPATH *datapath = NULL;
LOCK_STATE_EX dpLockState;
- NdisAcquireSpinLock(gOvsCtrlLock);
if (gOvsSwitchContext->dpNo != dpNo) {
status = STATUS_INVALID_PARAMETER;
- goto unlock;
+ goto exit;
}
datapath = &gOvsSwitchContext->datapath;
ASSERT(datapath);
- ASSERT(KeGetCurrentIrql() == DISPATCH_LEVEL);
- OvsAcquireDatapathWrite(datapath, &dpLockState, TRUE);
+ OvsAcquireDatapathWrite(datapath, &dpLockState, FALSE);
DeleteAllFlows(datapath);
OvsReleaseDatapath(datapath, &dpLockState);
-unlock:
- NdisReleaseSpinLock(gOvsCtrlLock);
+exit:
return status;
}
diff --git a/datapath-windows/ovsext/Switch.c b/datapath-windows/ovsext/Switch.c
index 9e1e9a4..4f784b8 100644
--- a/datapath-windows/ovsext/Switch.c
+++ b/datapath-windows/ovsext/Switch.c
@@ -35,10 +35,9 @@
#include "Debug.h"
POVS_SWITCH_CONTEXT gOvsSwitchContext;
-BOOLEAN gOvsInAttach;
+LONG volatile gOvsInAttach;
UINT64 ovsTimeIncrementPerTick;
-extern PNDIS_SPIN_LOCK gOvsCtrlLock;
extern NDIS_HANDLE gOvsExtDriverHandle;
extern NDIS_HANDLE gOvsExtDriverObject;
@@ -89,22 +88,18 @@ OvsExtAttach(NDIS_HANDLE ndisFilterHandle,
goto cleanup;
}
- NdisAcquireSpinLock(gOvsCtrlLock);
if (gOvsSwitchContext) {
- NdisReleaseSpinLock(gOvsCtrlLock);
OVS_LOG_TRACE("Exit: Failed to create OVS Switch, only one datapath is"
"supported, %p.", gOvsSwitchContext);
goto cleanup;
}
- if (gOvsInAttach) {
- NdisReleaseSpinLock(gOvsCtrlLock);
+
+ if (InterlockedCompareExchange(&gOvsInAttach, 1, 0)) {
/* Just fail the request. */
OVS_LOG_TRACE("Exit: Failed to create OVS Switch, since another attach"
"instance is in attach process.");
goto cleanup;
}
- gOvsInAttach = TRUE;
- NdisReleaseSpinLock(gOvsCtrlLock);
status = OvsInitIpHelper(ndisFilterHandle);
if (status != STATUS_SUCCESS) {
@@ -121,7 +116,7 @@ OvsExtAttach(NDIS_HANDLE ndisFilterHandle,
/*
* Register the switch context with NDIS so NDIS can pass it back to the
- * Filterxxx callback functions as the 'FilterModuleContext' parameter.
+ * FilterXXX callback functions as the 'FilterModuleContext' parameter.
*/
RtlZeroMemory(&ovsExtAttributes, sizeof(NDIS_FILTER_ATTRIBUTES));
ovsExtAttributes.Header.Revision = NDIS_FILTER_ATTRIBUTES_REVISION_1;
diff --git a/datapath-windows/ovsext/User.c b/datapath-windows/ovsext/User.c
index 03f0377..53c41f4 100644
--- a/datapath-windows/ovsext/User.c
+++ b/datapath-windows/ovsext/User.c
@@ -142,14 +142,12 @@ OvsCleanupPacketQueue(POVS_OPEN_INSTANCE instance)
}
/* Verify if gOvsSwitchContext exists. */
- OvsAcquireCtrlLock();
if (gOvsSwitchContext) {
/* Remove the instance from pidHashArray */
OvsAcquirePidHashLock();
OvsDelPidInstance(gOvsSwitchContext, instance->pid);
OvsReleasePidHashLock();
}
- OvsReleaseCtrlLock();
}
NTSTATUS
@@ -447,11 +445,9 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute)
OVS_PACKET_HDR_INFO layers;
POVS_VPORT_ENTRY vport;
- NdisAcquireSpinLock(gOvsCtrlLock);
-
if (execute->packetLen == 0) {
status = STATUS_INVALID_PARAMETER;
- goto unlock;
+ goto exit;
}
actions = execute->actions;
@@ -466,7 +462,7 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute)
execute->packetLen);
if (pNbl == NULL) {
status = STATUS_NO_MEMORY;
- goto unlock;
+ goto exit;
}
fwdDetail = NET_BUFFER_LIST_SWITCH_FORWARDING_DETAIL(pNbl);
@@ -481,11 +477,9 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute)
// XXX: Figure out if any of the other members of fwdDetail need to be set.
ndisStatus = OvsExtractFlow(pNbl, fwdDetail->SourcePortId, &key, &layers,
- NULL);
+ NULL);
if (ndisStatus == NDIS_STATUS_SUCCESS) {
- ASSERT(KeGetCurrentIrql() == DISPATCH_LEVEL);
- NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState,
- NDIS_RWL_AT_DISPATCH_LEVEL);
+ NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState, 0);
ndisStatus = OvsActionsExecute(gOvsSwitchContext, NULL, pNbl,
vport ? vport->portNo :
OVS_DEFAULT_PORT_NO,
@@ -506,8 +500,7 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute)
if (pNbl) {
OvsCompleteNBL(gOvsSwitchContext, pNbl, TRUE);
}
-unlock:
- NdisReleaseSpinLock(gOvsCtrlLock);
+exit:
return status;
}
@@ -630,7 +623,6 @@ OvsGetNextPacket(POVS_OPEN_INSTANCE instance)
/*
* ---------------------------------------------------------------------------
* Given a pid, returns the corresponding USER_PACKET_QUEUE.
- * gOvsCtrlLock must be acquired before calling this API.
* ---------------------------------------------------------------------------
*/
POVS_USER_PACKET_QUEUE
diff --git a/datapath-windows/ovsext/Vport.c b/datapath-windows/ovsext/Vport.c
index f46a0ac..1423ace 100644
--- a/datapath-windows/ovsext/Vport.c
+++ b/datapath-windows/ovsext/Vport.c
@@ -48,7 +48,6 @@
#define OVS_VPORT_DEFAULT_WAIT_TIME_MICROSEC 100
extern POVS_SWITCH_CONTEXT gOvsSwitchContext;
-extern PNDIS_SPIN_LOCK gOvsCtrlLock;
static VOID OvsInitVportWithPortParam(POVS_VPORT_ENTRY vport,
PNDIS_SWITCH_PORT_PARAMETERS portParam);
@@ -1366,8 +1365,6 @@ OvsConvertIfCountedStrToAnsiStr(PIF_COUNTED_STRING wStr,
* --------------------------------------------------------------------------
* Utility function that populates a 'OVS_VPORT_EXT_INFO' structure for the
* specified vport.
- *
- * Assumes that 'gOvsCtrlLock' is held.
* --------------------------------------------------------------------------
*/
NTSTATUS
@@ -1381,9 +1378,7 @@ OvsGetExtInfoIoctl(POVS_VPORT_GET vportGet,
BOOLEAN doConvert = FALSE;
RtlZeroMemory(extInfo, sizeof (POVS_VPORT_EXT_INFO));
- ASSERT(KeGetCurrentIrql() == DISPATCH_LEVEL);
- NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState,
- NDIS_RWL_AT_DISPATCH_LEVEL);
+ NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState, 0);
if (vportGet->portNo == 0) {
StringCbLengthA(vportGet->name, OVS_MAX_PORT_NAME_LENGTH - 1, &len);
vport = OvsFindVportByHvNameA(gOvsSwitchContext, vportGet->name);
@@ -1511,8 +1506,6 @@ OvsGetNetdevCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
return STATUS_INVALID_PARAMETER;
}
- OvsAcquireCtrlLock();
-
vportGet.portNo = 0;
RtlCopyMemory(&vportGet.name, NlAttrGet(netdevAttrs[OVS_VPORT_ATTR_NAME]),
NlAttrGetSize(netdevAttrs[OVS_VPORT_ATTR_NAME]));
@@ -1520,7 +1513,6 @@ OvsGetNetdevCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
status = OvsGetExtInfoIoctl(&vportGet, &info);
if (status == STATUS_DEVICE_DOES_NOT_EXIST) {
nlError = NL_ERROR_NODEV;
- OvsReleaseCtrlLock();
goto cleanup;
}
@@ -1530,7 +1522,6 @@ OvsGetNetdevCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
if (status == STATUS_SUCCESS) {
*replyLen = msgOut->nlMsg.nlmsgLen;
}
- OvsReleaseCtrlLock();
cleanup:
if (nlError != NL_ERROR_SUCCESS) {
@@ -1737,17 +1728,13 @@ OvsGetVportDumpNext(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
msgIn = instance->dumpState.ovsMsg;
- OvsAcquireCtrlLock();
-
/*
* XXX: when we implement OVS_DP_ATTR_USER_FEATURES in datapath,
* we'll need to check the OVS_DP_F_VPORT_PIDS flag: if it is set,
* it means we have an array of pids, instead of a single pid.
* ATM we assume we have one pid only.
*/
- ASSERT(KeGetCurrentIrql() == DISPATCH_LEVEL);
- NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState,
- NDIS_RWL_AT_DISPATCH_LEVEL);
+ NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState, 0);
if (gOvsSwitchContext->numHvVports > 0 ||
gOvsSwitchContext->numNonHvVports > 0) {
@@ -1808,8 +1795,6 @@ OvsGetVportDumpNext(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState);
- OvsReleaseCtrlLock();
-
/* if i < OVS_MAX_VPORT_ARRAY_SIZE => vport was found */
if (i < OVS_MAX_VPORT_ARRAY_SIZE) {
POVS_MESSAGE msgOut = (POVS_MESSAGE)usrParamsCtx->outputBuffer;
@@ -2186,8 +2171,6 @@ OvsSetVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
/* Output buffer has been validated while validating transact dev op. */
ASSERT(msgOut != NULL && usrParamsCtx->outputLength >= sizeof *msgOut);
- OvsAcquireCtrlLock();
-
NdisAcquireRWLockWrite(gOvsSwitchContext->dispatchLock, &lockState, 0);
if (vportAttrs[OVS_VPORT_ATTR_NAME] != NULL) {
PSTR portName = NlAttrGet(vportAttrs[OVS_VPORT_ATTR_NAME]);
@@ -2240,7 +2223,6 @@ OvsSetVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
Cleanup:
NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState);
- OvsReleaseCtrlLock();
if (nlError != NL_ERROR_SUCCESS) {
POVS_MESSAGE_ERROR msgError = (POVS_MESSAGE_ERROR)
--
1.9.0.msysgit.0
More information about the dev
mailing list