Fix duplicate guest trace batches#1619
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes duplicate guest trace span batches by moving trace-batch delivery to a dedicated TraceBatch OUT port, so the host only reads the trace registers on the specific VM exit that carries a new batch (instead of polling sticky registers on every exit).
Changes:
- Host: only reads guest registers and parses trace data on
VmExit::IoOutwhere the port isOutBAction::TraceBatch. - Guest: trace batches are emitted via a dedicated
TraceBatchOUT in the tracing subsystem; abort paths now callhyperlight_guest_tracing::flush()before emitting abort bytes. - Cleanup/docs: removes the old “piggyback trace batch on arbitrary OUT” API surface and updates tracing documentation accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hyperlight_host/src/sandbox/trace/context.rs | Removes the old “check registers for trace data” helper; trace handling is now driven by dedicated exits. |
| src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs | Parses trace batches only on TraceBatch port exits, avoiding re-reading sticky registers on unrelated exits. |
| src/hyperlight_guest/src/exit.rs | Flushes trace batches before deliberate abort sequences so the final buffered batch can be delivered. |
| src/hyperlight_guest/src/arch/amd64/exit.rs | Makes out32 a pure I/O primitive again (no embedded trace-batch side effects). |
| src/hyperlight_guest_tracing/src/state.rs | Sends trace batches to the host via a dedicated TraceBatch OUT and tightens inline-asm options. |
| src/hyperlight_guest_tracing/src/lib.rs | Trims exported API to the remaining supported tracing entrypoints. |
| docs/hyperlight-metrics-logs-and-traces.md | Documents the dedicated TraceBatch exit and new batching/flush behavior (with a couple clarifications needed per review comments). |
The host stamped the TraceBatch magic in r8 on every OUT and polled r8 on every VM exit. r8 is sticky, so the host re-read the same batch on later exits and emitted duplicate spans. Deliver batches on a dedicated TraceBatch port. The host reads guest registers only on that exit, so each batch is read once. Aborts flush the buffer over the same port so the final batch still reaches the host. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
a2fcbca to
9c871f5
Compare
jsturtevant
left a comment
There was a problem hiding this comment.
This looks like a nice simplification. We had existing tests that cover the tracing?
@dblnz for a second set of eyes
There was a problem hiding this comment.
Great work fixing this bug!
Overall, this change makes the tracing code on the guest side look much cleaner.
However, this change modifies behavior in a subtle way.
Previously, by using the r8 to signal a TraceBatch on each outb, the guest reported existing trace events on every outb regardless of the intended purpose of outb (whether it was a flush, a full trace buffer, or a host call). That was an implementation decision to minimize the number of VMExits (by sending tracing data on each VMExit) so that tracing is as accurate as it can be.
Going back to this PR, I like that it makes the codebase cleaner, but it will generate additional VMExits. I don't have the actual measurements to say for sure what kind of "performance penalty" there is, because they depend heavily on the application (how many trace events it generates between VMExits).
I think we want to have tracing as accurate as possible for users to get correct measurements.
What do you think?
The guest stamped the TraceBatch magic into r8 when an OUT carried a pending batch, and the host polled r8 on every VM exit. However nothing cleared r8, so later exits re-read the same buffer and emitted duplicate spans. Now batches are delivered on a dedicated TraceBatch port and the host reads the registers only on that exit, so each batch is read once. Aborts flush over the same port so the final batch still reaches the host.
Closes #1230