From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 9BC6B1FF0B7 for ; Fri, 30 Oct 2026 14:47:20 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 86E732168C; Fri, 30 Oct 2026 14:46:59 +0100 (CET) Message-ID: Date: Fri, 30 Oct 2026 14:46:51 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH qemu-server-rs v3 1/3] cpu: add pve-qemu-server-cpu crate for vCPU binding To: Elias Huhsovitz , pve-devel@lists.proxmox.com References: <20261022092227.58871-1-e.huhsovitz@proxmox.com> <20261022092227.58871-2-e.huhsovitz@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20261022092227.58871-2-e.huhsovitz@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793368011732 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.341 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) KAM_SHORT 0.001 Use of a URL Shortener for very short URL RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: S6V2WYZSOFQBRTFWTSVBKQGFQGCJWJGR X-Message-ID-Hash: S6V2WYZSOFQBRTFWTSVBKQGFQGCJWJGR X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: high level: I'd like to see more comments for methods to explain why they do what they do, or if they are complex a short summary of what they do. some comments inline On 10/22/26 11:23 AM, Elias Huhsovitz wrote: > Add a new library crate that implements CPU binding on top of libhwloc. > Two public entry points are exposed: > > * validate_binding(mode, sockets, cores, cfg) - pure pre-start > validation. Checks cfg syntax, host CPU existence, vCPU range, and > NUMA node existence against the local topology. No side effects. > > * bind_vcpus(mode, cfg, cpuinfo_json) - applies the binding to a running > QEMU process. Consumes the query-cpus-fast QMP response and binds each > vCPU thread by kernel TID. > > Five modes are implemented: > * none: no binding > * manual: explicit vCPU:hostCPU mapping > * one-to-one: one vCPU per physical core, SMT stripped > * numa: per-vNUMA-node set-to-set binding > * numa-manual: explicit vNUMA->host NUMA mapping > > hwloc is accessed via a small FFI subset (src/bindings.rs). > > Thread binding uses hwloc_linux_set_tid_cpubind, which accepts a kernel > TID directly. The TID used is reported by qemu via query-cpus-fast. > > Signed-off-by: Elias Huhsovitz > --- > Cargo.toml | 2 +- > build.sh | 2 +- > pve-qemu-server-cpu/Cargo.toml | 16 + > pve-qemu-server-cpu/debian/changelog | 5 + > pve-qemu-server-cpu/debian/control | 41 ++ > pve-qemu-server-cpu/debian/copyright | 18 + > pve-qemu-server-cpu/debian/debcargo.toml | 9 + > pve-qemu-server-cpu/src/bind.rs | 478 ++++++++++++++++++++++ > pve-qemu-server-cpu/src/error.rs | 38 ++ > pve-qemu-server-cpu/src/hwloc/bindings.rs | 171 ++++++++ > pve-qemu-server-cpu/src/hwloc/mod.rs | 222 ++++++++++ > pve-qemu-server-cpu/src/lib.rs | 92 +++++ > pve-qemu-server-cpu/src/qmp_cpu.rs | 99 +++++ > 13 files changed, 1191 insertions(+), 2 deletions(-) > create mode 100644 pve-qemu-server-cpu/Cargo.toml > create mode 100644 pve-qemu-server-cpu/debian/changelog > create mode 100644 pve-qemu-server-cpu/debian/control > create mode 100644 pve-qemu-server-cpu/debian/copyright > create mode 100644 pve-qemu-server-cpu/debian/debcargo.toml > create mode 100644 pve-qemu-server-cpu/src/bind.rs > create mode 100644 pve-qemu-server-cpu/src/error.rs > create mode 100644 pve-qemu-server-cpu/src/hwloc/bindings.rs > create mode 100644 pve-qemu-server-cpu/src/hwloc/mod.rs > create mode 100644 pve-qemu-server-cpu/src/lib.rs > create mode 100644 pve-qemu-server-cpu/src/qmp_cpu.rs > > diff --git a/Cargo.toml b/Cargo.toml > index 71bc8e5..c100e5a 100644 > --- a/Cargo.toml > +++ b/Cargo.toml > @@ -1,5 +1,6 @@ > [workspace] > members = [ > + "pve-qemu-server-cpu", > "pve-qemu-server-pci" > ] > exclude = [ > @@ -26,4 +27,3 @@ strum = { version = "0.26", features = ["derive"] } > > # proxmox dependencies > pve-api-types = "8.1" > - > diff --git a/build.sh b/build.sh > index 3c2293b..c3ee7b6 100755 > --- a/build.sh > +++ b/build.sh > @@ -28,7 +28,7 @@ debcargo package \ > --no-overlay-write-back \ > --directory "$PWD/"${BUILDDIR}"/${CRATE}" \ > "${CRATE}" \ > - "$(dpkg-parsechangelog -l "${CRATE}/debian/changelog" -SVersion | sed -e 's/-.*//')" > + "$(dpkg-parsechangelog -l "${CRATE}/debian/changelog" -SVersion | sed -e 's/-.*//' -e 's/~/-/')" > > cd "${BUILDDIR}/${CRATE}" > rm -f debian/source/format.debcargo.hint > diff --git a/pve-qemu-server-cpu/Cargo.toml b/pve-qemu-server-cpu/Cargo.toml > new file mode 100644 > index 0000000..b236796 > --- /dev/null > +++ b/pve-qemu-server-cpu/Cargo.toml > @@ -0,0 +1,16 @@ > +[package] > +name = "pve-qemu-server-cpu" > +version = "0.1.0" > +authors.workspace = true > +edition.workspace = true > +license.workspace = true > +repository.workspace = true > +homepage.workspace = true > +exclude.workspace = true > +rust-version.workspace = true > + > +[dependencies] > +serde = { version = "1", features = ["derive"] } > +serde_json = "1" > +thiserror = "2" > +libc = "0.2" > diff --git a/pve-qemu-server-cpu/debian/changelog b/pve-qemu-server-cpu/debian/changelog > new file mode 100644 > index 0000000..3731cfd > --- /dev/null > +++ b/pve-qemu-server-cpu/debian/changelog > @@ -0,0 +1,5 @@ > +rust-pve-qemu-server-cpu (0.1.0) trixie; urgency=medium > + > + * initial release > + > + -- Proxmox Support Team Tue, 01 Sep 2026 14:02:25 +0200 > diff --git a/pve-qemu-server-cpu/debian/control b/pve-qemu-server-cpu/debian/control > new file mode 100644 > index 0000000..1298d61 > --- /dev/null > +++ b/pve-qemu-server-cpu/debian/control > @@ -0,0 +1,41 @@ > +Source: rust-pve-qemu-server-cpu > +Section: rust > +Priority: optional > +Build-Depends: debhelper-compat (= 13), > + dh-sequence-cargo, > + libhwloc-dev (>= 2.12) > +Build-Depends-Arch: cargo:native , > + rustc:native (>= 1.94) , > + libstd-rust-dev , > + librust-libc-0.2+default-dev , > + librust-serde-1+default-dev , > + librust-serde-1+derive-dev , > + librust-serde-json-1+default-dev , > + librust-thiserror-2+default-dev > +Maintainer: Proxmox Support Team > +Standards-Version: 4.7.2 > +Vcs-Git: git://git.proxmox.com/git/pve-qemu-server-rs.git > +Vcs-Browser: https://git.proxmox.com/?p=pve-qemu-server-rs.git > +Homepage: https://proxmox.com > +X-Cargo-Crate: pve-qemu-server-cpu > + > +Package: librust-pve-qemu-server-cpu-dev > +Architecture: any > +Multi-Arch: same > +Depends: > + ${misc:Depends}, > + librust-libc-0.2+default-dev, > + librust-serde-1+default-dev, > + librust-serde-1+derive-dev, > + librust-serde-json-1+default-dev, > + librust-thiserror-2+default-dev > +Provides: > + librust-pve-qemu-server-cpu+default-dev (= ${binary:Version}), > + librust-pve-qemu-server-cpu-0-dev (= ${binary:Version}), > + librust-pve-qemu-server-cpu-0+default-dev (= ${binary:Version}), > + librust-pve-qemu-server-cpu-0.1-dev (= ${binary:Version}), > + librust-pve-qemu-server-cpu-0.1+default-dev (= ${binary:Version}), > + librust-pve-qemu-server-cpu-0.1.0-dev (= ${binary:Version}), > + librust-pve-qemu-server-cpu-0.1.0+default-dev (= ${binary:Version}) > +Description: Rust crate "pve-qemu-server-cpu" - Rust source code > + Source code for Debianized Rust crate "pve-qemu-server-cpu" > diff --git a/pve-qemu-server-cpu/debian/copyright b/pve-qemu-server-cpu/debian/copyright > new file mode 100644 > index 0000000..01138fa > --- /dev/null > +++ b/pve-qemu-server-cpu/debian/copyright > @@ -0,0 +1,18 @@ > +Format: https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/ > + > +Files: > + * > +Copyright: 2026 Proxmox Server Solutions GmbH > +License: AGPL-3.0-or-later > + This program is free software: you can redistribute it and/or modify it under > + the terms of the GNU Affero General Public License as published by the Free > + Software Foundation, either version 3 of the License, or (at your option) any > + later version. > + . > + This program is distributed in the hope that it will be useful, but WITHOUT > + ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS > + FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more > + details. > + . > + You should have received a copy of the GNU Affero General Public License along > + with this program. If not, see . > diff --git a/pve-qemu-server-cpu/debian/debcargo.toml b/pve-qemu-server-cpu/debian/debcargo.toml > new file mode 100644 > index 0000000..23feea4 > --- /dev/null > +++ b/pve-qemu-server-cpu/debian/debcargo.toml > @@ -0,0 +1,9 @@ > +overlay = "." > +crate_src_path = ".." > +maintainer = "Proxmox Support Team " > +allow_prerelease_deps = true > + > +[source] > +vcs_git = "git://git.proxmox.com/git/pve-qemu-server-rs.git" > +vcs_browser = "https://git.proxmox.com/?p=pve-qemu-server-rs.git" > +build_depends = ["libhwloc-dev (>= 2.12)"] > diff --git a/pve-qemu-server-cpu/src/bind.rs b/pve-qemu-server-cpu/src/bind.rs > new file mode 100644 > index 0000000..4f2927d > --- /dev/null > +++ b/pve-qemu-server-cpu/src/bind.rs > @@ -0,0 +1,478 @@ > +use std::collections::HashSet; > + > +use crate::hwloc::{self, CpuSet, Topology}; > +use crate::qmp_cpu::QmpCpu; > +use crate::BindError; > + > +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] > +struct Cpu(u32); > + > +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] > +struct NumaNodeId(u32); > + > +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] > +struct Vcpu(u32); for these i think it would make sense to implement deref too, so we can simply use it as their inner values? > + > +#[derive(Debug, Clone, PartialEq, Eq, Hash)] > +struct VnumaNode { > + id: NumaNodeId, > + cpus: Vec, wouldn't it make more sense for this to be a hashset? otherwise i could have duplicates in the list? AFAICS the only way we use it is by iterating, so a hashset makes more sense imo > +} > + > +#[derive(Debug, Clone)] > +struct BindRule { > + vcpus: Vec, same here > + cpuset: CpuSet, > +} > + > +#[derive(Debug, Clone)] > +struct NumaBindRule { > + vnode: VnumaNode, > + host_node: NumaNodeId, > +} > + > +impl From for Cpu { > + fn from(v: u32) -> Self { > + Self(v) > + } > +} > +impl From for Vcpu { > + fn from(v: u32) -> Self { > + Self(v) > + } > +} > +impl From for NumaNodeId { > + fn from(v: u32) -> Self { > + Self(v) > + } > +} > + > +trait BindableRule { > + fn vcpus(&self) -> &[Vcpu]; > + fn resolve_cpuset(&self, topo: &Topology) -> Result; > +} i'd really like to see some documentation for these here. What does a 'bindablerule' do? and what should it resolve to? > + > +impl BindableRule for BindRule { > + fn vcpus(&self) -> &[Vcpu] { > + &self.vcpus > + } > + fn resolve_cpuset(&self, _topo: &Topology) -> Result { > + Ok(self.cpuset.clone()) > + } > +} > + > +impl BindableRule for NumaBindRule { > + fn vcpus(&self) -> &[Vcpu] { > + &self.vnode.cpus > + } > + fn resolve_cpuset(&self, topo: &Topology) -> Result { > + hwloc::numa_node_cpuset(topo, self.host_node.0).ok_or(BindError::UnknownHostCpu(self.host_node.0)) > + } > +} instead of having a trait here, having an enum BindRule { Regular { .. }, Numa { .. } } would make the code more readable i think and forgo the generic functions. I don't forsee many more rule types and since the trait is not public, others can't implment it anyway. > + > +fn bind(topo: &Topology, vcpu: &Vcpu, tid: u32, set: &CpuSet) -> Result<(), BindError> { > + // Note: DO NOT singlify here — we want the scheduler to be free to move > + // the vCPU within the specified set. > + hwloc::bind_thread(topo, tid as i32, set).map_err(|message| BindError::Bind { > + vcpu: vcpu.0, > + message, > + }) > +} > + > +fn bind_rules( > + topo: &Topology, > + rules: &[R], > + qmp_cpuinfo: &[QmpCpu], > +) -> Result<(), BindError> { > + for rule in rules { > + let set = rule.resolve_cpuset(topo)?; > + for vcpu in rule.vcpus() { > + let tid = qmp_cpuinfo > + .get(vcpu.0 as usize) > + .ok_or(BindError::ThreadNotFound(vcpu.0))? > + .thread_id; > + bind(topo, vcpu, tid, &set)?; > + } > + } > + Ok(()) > +} > + > +pub enum Mode { > + None, > + Manual, > + OneToOne, > + Numa, > + NumaManual, > +} > + > +impl std::str::FromStr for Mode { > + type Err = BindError; > + fn from_str(s: &str) -> Result { > + match s { > + "none" => Ok(Mode::None), > + "manual" => Ok(Mode::Manual), > + "one-to-one" => Ok(Mode::OneToOne), > + "numa" => Ok(Mode::Numa), > + "numa-manual" => Ok(Mode::NumaManual), > + other => Err(BindError::BadSyntax(format!("unknown cpu-bind-mode '{other}'"))), > + } > + } > +} > + > +fn to_cpuset(cpus: &[u32]) -> CpuSet { > + let mut set = CpuSet::new(); > + for &c in cpus { > + set.set(c); > + } > + set > +} > + this could be a `From<&[u32]>` implementation on CpuSet (or simply have it inline in the one place it's needed) > +fn parse_cpu_binding_map(map: &str) -> Result, BindError> { > + if map.is_empty() { > + return Err(BindError::BadSyntax("binding map is empty".into())); > + } > + > + let mut rules = Vec::new(); > + for entry in map.split(';') { > + let (vcpu_side, host_side) = entry > + .split_once(':') > + .ok_or_else(|| BindError::MissingColon(entry.to_string()))?; > + > + let vcpus: Vec = parse_side(vcpu_side)?.into_iter().map(Vcpu::from).collect(); > + let cpuset = to_cpuset(&parse_side(host_side)?); > + > + rules.push(BindRule { vcpus, cpuset }); > + } > + Ok(rules) > +} > + > +fn parse_side(s: &str) -> Result, BindError> { > + if let Some(inner) = s.strip_prefix('{').and_then(|r| r.strip_suffix('}')) { > + let mut out = Vec::new(); > + for part in inner.split(',') { > + out.extend(parse_element(part)?); > + } > + Ok(out) > + } else { > + if s.contains(',') { > + return Err(BindError::BadSyntax(format!( > + "unions require braces: write '{{0,1}}', not '{s}'" > + ))); > + } > + parse_element(s) > + } > +} > + > +fn parse_element(s: &str) -> Result, BindError> { > + if let Some((a, b)) = s.split_once('-') { > + let a: u32 = a > + .parse() > + .map_err(|_| BindError::BadSyntax(format!("bad index in '{s}'")))?; > + let b: u32 = b > + .parse() > + .map_err(|_| BindError::BadSyntax(format!("bad index in '{s}'")))?; > + if a > b { > + return Err(BindError::BadSyntax(format!("reversed range '{s}'"))); > + } > + Ok((a..=b).collect()) this seems excessive? a config like 0-40000000 allocates a bunch of memory for saving a list of vcpus? we should either limit these upfront somewhere, or just saving a sparse list/range/etc. > + } else { > + Ok(vec![s > + .parse() > + .map_err(|_| BindError::BadSyntax(format!("bad index '{s}'")))?]) > + } > +} > + > +fn parse_index_list>(s: &str) -> Result, BindError> { > + if s.is_empty() { > + return Ok(Vec::new()); > + } > + let mut out = Vec::new(); > + for part in s.split(';') { > + out.extend(parse_element(part)?.into_iter().map(T::from)); > + } > + Ok(out) > +} > + > +fn parse_numa_binding_map(map: &str) -> Result, BindError> { > + if map.is_empty() { > + return Err(BindError::BadSyntax("numa binding map is empty".into())); > + } > + let mut pairs = Vec::new(); > + for entry in map.split(';') { > + let entry = entry.trim(); > + let (v, h) = entry > + .split_once(':') > + .ok_or_else(|| BindError::MissingColon(entry.to_string()))?; > + let vnode: u32 = v > + .trim() > + .parse() > + .map_err(|_| BindError::BadSyntax(format!("bad vNUMA node in '{entry}'")))?; > + let hnode: u32 = h > + .trim() > + .parse() > + .map_err(|_| BindError::BadSyntax(format!("bad host NUMA node in '{entry}'")))?; > + pairs.push((NumaNodeId(vnode), NumaNodeId(hnode))); > + } > + Ok(pairs) > +} > + > +fn collect_vnuma_nodes(qmp_cpuinfo: &[QmpCpu]) -> Vec { > + let max_node = qmp_cpuinfo > + .iter() > + .map(|c| c.props.node_id) > + .max() > + .unwrap_or(0); > + let mut buckets: Vec> = vec![Vec::new(); (max_node + 1) as usize]; > + for cpu in qmp_cpuinfo { > + buckets[cpu.props.node_id as usize].push(Vcpu::from(cpu.cpu_index)); > + } > + buckets > + .into_iter() > + .enumerate() > + .map(|(id, cpus)| VnumaNode { > + id: NumaNodeId(id as u32), > + cpus, > + }) > + .collect() > +} couldn't this be also inferred from the config instead from the qmp output? I guess it does not make much difference, but I'd like to use the config as 'authority' since the actual vm should (must?) follow the config anyway, otherwise we'd have a bug somewhere.... [snip] > diff --git a/pve-qemu-server-cpu/src/error.rs b/pve-qemu-server-cpu/src/error.rs > new file mode 100644 > index 0000000..73a569e > --- /dev/null > +++ b/pve-qemu-server-cpu/src/error.rs > @@ -0,0 +1,38 @@ > +use thiserror::Error; > + > +#[derive(Debug, Error)] > +pub enum BindError { > + // Parse errors > + #[error("map entry '{0}' is missing a ':' separator")] > + MissingColon(String), > + > + #[error("invalid syntax in binding map: '{0}'")] > + BadSyntax(String), > + > + // Validation errors (pre-start) > + #[error("host CPU {0} does not exist in this topology")] > + UnknownHostCpu(u32), > + > + #[error("vCPU {0} appears in more than one map entry")] > + VcpuReused(u32), > + > + #[error("vCPU {vcpu} is out of range (VM has vCPUs 0..{max})")] > + VcpuOutOfRange { vcpu: u32, max: u32 }, > + > + // Runtime errors (post-start) > + #[error("no running thread found for vCPU {0}")] > + ThreadNotFound(u32), > + > + #[error("invalid kernel TID {tid} for vCPU {vcpu}")] > + BadThreadId { vcpu: u32, tid: u32 }, > + > + #[error("failed to init host topology: '{0}'")] > + TopologyInit(String), > + > + #[error("binding vCPU {vcpu} failed: {message}")] > + Bind { > + vcpu: u32, > + /// Human-readable error from libhwloc (errno-based). > + message: String, > + }, > +} i we'd reuse the numaX binding modes, most of these would not be necessary (as well as the parsing etc.) in general if we'd restrict ourselves (for the moment) on the basic modes + using affinity to limit the set further. the amount of information we'd need from sysfs would be a numa node -> vcpu list sysfs read everything else (no smt sibling, only 'big' cores, etc.) we could do via the affinity and the vcpu -> hostnuma node manual pinning we could do via numaX the manual mode is not really necessary since a user can simply do it themselves if they want to.