diff mbox series

[v1,1/2] virtio-mem: Warn if a memory backend with "prealloc=on" is used

Message ID 20220118150712.139953-2-david@redhat.com
State New
Headers show
Series virtio-mem: Handle preallocation with migration | expand

Commit Message

David Hildenbrand Jan. 18, 2022, 3:07 p.m. UTC
"prealloc=on" for the memory backend does not work as expected, as
virtio-mem will simply discard all preallocated memory immediately again.
In the best case, it's an expensive NOP. In the worst case, it's an
unexpected allocation error.

Instead, "prealloc=on" should be specified for the virtio-mem device only,
such that virtio-mem will try preallocating memory before plugging
memory dynamically to the guest.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 hw/virtio/virtio-mem.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Dr. David Alan Gilbert Jan. 25, 2022, 11:19 a.m. UTC | #1
* David Hildenbrand (david@redhat.com) wrote:
> "prealloc=on" for the memory backend does not work as expected, as
> virtio-mem will simply discard all preallocated memory immediately again.
> In the best case, it's an expensive NOP. In the worst case, it's an
> unexpected allocation error.
> 
> Instead, "prealloc=on" should be specified for the virtio-mem device only,
> such that virtio-mem will try preallocating memory before plugging
> memory dynamically to the guest.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Why is this a warning rather than an error that stops creation of the
device?

Dave

> ---
>  hw/virtio/virtio-mem.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
> index 04c223b0c9..6c337db0a7 100644
> --- a/hw/virtio/virtio-mem.c
> +++ b/hw/virtio/virtio-mem.c
> @@ -765,6 +765,13 @@ static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    if (vmem->memdev->prealloc) {
> +        warn_report("'%s' property specifies a memdev with preallocation"
> +                    " enabled: %s. Instead, specify 'prealloc=on' for the"
> +                    " virtio-mem device. ", VIRTIO_MEM_MEMDEV_PROP,
> +                    object_get_canonical_path_component(OBJECT(vmem->memdev)));
> +    }
> +
>      if ((nb_numa_nodes && vmem->node >= nb_numa_nodes) ||
>          (!nb_numa_nodes && vmem->node)) {
>          error_setg(errp, "'%s' property has value '%" PRIu32 "', which exceeds"
> -- 
> 2.34.1
>
David Hildenbrand Jan. 25, 2022, 12:08 p.m. UTC | #2
On 25.01.22 12:19, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (david@redhat.com) wrote:
>> "prealloc=on" for the memory backend does not work as expected, as
>> virtio-mem will simply discard all preallocated memory immediately again.
>> In the best case, it's an expensive NOP. In the worst case, it's an
>> unexpected allocation error.
>>
>> Instead, "prealloc=on" should be specified for the virtio-mem device only,
>> such that virtio-mem will try preallocating memory before plugging
>> memory dynamically to the guest.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Why is this a warning rather than an error that stops creation of the
> device?

No strong opinion, an error might actually be better.
diff mbox series

Patch

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index 04c223b0c9..6c337db0a7 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -765,6 +765,13 @@  static void virtio_mem_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (vmem->memdev->prealloc) {
+        warn_report("'%s' property specifies a memdev with preallocation"
+                    " enabled: %s. Instead, specify 'prealloc=on' for the"
+                    " virtio-mem device. ", VIRTIO_MEM_MEMDEV_PROP,
+                    object_get_canonical_path_component(OBJECT(vmem->memdev)));
+    }
+
     if ((nb_numa_nodes && vmem->node >= nb_numa_nodes) ||
         (!nb_numa_nodes && vmem->node)) {
         error_setg(errp, "'%s' property has value '%" PRIu32 "', which exceeds"