[PATCH v2] virtio-balloon: optimize the virtio-balloon on the ARM platform

Yangming via posted 1 patch 1 year, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/afd620a5e7c14a0794812e72ba1af545@huawei.com
Maintainers: David Hildenbrand <david@redhat.com>, Igor Mammedov <imammedo@redhat.com>, Xiao Guangrong <xiaoguangrong.eric@gmail.com>, "Michael S. Tsirkin" <mst@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>
There is a newer version of this series
hw/mem/pc-dimm.c           |  7 +++++++
hw/virtio/virtio-balloon.c | 33 +++++----------------------------
include/hw/boards.h        |  2 ++
3 files changed, 14 insertions(+), 28 deletions(-)
[PATCH v2] virtio-balloon: optimize the virtio-balloon on the ARM platform
Posted by Yangming via 1 year, 2 months ago
Optimize the virtio-balloon feature on the ARM platform by adding
a variable to keep track of the current hot-plugged pc-dimm size,
instead of traversing the virtual machine's memory modules to count
the current RAM size during the balloon inflation or deflation
process. This variable can be updated only when plugging or unplugging
the device, which will result in an increase of approximately 60%
efficiency of balloon process on the ARM platform.

We tested the total amount of time required for the balloon inflation process on ARM:
inflate the balloon to 64GB of a 128GB guest under stress.
Before: 102 seconds
After: 42 seconds

Signed-off-by: Qi Xi <xiqi2@huawei.com>
Signed-off-by: Ming Yang yangming73@huawei.com
---
Refactor the code by adding comments and removing unnecessary code.

 hw/mem/pc-dimm.c           |  7 +++++++
 hw/virtio/virtio-balloon.c | 33 +++++----------------------------
 include/hw/boards.h        |  2 ++
 3 files changed, 14 insertions(+), 28 deletions(-)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 50ef83215c..3f2734a267 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -81,6 +81,10 @@ void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine)
 
     memory_device_plug(MEMORY_DEVICE(dimm), machine);
     vmstate_register_ram(vmstate_mr, DEVICE(dimm));
+    /* count only "real" DIMMs, not NVDIMMs */
+    if (!object_dynamic_cast(OBJECT(dimm), TYPE_NVDIMM)) {
+        machine->device_memory->dimm_size += vmstate_mr->size;
+    }
 }
 
 void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine)
@@ -90,6 +94,9 @@ void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine)
 
     memory_device_unplug(MEMORY_DEVICE(dimm), machine);
     vmstate_unregister_ram(vmstate_mr, DEVICE(dimm));
+    if (!object_dynamic_cast(OBJECT(dimm), TYPE_NVDIMM)) {
+        machine->device_memory->dimm_size -= vmstate_mr->size;
+    }
 }
 
 static int pc_dimm_slot2bitmap(Object *obj, void *opaque)
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
index 746f07c4d2..2814a47cb1 100644
--- a/hw/virtio/virtio-balloon.c
+++ b/hw/virtio/virtio-balloon.c
@@ -729,37 +729,14 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
     memcpy(config_data, &config, virtio_balloon_config_size(dev));
 }
 
-static int build_dimm_list(Object *obj, void *opaque)
-{
-    GSList **list = opaque;
-
-    if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
-        DeviceState *dev = DEVICE(obj);
-        if (dev->realized) { /* only realized DIMMs matter */
-            *list = g_slist_prepend(*list, dev);
-        }
-    }
-
-    object_child_foreach(obj, build_dimm_list, opaque);
-    return 0;
-}
-
 static ram_addr_t get_current_ram_size(void)
 {
-    GSList *list = NULL, *item;
-    ram_addr_t size = current_machine->ram_size;
-
-    build_dimm_list(qdev_get_machine(), &list);
-    for (item = list; item; item = g_slist_next(item)) {
-        Object *obj = OBJECT(item->data);
-        if (!strcmp(object_get_typename(obj), TYPE_PC_DIMM)) {
-            size += object_property_get_int(obj, PC_DIMM_SIZE_PROP,
-                                            &error_abort);
-        }
+    MachineState *machine = MACHINE(qdev_get_machine());
+    if (machine->device_memory) {
+        return machine->ram_size + machine->device_memory->dimm_size;
+    } else {
+        return machine->ram_size;
     }
-    g_slist_free(list);
-
-    return size;
 }
 
 static bool virtio_balloon_page_poison_support(void *opaque)
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 6fbbfd56c8..397ec10468 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -292,10 +292,12 @@ struct MachineClass {
  * @base: address in guest physical address space where the memory
  * address space for memory devices starts
  * @mr: address space container for memory devices
+ * @dimm_size: the sum of plugged DIMMs' sizes
  */
 typedef struct DeviceMemoryState {
     hwaddr base;
     MemoryRegion mr;
+    ram_addr_t dimm_size;
 } DeviceMemoryState;
 
 /**
-- 
2.33.0
Re: [PATCH v2] virtio-balloon: optimize the virtio-balloon on the ARM platform
Posted by Michael S. Tsirkin 1 year, 1 month ago
On Wed, Mar 01, 2023 at 06:38:13AM +0000, Yangming wrote:
> Optimize the virtio-balloon feature on the ARM platform by adding
> a variable to keep track of the current hot-plugged pc-dimm size,
> instead of traversing the virtual machine's memory modules to count
> the current RAM size during the balloon inflation or deflation
> process. This variable can be updated only when plugging or unplugging
> the device, which will result in an increase of approximately 60%
> efficiency of balloon process on the ARM platform.
> 
> We tested the total amount of time required for the balloon inflation process on ARM:
> inflate the balloon to 64GB of a 128GB guest under stress.
> Before: 102 seconds
> After: 42 seconds
> 
> Signed-off-by: Qi Xi <xiqi2@huawei.com>
> Signed-off-by: Ming Yang yangming73@huawei.com
> ---
> Refactor the code by adding comments and removing unnecessary code.
> 
>  hw/mem/pc-dimm.c           |  7 +++++++
>  hw/virtio/virtio-balloon.c | 33 +++++----------------------------
>  include/hw/boards.h        |  2 ++
>  3 files changed, 14 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 50ef83215c..3f2734a267 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -81,6 +81,10 @@ void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine)
>  
>      memory_device_plug(MEMORY_DEVICE(dimm), machine);
>      vmstate_register_ram(vmstate_mr, DEVICE(dimm));
> +    /* count only "real" DIMMs, not NVDIMMs */
> +    if (!object_dynamic_cast(OBJECT(dimm), TYPE_NVDIMM)) {
> +        machine->device_memory->dimm_size += vmstate_mr->size;
> +    }
>  }
>  
>  void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine)

vmstate_mr->size is Int128 you are not supposed to do math on it.

And generally poking at this struct is a bad idea.

I think memory_region_size will do what you want but not 100% sure.
Maybe you need to look at the flatview ...

David?


> @@ -90,6 +94,9 @@ void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine)
>  
>      memory_device_unplug(MEMORY_DEVICE(dimm), machine);
>      vmstate_unregister_ram(vmstate_mr, DEVICE(dimm));
> +    if (!object_dynamic_cast(OBJECT(dimm), TYPE_NVDIMM)) {
> +        machine->device_memory->dimm_size -= vmstate_mr->size;
> +    }
>  }
>  
>  static int pc_dimm_slot2bitmap(Object *obj, void *opaque)
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 746f07c4d2..2814a47cb1 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -729,37 +729,14 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>      memcpy(config_data, &config, virtio_balloon_config_size(dev));
>  }
>  
> -static int build_dimm_list(Object *obj, void *opaque)
> -{
> -    GSList **list = opaque;
> -
> -    if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
> -        DeviceState *dev = DEVICE(obj);
> -        if (dev->realized) { /* only realized DIMMs matter */
> -            *list = g_slist_prepend(*list, dev);
> -        }
> -    }
> -
> -    object_child_foreach(obj, build_dimm_list, opaque);
> -    return 0;
> -}
> -
>  static ram_addr_t get_current_ram_size(void)
>  {
> -    GSList *list = NULL, *item;
> -    ram_addr_t size = current_machine->ram_size;
> -
> -    build_dimm_list(qdev_get_machine(), &list);
> -    for (item = list; item; item = g_slist_next(item)) {
> -        Object *obj = OBJECT(item->data);
> -        if (!strcmp(object_get_typename(obj), TYPE_PC_DIMM)) {
> -            size += object_property_get_int(obj, PC_DIMM_SIZE_PROP,
> -                                            &error_abort);
> -        }
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +    if (machine->device_memory) {
> +        return machine->ram_size + machine->device_memory->dimm_size;
> +    } else {
> +        return machine->ram_size;
>      }
> -    g_slist_free(list);
> -
> -    return size;
>  }
>  
>  static bool virtio_balloon_page_poison_support(void *opaque)
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 6fbbfd56c8..397ec10468 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -292,10 +292,12 @@ struct MachineClass {
>   * @base: address in guest physical address space where the memory
>   * address space for memory devices starts
>   * @mr: address space container for memory devices
> + * @dimm_size: the sum of plugged DIMMs' sizes
>   */
>  typedef struct DeviceMemoryState {
>      hwaddr base;
>      MemoryRegion mr;
> +    ram_addr_t dimm_size;
>  } DeviceMemoryState;
>  
>  /**
> -- 
> 2.33.0
Re: [PATCH v2] virtio-balloon: optimize the virtio-balloon on the ARM platform
Posted by David Hildenbrand 1 year, 1 month ago
On 08.03.23 01:42, Michael S. Tsirkin wrote:
> On Wed, Mar 01, 2023 at 06:38:13AM +0000, Yangming wrote:
>> Optimize the virtio-balloon feature on the ARM platform by adding
>> a variable to keep track of the current hot-plugged pc-dimm size,
>> instead of traversing the virtual machine's memory modules to count
>> the current RAM size during the balloon inflation or deflation
>> process. This variable can be updated only when plugging or unplugging
>> the device, which will result in an increase of approximately 60%
>> efficiency of balloon process on the ARM platform.
>>
>> We tested the total amount of time required for the balloon inflation process on ARM:
>> inflate the balloon to 64GB of a 128GB guest under stress.
>> Before: 102 seconds
>> After: 42 seconds
>>
>> Signed-off-by: Qi Xi <xiqi2@huawei.com>
>> Signed-off-by: Ming Yang yangming73@huawei.com
>> ---
>> Refactor the code by adding comments and removing unnecessary code.
>>
>>   hw/mem/pc-dimm.c           |  7 +++++++
>>   hw/virtio/virtio-balloon.c | 33 +++++----------------------------
>>   include/hw/boards.h        |  2 ++
>>   3 files changed, 14 insertions(+), 28 deletions(-)
>>
>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>> index 50ef83215c..3f2734a267 100644
>> --- a/hw/mem/pc-dimm.c
>> +++ b/hw/mem/pc-dimm.c
>> @@ -81,6 +81,10 @@ void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine)
>>   
>>       memory_device_plug(MEMORY_DEVICE(dimm), machine);
>>       vmstate_register_ram(vmstate_mr, DEVICE(dimm));
>> +    /* count only "real" DIMMs, not NVDIMMs */
>> +    if (!object_dynamic_cast(OBJECT(dimm), TYPE_NVDIMM)) {
>> +        machine->device_memory->dimm_size += vmstate_mr->size;
>> +    }
>>   }
>>   
>>   void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine)
> 
> vmstate_mr->size is Int128 you are not supposed to do math on it.
> 
> And generally poking at this struct is a bad idea.
> 
> I think memory_region_size will do what you want but not 100% sure.
> Maybe you need to look at the flatview ...

Good point, we should use memory_region_size().

-- 
Thanks,

David / dhildenb
Re: [PATCH v2] virtio-balloon: optimize the virtio-balloon on the ARM platform
Posted by David Hildenbrand 1 year, 2 months ago
On 01.03.23 07:38, Yangming wrote:
> Optimize the virtio-balloon feature on the ARM platform by adding
> a variable to keep track of the current hot-plugged pc-dimm size,
> instead of traversing the virtual machine's memory modules to count
> the current RAM size during the balloon inflation or deflation
> process. This variable can be updated only when plugging or unplugging
> the device, which will result in an increase of approximately 60%
> efficiency of balloon process on the ARM platform.
> 
> We tested the total amount of time required for the balloon inflation process on ARM:
> inflate the balloon to 64GB of a 128GB guest under stress.
> Before: 102 seconds
> After: 42 seconds
> 
> Signed-off-by: Qi Xi <xiqi2@huawei.com>
> Signed-off-by: Ming Yang yangming73@huawei.com
> ---
> Refactor the code by adding comments and removing unnecessary code.
> 
>   hw/mem/pc-dimm.c           |  7 +++++++
>   hw/virtio/virtio-balloon.c | 33 +++++----------------------------
>   include/hw/boards.h        |  2 ++
>   3 files changed, 14 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 50ef83215c..3f2734a267 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -81,6 +81,10 @@ void pc_dimm_plug(PCDIMMDevice *dimm, MachineState *machine)
>   
>       memory_device_plug(MEMORY_DEVICE(dimm), machine);
>       vmstate_register_ram(vmstate_mr, DEVICE(dimm));
> +    /* count only "real" DIMMs, not NVDIMMs */
> +    if (!object_dynamic_cast(OBJECT(dimm), TYPE_NVDIMM)) {
> +        machine->device_memory->dimm_size += vmstate_mr->size;
> +    }
>   }
>   
>   void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine)
> @@ -90,6 +94,9 @@ void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine)
>   
>       memory_device_unplug(MEMORY_DEVICE(dimm), machine);
>       vmstate_unregister_ram(vmstate_mr, DEVICE(dimm));
> +    if (!object_dynamic_cast(OBJECT(dimm), TYPE_NVDIMM)) {
> +        machine->device_memory->dimm_size -= vmstate_mr->size;
> +    }
>   }
>   
>   static int pc_dimm_slot2bitmap(Object *obj, void *opaque)
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 746f07c4d2..2814a47cb1 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -729,37 +729,14 @@ static void virtio_balloon_get_config(VirtIODevice *vdev, uint8_t *config_data)
>       memcpy(config_data, &config, virtio_balloon_config_size(dev));
>   }
>   
> -static int build_dimm_list(Object *obj, void *opaque)
> -{
> -    GSList **list = opaque;
> -
> -    if (object_dynamic_cast(obj, TYPE_PC_DIMM)) {
> -        DeviceState *dev = DEVICE(obj);
> -        if (dev->realized) { /* only realized DIMMs matter */
> -            *list = g_slist_prepend(*list, dev);
> -        }
> -    }
> -
> -    object_child_foreach(obj, build_dimm_list, opaque);
> -    return 0;
> -}
> -
>   static ram_addr_t get_current_ram_size(void)
>   {
> -    GSList *list = NULL, *item;
> -    ram_addr_t size = current_machine->ram_size;
> -
> -    build_dimm_list(qdev_get_machine(), &list);
> -    for (item = list; item; item = g_slist_next(item)) {
> -        Object *obj = OBJECT(item->data);
> -        if (!strcmp(object_get_typename(obj), TYPE_PC_DIMM)) {
> -            size += object_property_get_int(obj, PC_DIMM_SIZE_PROP,
> -                                            &error_abort);
> -        }
> +    MachineState *machine = MACHINE(qdev_get_machine());
> +    if (machine->device_memory) {
> +        return machine->ram_size + machine->device_memory->dimm_size;
> +    } else {
> +        return machine->ram_size;
>       }
> -    g_slist_free(list);
> -
> -    return size;
>   }
>   
>   static bool virtio_balloon_page_poison_support(void *opaque)
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 6fbbfd56c8..397ec10468 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -292,10 +292,12 @@ struct MachineClass {
>    * @base: address in guest physical address space where the memory
>    * address space for memory devices starts
>    * @mr: address space container for memory devices
> + * @dimm_size: the sum of plugged DIMMs' sizes
>    */
>   typedef struct DeviceMemoryState {
>       hwaddr base;
>       MemoryRegion mr;
> +    ram_addr_t dimm_size;
>   } DeviceMemoryState;
>   
>   /**

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb