[v2,2/4] virtio-mem: Skip most of virtio_mem_unplug_all() without plugged memory

Commit Message

David Hildenbrand July 6, 2023, 7:56 a.m. UTC

Already when starting QEMU we perform one system reset that ends up
triggering virtio_mem_unplug_all() with no actual memory plugged yet.
That, in turn will trigger ram_block_discard_range() and perform some
other actions that are not required in that case.

Let's optimize virtio_mem_unplug_all() for the case that no memory is
plugged. This will be beneficial for x-ignore-shared support as well.

Tested-by: Mario Casquero <[e-mail]>
Signed-off-by: David Hildenbrand <[e-mail]>
---
 hw/virtio/virtio-mem.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

Comments

Juan Quintela July 6, 2023, 8:15 a.m. UTC | #1

David Hildenbrand <[e-mail]> wrote:
> Already when starting QEMU we perform one system reset that ends up
> triggering virtio_mem_unplug_all() with no actual memory plugged yet.
> That, in turn will trigger ram_block_discard_range() and perform some
> other actions that are not required in that case.
>
> Let's optimize virtio_mem_unplug_all() for the case that no memory is
> plugged. This will be beneficial for x-ignore-shared support as well.
>
> Tested-by: Mario Casquero <[e-mail]>
> Signed-off-by: David Hildenbrand <[e-mail]>

It works, so ...

Reviewed-by: Juan Quintela <[e-mail]>

>      RAMBlock *rb = vmem->memdev->mr.ram_block;
>  
> -    if (virtio_mem_is_busy()) {
> -        return -EBUSY;
> -    }
> -
> -    if (ram_block_discard_range(rb, 0, qemu_ram_get_used_length(rb))) {
> -        return -EBUSY;
> -    }
> -    virtio_mem_notify_unplug_all(vmem);
> -
> -    bitmap_clear(vmem->bitmap, 0, vmem->bitmap_size);
>      if (vmem->size) {
> +        if (virtio_mem_is_busy()) {
> +            return -EBUSY;

I see that the only way that virtio_men_is_busy() is true is if we are
in the middle of a migration.  In the case that vmem is 0, we don't
care.  So we are good.



> +        }
> +        if (ram_block_discard_range(rb, 0, qemu_ram_get_used_length(rb))) {
> +            return -EBUSY;
> +        }

Nothing to discard, so also good.

> +        virtio_mem_notify_unplug_all(vmem);

Nothing to notify, so also good.

> +        bitmap_clear(vmem->bitmap, 0, vmem->bitmap_size);
>          vmem->size = 0;
>          notifier_list_notify(&vmem->size_change_notifiers, &vmem->size);
>      }
> +
>      trace_virtio_mem_unplugged_all();
>      virtio_mem_resize_usable_region(vmem, vmem->requested_size, true);
>      return 0;

Once that we are here.  Do you remember _why_ do we allow virtio-mem
plug/unplug in the middle of a migration.

We forbid to plug/unplug everything else.  Why do we need to plug/unplug
virtio-mem during migration?

Thanks, Juan.

David Hildenbrand July 6, 2023, 8:38 a.m. UTC | #2

On 06.07.23 10:15, Juan Quintela wrote:
> David Hildenbrand <[e-mail]> wrote:
>> Already when starting QEMU we perform one system reset that ends up
>> triggering virtio_mem_unplug_all() with no actual memory plugged yet.
>> That, in turn will trigger ram_block_discard_range() and perform some
>> other actions that are not required in that case.
>>
>> Let's optimize virtio_mem_unplug_all() for the case that no memory is
>> plugged. This will be beneficial for x-ignore-shared support as well.
>>
>> Tested-by: Mario Casquero <[e-mail]>
>> Signed-off-by: David Hildenbrand <[e-mail]>
> 
> It works, so ...
> 
> Reviewed-by: Juan Quintela <[e-mail]>

Thanks!

[...]

>> +        bitmap_clear(vmem->bitmap, 0, vmem->bitmap_size);
>>           vmem->size = 0;
>>           notifier_list_notify(&vmem->size_change_notifiers, &vmem->size);
>>       }
>> +
>>       trace_virtio_mem_unplugged_all();
>>       virtio_mem_resize_usable_region(vmem, vmem->requested_size, true);
>>       return 0;
> 
> Once that we are here.  Do you remember _why_ do we allow virtio-mem
> plug/unplug in the middle of a migration.
> 
> We forbid to plug/unplug everything else.  Why do we need to plug/unplug
> virtio-mem during migration?

With virtio-mem you tell the VM the desired size for the device 
(requested-size), and the VM will select blocks to (un)plug and send 
(un)plug requests to the hypervisor in order to reach the requested size.

So changing the requested size in the hypervisor (by the QEMU user) and 
the VM processing that resize request is asynchronous -- similar to 
memory ballooning.

As the VM can send these (un)plug requests any time, and we exactly 
don't want to allow (un)plug during migration, we have 
virtio_mem_is_busy() to reject any such requests to tell the VM "please 
try again later".

Juan Quintela July 6, 2023, 1:27 p.m. UTC | #3

David Hildenbrand <[e-mail]> wrote:
> On 06.07.23 10:15, Juan Quintela wrote:
>> David Hildenbrand <[e-mail]> wrote:
>>> Already when starting QEMU we perform one system reset that ends up
>>> triggering virtio_mem_unplug_all() with no actual memory plugged yet.
>>> That, in turn will trigger ram_block_discard_range() and perform some
>>> other actions that are not required in that case.
>>>
>>> Let's optimize virtio_mem_unplug_all() for the case that no memory is
>>> plugged. This will be beneficial for x-ignore-shared support as well.
>>>
>>> Tested-by: Mario Casquero <[e-mail]>
>>> Signed-off-by: David Hildenbrand <[e-mail]>
>> It works, so ...
>> Reviewed-by: Juan Quintela <[e-mail]>
>
> Thanks!
>
> [...]
>
>>> +        bitmap_clear(vmem->bitmap, 0, vmem->bitmap_size);
>>>           vmem->size = 0;
>>>           notifier_list_notify(&vmem->size_change_notifiers, &vmem->size);
>>>       }
>>> +
>>>       trace_virtio_mem_unplugged_all();
>>>       virtio_mem_resize_usable_region(vmem, vmem->requested_size, true);
>>>       return 0;
>> Once that we are here.  Do you remember _why_ do we allow virtio-mem
>> plug/unplug in the middle of a migration.
>> We forbid to plug/unplug everything else.  Why do we need to
>> plug/unplug
>> virtio-mem during migration?
>
> With virtio-mem you tell the VM the desired size for the device
> (requested-size), and the VM will select blocks to (un)plug and send
> (un)plug requests to the hypervisor in order to reach the requested
> size.
>
> So changing the requested size in the hypervisor (by the QEMU user)
> and the VM processing that resize request is asynchronous -- similar
> to memory ballooning.
>
> As the VM can send these (un)plug requests any time, and we exactly
> don't want to allow (un)plug during migration, we have
> virtio_mem_is_busy() to reject any such requests to tell the VM
> "please try again later".

Ahh.

I see it now.

Thanks, Juan.

Patch

diff --git a/hw/virtio/virtio-mem.c b/hw/virtio/virtio-mem.c
index ec0ae32589..a922c21380 100644
--- a/hw/virtio/virtio-mem.c
+++ b/hw/virtio/virtio-mem.c
@@ -621,20 +621,20 @@  static int virtio_mem_unplug_all(VirtIOMEM *vmem)
 {
     RAMBlock *rb = vmem->memdev->mr.ram_block;
 
-    if (virtio_mem_is_busy()) {
-        return -EBUSY;
-    }
-
-    if (ram_block_discard_range(rb, 0, qemu_ram_get_used_length(rb))) {
-        return -EBUSY;
-    }
-    virtio_mem_notify_unplug_all(vmem);
-
-    bitmap_clear(vmem->bitmap, 0, vmem->bitmap_size);
     if (vmem->size) {
+        if (virtio_mem_is_busy()) {
+            return -EBUSY;
+        }
+        if (ram_block_discard_range(rb, 0, qemu_ram_get_used_length(rb))) {
+            return -EBUSY;
+        }
+        virtio_mem_notify_unplug_all(vmem);
+
+        bitmap_clear(vmem->bitmap, 0, vmem->bitmap_size);
         vmem->size = 0;
         notifier_list_notify(&vmem->size_change_notifiers, &vmem->size);
     }
+
     trace_virtio_mem_unplugged_all();
     virtio_mem_resize_usable_region(vmem, vmem->requested_size, true);
     return 0;