fix: compute jailer cpu time without underflow #5034
Merged
+17
−31
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Replaces a manual impl that was already missing some fields because they were added to the struct after the impl was written. Signed-off-by: Patrick Roy <[email protected]>
roypat
added
the
Status: Awaiting review
The jailer wishes to compute the CPU time it spents before exec-ing into Firecracker, so that it can be passed to Firecracker's `parent-cpu-us` flag and emitted as a metric. The CLOCK_PROCESS_CPUTIME_ID is reset on every fork() call, so we need to stitch together the different values from before the first fork, and then between each consecutive fork. However, we also need to account for the scenarios that we do not fork() at all (no daemonization). Now, this all goes slightly wrong because we want to exclude the time spent the call to Env::run() from the reported time (note that inside the jailer, this is only the time spent parsing arguments, but if a user ended up exec()-ing into the jailer, it would also include the time from prior to calling exec(), which makes sense to exclude). So we grab the value of CLOCK_PROCESS_CPUTIME_ID right after parsing arguments, and subtract it at the end. Sadly, in the case of daemonization, we subtract it not from the total amount of time spent in the jailer, but rather from the time spent since the final fork(). This works out if the time spent since the last fork was greater than the time spent parsing arguments, but if not, we have an integer underflow on our hands. Now, this is only a problem in debug builds, and in release builds, we underflow, and then _over_flow again during the next addition of the underflowed value to jailer_cpu_time_us, and Firecracker even seens a sane value for parent_time_cpu_us. But in debug builds, we panic. Fix this by breaking the subtraction and addition into two separate statements, to avoid relying on the double over-/underflow Closes firecracker-microvm#5033 Fixes: 68ab56e Signed-off-by: Patrick Roy <[email protected]>
roypat
force-pushed
the
fix-jailer-underflow
branch
from
d09494c to
a1d7820
Compare
roypat
requested review from
xmarcalx,
kalyazin,
pb8o and
Manciukic
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5034 +/- ##
==========================================
+ Coverage 83.13% 83.21% +0.07%
==========================================
Files 245 245
Lines 26651 26626 -25
==========================================
Hits 22156 22156
+ Misses 4495 4470 -25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Manciukic
reviewed
Feb 10, 2025
In `report_cpu_start_time()`, Firecracker computers the CPU time it took until the API socket was available as CLOCK_PROCESS_CPUTIME_ID - cpu_start_time + jailer_time However, here `cpu_start_time` is always zero currently (it's the value the jailer passes via `--cpu-start-time`), and in the case where the jailer exec()s into Firecracker (without prior fork() or clone()), this is wrong: CLOCK_PROCESS_CPUTIME_ID will now be the CPU time since the _jailer_ process started, and thus adding the jailer_time to it is nonsense. This all works out if we set cpu_start_time to be the CPU time right before we exec() into Firecracker: If we did not fork(), then it will cause the calculation above to correctly discard all time before exec()-ing into Firecracker (before re-adding specifically the time spent in the jailer). If we _did_ fork(), then it will be approximately zero, and thus preserve the current semantics (as the jailer time will only be the time _until_ we did the last fork(), and CLOCK_PROCESS_CPUTIME_ID will only be the absolute CPU time _since_ the last fork()). Signed-off-by: Patrick Roy <[email protected]>
The components that go into the calculation are 1. Exclude thing before calling Env::new() (including potential time spent in a parent of the jailer itself that ended up exec()-ing 2. The time up until a potential fork() for daemonization 3. The time spent in each temporary process during triple-fork()ing Signed-off-by: Patrick Roy <[email protected]>
roypat
force-pushed
the
fix-jailer-underflow
branch
from
5d2da19 to
9895cc9
Compare
zulinx86
approved these changes
Feb 11, 2025
Manciukic
reviewed
Feb 11, 2025
Manciukic
approved these changes
Feb 13, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Oh, here we go again.
The jailer wishes to compute the CPU time it spents before exec-ing into
Firecracker, so that it can be passed to Firecracker's
parent-cpu-usflag and emitted as a metric.
The CLOCK_PROCESS_CPUTIME_ID is reset on every fork() call, so we need
to stitch together the different values from before the first fork, and
then between each consecutive fork. However, we also need to account for
the scenarios that we do not fork() at all (no daemonization).
Now, this all goes slightly wrong because we want to exclude the time
spent the call to Env::run() from the reported time (note that inside
the jailer, this is only the time spent parsing arguments, but if a user
ended up exec()-ing into the jailer, it would also include the time from
prior to calling exec(), which makes sense to exclude). So we grab the
value of CLOCK_PROCESS_CPUTIME_ID right after parsing arguments, and
subtract it at the end. Sadly, in the case of daemonization, we subtract
it not from the total amount of time spent in the jailer, but rather
from the time spent since the final fork(). This works out if the time
spent since the last fork was greater than the time spent parsing
arguments, but if not, we have an integer underflow on our hands. Now,
this is only a problem in debug builds, and in release builds, we
underflow, and then _over_flow again during the next addition of the
underflowed value to jailer_cpu_time_us, and Firecracker even seens a
sane value for parent_time_cpu_us. But in debug builds, we panic.
Fix this by breaking the subtraction and addition into two separate
statements, to avoid relying on the double over-/underflow
Closes #5033
Fixes: 68ab56e
Signed-off-by: Patrick Roy [email protected]
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.