-
Notifications
You must be signed in to change notification settings - Fork 2k
DO NOT MERGE test #5264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
bchalios
wants to merge
10
commits into
firecracker-microvm:main
Choose a base branch
from
bchalios:greedy_block_test
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
DO NOT MERGE test #5264
+250
−133
Conversation
This file contains hidden or 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
The `max_size` field is public, so no need for a getter. Signed-off-by: Egor Lazarchuk <[email protected]>
Add a validation of the queue size set by the guest. Signed-off-by: Egor Lazarchuk <[email protected]>
The size of queue set by the driver must be always less or equal to the queue size in FC. The check for it now is done in `initialize` call. This removes the need for `actual_size` function. Signed-off-by: Egor Lazarchuk <[email protected]>
Currently block device has a guest notification logic inside it's request processing loop. This can create a situation when guest can continuously add more requests to the queue, making the whole request processing loop arbitrary long. This is an issue, since it block any other IO from being processed. The solution is to simply notify guest one time, after all current requests are processed. Signed-off-by: Egor Lazarchuk <[email protected]>
VIRTIO spec states: ``` After the device writes a descriptor index into the used ring: If the idx field in the used ring (which determined where that descriptor index was placed) was equal to used_event, the device MUST send a notification. ``` The current implementation does not follow this very precisely. It bumps used ring index when new descriptors are added to the used ring. But the check if the notification is needed is postponed to later processing stage. To be more VIRTIO spec compliant simply move the logic for updating the used ring index into the later processing stage as well, just before the check if the notification should be send. Signed-off-by: Egor Lazarchuk <[email protected]>
usize cannot be negative Signed-off-by: Egor Lazarchuk <[email protected]>
Fix a typo in a test comment Signed-off-by: Babis Chalios <[email protected]>
Firecracker IO is software emulated by the VMM thread of the Firecracker process. This is a single thread so it could be the case that a single device can starve other devices. This could happen if a guest keeps adding descriptors in a queue while we are processing it. Our emulation logic should include railguards against such behaviour. Add a test that verifies that this is the case. Signed-off-by: Babis Chalios <[email protected]>
Signed-off-by: Babis Chalios <[email protected]>
Signed-off-by: Babis Chalios <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5264 +/- ##
=======================================
Coverage 82.84% 82.85%
=======================================
Files 250 250
Lines 26967 26980 +13
=======================================
+ Hits 22342 22355 +13
Misses 4625 4625
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Changes
...
Reason
...
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 checkstyle
to 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
.