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

[Nithin Raju]

On Oct 3, 2014, at 12:37 AM, Sorin Vinturis <svinturis at cloudbasesolutions.com>
 wrote:

> 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: https://urldefense.proofpoint.com/v1/url?u=http://msdn.microsoft.com/en-us/library/windows/hardware/ff560697%28v%3Dvs.85%29.aspx&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=ubrOpIWavCMqX4l4j1LEVpTfDj%2FD5Qyn8KCoJIBGvzo%3D%0A&m=a0rSVp89vsxBPpDuejln00QgXe6PJJKGE7MaJ1cFXJc%3D%0A&s=798f5a32837de23aadcd16541678df71f50e0f6e0dd1a0e3b46dacdbf4980708.
> 
> 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.

Sorin,
ASSERT() are active only in debug builds and will be a no-op in release builds. So, when we do want to be concerned about performance, ASSERTs are out of the way. So, ASSERT is useful to catch any coding errors about assumptions made. Here we've assumed that we have acquired the CtrlLock. If that assumption changes for some reason, this ASSERT will help us catch that case.

-- Nithin