Re: [Qemu-devel] [PATCH v2 1/3] pc-dimm: add a function to calculate VM's current RAM size
On Wed, Nov 19, 2014 at 09:31:35AM -0700, Eric Blake wrote: > On 11/19/2014 09:06 AM, Michael S. Tsirkin wrote: > > This affects QMP right? > >>> > >>> I think later patches will tell how. CC'ing Eric. > >> > >> As far as I can tell, this is just correcting a reporting issue; the > >> existing QMP commands/events for tracking balloon size will now properly > >> account for hotplugged memory. > >> > >> What I don't know is if this change in semantics will affect any users. > >> Libvirt is not yet supporting memory hotplug, so ideally, fixing this > >> bug before libvirt uses memory hotplug means libvirt will never have to > >> worry about qemu versions that do incorrect reporting. > >> > >> The alternative is to declare that the existing QMP commands cannot > >> change in semantics for the existing members that it reports, and must > >> instead report additional dictionary members describing the amount of > >> hot-plugged memory, and then require that the client add the numbers > >> together itself. That sounds mean to the client, so I'm hoping we don't > >> have to go there. > > > > > > IOW you ack this patch for 2.2? > > > > Is memory hotplug one of the new features in 2.2? If so, then yes, we > should get its semantics right from the start (this is a bug fix to > avoid a release with broken semantics). On the other hand, if hotplug > existed in 2.1, then we already have a release with odd semantics, so > delaying this fix until 2.3 and leaving 2.2 with the same odd semantics > would not hurt, and it then becomes a judgment call of whether we are > rushing in a possibly incomplete solution by trying to get this into > 2.2. (Sorry I haven't been following the history of memory hotplug closer) AFAIK it's there since 2.0. > > -- > Eric Blake eblake redhat com+1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
Re: [Qemu-devel] [PATCH v2 1/3] pc-dimm: add a function to calculate VM's current RAM size
On 2014/11/19 18:32, Michael S. Tsirkin wrote:
On Mon, Nov 17, 2014 at 01:11:08PM +0800, zhanghailiang wrote:
The global parameter 'ram_size' does not take into account
the hotplugged memory.
In some codes, we use 'ram_size' as current VM's real RAM size,
which is not correct.
Add function 'get_current_ram_size' to calculate VM's current RAM size,
it will enumerate present memory devices and also plus ram_size.
Signed-off-by: zhanghailiang
This affects QMP right?
Yes, will affect the results of balloon commands ;)
Cc Luiz.
---
hw/mem/pc-dimm.c| 26 ++
include/exec/cpu-common.h | 1 +
stubs/qmp_pc_dimm_device_list.c | 5 +
3 files changed, 32 insertions(+)
diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index a800ea7..38465d0 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -62,6 +62,32 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
return 0;
}
+ram_addr_t get_current_ram_size(void)
+{
+MemoryDeviceInfoList *info_list = NULL;
+MemoryDeviceInfoList **prev = &info_list;
+MemoryDeviceInfoList *info;
+ram_addr_t size = ram_size;
+
+qmp_pc_dimm_device_list(qdev_get_machine(), &prev);
+for (info = info_list; info; info = info->next) {
+MemoryDeviceInfo *value = info->value;
+
+if (value) {
+switch (value->kind) {
+case MEMORY_DEVICE_INFO_KIND_DIMM:
+size += value->dimm->size;
+break;
+default:
+break;
+}
+}
+}
+qapi_free_MemoryDeviceInfoList(info_list);
+
+return size;
+}
+
static int pc_dimm_slot2bitmap(Object *obj, void *opaque)
{
unsigned long *bitmap = opaque;
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index 427b851..fcc3162 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -52,6 +52,7 @@ typedef uintptr_t ram_addr_t;
#endif
extern ram_addr_t ram_size;
+ram_addr_t get_current_ram_size(void);
/* memory API */
diff --git a/stubs/qmp_pc_dimm_device_list.c b/stubs/qmp_pc_dimm_device_list.c
index 5cb220c..b584bd8 100644
--- a/stubs/qmp_pc_dimm_device_list.c
+++ b/stubs/qmp_pc_dimm_device_list.c
@@ -5,3 +5,8 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
{
return 0;
}
+
+ram_addr_t get_current_ram_size(void)
+{
+return ram_size;
+}
--
1.7.12.4
.
Re: [Qemu-devel] [PATCH v2 1/3] pc-dimm: add a function to calculate VM's current RAM size
On 2014/11/20 0:31, Eric Blake wrote: On 11/19/2014 09:06 AM, Michael S. Tsirkin wrote: This affects QMP right? I think later patches will tell how. CC'ing Eric. As far as I can tell, this is just correcting a reporting issue; the existing QMP commands/events for tracking balloon size will now properly account for hotplugged memory. What I don't know is if this change in semantics will affect any users. Libvirt is not yet supporting memory hotplug, so ideally, fixing this bug before libvirt uses memory hotplug means libvirt will never have to worry about qemu versions that do incorrect reporting. The alternative is to declare that the existing QMP commands cannot change in semantics for the existing members that it reports, and must instead report additional dictionary members describing the amount of hot-plugged memory, and then require that the client add the numbers together itself. That sounds mean to the client, so I'm hoping we don't have to go there. IOW you ack this patch for 2.2? Is memory hotplug one of the new features in 2.2? If so, then yes, we No, after searching this feature in ChangeLogs, i found pc-dimm memory hotplug was supported since 2.1. It only supports for x86 target. And one more thing, i found in 2.2's Changelog it begin to support memory hotplug for s390 target, I'm not sure whether this problem also exists for s390. should get its semantics right from the start (this is a bug fix to avoid a release with broken semantics). On the other hand, if hotplug existed in 2.1, then we already have a release with odd semantics, so delaying this fix until 2.3 and leaving 2.2 with the same odd semantics would not hurt, and it then becomes a judgment call of whether we are rushing in a possibly incomplete solution by trying to get this into 2.2. (Sorry I haven't been following the history of memory hotplug closer)
Re: [Qemu-devel] [PATCH v2 1/3] pc-dimm: add a function to calculate VM's current RAM size
On 11/19/2014 09:06 AM, Michael S. Tsirkin wrote: This affects QMP right? >>> >>> I think later patches will tell how. CC'ing Eric. >> >> As far as I can tell, this is just correcting a reporting issue; the >> existing QMP commands/events for tracking balloon size will now properly >> account for hotplugged memory. >> >> What I don't know is if this change in semantics will affect any users. >> Libvirt is not yet supporting memory hotplug, so ideally, fixing this >> bug before libvirt uses memory hotplug means libvirt will never have to >> worry about qemu versions that do incorrect reporting. >> >> The alternative is to declare that the existing QMP commands cannot >> change in semantics for the existing members that it reports, and must >> instead report additional dictionary members describing the amount of >> hot-plugged memory, and then require that the client add the numbers >> together itself. That sounds mean to the client, so I'm hoping we don't >> have to go there. > > > IOW you ack this patch for 2.2? > Is memory hotplug one of the new features in 2.2? If so, then yes, we should get its semantics right from the start (this is a bug fix to avoid a release with broken semantics). On the other hand, if hotplug existed in 2.1, then we already have a release with odd semantics, so delaying this fix until 2.3 and leaving 2.2 with the same odd semantics would not hurt, and it then becomes a judgment call of whether we are rushing in a possibly incomplete solution by trying to get this into 2.2. (Sorry I haven't been following the history of memory hotplug closer) -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 1/3] pc-dimm: add a function to calculate VM's current RAM size
On Wed, Nov 19, 2014 at 08:52:19AM -0700, Eric Blake wrote: > On 11/19/2014 08:13 AM, Luiz Capitulino wrote: > > On Wed, 19 Nov 2014 12:32:46 +0200 > > "Michael S. Tsirkin" wrote: > > > >> On Mon, Nov 17, 2014 at 01:11:08PM +0800, zhanghailiang wrote: > >>> The global parameter 'ram_size' does not take into account > >>> the hotplugged memory. > >>> > >>> In some codes, we use 'ram_size' as current VM's real RAM size, > >>> which is not correct. > >>> > >>> Add function 'get_current_ram_size' to calculate VM's current RAM size, > >>> it will enumerate present memory devices and also plus ram_size. > >>> > >>> Signed-off-by: zhanghailiang > >> > >> > >> This affects QMP right? > > > > I think later patches will tell how. CC'ing Eric. > > As far as I can tell, this is just correcting a reporting issue; the > existing QMP commands/events for tracking balloon size will now properly > account for hotplugged memory. > > What I don't know is if this change in semantics will affect any users. > Libvirt is not yet supporting memory hotplug, so ideally, fixing this > bug before libvirt uses memory hotplug means libvirt will never have to > worry about qemu versions that do incorrect reporting. > > The alternative is to declare that the existing QMP commands cannot > change in semantics for the existing members that it reports, and must > instead report additional dictionary members describing the amount of > hot-plugged memory, and then require that the client add the numbers > together itself. That sounds mean to the client, so I'm hoping we don't > have to go there. IOW you ack this patch for 2.2?
Re: [Qemu-devel] [PATCH v2 1/3] pc-dimm: add a function to calculate VM's current RAM size
On 11/19/2014 08:13 AM, Luiz Capitulino wrote: > On Wed, 19 Nov 2014 12:32:46 +0200 > "Michael S. Tsirkin" wrote: > >> On Mon, Nov 17, 2014 at 01:11:08PM +0800, zhanghailiang wrote: >>> The global parameter 'ram_size' does not take into account >>> the hotplugged memory. >>> >>> In some codes, we use 'ram_size' as current VM's real RAM size, >>> which is not correct. >>> >>> Add function 'get_current_ram_size' to calculate VM's current RAM size, >>> it will enumerate present memory devices and also plus ram_size. >>> >>> Signed-off-by: zhanghailiang >> >> >> This affects QMP right? > > I think later patches will tell how. CC'ing Eric. As far as I can tell, this is just correcting a reporting issue; the existing QMP commands/events for tracking balloon size will now properly account for hotplugged memory. What I don't know is if this change in semantics will affect any users. Libvirt is not yet supporting memory hotplug, so ideally, fixing this bug before libvirt uses memory hotplug means libvirt will never have to worry about qemu versions that do incorrect reporting. The alternative is to declare that the existing QMP commands cannot change in semantics for the existing members that it reports, and must instead report additional dictionary members describing the amount of hot-plugged memory, and then require that the client add the numbers together itself. That sounds mean to the client, so I'm hoping we don't have to go there. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 1/3] pc-dimm: add a function to calculate VM's current RAM size
On Wed, 19 Nov 2014 12:32:46 +0200
"Michael S. Tsirkin" wrote:
> On Mon, Nov 17, 2014 at 01:11:08PM +0800, zhanghailiang wrote:
> > The global parameter 'ram_size' does not take into account
> > the hotplugged memory.
> >
> > In some codes, we use 'ram_size' as current VM's real RAM size,
> > which is not correct.
> >
> > Add function 'get_current_ram_size' to calculate VM's current RAM size,
> > it will enumerate present memory devices and also plus ram_size.
> >
> > Signed-off-by: zhanghailiang
>
>
> This affects QMP right?
I think later patches will tell how. CC'ing Eric.
> Cc Luiz.
>
>
> > ---
> > hw/mem/pc-dimm.c| 26 ++
> > include/exec/cpu-common.h | 1 +
> > stubs/qmp_pc_dimm_device_list.c | 5 +
> > 3 files changed, 32 insertions(+)
> >
> > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> > index a800ea7..38465d0 100644
> > --- a/hw/mem/pc-dimm.c
> > +++ b/hw/mem/pc-dimm.c
> > @@ -62,6 +62,32 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> > return 0;
> > }
> >
> > +ram_addr_t get_current_ram_size(void)
> > +{
> > +MemoryDeviceInfoList *info_list = NULL;
> > +MemoryDeviceInfoList **prev = &info_list;
> > +MemoryDeviceInfoList *info;
> > +ram_addr_t size = ram_size;
> > +
> > +qmp_pc_dimm_device_list(qdev_get_machine(), &prev);
> > +for (info = info_list; info; info = info->next) {
> > +MemoryDeviceInfo *value = info->value;
> > +
> > +if (value) {
> > +switch (value->kind) {
> > +case MEMORY_DEVICE_INFO_KIND_DIMM:
> > +size += value->dimm->size;
> > +break;
> > +default:
> > +break;
> > +}
> > +}
> > +}
> > +qapi_free_MemoryDeviceInfoList(info_list);
> > +
> > +return size;
> > +}
> > +
> > static int pc_dimm_slot2bitmap(Object *obj, void *opaque)
> > {
> > unsigned long *bitmap = opaque;
> > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> > index 427b851..fcc3162 100644
> > --- a/include/exec/cpu-common.h
> > +++ b/include/exec/cpu-common.h
> > @@ -52,6 +52,7 @@ typedef uintptr_t ram_addr_t;
> > #endif
> >
> > extern ram_addr_t ram_size;
> > +ram_addr_t get_current_ram_size(void);
> >
> > /* memory API */
> >
> > diff --git a/stubs/qmp_pc_dimm_device_list.c
> > b/stubs/qmp_pc_dimm_device_list.c
> > index 5cb220c..b584bd8 100644
> > --- a/stubs/qmp_pc_dimm_device_list.c
> > +++ b/stubs/qmp_pc_dimm_device_list.c
> > @@ -5,3 +5,8 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> > {
> > return 0;
> > }
> > +
> > +ram_addr_t get_current_ram_size(void)
> > +{
> > +return ram_size;
> > +}
> > --
> > 1.7.12.4
> >
>
Re: [Qemu-devel] [PATCH v2 1/3] pc-dimm: add a function to calculate VM's current RAM size
On Mon, Nov 17, 2014 at 01:11:08PM +0800, zhanghailiang wrote:
> The global parameter 'ram_size' does not take into account
> the hotplugged memory.
>
> In some codes, we use 'ram_size' as current VM's real RAM size,
> which is not correct.
>
> Add function 'get_current_ram_size' to calculate VM's current RAM size,
> it will enumerate present memory devices and also plus ram_size.
>
> Signed-off-by: zhanghailiang
This affects QMP right?
Cc Luiz.
> ---
> hw/mem/pc-dimm.c| 26 ++
> include/exec/cpu-common.h | 1 +
> stubs/qmp_pc_dimm_device_list.c | 5 +
> 3 files changed, 32 insertions(+)
>
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index a800ea7..38465d0 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -62,6 +62,32 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> return 0;
> }
>
> +ram_addr_t get_current_ram_size(void)
> +{
> +MemoryDeviceInfoList *info_list = NULL;
> +MemoryDeviceInfoList **prev = &info_list;
> +MemoryDeviceInfoList *info;
> +ram_addr_t size = ram_size;
> +
> +qmp_pc_dimm_device_list(qdev_get_machine(), &prev);
> +for (info = info_list; info; info = info->next) {
> +MemoryDeviceInfo *value = info->value;
> +
> +if (value) {
> +switch (value->kind) {
> +case MEMORY_DEVICE_INFO_KIND_DIMM:
> +size += value->dimm->size;
> +break;
> +default:
> +break;
> +}
> +}
> +}
> +qapi_free_MemoryDeviceInfoList(info_list);
> +
> +return size;
> +}
> +
> static int pc_dimm_slot2bitmap(Object *obj, void *opaque)
> {
> unsigned long *bitmap = opaque;
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index 427b851..fcc3162 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -52,6 +52,7 @@ typedef uintptr_t ram_addr_t;
> #endif
>
> extern ram_addr_t ram_size;
> +ram_addr_t get_current_ram_size(void);
>
> /* memory API */
>
> diff --git a/stubs/qmp_pc_dimm_device_list.c b/stubs/qmp_pc_dimm_device_list.c
> index 5cb220c..b584bd8 100644
> --- a/stubs/qmp_pc_dimm_device_list.c
> +++ b/stubs/qmp_pc_dimm_device_list.c
> @@ -5,3 +5,8 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> {
> return 0;
> }
> +
> +ram_addr_t get_current_ram_size(void)
> +{
> +return ram_size;
> +}
> --
> 1.7.12.4
>
Re: [Qemu-devel] [PATCH v2 1/3] pc-dimm: add a function to calculate VM's current RAM size
On Mon, 17 Nov 2014 13:11:08 +0800
zhanghailiang wrote:
> The global parameter 'ram_size' does not take into account
> the hotplugged memory.
>
> In some codes, we use 'ram_size' as current VM's real RAM size,
> which is not correct.
>
> Add function 'get_current_ram_size' to calculate VM's current RAM
> size, it will enumerate present memory devices and also plus ram_size.
>
> Signed-off-by: zhanghailiang
> ---
> hw/mem/pc-dimm.c| 26 ++
> include/exec/cpu-common.h | 1 +
> stubs/qmp_pc_dimm_device_list.c | 5 +
> 3 files changed, 32 insertions(+)
>
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index a800ea7..38465d0 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -62,6 +62,32 @@ int qmp_pc_dimm_device_list(Object *obj, void
> *opaque) return 0;
> }
>
> +ram_addr_t get_current_ram_size(void)
> +{
> +MemoryDeviceInfoList *info_list = NULL;
> +MemoryDeviceInfoList **prev = &info_list;
> +MemoryDeviceInfoList *info;
> +ram_addr_t size = ram_size;
> +
> +qmp_pc_dimm_device_list(qdev_get_machine(), &prev);
> +for (info = info_list; info; info = info->next) {
> +MemoryDeviceInfo *value = info->value;
> +
> +if (value) {
> +switch (value->kind) {
> +case MEMORY_DEVICE_INFO_KIND_DIMM:
> +size += value->dimm->size;
> +break;
> +default:
> +break;
> +}
> +}
> +}
> +qapi_free_MemoryDeviceInfoList(info_list);
> +
> +return size;
> +}
function is a generic one and it could handle not only pc-dimm device
in future, so it would be better to put it in some device neutral place.
That would allow to drop following stub as well.
> +
> static int pc_dimm_slot2bitmap(Object *obj, void *opaque)
> {
> unsigned long *bitmap = opaque;
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index 427b851..fcc3162 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -52,6 +52,7 @@ typedef uintptr_t ram_addr_t;
> #endif
>
> extern ram_addr_t ram_size;
> +ram_addr_t get_current_ram_size(void);
>
> /* memory API */
>
> diff --git a/stubs/qmp_pc_dimm_device_list.c
> b/stubs/qmp_pc_dimm_device_list.c index 5cb220c..b584bd8 100644
> --- a/stubs/qmp_pc_dimm_device_list.c
> +++ b/stubs/qmp_pc_dimm_device_list.c
> @@ -5,3 +5,8 @@ int qmp_pc_dimm_device_list(Object *obj, void *opaque)
> {
> return 0;
> }
> +
> +ram_addr_t get_current_ram_size(void)
> +{
> +return ram_size;
> +}
