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

Turn down the volume on tokio threads #699

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pfmooney
Copy link
Collaborator

@pfmooney pfmooney commented May 3, 2024

Rather than letting Tokio go wild with its default of spawning worker threads equal to the number of visible host CPUs, we constrain that number to the number of guest vCPUs (down to a minimum of 8).

Fixes #686

@pfmooney pfmooney added this to the 9 milestone May 3, 2024
@pfmooney
Copy link
Collaborator Author

pfmooney commented May 3, 2024

Due to the unknown performance implications when it comes to crucible upstairs, this should not be merged until measurement can be performed on that front.

@pfmooney pfmooney requested a review from hawkw May 7, 2024 16:48
bin/propolis-standalone/src/main.rs Outdated Show resolved Hide resolved
bin/propolis-server/src/lib/server.rs Outdated Show resolved Hide resolved
Rather than letting Tokio go wild with its default of spawning worker
threads equal to the number of visible host CPUs, we constrain that
number to the number of guest vCPUs (down to a minimum of 8).

Fixes oxidecomputer#686
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Looks good to me whenever we've done the requisite performance analysis!

@@ -42,6 +42,7 @@ use std::{
time::Duration,
};

use anyhow::Context as AnyhowContext;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hot tip: if you need trait methods to be in-scope but the name is colliding, you can do

Suggested change
use anyhow::Context as AnyhowContext;
use anyhow::Context as _;

pfmooney pushed a commit to pfmooney/propolis that referenced this pull request May 16, 2024
Picks up:

    * 9f69dea - Actually provision read-only downstairs! (#728)
    * 92ae012 - crutest use SIGUSR1 to stop tests (oxidecomputer#699)
    * 47f3569 - Add build info and version commands (oxidecomputer#709)
    * 07f0a9e - Give up on a downstairs (#725)
    * 9985a4c - Slightly better versioning between upstairs and downstairs (oxidecomputer#705)
    * 0b34a53 - Fix paths in method scripts (oxidecomputer#703)
    * 12b65eb - [smf] Allow Crucible Zones to be self-assembling (oxidecomputer#498)

Also updates uuid dep to 1.3.2.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bound tokio worker thread count
3 participants