Skip to content

Change vCPU threads exit behavior in order to avoid sigprocmask#1603

Merged
serban300 merged 3 commits intofirecracker-microvm:masterfrom
serban300:sigprocmask
Feb 17, 2020
Merged

Change vCPU threads exit behavior in order to avoid sigprocmask#1603
serban300 merged 3 commits intofirecracker-microvm:masterfrom
serban300:sigprocmask

Conversation

@serban300
Copy link
Copy Markdown
Contributor

@serban300 serban300 commented Feb 13, 2020

Reason for This PR

fixes #1456

Description of Changes

Don't allow vCPU threads to exit. If they finished execution, they will signal the VMM to exit and then wait forever.

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. Where there are two options, keep one.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • Either this PR is linked to an issue, or, the reason for this PR is
    clearly provided.
  • The description of changes is clear and encompassing.
  • Either no docs need to be updated as part of this PR, or, the required
    doc changes are included in this PR. Docs in scope are all *.md files
    located either in the repository root, or in the docs/ directory.
  • Either no code has been touched, or, code-level documentation for touched
    code is included in this PR.
  • Either no API changes are included in this PR, or, the API changes are
    reflected in firecracker/swagger.yaml.
  • Either the changes in this PR have no user impact, or, the changes in
    this PR have user impact and have been added to the CHANGELOG.md file.
  • Either no new unsafe code has been added, or, the newly added unsafe
    code is unavoidable and properly documented.

// This is the main loop of the `Exited` state.
fn exited(&mut self) -> StateMachine<Self> {
// Transition to the exited state
fn exit(&mut self, exit_code: u8) -> StateMachine<Self> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: why is this a separate function and not just the exited state?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because it needs to receive the exit_code as a parameter. From what I understand a StateFn can't receive parameters.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you say about making the exit_code a field on the Vcpu struct in order to work around this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Personally I don't like it too much because exit_code is not generic state for the vCPU. It's just a code that has to be use once, right before exit. It would be really nice if we could send parameters to the state functions. Since it's not possible for the moment, I personally prefer to have the intermediate function exit() that receives the exit_code parameter.

@serban300 serban300 force-pushed the sigprocmask branch 2 times, most recently from 84f4c95 to 61a5a6a Compare February 13, 2020 15:26
kzys pushed a commit to kzys/firecracker-containerd that referenced this pull request Feb 13, 2020
Try firecracker-microvm/firecracker#1603.

Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
@kzys
Copy link
Copy Markdown

kzys commented Feb 13, 2020

firecracker-containerd's integration tests are passing with this change!

firecracker-microvm/firecracker-containerd#383

acatangiu
acatangiu previously approved these changes Feb 13, 2020
@serban300 serban300 marked this pull request as ready for review February 13, 2020 17:17
@serban300 serban300 changed the title [Draft] Change vCPU threads exit behavior in order to avoid sigprocmask Change vCPU threads exit behavior in order to avoid sigprocmask Feb 13, 2020
acatangiu
acatangiu previously approved these changes Feb 14, 2020
@acatangiu
Copy link
Copy Markdown
Contributor

Actually a CHANGELOG::#Fixed entry would also be in order.

@dianpopa dianpopa added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Feb 14, 2020
@serban300
Copy link
Copy Markdown
Contributor Author

Actually a CHANGELOG::#Fixed entry would also be in order.

Makes sense. Done.

acatangiu
acatangiu previously approved these changes Feb 14, 2020
Serban Iorga added 3 commits February 17, 2020 13:15
When the emulation thread issues a VcpuEvent::Exit to a certain vCPU,
the vCPU state machine finishes execution but also sends a signal to the
emulation thread to close the VMM. This seems redundant. The emulation thread
can simply close the VMM directly.

Signed-off-by: Serban Iorga <seriorga@amazon.com>
Signed-off-by: Serban Iorga <seriorga@amazon.com>
exit the firecracker process with success code on KVM_EXIT_SHUTDOWN or
KVM_EXIT_HLT

Signed-off-by: Serban Iorga <seriorga@amazon.com>
@serban300 serban300 merged commit 3a9a1ac into firecracker-microvm:master Feb 17, 2020
@serban300 serban300 deleted the sigprocmask branch May 10, 2021 07:22
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.

Occasional KVM_EXIT_SHUTDOWN and bad syscall (14) during VM shutdown

4 participants