Re: [pve-devel] [PATCH v3 qemu-server 08/13] memory: don't use foreach_reversedimm for unplug

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

Thanks for looking into this! The new way is more straightforward for
sure :)

Am 02.02.23 um 12:03 schrieb Alexandre Derumier:
> @@ -316,30 +284,33 @@ sub qemu_memory_hotplug {
>  
>      } else {
>  
> -	foreach_reverse_dimm($conf, $vmid, $value, $sockets, sub {
> -	    my ($conf, $vmid, $name, $dimm_size, $numanode, $current_size, $memory) = @_;
> +	my $dimms = qemu_dimm_list($vmid);

Looking at qemu_dimm_list(), I feel like we should start filtering to
only return dimms (based on the id or is there something better?), since
we now also iterate over them and not only check for existence.
Currently, qemu_dimm_list() returns all memory objects which would
include the virtiomem devices. The function is not called if virtiomem
is used, but this would make it future-proof (and avoid breakage if
people have manually attached non-dimm memory devices, although I'd say
that's not supported from our perspective ;)).

> +	my $current_size = $memory;
> +	for my $name (sort { $dimms->{$b}{slot} <=> $dimms->{$a}{slot} } keys %$dimms) {

Style nit: please use $dimms->{$b}->{slot}

>  
> -		return if $current_size >= get_current_memory($conf->{memory});
> +	    my $dimm_size = $dimms->{$name}->{size} / 1024 / 1024;
>  
> -		print "try to unplug memory dimm $name\n";
> +	    last if $current_size <= $value || $current_size <= $static_memory;

Nit: the second half of the condition is not really needed. We already
assert at the start of qemu_memory_hotplug() that $value >=
$static_memory and if we didn't somehow end up with values not matching
reality in the config, we should reach $static_memory only after
unplugging everything, or not?

>  
> -		my $retry = 0;
> -		while (1) {
> -		    eval { PVE::QemuServer::qemu_devicedel($vmid, $name) };
> -		    sleep 3;
> -		    my $dimm_list = qemu_dimm_list($vmid);
> -		    last if !$dimm_list->{$name};
> -		    raise_param_exc({ $name => "error unplug memory module" }) if $retry > 5;
> -		    $retry++;
> -		}
> +	    print "try to unplug memory dimm $name\n";
>  
> -		#update conf after each succesful module unplug
> -		$newmem->{current} = $current_size;
> -		$conf->{memory} = print_memory($newmem);
> +	    my $retry = 0;
> +	    while (1) {
> +		eval { PVE::QemuServer::qemu_devicedel($vmid, $name) };
> +		sleep 3;
> +		my $dimm_list = qemu_dimm_list($vmid);
> +		last if !$dimm_list->{$name};
> +		raise_param_exc({ $name => "error unplug memory module" }) if $retry > 5;
> +		$retry++;
> +	    }
> +	    $current_size -= $dimm_size;
> +	    #update conf after each succesful module unplug
> +	    $newmem->{current} = $current_size;
> +	    $conf->{memory} = print_memory($newmem);
>  
> -		eval { PVE::QemuServer::qemu_objectdel($vmid, "mem-$name"); };
> -		PVE::QemuConfig->write_config($vmid, $conf);
> -	});
> +	    eval { PVE::QemuServer::qemu_objectdel($vmid, "mem-$name"); };
> +	    PVE::QemuConfig->write_config($vmid, $conf);
> +	}
>      }
>      return $conf->{memory};
>  }