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.

Sign up for GitHub

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

Jump to bottom

[RFC] Upgrade Rust toolchain to 1.49.0 #2560

Closed
wants to merge 5 commits into from
Closed

[RFC] Upgrade Rust toolchain to 1.49.0 #2560

wants to merge 5 commits into from

Conversation

serban300
Copy link
Contributor

@serban300 serban300 commented May 13, 2021 •
edited
Loading

Reason for This PR

#2434

Description of Changes

Upgrade Rust toolchain to 1.49.0

The main highlight of this upgrade is the aarch64-unknown-linux-gnu target Tier 1 support.

  • This functionality can be added in rust-vmm.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any newly added unsafe code is properly documented.
  • Any API changes are reflected in firecracker/swagger.yaml.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

Things worth mentioning:

  1. In order to workaround the issue Linker error/regression introduced with rustc 1.48.0 on aarch64-unknown-linux-musl target rust-lang/rust#79791 on aarch64 we need to build libfdt manually, appending -fno-stack-protector to CFLAGS
  2. The process startup time increases on aarch64 when using jailer. Maybe because of this

If all these issues are acceptable, we can promote this draft to an actual pull request.

@serban300 serban300 self-assigned this May 13, 2021
@acatangiu
Copy link
Contributor

The regression on the failing block performance tests looks quite big! I propose we drill down a bit there to get more info.

@serban300
Copy link
Contributor Author

serban300 commented May 14, 2021 •
edited
Loading

The regression on the failing block performance tests looks quite big! I propose we drill down a bit there to get more info.

@acatangiu Sorry, my bad. I forgot to pin the Firecracker vcpu threads before running the block performance tests. Now they pass successfully.

@acatangiu
Copy link
Contributor

acatangiu commented May 14, 2021 •
edited
Loading

@acatangiu Sorry, my bad. I forgot to pin the Firecracker vcpu threads before running the block performance tests. Now they pass successfully.

Phew! 👍

The process startup time increases on aarch64 when using jailer. Maybe because of this

What are the numbers? 😄

L.E. Nevermind, they are included in the PR, sorry for the spam.

acatangiu
acatangiu previously approved these changes May 14, 2021
View reviewed changes
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

tests/integration_tests/performance/test_process_startup_time.py
@@ -9,7 +9,7 @@

import host_tools.logging as log_tools

MAX_STARTUP_TIME_CPU_US = {'x86_64': 5500, 'aarch64': 2800}
MAX_STARTUP_TIME_CPU_US = {'x86_64': 5500, 'aarch64': 3200}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is fine, 400us is negligible.

@acatangiu
Copy link
Contributor

Also, why not latest then? :D https://blog.rust-lang.org/2021/05/10/Rust-1.52.1.html

@serban300
Copy link
Contributor Author

Also, why not latest then? :D https://blog.rust-lang.org/2021/05/10/Rust-1.52.1.html

Upgrading to the latest Rust toolchain sounds good to me. I'll get back to this item after implementing #2153 .

Serban Iorga added 5 commits May 21, 2021 16:26
Upgrade Rust toolchain to 1.49
b56d050 
Signed-off-by: Serban Iorga <[email protected]>
adjust coverage and perf numbers
f2ea117 
Signed-off-by: Serban Iorga <[email protected]>
fix trivial clippy warnings
a093f12 
Signed-off-by: Serban Iorga <[email protected]>
Silence false clippy warning
eb27c8a 
Signed-off-by: Serban Iorga <[email protected]>
fix clippy::result_unit_err
31de1b4 
Signed-off-by: Serban Iorga <[email protected]>
@serban300
Copy link
Contributor Author

Resolving in favor of #2579

@serban300 serban300 closed this May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acatangiu acatangiu acatangiu left review comments

Assignees

@serban300 serban300

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@serban300 @acatangiu