Skip to content

Queue tweaks #4726

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

Merged
merged 3 commits into from
Aug 27, 2024
Merged

Queue tweaks #4726

merged 3 commits into from
Aug 27, 2024

Conversation

ShadowCurse
Copy link
Contributor

@ShadowCurse ShadowCurse commented Aug 12, 2024

Changes

  • Update calculations in the queue to use std::mem::size_of instead of hard-coded values
  • Add ability to reload IoVecBuffMut the same way as it can be done with non Mut variant

Reason

These were part of #4658, but because these changes seem generic, move into separate PR.

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

  • If a specific issue led to this PR, this PR closes the issue.
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this
    PR.
  • API changes follow the Runbook for Firecracker API changes.
  • User-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.
  • New TODOs link to an issue.
  • Commits meet
    contribution quality standards.

  • This functionality cannot be added in rust-vmm.

Copy link

codecov bot commented Aug 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.34%. Comparing base (167b1b0) to head (25b0b15).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4726      +/-   ##
==========================================
- Coverage   84.37%   84.34%   -0.03%     
==========================================
  Files         249      249              
  Lines       27452    27461       +9     
==========================================
  Hits        23162    23162              
- Misses       4290     4299       +9     
Flag Coverage Δ
5.10-c5n.metal 84.57% <100.00%> (-0.04%) ⬇️
5.10-m5n.metal 84.55% <100.00%> (-0.04%) ⬇️
5.10-m6a.metal 83.84% <100.00%> (-0.04%) ⬇️
5.10-m6g.metal 80.91% <100.00%> (-0.04%) ⬇️
5.10-m6i.metal 84.54% <100.00%> (-0.04%) ⬇️
5.10-m7g.metal 80.91% <100.00%> (-0.04%) ⬇️
6.1-c5n.metal 84.57% <100.00%> (-0.04%) ⬇️
6.1-m5n.metal 84.55% <100.00%> (-0.04%) ⬇️
6.1-m6a.metal 83.84% <100.00%> (-0.04%) ⬇️
6.1-m6g.metal 80.91% <100.00%> (-0.04%) ⬇️
6.1-m6i.metal 84.54% <100.00%> (-0.04%) ⬇️
6.1-m7g.metal 80.91% <100.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ShadowCurse ShadowCurse force-pushed the queue_tweaks branch 2 times, most recently from 96211c9 to 4994c1b Compare August 12, 2024 13:59
@ShadowCurse ShadowCurse self-assigned this Aug 12, 2024
@ShadowCurse ShadowCurse marked this pull request as ready for review August 12, 2024 14:00
@ShadowCurse ShadowCurse added Status: Awaiting review Indicates that a pull request is ready to be reviewed Type: Enhancement Indicates new feature requests labels Aug 12, 2024
@ShadowCurse ShadowCurse requested review from roypat and bchalios August 12, 2024 14:06
roypat
roypat previously approved these changes Aug 12, 2024
roypat
roypat previously approved these changes Aug 12, 2024
Copy link
Contributor

@bchalios bchalios left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got some comments, mainly regarding documentation of the changes in the commit messages. Please be wordy in these. It might be obvious now why we did this change, but months/years from now it won't.

Also, have you checked whether the new functions added here affect kani proofs somehow, or whether we need to add new proofs? (cc @roypat).

Finally, I will be approving this only after I take a look at the mergeable buffers PR. The usefulness of these commits depends on that other PR. Please do NOT merge until then.

@bchalios
Copy link
Contributor

Actually, kani does break :/

@roypat
Copy link
Contributor

roypat commented Aug 12, 2024

Also, have you checked whether the new functions added here affect kani proofs somehow, or whether we need to add new proofs? (cc @roypat).

On this part, I think we are fine because we're only moving code around inside functions that are covered (and get_desc_chain has all possible inputs covered because avail_idx is non-deterministic in the proof for do_pop_unchecked).

The timeout simply means that we wandered off the happy path of "verification finishes in a reasonable amount of time" 😭

@ShadowCurse ShadowCurse force-pushed the queue_tweaks branch 2 times, most recently from b475b72 to 68c10f0 Compare August 13, 2024 10:27
@bchalios
Copy link
Contributor

On this part, I think we are fine because we're only moving code around inside functions that are covered (and get_desc_chain has all possible inputs covered because avail_idx is non-deterministic in the proof for do_pop_unchecked).

You are right, today! However, this is all preparatory work for the next PR which will actually make direct use of these new functions. This is what I'm worried about.

@ShadowCurse ShadowCurse force-pushed the queue_tweaks branch 3 times, most recently from 476d541 to f8299b4 Compare August 21, 2024 14:10
@ShadowCurse ShadowCurse requested a review from roypat August 21, 2024 17:27
@ShadowCurse ShadowCurse force-pushed the queue_tweaks branch 3 times, most recently from e96abbb to 4c8a332 Compare August 27, 2024 11:30
Now IoVecBufferMut can be reloaded from DescriptorChain
same way as IoVecBuffer does it.
This is helpful to avoid unnecessary allocations/deallocations
when reusing same buffer.

Signed-off-by: Egor Lazarchuk <[email protected]>
Replace numbers with more descriptive `size_of`
methods.

Signed-off-by: Egor Lazarchuk <[email protected]>
`Self::default()` is a bit more compact.

Signed-off-by: Egor Lazarchuk <[email protected]>
@ShadowCurse ShadowCurse merged commit 3db25a1 into firecracker-microvm:main Aug 27, 2024
7 checks passed
@ShadowCurse ShadowCurse deleted the queue_tweaks branch August 27, 2024 13:38
bchalios added a commit to bchalios/firecracker that referenced this pull request Sep 2, 2024
Some changes that we did in the VirtIO queue in this PR:
firecracker-microvm#4726 pushed kani
proofs close to the time limit we set in the CI. This commit just
increases the timeout from 30 to 40 minutes.

Signed-off-by: Babis Chalios <[email protected]>
@bchalios bchalios mentioned this pull request Sep 2, 2024
9 tasks
bchalios added a commit to bchalios/firecracker that referenced this pull request Sep 2, 2024
Some changes that we did in the VirtIO queue in this PR:
firecracker-microvm#4726 pushed kani
proofs close to the time limit we set in the CI. This commit just
increases the timeout from 30 to 40 minutes.

Signed-off-by: Babis Chalios <[email protected]>
bchalios added a commit to bchalios/firecracker that referenced this pull request Sep 2, 2024
Some changes that we did in the VirtIO queue in this PR:
firecracker-microvm#4726 pushed kani
proofs close to the time limit we set in the CI. This commit just
increases the timeout from 30 to 40 minutes.

Signed-off-by: Babis Chalios <[email protected]>
bchalios added a commit to bchalios/firecracker that referenced this pull request Sep 2, 2024
Some changes that we did in the VirtIO queue in this PR:
firecracker-microvm#4726 pushed kani
proofs close to the time limit we set in the CI. This commit just
increases the timeout from 30 to 40 minutes.

Signed-off-by: Babis Chalios <[email protected]>
bchalios added a commit that referenced this pull request Sep 2, 2024
Some changes that we did in the VirtIO queue in this PR:
#4726 pushed kani
proofs close to the time limit we set in the CI. This commit just
increases the timeout from 30 to 40 minutes.

Signed-off-by: Babis Chalios <[email protected]>
bchalios added a commit to bchalios/firecracker that referenced this pull request Sep 2, 2024
Some changes that we did in the VirtIO queue in this PR:
firecracker-microvm#4726 pushed kani
proofs close to the time limit we set in the CI. This commit just
increases the timeout from 30 to 40 minutes.

Signed-off-by: Babis Chalios <[email protected]>
(cherry picked from commit b8ca08e)
bchalios added a commit to bchalios/firecracker that referenced this pull request Sep 2, 2024
Some changes that we did in the VirtIO queue in this PR:
firecracker-microvm#4726 pushed kani
proofs close to the time limit we set in the CI. This commit just
increases the timeout from 30 to 40 minutes.

(cherry picked from commit b8ca08e)

Signed-off-by: Babis Chalios <[email protected]>
bchalios added a commit that referenced this pull request Sep 2, 2024
Some changes that we did in the VirtIO queue in this PR:
#4726 pushed kani
proofs close to the time limit we set in the CI. This commit just
increases the timeout from 30 to 40 minutes.

(cherry picked from commit b8ca08e)

Signed-off-by: Babis Chalios <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Awaiting review Indicates that a pull request is ready to be reviewed Type: Enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants