[vioscsi] DriverEntry() improved tracing #1215
Conversation
benyamin-codez
requested review from
vrozenfe and
YanVugenfirer
as code owners
|
Many thanks for the review, Yan...! Unfortunately I've run out of time for now... Best regards, |
|
@YanVugenfirer @vrozenfe @kostyanf14 This one should be right to go again. I've fixed the problem breaking compilation (my bad), so the All appears to work as expected in both Win10 and Win11. |
|
I believe those check failures are all known issues, yes...? |
|
@benyamin-codez Disk Verification (LOGO) and Flush tests failures are known issue. |
|
|
||
| #if !defined(RUN_UNCHECKED) || defined(RUN_MIN_CHECKED) | ||
| RhelDbgPrint(TRACE_LEVEL_NONE, " VIOSCSI driver starting..."); | ||
| RhelDbgPrint(TRACE_LEVEL_NONE, " Built on %s at %s \n", __DATE__, __TIME__); |
There was a problem hiding this comment.
Again, why not to print this alway?
This is not a "performance path," and it is degrading existing behavior.
There was a problem hiding this comment.
The mechanism provides a means to run without WPP which can result in an approx. 80-100 μs latency improvement and an increase of 20-50k IOPS for 4KiB random I/O (with 32 queues, 16 threads on 4KiB segments). When WPP is turned off, these macros stop compile-time errors when building.
The mechanism is also quite useful during development to help determine the overhead of various tracing changes.
vioscsi/vioscsi.c
Outdated
| #if !defined(RUN_UNCHECKED) | ||
| switch (NTDDI_VERSION) { | ||
| case NTDDI_WIN10: | ||
| RhelDbgPrint(TRACE_LEVEL_VERBOSE, " NTDDI_VERSION : ABRACADABRA_THRESHOLD | Windows 10.0.10240 | 1507 | Threshold 1 \n"); |
There was a problem hiding this comment.
I am not sure I am OK with such a construct in a driver, even for debugging purposes. It would be better to have a wiki page.
Also, I mentioned before - controlling the amount of debug prints better be done in run time.
@vrozenfe, what do you think?
Even if we go for this:
- Remove "ABRACADABRA_ :)
- Move the switch/case to a separate function (compile under pre-processor conditions) to keep DriverEntry cleaner.
There was a problem hiding this comment.
Also, I mentioned before - controlling the amount of debug prints better be done in run time.
This would be great, but I'm not sure how to do this with the way WPP works.
When the WPP runs it creates the relevant *.tmh file(s) with entries for any and all matching functions as determined by the scan file. This means that the WPP performance overhead is directly proportional to the number of entries, regardless of whether they are printed. Therefore, changes to the verbosity of debug messages will have no effect on performance with ETW - it will consistently be the worst possible.
This is not the case with the kDebugPrint (non-default) option, but I note my previous comment too, where I confirmed issues with boot-time and the HwStorPortProhibitedDDIs rule-set.
In any case, we presently choose between ETW (WPP) and kDebugPrint at compile-time.
Also, please see the changes I propose in PR #1228.
I am not sure I am OK with such a construct in a driver, even for debugging purposes.
This is very useful information to have for the purposes of advertising feature-set availability and for configuration confirmation. The prints would be most useful to sys admins preparing or troubleshooting configuration options. Here is an example of what it looks like in TraceView:
You might notice the line at the bottom saying:
ConfigInfo->DmaAddressWidth is NOT supported in this version of the driver.
This is an example where the feature is not available as it is built with THRESHOLD when it needs WIN10_VB. One can then refer to the NTDDI_VERSION above to give this context.
It would be better to have a wiki page.
I don't know about better.. 8^)
...but it would be helpful to a range of roles to know what the NTDDI_VERSION and feature-set is.
Remove "ABRACADABRA_ :)
Can do... 8^D
With the squash... 8^d
Move the switch/case to a separate function (compile under pre-processor conditions) to keep DriverEntry cleaner.
This is run-time only...
...but I'm curious as to how this might work... 8^d
If you are worried about using switch-case, iirc, this is normally compiled as if-else...
EDIT: Corrected reference: tmf.* --> tmh.*
There was a problem hiding this comment.
Move the switch/case to a separate function (...) to keep DriverEntry cleaner.
Done...!
8^d
There was a problem hiding this comment.
I think this one is now resolved...!?
|
Also, please squash the commits. |
benyamin-codez
force-pushed
the
vioscsi-driver-entry
branch
3 times, most recently
from
4bb4926 to
000c987
Compare
|
Modded, squashed and rebased... 8^d |
benyamin-codez
force-pushed
the
vioscsi-driver-entry
branch
from
000c987 to
de27666
Compare
1. Show the RegistryPath. 2. Show the CrashDump Mode. 3. Show StorPortInitialize() return value (including LONG). 4. Show NTDDI_VERSION. 5. Split NTDDI_ definitions to new file ntddi_ver.h and addedd missing NTDDI definitions 6. Removed references to obsoleted RUN_MIN_CHECKED definition (PR virtio-win#1228). Signed-off-by: benyamin-codez <[email protected]>
benyamin-codez
force-pushed
the
vioscsi-driver-entry
branch
from
de27666 to
fca1d26
Compare
|
Maybe some |
Show the RegistryPath.
Show the CrashDump Mode.
Show
StorPortInitialize()return value (including LONG).Show
NTDDI_VERSION.Also added
hwInitData.AddressTypeFlags.Added missing
NTDDIdefinitions invioscsi.h.