[ovs-dev] [PATCH v1 1/2] datapath-windows: Fix CtrlLock acquire issue in OvsReadEventCmdHandler
Ankur Sharma
ankursharma at vmware.com
Tue Oct 14 15:59:57 UTC 2014
Hi Sorin,
Sounds fair. I will introduce a new lock in v2.
Thanks.
Regards,
Ankur
________________________________________
From: Sorin Vinturis <svinturis at cloudbasesolutions.com>
Sent: Monday, October 13, 2014 11:26 PM
To: Ankur Sharma; dev at openvswitch.org
Subject: RE: [ovs-dev] [PATCH v1 1/2] datapath-windows: Fix CtrlLock acquire issue in OvsReadEventCmdHandler
Hi Ankur,
In the Event.c the CtrlLock was used for event queue manipulation through the use of the OvsAcquireEventQueueLock and OvsReleaseEventQueueLock functions that you removed in this patch. But the CtrlLock should be used to guard only one shared resource, and is already used for guarding the datapath. So I propose here to keep the OvsAcquireEventQueueLock and OvsReleaseEventQueueLock functions, but use another lock for guarding the event queue.
What do you think?
Thanks,
Sorin
-----Original Message-----
From: dev [mailto:dev-bounces at openvswitch.org] On Behalf Of Ankur Sharma
Sent: Tuesday, October 14, 2014 8:40 AM
To: dev at openvswitch.org
Subject: [ovs-dev] [PATCH v1 1/2] datapath-windows: Fix CtrlLock acquire issue in OvsReadEventCmdHandler
OvsReadEventCmdHandler was calling OvsRemoveEventEntry after acquiring CtrlLock. OvsRemoveEventEntry in turn also tries to acquire the same lock.
Fixed the same.
Also, in Event.c we removed the APIs OvsAcquireEventQueueLock and OvsReleaseEventQueueLock. These APIs were acquiring and releasing CtrlLock only. Apis OvsAcquireCtrlLock and OvsReleaseCtrlLock should be used for this purpose.
Signed-off-by: Ankur Sharma <ankursharma at vmware.com>
---
datapath-windows/ovsext/Datapath.c | 3 ++-
datapath-windows/ovsext/Event.c | 54 ++++++++++++++++----------------------
2 files changed, 25 insertions(+), 32 deletions(-)
diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c
index 6c78ab8..cb391a6 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -2133,9 +2133,11 @@ OvsReadEventCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
OvsAcquireCtrlLock();
if (!gOvsSwitchContext) {
+ OvsReleaseCtrlLock();
status = STATUS_SUCCESS;
goto cleanup;
}
+ OvsReleaseCtrlLock();
/* remove an event entry from the event queue */
status = OvsRemoveEventEntry(usrParamsCtx->ovsInstance, &eventEntry); @@ -2149,7 +2151,6 @@ OvsReadEventCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
}
cleanup:
- OvsReleaseCtrlLock();
return status;
}
#endif /* OVS_USE_NL_INTERFACE */
diff --git a/datapath-windows/ovsext/Event.c b/datapath-windows/ovsext/Event.c index 467771d..0a45f24 100644
--- a/datapath-windows/ovsext/Event.c
+++ b/datapath-windows/ovsext/Event.c
@@ -47,18 +47,6 @@ OvsCleanupEventQueue()
ASSERT(ovsNumEventQueue == 0);
}
-static __inline VOID
-OvsAcquireEventQueueLock()
-{
- NdisAcquireSpinLock(gOvsCtrlLock);
-}
-
-static __inline VOID
-OvsReleaseEventQueueLock()
-{
- NdisReleaseSpinLock(gOvsCtrlLock);
-}
-
/*
* --------------------------------------------------------------------------
* Cleanup the event queue of the OpenInstance.
@@ -74,7 +62,7 @@ OvsCleanupEvent(POVS_OPEN_INSTANCE instance)
POVS_EVENT_QUEUE_ELEM elem;
PLIST_ENTRY link, next;
- OvsAcquireEventQueueLock();
+ OvsAcquireCtrlLock();
RemoveEntryList(&queue->queueLink);
ovsNumEventQueue--;
if (queue->pendingIrp) {
@@ -87,7 +75,7 @@ OvsCleanupEvent(POVS_OPEN_INSTANCE instance)
}
}
instance->eventQueue = NULL;
- OvsReleaseEventQueueLock();
+ OvsReleaseCtrlLock();
if (irp) {
OvsCompleteIrpRequest(irp, 0, STATUS_SUCCESS);
}
@@ -125,7 +113,7 @@ OvsPostEvent(UINT32 portNo,
OVS_LOG_TRACE("Enter: portNo: %#x, status: %#x", portNo, status);
- OvsAcquireEventQueueLock();
+ OvsAcquireCtrlLock();
LIST_FORALL(&ovsEventQueue, link) {
queue = CONTAINING_RECORD(link, OVS_EVENT_QUEUE, queueLink); @@ -170,7 +158,7 @@ OvsPostEvent(UINT32 portNo,
}
}
}
- OvsReleaseEventQueueLock();
+ OvsReleaseCtrlLock();
while (!IsListEmpty(&list)) {
entry = RemoveHeadList(&list);
irp = CONTAINING_RECORD(entry, IRP, Tail.Overlay.ListEntry); @@ -214,7 +202,7 @@ OvsSubscribeEventIoctl(PFILE_OBJECT fileObject,
return STATUS_INVALID_PARAMETER;
}
- OvsAcquireEventQueueLock();
+ OvsAcquireCtrlLock();
instance = OvsGetOpenInstance(fileObject, request->dpNo);
@@ -276,7 +264,7 @@ done_event_subscribe:
irp = NULL;
}
}
- OvsReleaseEventQueueLock();
+ OvsReleaseCtrlLock();
if (irp) {
OvsCompleteIrpRequest(queue->pendingIrp, 0, STATUS_SUCCESS);
}
@@ -286,7 +274,7 @@ done_event_subscribe:
}
OvsFreeMemory(queue);
} else {
- OvsReleaseEventQueueLock();
+ OvsReleaseCtrlLock();
}
OVS_LOG_TRACE("Exit: subscribe event with status: %#x.", status);
return status;
@@ -337,10 +325,11 @@ OvsPollEventIoctl(PFILE_OBJECT fileObject,
}
poll = (POVS_EVENT_POLL)inputBuffer;
- OvsAcquireEventQueueLock();
+ /* XXX: Verify if we really need a global lock here */
+ OvsAcquireCtrlLock();
instance = OvsGetOpenInstance(fileObject, poll->dpNo);
if (instance == NULL) {
- OvsReleaseEventQueueLock();
+ OvsReleaseCtrlLock();
*replyLen = 0;
OVS_LOG_TRACE("Exit: can not find Open instance");
return STATUS_INVALID_PARAMETER; @@ -371,7 +360,7 @@ OvsPollEventIoctl(PFILE_OBJECT fileObject,
queue->numElems--;
}
event_poll_done:
- OvsReleaseEventQueueLock();
+ OvsReleaseCtrlLock();
*replyLen = sizeof (OVS_EVENT_STATUS) +
numEntry * sizeof (OVS_EVENT_ENTRY);
OVS_LOG_TRACE("Exit: numEventPolled: %d", numEntry); @@ -409,17 +398,19 @@ OvsCancelIrp(PDEVICE_OBJECT deviceObject,
if (fileObject == NULL) {
goto done;
}
- OvsAcquireEventQueueLock();
+
+ /* XXX: Verify if we really need a global lock here */
+ OvsAcquireCtrlLock();
instance = (POVS_OPEN_INSTANCE)fileObject->FsContext;
if (instance == NULL || instance->eventQueue == NULL) {
- OvsReleaseEventQueueLock();
+ OvsReleaseCtrlLock();
goto done;
}
queue = instance->eventQueue;
if (queue->pendingIrp == irp) {
queue->pendingIrp = NULL;
}
- OvsReleaseEventQueueLock();
+ OvsReleaseCtrlLock();
done:
OvsCompleteIrpRequest(irp, 0, STATUS_CANCELLED); } @@ -457,18 +448,19 @@ OvsWaitEventIoctl(PIRP irp,
}
poll = (POVS_EVENT_POLL)inputBuffer;
- OvsAcquireEventQueueLock();
+ /* XXX: Verify if we really need a global lock here */
+ OvsAcquireCtrlLock();
instance = OvsGetOpenInstance(fileObject, poll->dpNo);
if (instance == NULL) {
- OvsReleaseEventQueueLock();
+ OvsReleaseCtrlLock();
OVS_LOG_TRACE("Exit: Can not find open instance, dpNo: %d", poll->dpNo);
return STATUS_INVALID_PARAMETER;
}
queue = (POVS_EVENT_QUEUE)instance->eventQueue;
if (queue->pendingIrp) {
- OvsReleaseEventQueueLock();
+ OvsReleaseCtrlLock();
OVS_LOG_TRACE("Exit: Event queue already in pending state");
return STATUS_DEVICE_BUSY;
}
@@ -488,7 +480,7 @@ OvsWaitEventIoctl(PIRP irp,
queue->pendingIrp = irp;
}
}
- OvsReleaseEventQueueLock();
+ OvsReleaseCtrlLock();
if (cancelled) {
OvsCompleteIrpRequest(irp, 0, STATUS_CANCELLED);
OVS_LOG_INFO("Event IRP cancelled: %p", irp); @@ -514,7 +506,7 @@ OvsRemoveEventEntry(POVS_OPEN_INSTANCE instance,
POVS_EVENT_QUEUE queue;
POVS_EVENT_QUEUE_ELEM elem;
- OvsAcquireEventQueueLock();
+ OvsAcquireCtrlLock();
queue = (POVS_EVENT_QUEUE)instance->eventQueue;
@@ -533,6 +525,6 @@ OvsRemoveEventEntry(POVS_OPEN_INSTANCE instance,
}
remove_event_done:
- OvsReleaseEventQueueLock();
+ OvsReleaseCtrlLock();
return status;
}
--
1.9.1
_______________________________________________
dev mailing list
dev at openvswitch.org
https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=f6EhnZ0ORGZNt5QbYmRaOxfWfx%2Bqd3KEiPf3%2FYaollU%3D%0A&m=DTYEJ5sq67Kr6Kg2jBuTlMx5oIt80fHn2zcxwTQ0vY0%3D%0A&s=2e6fb075f20fdbf19123c24b1b067470909c4f6f4e112e39d9dce7a044c12f5d
More information about the dev
mailing list