Hi Nithin, The method to protect the ‘gOvsSwitchContext’ using the ‘gOvsControlLock’ is inefficient and inappropriate. Using the ‘gOvsControlLock’ to access the ‘gOvsSwitchContext’ makes worthless the use of the other locks, like dispatchLock or flow table lock that protects data contained in the ‘gOvsSwitchContext’ structure. Because the caller that acquired the spin lock gains exclusive access to the resource. So this is not a proper way to protect the ‘gOvsSwitchContext’ against deallocation. A proper way is the use of a reference count, which was already done in a previous patch.
In the filter attach routine, to protect against multiple calls, indeed we need to test and set if the instance is in attach process in one step. I will add a v2 patch with the necessary change. Thanks, Sorin -----Original Message----- From: Nithin Raju [mailto:nit...@vmware.com] Sent: Monday, 13 April, 2015 18:52 To: Sorin Vinturis Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH] datapath-windows: Removed gOvsCtrlLock global spinlock 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_ma > ilman_listinfo_dev&d=AwIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt- > uEs&r=pNHQcdr7B40b4h6Yb7FIedI1dnBsxdDuTLBYD3JqV80&m=1woBzhbMDqN8Ig5JKS > Qh2axrvBiwf2lmLQPpz6r7c1o&s=ynOFIczTMLXTkhSMpv-PYjU8MIE9x28kdHrP7gX1iP > 0&e= _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
