Add ETW disk i/o events [chromium/src : main]
David Bienvenu (Gerrit)
David Bienvenu voted and added 1 comment![]( "Open in Gerrit")
Votes added by David Bienvenu
| Commit-Queue | +1 |
1 comment
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bienvenu (Gerrit)
David Bienvenu added 1 comment![]( "Open in Gerrit")
gentle ping
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 5 comments![]( "Open in Gerrit")
Cool!
static const double qpc_ticks_per_second = []() {
LARGE_INTEGER perf_counter_frequency = {};
::QueryPerformanceFrequency(&perf_counter_frequency);
double frequency = static_cast<double>(perf_counter_frequency.QuadPart);
CHECK_GT(frequency, 0.0);
return frequency;
}();FYI: This somewhat conflict with jesse's https://chromium-review.git.corp.google.com/c/chromium/src/+/7794326/7/components/tracing/common/etw_consumer_win.cc
Making this a function static variable seems like the right approach.
for (const auto& keyword : etw_config.disk_provider_events()) {I meant for those to follow https://learn.microsoft.com/en-us/windows/win32/etw/system-providers (even though some are then converted to kernel flags)
Where both disk and file are in "system io", but it got offtrack.
Could we merge EtwSystemFlagsFromFileProvider/EtwSystemFlagsFromDiskProvider in a way that it doesn't matter which we chose to use (and hopefully we eventually merge both); in the mean time we can just keep using "file provider".
return active_processes_->GetThreadCategory(thread_id) ==
ActiveProcesses::Category::kClient;It'd be ok to also record disk IO events for system processes.
provider: 'file',Related to my comment on EtwSystemFlagsFromDiskProvider, could we make this "file" so we use a single provider.
Related details
- David Bienvenu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bienvenu (Gerrit)
David Bienvenu added 1 comment![]( "Open in Gerrit")
provider: 'file',Related to my comment on EtwSystemFlagsFromDiskProvider, could we make this "file" so we use a single provider.
Do you mean change the Disk I/O event below to have provider 'file'?
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
provider: 'file',David BienvenuRelated to my comment on EtwSystemFlagsFromDiskProvider, could we make this "file" so we use a single provider.
Do you mean change the Disk I/O event below to have provider 'file'?
- David Bienvenu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bienvenu (Gerrit)
David Bienvenu added 1 comment![]( "Open in Gerrit")
David BienvenuRelated to my comment on EtwSystemFlagsFromDiskProvider, could we make this "file" so we use a single provider.
Etienne Pierre-DorayDo you mean change the Disk I/O event below to have provider 'file'?
Yes
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 2 comments![]( "Open in Gerrit")
for (const auto& keyword : etw_config.disk_provider_events()) {I meant for those to follow https://learn.microsoft.com/en-us/windows/win32/etw/system-providers (even though some are then converted to kernel flags)
Where both disk and file are in "system io", but it got offtrack.Could we merge EtwSystemFlagsFromFileProvider/EtwSystemFlagsFromDiskProvider in a way that it doesn't matter which we chose to use (and hopefully we eventually merge both); in the mean time we can just keep using "file provider".
I added system_io to follow https://learn.microsoft.com/en-us/windows/win32/etw/system-providers
But the individual keyword can remain the same (similar to SchedulerProvider):
```
ULONG EtwSystemFlagsFromSystemIOProvider(std::string_view keyword) {
if (keyword == "FILE_IO") {
return EVENT_TRACE_FLAG_FILE_IO | EVENT_TRACE_FLAG_FILE_IO_INIT;
} else if (keyword == "DISK_IO") {
return EVENT_TRACE_FLAG_DISK_IO | EVENT_TRACE_FLAG_DISK_IO_INIT;
}
return 0;
}
```
{
name: 'System I/O',
keyword: 'SYSTEM_IO',
provider: 'system_io',
description: 'Enables System I/O events',
},Same as other comment, we can keep individual keywords:
```
{
name: 'File I/O',
keyword: 'FILE_IO',
provider: 'system_io',
description: 'Enables file I/O events',
},
{
name: 'Disk I/O',
keyword: 'DISK_IO',
provider: 'system_io',
description: 'Enables disk I/O events',
},
```
Related details
- David Bienvenu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bienvenu (Gerrit)
David Bienvenu added 4 comments![]( "Open in Gerrit")
static const double qpc_ticks_per_second = []() {
LARGE_INTEGER perf_counter_frequency = {};
::QueryPerformanceFrequency(&perf_counter_frequency);
double frequency = static_cast<double>(perf_counter_frequency.QuadPart);
CHECK_GT(frequency, 0.0);
return frequency;
}();FYI: This somewhat conflict with jesse's https://chromium-review.git.corp.google.com/c/chromium/src/+/7794326/7/components/tracing/common/etw_consumer_win.cc
Making this a function static variable seems like the right approach.
I've stolen Jesse's change and switched to nano seconds - I will need to make a small perfetto change to reflect this.
for (const auto& keyword : etw_config.disk_provider_events()) {I meant for those to follow https://learn.microsoft.com/en-us/windows/win32/etw/system-providers (even though some are then converted to kernel flags)
Where both disk and file are in "system io", but it got offtrack.Could we merge EtwSystemFlagsFromFileProvider/EtwSystemFlagsFromDiskProvider in a way that it doesn't matter which we chose to use (and hopefully we eventually merge both); in the mean time we can just keep using "file provider".
Done
return active_processes_->GetThreadCategory(thread_id) ==
ActiveProcesses::Category::kClient;It'd be ok to also record disk IO events for system processes.
Acknowledged. I've added a TODO for this
name: 'System I/O',
keyword: 'SYSTEM_IO',
provider: 'system_io',
description: 'Enables System I/O events',
},Same as other comment, we can keep individual keywords:
```
{
name: 'File I/O',
keyword: 'FILE_IO',
provider: 'system_io',
description: 'Enables file I/O events',
},
{
name: 'Disk I/O',
keyword: 'DISK_IO',
provider: 'system_io',
description: 'Enables disk I/O events',
},
```
Done - I removed the the system i/o entry and aded the disk and file i/o ones w/ the system_provider.
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 3 comments![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Code-Review | +1 |
3 comments
base::debug::WaitForDebugger(We shouldn't land this?
Or guard behind command_line->HasSwitch(switches::kWaitForDebugger)?
return active_processes_->GetThreadCategory(thread_id) ==
ActiveProcesses::Category::kClient;David BienvenuIt'd be ok to also record disk IO events for system processes.
Acknowledged. I've added a TODO for this
This should simply be
```
active_processes_->GetThreadCategory(thread_id) !=
ActiveProcesses::Category::kOther;
```
Why not do it now?
Related details
- David Bienvenu
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bienvenu (Gerrit)
David Bienvenu added 2 comments![]( "Open in Gerrit")
We shouldn't land this?
Or guard behind command_line->HasSwitch(switches::kWaitForDebugger)?
oh, sorry, removed! Breaking into the debugger is the only way I can get `etw_controller_` to start a session successfully. Which I'll investigate more once this is in.
return active_processes_->GetThreadCategory(thread_id) ==
ActiveProcesses::Category::kClient;David BienvenuIt'd be ok to also record disk IO events for system processes.
Etienne Pierre-DorayAcknowledged. I've added a TODO for this
This should simply be
```
active_processes_->GetThreadCategory(thread_id) !=
ActiveProcesses::Category::kOther;
```Why not do it now?
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 1 comment![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Code-Review | +1 |
- David Bienvenu
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bienvenu (Gerrit)
David Bienvenu voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Add ETW disk i/o events
Add support for requesting and parsing ETW disk i/o events, and UI
to turn them on in local tracing. Perfetto already supports displaying
disk i/o events, so I've tested that all the disk i/o events show
up in Perfetto with a trace generated with this CL.
- M chrome/windows_services/elevated_tracing_service/system_tracing_session.cc
- M components/tracing/common/etw_consumer_win.cc
- M components/tracing/common/etw_consumer_win.h
- M components/tracing/common/etw_consumer_win_unittest.cc
- M components/tracing/common/etw_system_data_source_win.cc
- M components/tracing/common/inclusion_policy_win.cc
- M components/tracing/common/inclusion_policy_win.h
- M content/browser/resources/traces_internals/trace_recorder.ts
Code-Review: +1 by Etienne Pierre-Doray
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |