bins: intial CPU binding support #991
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
add bindings for `processor_bind()`, the mechanism to make this happen. then, in propolis-standalone, allow a choice of how binding should be done: allow vCPUs to roam with `cpu_binding = "any"` or pin vCPUs to CPUs 1:1 from the last processor downwards with `cpu_binding = "from-last"`. in `propolis-server`, pick between these two strategies automatically based on how the vCPU count compares to the number of available processors. a VM that would use more than half of available processors is bound in the same way as `propolis-standalone` with `cpu_binding = "from-last"`, but otherwise continues to have unbound vCPUs. in the future, Nexus will dictate `propolis-server`'s vCPU bindings, but exactly how is a bit of an open question.
hawkw
approved these changes
Dec 19, 2025
| @@ -0,0 +1,8 @@ | |||
| [package] | |||
| name = "pbind" | |||
Member
There was a problem hiding this comment.
hm, since this is a low-level binding to the C headers, should this perhaps be pbind-sys?
|
|
||
| res.try_into().map_err(|_| { | ||
| // sysconf() reports more than 2^31 processors?! | ||
| Error::other(format!("too many processors: {}", res)) |
Member
There was a problem hiding this comment.
turbo nit: perhaps res could be inlined here?
Suggested change
| Error::other(format!("too many processors: {}", res)) | |
| Error::other(format!("too many processors: {res}")) |
Comment on lines
+48
to
+50
| // Returns an `i32` to match `processorid_t`, so that `0..online_cpus()` | ||
| // produces a range of processor IDs without additional translation needed. | ||
| pub fn online_cpus() -> Result<i32, Error> { |
Member
There was a problem hiding this comment.
maybe worth noting in the comment that this is a wrapper around sysconf(_SC_NPROCESSORS_ONLN)?
Comment on lines
+294
to
+295
| let vcpu_count: i32 = | ||
| machine.vcpus.len().try_into().expect("<2^31 vCPUs"); |
Member
There was a problem hiding this comment.
nit, considered not that big a deal: this is an error in the user's config file, rather than e.g. a syscall we expect should always work not working. can we make that a little clearer perhaps?
Comment on lines
+80
to
+82
| /// vCPUs are bound to the highest-numbered processors in the system, one | ||
| /// vCPU per CPU, with the last vCPU bound to the last physical processor. | ||
| FromLast, |
Comment on lines
+591
to
+600
| let bind_cpus = if vcpu_count > total_cpus / 2 { | ||
| let mut bind_cpus = Vec::new(); | ||
| for i in 0..vcpu_count { | ||
| // Bind to the upper range of CPUs, fairly arbitrary. | ||
| bind_cpus.push(total_cpus - vcpu_count + i); | ||
| } | ||
| Some(bind_cpus) | ||
| } else { | ||
| None | ||
| }; |
Member
There was a problem hiding this comment.
maybe we ought to log someplace whether we have decided to bind vCPUs or not?
Comment on lines
+592
to
+596
| let mut bind_cpus = Vec::new(); | ||
| for i in 0..vcpu_count { | ||
| // Bind to the upper range of CPUs, fairly arbitrary. | ||
| bind_cpus.push(total_cpus - vcpu_count + i); | ||
| } |
Member
There was a problem hiding this comment.
seems like this could be
Suggested change
| let mut bind_cpus = Vec::new(); | |
| for i in 0..vcpu_count { | |
| // Bind to the upper range of CPUs, fairly arbitrary. | |
| bind_cpus.push(total_cpus - vcpu_count + i); | |
| } | |
| ((total_cpus-vcpu_count)..total_cpus).iter().collect() |
or something but maybe that's weirder and worse
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this is the interim option I'd described in #984.
add bindings for
processor_bind(), the mechanism to make this happen. then, in propolis-standalone, allow a choice of how binding should be done: allow vCPUs to roam withcpu_binding = "any"or pin vCPUs to CPUs 1:1 from the last processor downwards withcpu_binding = "from-last".in
propolis-server, pick between these two strategies automatically based on how the vCPU count compares to the number of available processors. a VM that would use more than half of available processors is bound in the same way aspropolis-standalonewithcpu_binding = "from-last", but otherwise continues to have unbound vCPUs.in the future, Nexus will dictate
propolis-server's vCPU bindings, but exactly how is a bit of an open question.