[ovs-dev] [PATCH v1 1/2] datapath-windows: Fix CtrlLock acquire issue in OvsReadEventCmdHandler

Ankur Sharma ankursharma at vmware.com
Tue Oct 14 05:39:59 UTC 2014


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




More information about the dev mailing list