Re: [pve-devel] [PATCH v2 qemu-server 4/9] config: memory: add 'max' option

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

Am 04.01.23 um 07:42 schrieb Alexandre Derumier:
> max can be multiple of 64GB only,
> The dimm size is compute from the max memory
> 
> we can have 64 slots:
> 
> 64GB = 64 slots x 1GB
> 128GB = 64 slots x 2GB
> ..
> 4TB = 64 slots x 64GB
> 
> Also, with numa, we need to share slot between (up to 8) sockets.
> 
> 64 is a multiple of 8,
> 
> 64GB = 8 sockets * 8 slots * 1GB
> 128GB = 8 sockets * 8 slots * 2GB
> ...
> 
> and with virtio-mem,
> we can have 32000 blocks of 2M minimum
> 
> 64GB = 32000 * 2MB

The units above should all be TiB, GiB and MiB, right? The virtio-mem
documentation also talks about MiB. But then 32000 * 2MiB isn't 64 GiB.

> 
> Signed-off-by: Alexandre Derumier <aderumier@odiso.com>
> ---
>  PVE/QemuServer/Memory.pm | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/QemuServer/Memory.pm b/PVE/QemuServer/Memory.pm
> index e9c0115..1c4f356 100644
> --- a/PVE/QemuServer/Memory.pm
> +++ b/PVE/QemuServer/Memory.pm
> @@ -5,6 +5,7 @@ use warnings;
>  
>  use PVE::Tools qw(run_command lock_file lock_file_full file_read_firstline dir_glob_foreach);
>  use PVE::Exception qw(raise raise_param_exc);
> +use PVE::GuestHelpers qw(safe_string_ne safe_num_ne safe_boolean_ne);
>  
>  use PVE::QemuServer;
>  use PVE::QemuServer::Monitor qw(mon_cmd);
> @@ -25,7 +26,14 @@ my $memory_fmt = {
>  	optional => 1,
>  	minimum => 16,
>  	default => 512,
> -    }
> +    },
> +    max => {
> +	type => 'integer',
> +	optional => 1,
> +	type => 'integer',
> +	minimum => 65536,
> +	maximum => 4194304
> +    },
>  };
>  
>  sub print_memory {
> @@ -43,6 +51,9 @@ sub parse_memory {
>  
>      $res = eval { PVE::JSONSchema::parse_property_string($memory_fmt, $value) };
>      die $@ if $@;
> +
> +    die "max memory need to be a multiple of 64GB" if $res->{max} && $res->{max} % 65536 != 0;

s/GB/GiB/
Missing newline at the end of error message

You could also add a dedicated verify method for the format, for example
like pve_verify_hotplug_features(). Then this check is already done at
parameter verification time.

> +
>      return $res;
>  }
>  
> @@ -223,6 +234,11 @@ sub qemu_memory_hotplug {
>      my $oldmem = parse_memory($conf->{memory});
>      my $newmem = parse_memory($value);
>  
> +    # skip non hotpluggable value
> +    if (safe_num_ne($newmem->{max}, $oldmem->{max})) {
> +	die "skip\n";
> +    }

Please move this to the call sites. The "die "skip""-logic should not
cross function boundaries.

> +
>      my $memory = $oldmem->{current};
>      $value = $newmem->{current};
>