[ovs-dev] [PATCH v2] datapath-windows: Incorrect assumption of the IRQL

[Sorin Vinturis]

Hi Eitan,

The reason for pushing this patch was to optimize the code when obtaining a lock. Thus I have set the flag parameters of NdisAcquireRWLockXXX() functions  to appropriate values, i.e. NDIS_RWL_AT_DISPATCH_LEVEL, each time we know for sure that the current IRQL is DISPATCH_LEVEL; for example, when we already acquired a spin lock.

The NdisAcquireRWLockRead() function checks the current IRQL if the flag parameter is set to 0. And the check performed by the latter function is more efficient then KeGetCurrentIrql(). Please see NdisAcquireRWLockRead function documentation from here: http://msdn.microsoft.com/en-us/library/windows/hardware/ff560697(v=vs.85).aspx.

ASSERT(KeGetCurrentIrql() == DISPATCH_LEVEL);

The assert above calls the KeGetCurrentIrql() to obtain the current IRQL. But instead of using the above assert, we can just set the flag parameter of the NdisAcquireRWLockRead() function to 0, because the IRQL is checked more efficient. This was the case in the original code so we might just drop this patch if this solution is preferred.

I understand that the requested assert is used just to be cautious, but this adds an useless overhead.

I prefer to drop the use of the above asserts, when we are inside a spin lock, and be careful, when the spin lock is removed, to verify that the flag parameters of the NdisAcquireRWLockXXX() functions are correctly set.

Regards,
Sorin

-----Original Message-----
From: Eitan Eliahu [mailto:eliahue at vmware.com] 
Sent: Thursday, October 2, 2014 7:02 PM
To: Sorin Vinturis; dev at openvswitch.org
Subject: RE: [ovs-dev] [PATCH v2] datapath-windows: Incorrect assumption of the IRQL


Hi Sorin,
Is it possible to assert on the IRQL before we assume DISPATCH_LEVEL:

+    NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState,
+        NDIS_RWL_AT_DISPATCH_LEVEL);

Mt concern that we might remove the first lock or we might change order and we will come with the wrong IRQL.
Thanks,
Eitan

-----Original Message-----
From: dev [mailto:dev-bounces at openvswitch.org] On Behalf Of Sorin Vinturis
Sent: Thursday, October 02, 2014 1:53 AM
To: dev at openvswitch.org
Subject: [ovs-dev] [PATCH v2] datapath-windows: Incorrect assumption of the IRQL

Acquiring a spin lock raises the IRQL to DISPATCH_LEVEL. But in many places of the code, while holding a spin lock, there are useless checks for the current IRQL against DISPATCH_LEVEL.
Also, the dispatch flag is not correctly set when calling
NdisAcquireRWLockXXX() functions, which causes an extra check of the current IRQL.

Signed-off-by: Sorin Vinturis <svinturis at cloudbasesolutions.com>
Reported-by: Sorin Vinturis <svinturis at cloudbasesolutions.com>
Reported-at: https://urldefense.proofpoint.com/v1/url?u=https://github.com/openvswitch/ovs-issues/issues/47&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=yTvML8OxA42Jb6ViHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=0WWU%2Fzo6SqL595SrG7V3y0gytSAVoWAaZGTf36TpGog%3D%0A&s=91a7b2ccaf5971e25049fd5b24450729d6ae1671146826b52d4a0cc03e331b52
---
 datapath-windows/ovsext/Datapath.c | 3 ++-
 datapath-windows/ovsext/Flow.c     | 9 +++++----
 datapath-windows/ovsext/PacketIO.c | 2 +-
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/datapath-windows/ovsext/Datapath.c b/datapath-windows/ovsext/Datapath.c
index 44cdfc9..96f2180 100644
--- a/datapath-windows/ovsext/Datapath.c
+++ b/datapath-windows/ovsext/Datapath.c
@@ -1398,7 +1398,8 @@ OvsGetVportDumpNext(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
      * ATM we assume we have one pid only.
     */
 
-    NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState, 0);
+    NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState,
+        NDIS_RWL_AT_DISPATCH_LEVEL);
 
     if (gOvsSwitchContext->numVports > 0) {
         /* inBucket: the bucket, used for lookup */ diff --git a/datapath-windows/ovsext/Flow.c b/datapath-windows/ovsext/Flow.c index 5cab6e1..0fad079 100644
--- a/datapath-windows/ovsext/Flow.c
+++ b/datapath-windows/ovsext/Flow.c
@@ -1150,7 +1150,8 @@ OvsDoDumpFlows(OvsFlowDumpInput *dumpInput,
 
     datapath = &gOvsSwitchContext->datapath;
     ASSERT(datapath);
-    OvsAcquireDatapathRead(datapath, &dpLockState, FALSE);
+    ASSERT(KeGetCurrentIrql() == DISPATCH_LEVEL);
+    OvsAcquireDatapathRead(datapath, &dpLockState, TRUE);
 
     head = &datapath->flowTable[rowIndex];
     node = head->Flink;
@@ -1297,7 +1298,7 @@ OvsPutFlowIoctl(PVOID inputBuffer,
 
     datapath = &gOvsSwitchContext->datapath;
     ASSERT(datapath);
-    OvsAcquireDatapathWrite(datapath, &dpLockState, FALSE);
+    OvsAcquireDatapathWrite(datapath, &dpLockState, TRUE);
     status = HandleFlowPut(put, datapath, stats);
     OvsReleaseDatapath(datapath, &dpLockState);
 
@@ -1489,7 +1490,7 @@ OvsGetFlowIoctl(PVOID inputBuffer,
 
     datapath = &gOvsSwitchContext->datapath;
     ASSERT(datapath);
-    OvsAcquireDatapathRead(datapath, &dpLockState, FALSE);
+    OvsAcquireDatapathRead(datapath, &dpLockState, TRUE);
     flow = OvsLookupFlow(datapath, &getInput->key, &hash, FALSE);
     if (!flow) {
         status = STATUS_INVALID_PARAMETER; @@ -1524,7 +1525,7 @@ OvsFlushFlowIoctl(UINT32 dpNo)
 
     datapath = &gOvsSwitchContext->datapath;
     ASSERT(datapath);
-    OvsAcquireDatapathWrite(datapath, &dpLockState, FALSE);
+    OvsAcquireDatapathWrite(datapath, &dpLockState, TRUE);
     DeleteAllFlows(datapath);
     OvsReleaseDatapath(datapath, &dpLockState);
 
diff --git a/datapath-windows/ovsext/PacketIO.c b/datapath-windows/ovsext/PacketIO.c
index ac7862d..87d7037 100644
--- a/datapath-windows/ovsext/PacketIO.c
+++ b/datapath-windows/ovsext/PacketIO.c
@@ -268,7 +268,7 @@ OvsStartNBLIngress(POVS_SWITCH_CONTEXT switchContext,
             }
 
             ASSERT(KeGetCurrentIrql() == DISPATCH_LEVEL);
-            OvsAcquireDatapathRead(datapath, &dpLockState, dispatch);
+            OvsAcquireDatapathRead(datapath, &dpLockState, TRUE);
 
             flow = OvsLookupFlow(datapath, &key, &hash, FALSE);
             if (flow) {
--
1.9.0.msysgit.0
_______________________________________________
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=yTvML8OxA42Jb6ViHe7fUXbvPVOYDPVq87w43doxtlY%3D%0A&m=0WWU%2Fzo6SqL595SrG7V3y0gytSAVoWAaZGTf36TpGog%3D%0A&s=f9dd8af50a47fb88ce79c74fb070923f505fdde565a52320d1db7a4c5217fc12