Hi All,
I'm working on moving from WDK 6000 to 7100.0.0, and maybe just ran into a prefast issue, or else I'm missing some deep subtle understanding of something. Just to preface this, my driver was Prefast clean under WDK 6000.
I have a statistics data structure that I synchronize between my ISR (this is a PCI device), various internal driver threads, as well as an IOCTL that fetches this data for user apps. To synchronize this, I access the structure using KeAcquireInterruptSpinLock / KeReleaseInterruptSpinLock pairs (because it needs to be sync'ed with the ISR).
The latest version of Prefast has decided that this is not a good idea anymore:
======================================================
warning 28121 : The function 'KeReleaseInterruptSpinLock(DevExt->InterruptObject, oldIrql)' is not permitted to be called at the current IRQ level. The current level is too high: IRQL was last set to 31 at line 1015. The level might have been inferred from the function signature.
File path: ***\pci\driver\hwmonitor.cpp
Function: RecordOkHighStats
Line: 1036
996
997 ///////////////////////////////////////////////////////////////////////////////
998 //
999 // RecordOkHighStats
1000 //
1001 ///////////////////////////////////////////////////////////////////////////////
1002 void
1003 RecordOkHighStats( PGEMINI_DEVICE_EXT DevExt,
1004 ULONG StatusRegister)
1005 {
1006 //---------------------------------------
PREfast analysis path begins
1007 KIRQL oldIrql;
1008 //---------------------------------------
1009
1010 //
1011 // Acquire our interrupt spinlock
1012 //
1013 //---------------------------------------------------------------------
1014 //
1015 oldIrql = KeAcquireInterruptSpinLock(DevExt->InterruptObject);
1016
1017 if (!(StatusRegister & STATUS_BIT_PWR_OK_2_5)){
1018 DevExt->Statistics.Errors.Power_2_5++;
1019 }
1020
1021 if (!(StatusRegister & STATUS_BIT_PWR_OK_1_25)){
1022 DevExt->Statistics.Errors.Power_1_25++;
1023 }
1024
1025 if (!(StatusRegister & STATUS_BIT_RECV_CARRIER)){
1026 DevExt->Statistics.Errors.RecvCarrier++;
1027 }
1028
1029 if (!(StatusRegister & STATUS_BIT_LINK_UP)){
1030 DevExt->Statistics.Errors.LinkUp++;
1031 }
1032
1033 //
1034 // Release our interrupt spinlock
1035 //
1036 KeReleaseInterruptSpinLock(DevExt->InterruptObject, oldIrql);
hwmonitor.cpp(1036) : warning 28121: The function 'KeReleaseInterruptSpinLock(DevExt->InterruptObject, oldIrql)' is not permitted to be called at the current IRQ level. The current level is too high: IRQL was last set to 31 at line 1015. The level might have been inferred from the function signature.
Found in function 'RecordOkHighStats'
Path includes 7 statements on the following lines:
1007 1015 1017 1021 1025 1029 1036
I thought that calling KeAcquireInterruptSpinLock() set IRQL = DIRQL, not 31?
So is this is a Prefast bug, or have I missed something here?
Thanks!
Best regards,
-Mike
xxxxx@hologic.com wrote:
…
1033 //
1034 // Release our interrupt spinlock
1035 //
1036 KeReleaseInterruptSpinLock(DevExt->InterruptObject, oldIrql);
…
I thought that calling KeAcquireInterruptSpinLock() set IRQL = DIRQL, not 31?
DIRQL is variable, depending on the device, so Prefast assumes the
highest level it can possibly be.
So is this is a Prefast bug, or have I missed something here?
I’m looking forward to hearing the answer to this. Have you considered
using KeSynchronizeExecution instead? Even the docs say that’s
generally faster.
–
Tim Roberts, xxxxx@probo.com
Providenza & Boekelheide, Inc.
(sorry to wander this thread off the OPs topic)
KeSynchronizeExecution is a PITA to use. I’ve always hated that function. I’ve always surmised that it was designed to be deliberately annoying to call so that people would have to think twice before trying to serialize code with their ISR. Seriously.
That makes no sense… and I humbly beg to differ with the docs.
The two functions are entirely equivalent: They raise to Sync IRQL and acquire the interrupt spin lock. What else COULD they do?
Peter
OSR
From:
> The two functions are entirely equivalent: They raise to Sync IRQL and
> acquire the interrupt spin lock. What else COULD they do?
Acquiring and releasing the spin lock explicitly is two calls & returns. One
or both of them will be optimized by putting one or both of the function
addresses in a register. KeSynchronizeExecution is three calls and returns
because it calls your synch function, and I don’t think the register
optimization is useful because of needing to push and pop the registers the
compiler would use. So KeSE would have to be slower.
Walter Oney
Consulting and Training
www.oneysoft.com
xxxxx@osr.com wrote:
(sorry to wander this thread off the OPs topic)
KeSynchronizeExecution is a PITA to use. I’ve always hated that function. I’ve always surmised that it was designed to be deliberately annoying to call so that people would have to think twice before trying to serialize code with their ISR. Seriously.
That makes no sense… and I humbly beg to differ with the docs.
The two functions are entirely equivalent: They raise to Sync IRQL and acquire the interrupt spin lock. What else COULD they do?
Your point is embarrassingly good, so I disassembled them both to
check. The only difference between the two is that
KeSynchronizeExecution grabs the spinlock inline, by doing a “lock bts”,
instead of calling KefAcquireSpinLockAtDpcLevel.
So, you end up with four calls:
call KeSynchronizeExecution
call KeRaiseIrql
call function
call KeLowerIrql
Instead of seven:
call KeAcquireInterruptSpinLock
call KeRaiseIrql
call KefAcquireSpinLockAtDpcLevel
call function
call KeReleaseInterruptSpinLock
call KefReleaseSpinLockFromDpcLevel
call KeLowerIrql
Mighty subtle distinction.
–
Tim Roberts, xxxxx@probo.com
Providenza & Boekelheide, Inc.
From: “Tim Roberts”
> Your point is embarrassingly good, so I disassembled them both to
> check.
I wrote too fast. So KeSE really IS faster. Go figure…
Walter Oney
Consulting and Training
www.oneysoft.com
Should be:
Instead of SIX:
call KeAcquireInterruptSpinLock
call KeRaiseIrql
call KefAcquireSpinLockAtDpcLevel
(inline code goes here… no need for function call)
call KeReleaseInterruptSpinLock
call KefReleaseSpinLockFromDpcLevel
call KeLowerIrql
Not to mention… When you disassembled the code (which version of the OS did you use) did you see any calls to ETW functions in the KeSynchronizeExecution case (like before and after the call to the “function” routine)?
Regardless of whether the ETW function calls are there or not, 4 calls versus 6 calls is a distinction without any difference. And purely one of the vagaries of implementation.
So, I repeat: I (not so humbly this time, having taken the time to examine the code) beg to differ with the docs, and in fact consider this to be a documentation error.
These two calls perform the same function, and use exactly the same mechanisms to achieve their results. They are therefore entirely equivalent. One might be faster in one version of Windows, the other might be faster in a different version of Windows… this is purely a matter of implenentation, and not something the docs should even ATTEMPT to contemplate.
Peter
OSR
xxxxx@osr.com wrote:
Not to mention… When you disassembled the code (which version of the OS did you use)
XP SP3 32-bit.
did you see any calls to ETW functions in the KeSynchronizeExecution case (like before and after the call to the “function” routine)?
Nope. It is exactly 21 x86 instructions long – 63 bytes of code.
Call, lock bts, call, call, mov, ret.
So, I repeat: I (not so humbly this time, having taken the time to examine the code) beg to differ with the docs, and in fact consider this to be a documentation error.
These two calls perform the same function, and use exactly the same mechanisms to achieve their results. They are therefore entirely equivalent. One might be faster in one version of Windows, the other might be faster in a different version of Windows… this is purely a matter of implenentation, and not something the docs should even ATTEMPT to contemplate.
I agree. The difference is philosophical, not technical. Use the one
that feels most natural.
–
Tim Roberts, xxxxx@probo.com
Providenza & Boekelheide, Inc.
Ah! I was looking at Vista.
Which just goes to prove my thesis: Given that these two routines perform the same general function, using the same mechanisms, which is “faster” will depend on the implementation on any given system… not to mention the processor architecture.
“Feedback” sent…
Peter
OSR
It’s good to hear everyone else echo how I thought this all worked.
I tend to use KeAcquireInterruptSpinLock / KeReleaseInterruptSpinLock if only because it feels more natural to me. I do a lot of embedded work, so pairing Locks(ISR Disables) with Unlocks (ISR Enables) is just how I think I guess.
And I would humbly suggest that there is a bug in Prefast in this case. The following code is correct usage and shouldn’t get flagged:
KIRQL OldIrql;
OldIrql = KeAcquireInterruptSpinLock(DevExt->InterruptObject);
// Do work here - make it quick, ISR’s are disabled!
KeReleaseInterruptSpinLock(DevExt->InterruptObject, OldIrql);
So for now until it’s fixed, I’m making use of the following:
#pragma prefast(suppress:28121, “This is correct usage of KeReleaseInterruptSpinLock”)
-Mike
I agree. There’s neither anything wrong with your usage nor your code… as least as far as I can see.
Hopefully, one of our MSFT colleagues who read the list will file the bug for us.
The community needs a good way to submit bugs like this to Microsoft, without having to bother a Microsoft list member. Back in the day, we had a mechanism to do that here on OSR Online. But those days are sadly gone.
Peter
OSR