Re: [pve-devel] [PATCH v2 qemu-server 0/9] rework memory hotplug + virtiomem

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

Am 04.01.23 um 07:42 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.
> 
> 
> Changelog v2:
> 
> update differents patches based on Fiona comments.
> (I have send 2 others mails with comments not yet addressed)
> 
> Biggest change is on virtio-mem, instead of trying to have same amount of memory
> on each virtiomem (some block could be unmovable and break unplug),
> we try to balance/dispatch remaining block on other available virtiomems.
> 
> Also, the minimum blocksize supported by linux guest os is 4MB currently,
> even if virtiomem can use 2MB on qemu side.
> 
> Patch10 with hotplug fix has be merged in others patches.

Thank you for tackling this! Most of my comments are nits/suggestions
for improvements, but there are a few real issues I found. The issues
and some of the more important suggestions (details in the patches):

Patch 2/9 does a breaking change by changing with what value gets
written to the config.

In patch 4/9 the die "skip"-logic really should be at the call side instead.

In patch 6/9 something is wrong with the calculation for unplugging in
combination with 'max' (it uses the wrong dimm IDs).

In patch 8/9 you really should mention that virtio-mem is a technology
preview. Also the whole "retry handling/marking as error" logic is a bit
much, and would ideally be its own patch, if we even want this much
complexity.

Missing adaptation for ha-manager.