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

jailer: fix perf regression when closing open FDs #3595

Merged
merged 4 commits into from
Apr 14, 2023

Conversation

dianpopa
Copy link
Contributor

@dianpopa dianpopa commented Apr 6, 2023

Changes

Fixes the mechanism for closing open FDs in jailer which was
based on the SC_OPEN_MAX system constant. Such an approach can lead
to bad performance when this value is very high.

The method for closing file descriptors is now chosen based on the environment:

  1. we try to call into the close_range syscall (available on kernels >=5.9)
  2. we fallback to reading from /proc/self/fd (for kernels <5.9)

Reason

Fixes #3542 - a bug where firecracker would end up spending minutes starting which arises when running on systems with OPEN_MAX set to a very high number.

Thanks @gorbak25 for reporting the issue and proposing the fix!

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.

src/jailer/src/main.rs Outdated Show resolved Hide resolved
src/jailer/src/main.rs Outdated Show resolved Hide resolved
src/jailer/src/main.rs Show resolved Hide resolved
Fixes the mechanism for closing open FDs in jailer which was
based on the SC_OPEN_MAX system constant. Such an approach can lead
to bad performance when this value is very high.

The method for closing file descriptors is now chosen based
on the environment:
1. we try to call into the close_range syscall
   (available on kernels >=5.9)
2. we fallback to reading from /proc/self/fd (for kernels <5.9)

Fixes firecracker-microvm#3542.

Signed-off-by: Grzegorz Uriasz <gorbak25@gmail.com>
Co-authored-by: Diana Popa <dpopa@amazon.com>
Signed-off-by: Diana Popa <dpopa@amazon.com>
@dianpopa dianpopa added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Apr 12, 2023
Cargo.lock Show resolved Hide resolved
Signed-off-by: Diana Popa <dpopa@amazon.com>
Copy link
Contributor Author

@dianpopa dianpopa left a comment

Choose a reason for hiding this comment

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

@Lumivo @aTc @roypat I also updated the changelog, can you take another look?
@roypat addressed your comment around the nix dependency

src/jailer/src/main.rs Show resolved Hide resolved
src/jailer/src/main.rs Outdated Show resolved Hide resolved
src/jailer/src/main.rs Outdated Show resolved Hide resolved
Cargo.lock Show resolved Hide resolved
src/jailer/src/main.rs Show resolved Hide resolved
@dianpopa dianpopa merged commit e5b5cd5 into firecracker-microvm:main Apr 14, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Firecracker uses up 100% CPU after an upgrade from v1.1.2 to v1.3.1
5 participants