Hi Eitan, Sure, i will take care of this. I thought that CtrlLock is used for all the global resources.
Thanks. Regards, Ankur ________________________________________ From: Eitan Eliahu Sent: Tuesday, October 14, 2014 5:56 AM To: Ankur Sharma; dev@openvswitch.org Subject: RE: [ovs-dev] [PATCH v1 1/2] datapath-windows: Fix CtrlLock acquire issue in OvsReadEventCmdHandler Hi Ankur, The event lock should protect only the event queue. It should not be confused with the other lock which protect the user mode interface (CtrlLock). I would suggest to use a specific lock to protect the event queue rather using the CtrlLock. -static __inline VOID -OvsAcquireEventQueueLock() -{ - NdisAcquireSpinLock(gOvsCtrlLock); <--- -} - -static __inline VOID -OvsReleaseEventQueueLock() -{ - NdisReleaseSpinLock(gOvsCtrlLock); <--- -} - Thanks, Eitan -----Original Message----- From: dev [mailto:dev-boun...@openvswitch.org] On Behalf Of Ankur Sharma Sent: Monday, October 13, 2014 10:40 PM 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=yTvML8OxA42Jb6ViHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=Ndvu0fTzGpTOCQwS1aXuKAXPopZjNHIQu%2FUroKhoXWQ%3D%0A&s=617cdf4f3c108cdbf014c1e082bfbb61a34f22ae06c0d8731c549894117c93bc _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev