Re: [pve-devel] [PATCH qemu-server 00/10] rework memory hotplug + virtiomem

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

Am 09.12.22 um 20:27 schrieb Alexandre Derumier:
> This patch series rework the current memory hotplug + virtiomem.
> 
> memory option now have extra options:
> 
> memory: [[current=]<integer>] [,max=<enum>] [,virtio=<1|0>]
> ex: memory: current=1024,max=131072,virtio=1
> 
> 
> patches 1-2: add a memory parser
> 
> patches 3-7: add the max option with 64 static dimm hotplug
> 
> for classic memory hotplug, when maxmemory is defined,
> we use 64 fixed size dimm.
> The max option is a multiple of 64GB.
> 
> patches 8-9: add virtio-mem
> 
> The virtio option enable new virtio-mem support,
> instead of plugging dimm, it's add/removed block inside
> big dimm.
> virtio-mem can use 32000 blocks, the blocksize is compute from
> max memory.
> 
> patch10: hotplug fix
> 
> Alexandre Derumier (10):
>   test: add memory tests
>   add memory parser
>   config: memory: add 'max' option
>   memory: add get_static_mem
>   memory: get_max_mem: use config memory max
>   memory: use 64 slots && static dimm size with max is defined
>   test: add memory-max tests
>   memory: add virtio-mem support
>   tests: add virtio-mem tests
>   memory: fix hotplug with virtiomem && maxmem
> 
The general ideas looks fine to me and my basic testing seemed fine too,
but most patches could use a few improvements. The biggest issue is that
getting static information in HA manager is not adapted to the change.

The virtio feature should be marked as technology preview and mentioned
that it only works for guests running Linux >= 5.8 [0].

IMHO, it'd be nicer to add the property string handling/registering in
the Memory.pm module directly, so that all is contained in one place.

I also think that using a validator for the format might be worth it.

Also, the tests should be made more host-independent, but for the NUMA
node issue 01/10 that might require a bit of rework, to be able to mock
the relevant parts.

Should the virtio feature be QEMU-version-guarded? It's explicitly
opt-in and technology preview, so maybe not really needed?

See the individual replies on patches for details.

[0]: https://virtio-mem.gitlab.io/user-guide/