tracing: Fix missing file i/o start ops. [chromium/src : main]
David Bienvenu (Gerrit)
David Bienvenu added 1 comment![]( "Open in Gerrit")
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")
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 1 comment![]( "Open in Gerrit")
Nice finding!
Tracking file i/o events from all processes to handle case where non
Chrome file i/o event completion happens to run on one of Chrome's
threads.
My main question: All IO events have ttid field (except for FileIo_OpEnd); Should we use ttid (instead of header.ThreadId) to decide if we should emit the event (this is perhaps more indicative of who the event belongs to)?
This would imply we need need to either track unfinished events (by irp) until we see the FileIo_OpEnd, or we unconditionally emit all FileIo_OpEnd.
Related details
- 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 added 1 comment![]( "Open in Gerrit")
Nice finding!
Tracking file i/o events from all processes to handle case where non
Chrome file i/o event completion happens to run on one of Chrome's
threads.My main question: All IO events have ttid field (except for FileIo_OpEnd); Should we use ttid (instead of header.ThreadId) to decide if we should emit the event (this is perhaps more indicative of who the event belongs to)?
This would imply we need need to either track unfinished events (by irp) until we see the FileIo_OpEnd, or we unconditionally emit all FileIo_OpEnd.
I think when I looked at this before, they were generally the same. But, it's only the FileIo_OpEnd events that appear to get run unrelated threads, and they don't have a ttid field. I can programatically check that they're the same to be sure, but I don't think it would affect this CL.
That's essentially what we're doing - tracking unfinished events by irp until we either see an op end event for that irp, or a start event with an existing unfinished irp event.
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")
David BienvenuNice finding!
Tracking file i/o events from all processes to handle case where non
Chrome file i/o event completion happens to run on one of Chrome's
threads.My main question: All IO events have ttid field (except for FileIo_OpEnd); Should we use ttid (instead of header.ThreadId) to decide if we should emit the event (this is perhaps more indicative of who the event belongs to)?
This would imply we need need to either track unfinished events (by irp) until we see the FileIo_OpEnd, or we unconditionally emit all FileIo_OpEnd.
I think when I looked at this before, they were generally the same. But, it's only the FileIo_OpEnd events that appear to get run unrelated threads, and they don't have a ttid field. I can programatically check that they're the same to be sure, but I don't think it would affect this CL.
That's essentially what we're doing - tracking unfinished events by irp until we either see an op end event for that irp, or a start event with an existing unfinished irp event.
I instrumented DecodeFileIoReadWriteEvent and ttid was always the same as headerThreadId, so I don't think that's an issue.
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 2 comments![]( "Open in Gerrit")
David BienvenuNice finding!
Tracking file i/o events from all processes to handle case where non
Chrome file i/o event completion happens to run on one of Chrome's
threads.My main question: All IO events have ttid field (except for FileIo_OpEnd); Should we use ttid (instead of header.ThreadId) to decide if we should emit the event (this is perhaps more indicative of who the event belongs to)?
This would imply we need need to either track unfinished events (by irp) until we see the FileIo_OpEnd, or we unconditionally emit all FileIo_OpEnd.
David BienvenuI think when I looked at this before, they were generally the same. But, it's only the FileIo_OpEnd events that appear to get run unrelated threads, and they don't have a ttid field. I can programatically check that they're the same to be sure, but I don't think it would affect this CL.
That's essentially what we're doing - tracking unfinished events by irp until we either see an op end event for that irp, or a start event with an existing unfinished irp event.
I instrumented DecodeFileIoReadWriteEvent and ttid was always the same as headerThreadId, so I don't think that's an issue.
okk thanks for checking!
That's essentially what we're doing - tracking unfinished events by irp until we either see an op end event for that irp, or a start event with an existing unfinished irp event.
active_irps_from_non_chrome_threads_ sounds like we're doing the opposite (track events on non-chrome), but my understanding of the problem is that we want to track events started on chrome thread so that we can emit the end (regardless of which thread it shows up on)?
Or if the issue really is that we get "OpEnd / EndOperation" without the start, can we ignore those in trace_processor (where we already track the begin/end pairs)
const uint64_t irp_ptr =
ExtractIrpPtrFromEvent(opcode, pointer_size, packet_data);
I suspect the logic could be simplified if folded within DecodeFileIo*Event (at the cost of some repetition), and make a decision about should_record only then (based on my understanding).
e.g. for DecodeFileIoCreateEvent:
```
const bool should_record =
inclusion_policy_.ShouldRecordFileIoEvents(header.ThreadId);
if (!should_record) {
return;
}
...
const uint64_t irp_ptr = CopyPointerHash(iterator, pointer_size);
active_irps_.insert(irp_ptr);
// Read the contents of `packet_data` and generate a `FileIoCreate` event.
base::BufferIterator<const uint8_t> iterator{packet_data};
auto* event = MakeNextEvent(header, buffer_context);
event->set_thread_id(header.ThreadId);
auto* file_io_create = event->set_file_io_create();
file_io_create->set_irp_ptr(irp_ptr);
```
and in DecodeFileIoOpEndEvent:
```
const uint64_t irp_ptr = CopyPointerHash(iterator, pointer_size);
if (!active_irps_.erase(irp_ptr)) {
return;
}
// Read the contents of `packet_data` and generate a `FileIoOpEnd` event.
base::BufferIterator<const uint8_t> iterator{packet_data};
auto* event = MakeNextEvent(header, buffer_context);
event->set_thread_id(header.ThreadId);
auto* file_io_op_end = event->set_file_io_op_end();
file_io_op_end->set_irp_ptr(irp_ptr);
```
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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by David Bienvenu
| Commit-Queue | +1 |
David BienvenuNice finding!
Tracking file i/o events from all processes to handle case where non
Chrome file i/o event completion happens to run on one of Chrome's
threads.My main question: All IO events have ttid field (except for FileIo_OpEnd); Should we use ttid (instead of header.ThreadId) to decide if we should emit the event (this is perhaps more indicative of who the event belongs to)?
This would imply we need need to either track unfinished events (by irp) until we see the FileIo_OpEnd, or we unconditionally emit all FileIo_OpEnd.
David BienvenuI think when I looked at this before, they were generally the same. But, it's only the FileIo_OpEnd events that appear to get run unrelated threads, and they don't have a ttid field. I can programatically check that they're the same to be sure, but I don't think it would affect this CL.
That's essentially what we're doing - tracking unfinished events by irp until we either see an op end event for that irp, or a start event with an existing unfinished irp event.
Etienne Pierre-DorayI instrumented DecodeFileIoReadWriteEvent and ttid was always the same as headerThreadId, so I don't think that's an issue.
okk thanks for checking!
That's essentially what we're doing - tracking unfinished events by irp until we either see an op end event for that irp, or a start event with an existing unfinished irp event.
active_irps_from_non_chrome_threads_ sounds like we're doing the opposite (track events on non-chrome), but my understanding of the problem is that we want to track events started on chrome thread so that we can emit the end (regardless of which thread it shows up on)?
Or if the issue really is that we get "OpEnd / EndOperation" without the start, can we ignore those in trace_processor (where we already track the begin/end pairs)
No, it's the opposite - we need to track events started on non-chrome threads because the end event might be emitted on a chrome thread. Basically, after the file read/write finishes, Windows just picks some idle thread to run the end operation on, in some cases.
const uint64_t irp_ptr =
ExtractIrpPtrFromEvent(opcode, pointer_size, packet_data);
This CL move the irp_ptr handling from non Chrome threads into the individual file op decoders. It's slightly more efficient in that we only create one base::BufferIterator<const uint8_t> iterator. However, I found that the Flt ops for non Chrome threads can also have the end op run on Chrome threads, and there may be other kinds of ops that have the same issue, which would increase the code duplication. I might have a go at instrumenting the code to see which ops can have their end events run on Chrome threads, and their relative frequency.
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. |