From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 3CDDB1FF09B for ; Mon, 28 Sep 2026 10:29:18 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id DF9162171D; Mon, 28 Sep 2026 10:29:13 +0200 (CEST) Message-ID: <4beebf9d-7f9f-4609-bd9b-7ba44704a5dc@proxmox.com> Date: Mon, 28 Sep 2026 10:28:58 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [RFC qemu-server v2 0/4] fix #7282: allow (NUMA aware) vCPU pinning To: Elias Huhsovitz , pve-devel@lists.proxmox.com References: <20260921095404.61552-1-e.huhsovitz@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790584147877 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.426 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) 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: 3BNO2RL5CO52OEUUQFCRBCSIRQNEJRLU X-Message-ID-Hash: 3BNO2RL5CO52OEUUQFCRBCSIRQNEJRLU 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: Hi, On 9/25/26 11:31 AM, Elias Huhsovitz wrote: > Thanks for the feedback! > > Sorry for not running make tidy, I always forget... I also had some > kind of auto-formatter that was applied on save via my editor. > > See further comments/questions below. > > On Thu Sep 24, 2026 at 2:25 PM CEST, Dominik Csapak wrote: >> I'm in the progress of looking through the code, but want >> to give some high level feedback 'early' >> >> I think the approach goes to far for what was intended: >> >> The pinning setting now also influences memory behavior >> but we already have the numaX options for that, the intention >> was that this is just related to the virtual cores. >> I'd prefer for it to stay that way. > > This is because i noticed major performance inconsistencies when I > didn't explicity define the numaX in this shape to bind the memory: > > numaX: cpus=0-1,hostnodes=0,memory=2048,policy=bind > >> If we want to modify the memory handling, we'll have to do >> that in the memory/numa/etc. options (or a new separate >> one) but don't conflate it with the CPU pinning. > > I will include this in a later patch. My reasoning was that I didn't > want to shift the responsibility to the user, but I guess that should > be handled in another layer of abstraction. yeah, we can do some stuff automatically, but having 'conflicting' config values makes no sense. IMHO the feature I imagined could be cli only for the beginning, no need to expose it to the user. If we want to do that we should also bring the numaX configs to the ui as well. and just for the record, yes we can absolutely make this the responsibility of the user. Most won't need these kind of performance tuning settings after all. > >> Also, AFAIU this can also change the guest hardware >> depending on the host topology no? >> If yes, this is a no-go since this breaks live migration, >> potentially suspend/resume and backup/restore. > > To my understanding binding the numa nodes to specific memory > locations would definitely break live migration. (But i am not sure how > to even fix this) > not sure i understand you correctly here. just using the numaX with hostnodes would not break live migration, but dynamically creating the qemu commandline on vm start depending on the host load will definitely. as long as the hardware layout from the guests POV is the same, it doesn't matter to which nodes the vm is bound to. >> It also can mess up the guest licensing when this is fixed >> for some sockets for example. > > I didn't think about this, good point. > >> In general such an options should never overwrite the >> explicit values in the config. > > I belive I only added values and didn't override them, but I guess that > is also additional config that the user did not ask for. correct me if i'm wrong, but your patch could override the sockets/cores config, no? > >> Iff we want such a thing, this must be reflected in the >> sockets/cores/etc config (e.g. by setting 'auto' or >> some thing) and must be live migration blocker. >> >> In a first applied version I'd really like for it to focus >> on pinning vcpus on specific cores (or sets of cores). >> >> This should be opt-in and align with already existing options >> (such as numa/memory/sockets/etc) and not break >> live migration (e.g. it is ok to pin to different >> cpus/hostnodes on another node, but the hardware must >> stay the same) >> >> Making memory hostnode selection automatic and inferring guest >> topology can also make sense, but should be a separate >> patch/series and isn't really what the bug was about. >> >> Let's keep the focus narrow, this also will make the patches >> smaller and more easily digestible ;) >> >> I'll answer on the individual patches for style/code things too > > I prose the following > order of patches: > ----------------- > 1. Read/Store CPU Topology. i.e. have some kind of internal > datastrucutre that can be queried for CPU Topology information. > > 2. Introduce pinning=one-to-one|numa settings. (SMP aware via CPU > topology). i'd maybe put these two together, there is not much to gain from having the structure first without any users in the git history, but it's a minor point and probably doesn't matter either way. > > 3. Automatic host node selection and memory binding in NUMA mode. > (Opt-in) i'd maybe leave that out for now and concentrate on the cpu pinning side. the user can manually pin the numa nodes themselves (for now) > > 4. I/O Thread control, as described in [0]. The format could mirror the > affinity setting, e.g. a user could configure: iothreadpin: 0-5. would do switch this with the one before, so we first have pinning cpu, pinning io threds, then numa snode selection. > > 5. CPU Isolation: using a combination of > - isolcpus > - cpuset > - qemu.slice > > (not sure what is preffered at the moment. Some users seem to > swear by simply setting `isolcpus`.) > > 6. Expose configurations via UI. we can do that now ofc, but this can be added after the fact too, so no need to rush it. > > Optional Patches: > ----------------- > a. Make CPU Topology via API (Perhaps useful for future UI features, or > making scripting easier). what exactly do you mean by this? do you mean you want to expose it via the api? could be ok, but without a user (e.g. the webui) I'm not sure it's worth it for now. > > b. Introduce pinning=balanced mode. > > c. pre-Huge Page memory compaction, as mentioned in [0]. > > Does this sound ok, or do you have a better sequence in mind? > > Additional questions: > --------------------- > Q1: Is it not ok to introduce new perl modules? I didn't really find any > existing perl modules where was happy to put the code into from a > semantic standpoint. > > Q2: What is your opinion on the pinning=balanced mode (better name TBD)? > > Q3: Since we are planning a to move the PCI code to rust: Should the CPU > Topology code ideally also live in rust? > > References > ---------- > [0] https://forum.proxmox.com/threads/hey-proxmox-community-lets-talk-about-resources-isolation.124256/