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

[Bug] Duplicated attach_pid and create_dir_all on cgroup v2 #2856

Open
3 tasks done
XieGuochao opened this issue Jan 12, 2022 · 3 comments · May be fixed by #4387
Open
3 tasks done

[Bug] Duplicated attach_pid and create_dir_all on cgroup v2 #2856

XieGuochao opened this issue Jan 12, 2022 · 3 comments · May be fixed by #4387
Assignees
@pb8o
Labels
Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` Type: Performance

Comments

@XieGuochao
Copy link

Describe the bug

The current implementation contains duplicated attach_pid and create_dir_all. Since cgroup v2 uses a unified hierarchy, there is no need to do multiple create_dir_all and attach_pid. Only one create_dir_all and one attach_pid are sufficient. Such redundant overheads will cause significant latency when multiple Firecrackers boot in parallel.

To Reproduce

You may use strace to see the redundant operations.

Additional context

The relevant codes are in src/jailer/src/cgroup.rs and src/jailer/src/env.rs. You may need to distinguish the cgroup V1 and V2 when preparing the cgroup environment.

Checks

  • Have you searched the Firecracker Issues database for similar problems?
  • Have you read the existing relevant Firecracker documentation?
  • Are you certain the bug being reported is a Firecracker issue?
@XieGuochao XieGuochao added the Type: Bug Indicates an unexpected problem or unintended behavior label Jan 12, 2022
@JonathanWoollett-Light JonathanWoollett-Light added the Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` label Apr 11, 2023
@pb8o
Copy link
Contributor

pb8o commented Aug 23, 2023

Hi, I hadn't seen this issue before. Thanks for the suggestion. It is very interesting, do you have a way of reproducing this so we can analyze the performance difference?

@pb8o pb8o added Type: Performance and removed Type: Bug Indicates an unexpected problem or unintended behavior labels Aug 23, 2023
@XieGuochao
Copy link
Author

Hi, a micro-benchmark that creates Firecrackers (jailers) with multiple threads can reproduce this issue, although I am not sure if this is a case in production where cloud providers bypass concurrency contentions with tricks like reusing Firecrackers. If this is a valuable use case, I can draft the micro-benchmark to reproduce this.

@pb8o pb8o self-assigned this Oct 5, 2023
@pb8o
Copy link
Contributor

pb8o commented Oct 5, 2023

I think having a simple example how to set up the cgroups and how to run one firecracker would be enough to reproduce it. Thanks!

@mancio-aws mancio-aws mentioned this issue Jan 19, 2024
Closed
9 tasks
@pb8o pb8o linked a pull request Jan 22, 2024 that will close this issue
Open
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@pb8o pb8o

Labels
Priority: Low Indicates that an issue or pull request should be resolved behind issues or pull requests labelled ` Type: Performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

refactor cgroup to allow multiple properties per controller mancio-aws/firecracker
3 participants
@JonathanWoollett-Light @XieGuochao @pb8o