Sorin,
In general, ‘gOvsControlLock’ is there to protect ‘gOvsSwitchContext’.
Basically, while we are accessing ‘gOvsSwitchContext’ from the netlink path or
even during an NDIS switch callback, we want to make sure ‘gOvsSwitchContext’
doesn’t get deallocated under the rug in another context.
Also, if you look in Switch.c, ‘gOvsControlLock’ protects against multiple
attach handlers (on different switches) being called concurrently.
With your change what would be the mechanism to ensure there protections?
Also, would it be possible to figure out how expensive (in CPU cost)
‘gOvsControlLock’ is at all, and how much we save by removing it?
thanks,
-- Nithin
> On Mar 23, 2015, at 6:43 AM, Sorin Vinturis
> <svintu...@cloudbasesolutions.com> wrote:
>
> 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.
>
> Signed-off-by: Sorin Vinturis <svintu...@cloudbasesolutions.com>
> ---
> datapath-windows/ovsext/Datapath.c | 15 ---------------
> datapath-windows/ovsext/Flow.c | 39 +++++++++++++-------------------------
> datapath-windows/ovsext/Switch.c | 5 -----
> datapath-windows/ovsext/User.c | 22 ++++-----------------
> datapath-windows/ovsext/Vport.c | 22 ++-------------------
> 5 files changed, 19 insertions(+), 84 deletions(-)
>
> diff --git a/datapath-windows/ovsext/Datapath.c
> b/datapath-windows/ovsext/Datapath.c
> index 8b561a3..6a76a12 100644
> --- a/datapath-windows/ovsext/Datapath.c
> +++ b/datapath-windows/ovsext/Datapath.c
> @@ -918,14 +918,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. */
> @@ -1012,7 +1009,6 @@ OvsGetPidCmdHandler(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
> @@ -1212,9 +1208,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;
> @@ -1301,11 +1295,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) {
> @@ -1317,19 +1309,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);
>
> @@ -1411,7 +1400,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
> @@ -1515,8 +1503,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) {
> @@ -1532,7 +1518,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 d3de8cc..1f1bd2f 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;
> }
>
> @@ -2305,16 +2298,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;
> @@ -2326,8 +2317,7 @@ OvsGetFlowIoctl(PVOID inputBuffer,
>
> dp_unlock:
> OvsReleaseDatapath(datapath, &dpLockState);
> -unlock:
> - NdisReleaseSpinLock(gOvsCtrlLock);
> +exit:
> return status;
> }
>
> @@ -2338,21 +2328,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 8366944..0c40916 100644
> --- a/datapath-windows/ovsext/Switch.c
> +++ b/datapath-windows/ovsext/Switch.c
> @@ -38,7 +38,6 @@ POVS_SWITCH_CONTEXT gOvsSwitchContext;
> BOOLEAN gOvsInAttach;
> UINT64 ovsTimeIncrementPerTick;
>
> -extern PNDIS_SPIN_LOCK gOvsCtrlLock;
> extern NDIS_HANDLE gOvsExtDriverHandle;
> extern NDIS_HANDLE gOvsExtDriverObject;
>
> @@ -83,22 +82,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);
> /* 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) {
> diff --git a/datapath-windows/ovsext/User.c b/datapath-windows/ovsext/User.c
> index d8a657e..efe52b1 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
> @@ -163,12 +161,6 @@ OvsSubscribeDpIoctl(PVOID instanceP,
> if (instance->packetQueue && !join) {
> /* unsubscribe */
> OvsCleanupPacketQueue(instance);
> -
> - OvsAcquirePidHashLock();
> - /* Remove the instance from pidHashArray */
> - OvsDelPidInstance(gOvsSwitchContext, pid);
> - OvsReleasePidHashLock();
> -
> } else if (instance->packetQueue == NULL && join) {
> queue = (POVS_USER_PACKET_QUEUE) OvsAllocateMemory(sizeof *queue);
> if (queue == NULL) {
> @@ -446,11 +438,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;
> @@ -465,7 +455,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);
> @@ -482,9 +472,7 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute)
> ndisStatus = OvsExtractFlow(pNbl, fwdDetail->SourcePortId, &key, &layers,
> 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,
> @@ -505,8 +493,7 @@ OvsExecuteDpIoctl(OvsPacketExecute *execute)
> if (pNbl) {
> OvsCompleteNBL(gOvsSwitchContext, pNbl, TRUE);
> }
> -unlock:
> - NdisReleaseSpinLock(gOvsCtrlLock);
> +exit:
> return status;
> }
>
> @@ -629,7 +616,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 c9dfaea..cfe757d 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);
> @@ -1364,8 +1363,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
> @@ -1379,9 +1376,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);
> @@ -1509,8 +1504,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]));
> @@ -1518,7 +1511,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;
> }
>
> @@ -1528,7 +1520,6 @@ OvsGetNetdevCmdHandler(POVS_USER_PARAMS_CONTEXT
> usrParamsCtx,
> if (status == STATUS_SUCCESS) {
> *replyLen = msgOut->nlMsg.nlmsgLen;
> }
> - OvsReleaseCtrlLock();
>
> cleanup:
> if (nlError != NL_ERROR_SUCCESS) {
> @@ -1735,17 +1726,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) {
> @@ -1806,8 +1793,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;
> @@ -2184,8 +2169,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]);
> @@ -2238,7 +2221,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
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_mailman_listinfo_dev&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=1woBzhbMDqN8Ig5JKSQh2axrvBiwf2lmLQPpz6r7c1o&s=ynOFIczTMLXTkhSMpv-PYjU8MIE9x28kdHrP7gX1iP0&e=
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
