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))
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id 5A8FB9649E
for <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <f.ebner@proxmox.com>
To: pve-devel@lists.proxmox.com, "aderumier@odiso.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 <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: 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
> };