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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Feb 28, 2023 at 03:26:56AM +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
> > > ---
> > >   hw/mem/pc-dimm.c           |  2 ++
> > >   hw/virtio/virtio-balloon.c | 33 +++++----------------------------
> > >   include/hw/boards.h        |  1 +
> > >   3 files changed, 8 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index
> > > 50ef83215c..192fc7922c 100644
> > > --- a/hw/mem/pc-dimm.c
> > > +++ b/hw/mem/pc-dimm.c
> > > @@ -81,6 +81,7 @@ void pc_dimm_plug(PCDIMMDevice *dimm,
> > MachineState
> > > *machine)
> > >
> > >       memory_device_plug(MEMORY_DEVICE(dimm), machine);
> > >       vmstate_register_ram(vmstate_mr, DEVICE(dimm));
> > > +    machine->device_memory->dimm_size += vmstate_mr->size;
> > >   }
> > >
> > >   void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine)
> > @@
> > > -90,6 +91,7 @@ void pc_dimm_unplug(PCDIMMDevice *dimm,
> > MachineState
> > > *machine)
> > >
> > >       memory_device_unplug(MEMORY_DEVICE(dimm), machine);
> > >       vmstate_unregister_ram(vmstate_mr, DEVICE(dimm));
> > > +    machine->device_memory->dimm_size -= vmstate_mr->size;
> > >   }
> > 
> > Ahh, missed that my previous comment was not addressed: we only want to
> > track "real" DIMMs, not NVDIMMs.
> > 
> > --
> > Thanks,
> > 
> > David / dhildenb
> 
> 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
> ---
>  hw/mem/pc-dimm.c           |  8 ++++++++
>  hw/virtio/virtio-balloon.c | 33 +++++----------------------------
>  include/hw/boards.h        |  1 +
>  3 files changed, 14 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 50ef83215c..2107615016 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));
> +    bool is_nvdimm = object_dynamic_cast(OBJECT(dimm), TYPE_NVDIMM);
> +    if (!is_nvdimm) {
> +        machine->device_memory->dimm_size += vmstate_mr->size;
> +    }
>  }
>  
>  void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine)
> @@ -90,6 +94,10 @@ void pc_dimm_unplug(PCDIMMDevice *dimm, MachineState *machine)
>  
>      memory_device_unplug(MEMORY_DEVICE(dimm), machine);
>      vmstate_unregister_ram(vmstate_mr, DEVICE(dimm));
> +    bool is_nvdimm = object_dynamic_cast(OBJECT(dimm), TYPE_NVDIMM);
> +    if (!is_nvdimm) {
> +        machine->device_memory->dimm_size -= vmstate_mr->size;
> +    }
>  }
>

add comments here explaining why are nvdimms excluded?
  
>  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..80bbb59132 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 != NULL) {

just if (machine->device_memory) is equivalent and shorter

> +        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..551b4b419e 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -296,6 +296,7 @@ struct MachineClass {
>  typedef struct DeviceMemoryState {
>      hwaddr base;
>      MemoryRegion mr;
> +    ram_addr_t dimm_size;

add a comment explaining what this is?

>  } DeviceMemoryState;
>  
>  /**
> -- 
> 2.33.0
>