From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 5A8FB9649E for ; Tue, 24 Jan 2023 14:06:13 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 40A4D3E0A for ; Tue, 24 Jan 2023 14:06:13 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 24 Jan 2023 14:06:12 +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 0BE2F45EAA; Tue, 24 Jan 2023 14:06:12 +0100 (CET) Message-ID: <70ef3f8a-2a0c-f7da-8f04-d4b73e13df9d@proxmox.com> Date: Tue, 24 Jan 2023 14:06:10 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 From: Fiona Ebner To: pve-devel@lists.proxmox.com, "aderumier@odiso.com" References: <20230104064303.2898194-1-aderumier@odiso.com> <20230104064303.2898194-7-aderumier@odiso.com> Content-Language: en-US In-Reply-To: <20230104064303.2898194-7-aderumier@odiso.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.590 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 -1.148 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. [qmpqga.pm] Subject: Re: [pve-devel] [PATCH v2 qemu-server 6/9] memory: use 64 slots && static dimm size when max is defined X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 24 Jan 2023 13:06:13 -0000 Am 04.01.23 um 07:43 schrieb Alexandre Derumier: > @@ -185,14 +191,15 @@ sub foreach_dimm{ > my ($conf, $vmid, $memory, $sockets, $func) = @_; > > my $dimm_id = 0; > - my $current_size = 0; > + my $current_size = get_static_mem($conf); Nit: Using the new method could/should be part of patch 3/9 already > my $dimm_size = 0; > > - if($conf->{hugepages} && $conf->{hugepages} == 1024) { > - $current_size = 1024 * $sockets; > + my $confmem = parse_memory($conf->{memory}); > + if ($confmem->{max}) { > + $dimm_size = $confmem->{max} / $MAX_SLOTS; > + } elsif($conf->{hugepages} && $conf->{hugepages} == 1024) { > $dimm_size = 1024; > } else { > - $current_size = 1024; > $dimm_size = 512; > } > Question about the existing code: The loops below can count up to $dimm_id 255, but in the commit message you say that there are at most 255 slots (so the highest ID is 254?). But yeah, it only becomes relevant when going all the way to approximately 4 TiB. > @@ -209,7 +216,7 @@ sub foreach_dimm{ > &$func($conf, $vmid, $name, $dimm_size, $numanode, $current_size, $memory); > return $current_size if $current_size >= $memory; > } > - $dimm_size *= 2; > + $dimm_size *= 2 if !$confmem->{max}; > } > } > > @@ -220,7 +227,12 @@ sub foreach_reverse_dimm { Question about the existing code: There is my $dimm_id = 253; Shouldn't that start at 254 (highest valid ID we can count up to?). Again only becomes relevant with a lot of memory. > my $current_size = 0; > my $dimm_size = 0; > > - if($conf->{hugepages} && $conf->{hugepages} == 1024) { > + my $confmem = parse_memory($conf->{memory}); > + if ($confmem->{max}) { > + $dimm_id = $MAX_SLOTS - 1; > + $current_size = $confmem->{max}; Does this need to be $confmem->{max} + $static_size? See below for a description of the issue. Didn't think about it in detail, so please double check ;) > + $dimm_size = $confmem->{max} / $MAX_SLOTS; > + } elsif ($conf->{hugepages} && $conf->{hugepages} == 1024) { > $current_size = 8355840; > $dimm_size = 131072; > } else { Nit: the loops below here are for (my $j = 0; $j < 8; $j++) { for (my $i = 0; $i < 32; $i++) { so it looks like potentially iterating more often than $MAX_SLOTS and reaching negative $dimm_ids. I know that we should always return from the loop earlier than that, but maybe it can be improved by extracting the inner part in a sub/closure and using different loops depending on how many slots there are? Same applies to foreach_dimm(). Real issue: something is wrong with the calculation for unplugging in combination with 'max' (it uses the wrong dimm IDs): > root@pve701 ~ # cat qmpqga.pm > #!/bin/perl > > use strict; > use warnings; > > use Data::Dumper; > $Data::Dumper::Sortkeys = 1; > use Time::HiRes qw(usleep ualarm gettimeofday tv_interval); > > use PVE::QemuServer::Monitor qw(mon_cmd); > > my $vmid = shift or die "need to specify vmid\n"; > > my $res = eval { mon_cmd($vmid, "query-memory-devices") }; > warn $@ if $@; > for my $dimm ($res->@*) { > my ($id, $size) = $dimm->{data}->@{qw(id size)}; > print "$id: $size\n"; > } > $res = eval { mon_cmd($vmid, "query-memory-size-summary") }; > warn $@ if $@; > print Dumper($res); > > root@pve701 ~ # qm set 131 --memory 4096,max=65536 > update VM 131: -memory 4096,max=65536 > root@pve701 ~ # qm start 131 > root@pve701 ~ # perl qmpqga.pm 131 > $VAR1 = { > 'base-memory' => 4294967296, > 'plugged-memory' => 0 > }; > root@pve701 ~ # qm set 131 --memory 8192,max=65536 > update VM 131: -memory 8192,max=65536 > root@pve701 ~ # perl qmpqga.pm 131 > dimm0: 1073741824 > dimm1: 1073741824 > dimm2: 1073741824 > dimm3: 1073741824 > $VAR1 = { > 'base-memory' => 4294967296, > 'plugged-memory' => 4294967296 > }; > root@pve701 ~ # qm set 131 --memory 4096,max=65536 > update VM 131: -memory 4096,max=65536 > try to unplug memory dimm dimm7 > try to unplug memory dimm dimm6 > try to unplug memory dimm dimm5 > try to unplug memory dimm dimm4 Those are the wrong IDs, so the memory stays plugged. > root@pve701 ~ # perl qmpqga.pm 131 > dimm0: 1073741824 > dimm1: 1073741824 > dimm2: 1073741824 > dimm3: 1073741824 > $VAR1 = { > 'base-memory' => 4294967296, > 'plugged-memory' => 4294967296 > };