From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <f.ebner@proxmox.com> Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 04BC1F5E6 for <pve-devel@lists.proxmox.com>; Fri, 16 Dec 2022 14:38:28 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E2286224DF for <pve-devel@lists.proxmox.com>; Fri, 16 Dec 2022 14:38:27 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for <pve-devel@lists.proxmox.com>; Fri, 16 Dec 2022 14:38:26 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id B4E024381A; Fri, 16 Dec 2022 14:38:26 +0100 (CET) Message-ID: <3f3ceed3-3f1d-217d-a898-5db818bbc1a0@proxmox.com> Date: Fri, 16 Dec 2022 14:38:25 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 Content-Language: en-US To: pve-devel@lists.proxmox.com, aderumier@odiso.com References: <20221209192726.1499142-1-aderumier@odiso.com> From: Fiona Ebner <f.ebner@proxmox.com> In-Reply-To: <20221209192726.1499142-1-aderumier@odiso.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.029 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [memory.pm, gitlab.io] Subject: Re: [pve-devel] [PATCH qemu-server 00/10] rework memory hotplug + virtiomem X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/> List-Post: <mailto:pve-devel@lists.proxmox.com> List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>, <mailto:pve-devel-request@lists.proxmox.com?subject=subscribe> X-List-Received-Date: Fri, 16 Dec 2022 13:38:28 -0000 Am 09.12.22 um 20:27 schrieb Alexandre Derumier: > This patch series rework the current memory hotplug + virtiomem. > > memory option now have extra options: > > memory: [[current=]<integer>] [,max=<enum>] [,virtio=<1|0>] > ex: memory: current=1024,max=131072,virtio=1 > > > patches 1-2: add a memory parser > > patches 3-7: add the max option with 64 static dimm hotplug > > for classic memory hotplug, when maxmemory is defined, > we use 64 fixed size dimm. > The max option is a multiple of 64GB. > > patches 8-9: add virtio-mem > > The virtio option enable new virtio-mem support, > instead of plugging dimm, it's add/removed block inside > big dimm. > virtio-mem can use 32000 blocks, the blocksize is compute from > max memory. > > patch10: hotplug fix > > Alexandre Derumier (10): > test: add memory tests > add memory parser > config: memory: add 'max' option > memory: add get_static_mem > memory: get_max_mem: use config memory max > memory: use 64 slots && static dimm size with max is defined > test: add memory-max tests > memory: add virtio-mem support > tests: add virtio-mem tests > memory: fix hotplug with virtiomem && maxmem > The general ideas looks fine to me and my basic testing seemed fine too, but most patches could use a few improvements. The biggest issue is that getting static information in HA manager is not adapted to the change. The virtio feature should be marked as technology preview and mentioned that it only works for guests running Linux >= 5.8 [0]. IMHO, it'd be nicer to add the property string handling/registering in the Memory.pm module directly, so that all is contained in one place. I also think that using a validator for the format might be worth it. Also, the tests should be made more host-independent, but for the NUMA node issue 01/10 that might require a bit of rework, to be able to mock the relevant parts. Should the virtio feature be QEMU-version-guarded? It's explicitly opt-in and technology preview, so maybe not really needed? See the individual replies on patches for details. [0]: https://virtio-mem.gitlab.io/user-guide/