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:elia...@vmware.com] Sent: Thursday, October 2, 2014 7:02 PM To: Sorin Vinturis; dev@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-boun...@openvswitch.org] On Behalf Of Sorin Vinturis Sent: Thursday, October 02, 2014 1:53 AM To: dev@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 <svintu...@cloudbasesolutions.com> Reported-by: Sorin Vinturis <svintu...@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@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 _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
