Skip to content
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

Fix 16K host page support #4916

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ShadowCurse
Copy link
Contributor

@ShadowCurse ShadowCurse commented Nov 15, 2024

Changes

Separate PAGE_SIZE constant into 2: GUEST_PAGE_SIZE and HOST_PAGE_SIZE.
HOST_PAGE_SIZE has a default of 4K, but is also queried at runtime to support other page sizes.

The only structure which did not work with non default 4K pages is IovDeque. This PR updates it
and removes restrictions on the queue size and on the host page size.

Tested16K pages support on rp5 with 16K page kernel.

Reason

Previously Firecracker was working on hosts with non 4K pages, but with 1.10 version we introduced IovDeque type
which assumed the host page size to be 4K.

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

  • I have read and understand CONTRIBUTING.md.
  • I have run tools/devtool checkstyle to verify that the PR passes the
    automated style checks.
  • I have described what is done in these changes, why they are needed, and
    how they are solving the problem in a clear and encompassing way.
  • I have updated any relevant documentation (both in code and in the docs)
    in the PR.
  • I have mentioned all user-facing changes in CHANGELOG.md.
  • If a specific issue led to this PR, this PR closes the issue.
  • When making API changes, I have followed the
    Runbook for Firecracker API changes.
  • I have tested all new and changed functionalities in unit tests and/or
    integration tests.
  • I have linked an issue to every new TODO.

  • This functionality cannot be added in rust-vmm.

@ShadowCurse ShadowCurse self-assigned this Nov 15, 2024
Copy link

codecov bot commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 88.57143% with 4 lines in your changes missing coverage. Please review.

Project coverage is 83.93%. Comparing base (70ac154) to head (ad8d232).

Files with missing lines Patch % Lines
src/firecracker/src/main.rs 0.00% 2 Missing ⚠️
src/vmm/src/arch/mod.rs 81.81% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4916      +/-   ##
==========================================
- Coverage   83.93%   83.93%   -0.01%     
==========================================
  Files         248      248              
  Lines       27791    27814      +23     
==========================================
+ Hits        23327    23345      +18     
- Misses       4464     4469       +5     
Flag Coverage Δ
5.10-c5n.metal 84.51% <87.87%> (-0.01%) ⬇️
5.10-m5n.metal 84.50% <87.87%> (+<0.01%) ⬆️
5.10-m6a.metal 83.78% <87.87%> (+<0.01%) ⬆️
5.10-m6g.metal 80.61% <88.23%> (-0.01%) ⬇️
5.10-m6i.metal 84.48% <87.87%> (-0.01%) ⬇️
5.10-m7g.metal 80.61% <88.23%> (-0.01%) ⬇️
6.1-c5n.metal 84.51% <87.87%> (-0.01%) ⬇️
6.1-m5n.metal 84.50% <87.87%> (+<0.01%) ⬆️
6.1-m6a.metal 83.77% <87.87%> (-0.01%) ⬇️
6.1-m6g.metal 80.61% <88.23%> (-0.01%) ⬇️
6.1-m6i.metal 84.48% <87.87%> (-0.02%) ⬇️
6.1-m7g.metal 80.61% <88.23%> (-0.01%) ⬇️

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 net_iov_16K_fix branch 2 times, most recently from 2f523ef to a879102 Compare November 15, 2024 13:55
@ShadowCurse ShadowCurse marked this pull request as ready for review November 15, 2024 14:31
@ShadowCurse ShadowCurse requested review from bchalios and roypat and removed request for bchalios November 15, 2024 14:33
@ShadowCurse ShadowCurse added Status: Awaiting review Indicates that a pull request is ready to be reviewed Type: Fix Indicates a fix to existing code labels Nov 15, 2024
src/vmm/src/arch/mod.rs Outdated Show resolved Hide resolved
@ShadowCurse ShadowCurse force-pushed the net_iov_16K_fix branch 5 times, most recently from a10e8bb to c8f2643 Compare November 21, 2024 15:47
@ShadowCurse ShadowCurse requested review from kalyazin and pb8o November 21, 2024 17:15
@ShadowCurse ShadowCurse force-pushed the net_iov_16K_fix branch 3 times, most recently from 9c4e018 to b86cdc7 Compare November 26, 2024 09:36
src/vmm/src/devices/virtio/iov_deque.rs Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/vmm/src/arch/aarch64/mod.rs Show resolved Hide resolved
src/vmm/src/arch/mod.rs Outdated Show resolved Hide resolved
@ShadowCurse ShadowCurse force-pushed the net_iov_16K_fix branch 3 times, most recently from 173d4b6 to 2b43582 Compare November 27, 2024 19:01
kalyazin
kalyazin previously approved these changes Nov 28, 2024
CHANGELOG.md Outdated Show resolved Hide resolved
src/vmm/src/arch/mod.rs Outdated Show resolved Hide resolved
src/vmm/src/devices/virtio/iov_deque.rs Outdated Show resolved Hide resolved
@ShadowCurse ShadowCurse force-pushed the net_iov_16K_fix branch 4 times, most recently from b4c65dd to 661d027 Compare November 29, 2024 10:21
@ShadowCurse ShadowCurse requested a review from roypat November 29, 2024 10:54
@ShadowCurse ShadowCurse force-pushed the net_iov_16K_fix branch 4 times, most recently from 9f67b77 to d51aca0 Compare December 11, 2024 17:23
@ShadowCurse ShadowCurse added Status: Awaiting review Indicates that a pull request is ready to be reviewed and removed Status: Awaiting review Indicates that a pull request is ready to be reviewed labels Dec 11, 2024
@ShadowCurse ShadowCurse requested a review from kalyazin December 11, 2024 17:27
Firecracker was assuming page sizes for both
host and guest are 4K. But they can differ, so
split into 2 values.

Signed-off-by: Egor Lazarchuk <[email protected]>
Define global variable with host page size and
update it at the very beginning of the main function
in Firecracker. This way data types which rely on
specific host page size can adapt to it.

Signed-off-by: Egor Lazarchuk <[email protected]>
Remove restriction on size and host page size.

Signed-off-by: Egor Lazarchuk <[email protected]>
Add note about making `IovDeque` to work with any
host page size.

Signed-off-by: Egor Lazarchuk <[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: Fix Indicates a fix to existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants