diff mbox

[ovs-dev] datapath-windows: Avoid busy wait in OvsStartIpHelper

Message ID DM5PR05MB319365AF4C1260427FAA8868A2B40@DM5PR05MB3193.namprd05.prod.outlook.com
State Superseded
Headers show

Commit Message

Shashank Ram Nov. 22, 2016, 11:48 p.m. UTC 
Hi Yin, looking at the following code, there is a lock that needs to be acquired prior to incrementing the 'ovsNumIpHelperRequests' counter. So consider the following:


1. At the start, ovsNumIpHelperRequests = 0 & IPHelper thread is sleeping

2. OvsInternalAdapterUp() is invoked, IPHelper lock is acquired, ovsNumIpHelperRequests is incremented to 1, IP Helper thread is woken up, ovsNumIpHelperRequests is decremented to 0, IPHelper lock is released.

3. If OvsInternalAdapterUp() is called during step 2., then it will wait for the lock to be released, which is after the ovsNumIpHelperRequests is set to 0.


So I don't see how the ovsNumIpHelperRequests can ever be > 1.


Thanks,

Shashank
diff mbox

Patch

diff --git a/datapath-windows/ovsext/IpHelper.c b/datapath-windows/ovsext/IpHelper.c
index d747e8c..559ddaa 100644
--- a/datapath-windows/ovsext/IpHelper.c
+++ b/datapath-windows/ovsext/IpHelper.c
@@ -1455,12 +1455,14 @@  OvsStartIpHelper(PVOID data)
     POVS_IPNEIGH_ENTRY ipn;
     PLIST_ENTRY link;
     UINT64   timeVal, timeout;
+    PLARGE_INTEGER threadSleepTimeout;

     OVS_LOG_INFO("Start the IP Helper Thread, context: %p", context);

     NdisAcquireSpinLock(&ovsIpHelperLock);
     while (!context->exit) {

+        threadSleepTimeout = NULL;
         timeout = 0;
         while (!IsListEmpty(&ovsIpHelperRequestList)) {
             if (context->exit) {
@@ -1468,6 +1470,7 @@  OvsStartIpHelper(PVOID data)
             }
             link = ovsIpHelperRequestList.Flink;
             RemoveEntryList(link);
+            ovsNumIpHelperRequests--;
             NdisReleaseSpinLock(&ovsIpHelperLock);
             req = CONTAINING_RECORD(link, OVS_IP_HELPER_REQUEST, link);
             switch (req->command) {
@@ -1497,6 +1500,7 @@  OvsStartIpHelper(PVOID data)
             KeQuerySystemTime((LARGE_INTEGER *)&timeVal);
             if (ipn->timeout > timeVal) {
                 timeout = ipn->timeout;
+                threadSleepTimeout = (PLARGE_INTEGER)&timeout;
                 break;
             }
             ipAddr = ipn->ipAddr;
@@ -1519,8 +1523,13 @@  ip_helper_wait:
         KeClearEvent(&context->event);
         NdisReleaseSpinLock(&ovsIpHelperLock);

+        /*
+         * Wait indefinitely for the thread to be woken up.
+         * Passing NULL as the Timeout value in the below
+         * call to KeWaitForSingleObject achieves this.
+         */
         KeWaitForSingleObject(&context->event, Executive, KernelMode,
-                              FALSE, (LARGE_INTEGER *)&timeout);
+                              FALSE, threadSleepTimeout);
         NdisAcquireSpinLock(&ovsIpHelperLock);
     }
     NdisReleaseSpinLock(&ovsIpHelperLock);