Hi Sorin, Sounds fair. I will introduce a new lock in v2.
Thanks. Regards, Ankur ________________________________________ From: Sorin Vinturis <svintu...@cloudbasesolutions.com> Sent: Monday, October 13, 2014 11:26 PM To: Ankur Sharma; dev@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-boun...@openvswitch.org] On Behalf Of Ankur Sharma Sent: Tuesday, October 14, 2014 8:40 AM To: dev@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 <ankursha...@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@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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
