Re: [pve-devel] [PATCH v2 qemu-server 6/9] memory: use 64 slots && static dimm size when max is defined

[Fiona Ebner <f.ebner@proxmox.com>]

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
>         };