Re: [pve-devel] [PATCH v3 qemu-server 11/13] memory: add virtio-mem support

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

Am 02.02.23 um 12:03 schrieb Alexandre Derumier:
> @@ -157,6 +168,109 @@ sub get_current_memory {
>      return $memory->{current};
>  }
>  
> +sub get_virtiomem_block_size {
> +    my ($conf) = @_;
> +
> +    my $sockets = $conf->{sockets} || 1;
> +    my $MAX_MEM = get_max_mem($conf);
> +    my $static_memory = get_static_mem($conf, $sockets);
> +    my $memory = get_current_memory($conf->{memory});
> +
> +    #virtiomem can map 32000 block size.
> +    #try to use lowest blocksize, lower = more chance to unplug memory.
> +    my $blocksize = ($MAX_MEM - $static_memory) / 32000;
> +    #2MB is the minimum to be aligned with THP
> +    $blocksize = 2 if $blocksize < 2;
> +    $blocksize = 2**(ceil(log($blocksize)/log(2)));
> +    #Linux guest kernel only support 4MiB block currently (kernel <= 6.2)
> +    $blocksize = 4 if $blocksize < 4;
> +
> +    return $blocksize;
> +}
> +
> +my sub get_virtiomem_total_current_size {
> +    my ($mems) = @_;
> +    my $size = 0;
> +    for my $mem (values %$mems) {
> +	$size += $mem->{current};
> +    }
> +    return $size;
> +}
> +
> +my sub balance_virtiomem {

The function should die if not all memory can be plugged.

> +    my ($vmid, $virtiomems, $blocksize, $target_virtiomem_total) = @_;
> +
> +    my $nb_virtiomem = keys %$virtiomems;

Style nit: explicit scalar() would be nice.

> +
> +    print"try to balance memory on $nb_virtiomem virtiomems\n";
> +    die "error. no more available blocks in virtiomem to balance the remaining memory" if $target_virtiomem_total < 0;
> +
> +    #if we can't share exactly the same amount, we add the remainder on last node
> +    my $virtiomem_target_aligned = int( $target_virtiomem_total / $nb_virtiomem / $blocksize) * $blocksize;
> +    my $virtiomem_target_remaining = $target_virtiomem_total - ($virtiomem_target_aligned * ($nb_virtiomem-1));
> +
> +    my $i = 0;
> +    foreach my $id (sort keys %$virtiomems) {
> +	my $virtiomem = $virtiomems->{$id};
> +	$i++;
> +	my $virtiomem_target = $i == $nb_virtiomem ? $virtiomem_target_remaining : $virtiomem_target_aligned;
> +	$virtiomem->{completed} = 0;
> +	$virtiomem->{retry} = 0;
> +	$virtiomem->{target} = $virtiomem_target;
> +
> +	print "virtiomem$id: set-requested-size : $virtiomem_target\n";
> +	mon_cmd($vmid, 'qom-set', path => "/machine/peripheral/virtiomem$id", property => "requested-size", value => $virtiomem_target * 1024 * 1024);

Style nit: some lines are over 100 chars and some quite a bit

> +    }
> +
> +    while (1) {
> +
> +	sleep 1;
> +	my $total_finished = 0;
> +
> +	foreach my $id (keys %$virtiomems) {
> +
> +	    my $virtiomem = $virtiomems->{$id};
> +
> +	    if ($virtiomem->{error} || $virtiomem->{completed}) {
> +		$total_finished++;
> +		next;
> +	    }
> +
> +	    my $size = mon_cmd($vmid, 'qom-get', path => "/machine/peripheral/virtiomem$id", property => "size");
> +	    $virtiomem->{current} = $size / 1024 / 1024;
> +	    print"virtiomem$id: virtiomem->last: $virtiomem->{last} virtiomem->current: $virtiomem->{current} virtio_mem_target:$virtiomem->{target}\n";
> +
> +	    if($virtiomem->{current} == $virtiomem->{target}) {
> +		print"virtiomem$id: completed\n";
> +		$virtiomem->{completed} = 1;
> +		next;
> +	    }
> +
> +	    if($virtiomem->{current} != $virtiomem->{last}) {
> +		#if value has changed, but not yet completed
> +		print "virtiomem$id: changed but don't not reach target yet\n";
> +		$virtiomem->{retry} = 0;
> +		$virtiomem->{last} = $virtiomem->{current};
> +		next;
> +	    }
> +
> +	    if($virtiomem->{retry} >= 5) {
> +		print "virtiomem$id: too many retry. set error\n";
> +		$virtiomem->{error} = 1;
> +		#as change is async, we don't want that value change after the api call
> +		eval {
> +		    mon_cmd($vmid, 'qom-set', path => "/machine/peripheral/virtiomem$id", property => "requested-size", value => $virtiomem->{current} * 1024 *1024);
> +		};
> +	    }
> +	    print"virtiomem$id: increase retry: $virtiomem->{retry}\n";
> +	    $virtiomem->{retry}++;
> +	}
> +
> +	my $nb_virtiomem = keys %$virtiomems;

Redeclares variable. The number can't change from before, or am I
missing something?

> +	return if $total_finished == $nb_virtiomem;

Style nit: could also use
while ($total_finished != $nb_virtiomem)
at the beginning of the loop.

> +    }
> +}
> +
>  sub get_numa_node_list {
>      my ($conf) = @_;
>      my @numa_map;
> @@ -237,7 +351,39 @@ sub qemu_memory_hotplug {
>      my $MAX_MEM = get_max_mem($conf);
>      die "you cannot add more memory than max mem $MAX_MEM MB!\n" if $value > $MAX_MEM;
>  
> -    if ($value > $memory) {
> +    my $confmem = parse_memory($conf->{memory});

You can just re-use the existing $oldmem?

> +
> +    if ($confmem->{virtio}) {
> +	my $blocksize = get_virtiomem_block_size($conf);
> +
> +	my $virtiomems = {};
> +
> +	for (my $i = 0; $i < $sockets; $i++) {
> +	    my $size = mon_cmd($vmid, 'qom-get', path => "/machine/peripheral/virtiomem$i", property => "size");
> +	    $size = $size / 1024 /1024;
> +	    $virtiomems->{$i} = {
> +		current => $size,
> +		last => $size,
> +		error => 0,
> +		completed => 0,
> +		retry => 0
> +	    };
> +	}
> +
> +	my $target_virtiomem_total = $value - $static_memory;
> +	my $err;
> +	eval {
> +	    balance_virtiomem($vmid, $virtiomems, $blocksize, $target_virtiomem_total);
> +	};
> +	$err = $@ if $@;
> +
> +	my $current_memory = $static_memory + get_virtiomem_total_current_size($virtiomems);
> +	$newmem->{current} = $current_memory;
> +	$conf->{memory} = print_memory($newmem);
> +	PVE::QemuConfig->write_config($vmid, $conf);
> +	die $err if $err;
> +
> +    } elsif ($value > $memory) {
>  
>  	my $numa_hostmap;
>  
> @@ -441,17 +590,42 @@ sub config {
>      }
>  
>      if ($hotplug) {
> -	foreach_dimm($conf, $vmid, $memory, $static_memory, sub {
> -	    my ($conf, $vmid, $name, $dimm_size, $numanode, $current_size, $memory) = @_;
>  
> -	    my $mem_object = print_mem_object($conf, "mem-$name", $dimm_size);
> +	my $confmem = parse_memory($conf->{memory});
> +
> +	if ($confmem->{'virtio'}) {
>  
> -	    push @$cmd, "-object" , $mem_object;
> -	    push @$cmd, "-device", "pc-dimm,id=$name,memdev=mem-$name,node=$numanode";
> +	    my $MAX_MEM = get_max_mem($conf);
> +	    my $node_maxmem = ($MAX_MEM - $static_memory) / $sockets;
> +	    my $node_mem = ($memory - $static_memory) / $sockets;

Nit: If the number of $sockets is not a power of 2, I think this breaks.
But I guess we already don't support it. Running current version without
your patches (for a VM with memory hotplug):
root@pve701 ~ # qm set 131 --sockets 3
update VM 131: -sockets 3
root@pve701 ~ # qm set 131 -memory 8192
update VM 131: -memory 8192
root@pve701 ~ # qm start 131
kvm: total memory for NUMA nodes (0x3fffffff) should equal RAM size
(0x40000000)
start failed: QEMU exited with code 1

I guess we can just fix it up together with the existing rounding issue
if/when somebody complains about it ;)

> +	    my $blocksize = get_virtiomem_block_size($conf);
>  
> -	    die "memory size ($memory) must be aligned to $dimm_size for hotplugging\n"
> -		if $current_size > $memory;
> -	});
> +	    die "memory need to be a multiple of $blocksize MiB with maxmemory $MAX_MEM MiB when virtiomem is enabled\n"
> +		if $memory % $blocksize != 0;
> +
> +	    for (my $i = 0; $i < $sockets; $i++)  {
> +
> +		my $id = "virtiomem$i";
> +		my $mem_object = print_mem_object($conf, "mem-$id", $node_maxmem);
> +		push @$cmd, "-object" , "$mem_object,reserve=off";
> +
> +		my $mem_device = "virtio-mem-pci,block-size=${blocksize}M,requested-size=${node_mem}M,id=$id,memdev=mem-$id,node=$i";
> +		$mem_device .= ",prealloc=on" if $conf->{hugepages};
> +		$mem_devices->{$id} = $mem_device;
> +	    }
> +	} else {
> +	    foreach_dimm($conf, $vmid, $memory, $static_memory, sub {
> +		my ($conf, $vmid, $name, $dimm_size, $numanode, $current_size, $memory) = @_;
> +
> +		my $mem_object = print_mem_object($conf, "mem-$name", $dimm_size);
> +
> +		push @$cmd, "-object" , $mem_object;
> +		push @$cmd, "-device", "pc-dimm,id=$name,memdev=mem-$name,node=$numanode";
> +
> +		die "memory size ($memory) must be aligned to $dimm_size for hotplugging\n"
> +		    if $current_size > $memory;
> +	    });
> +	}
>      }
>  }
>