* [Qemu-devel] [PATCH] numa: check for busy memory backend
@ 2014-06-25 9:04 Hu Tao
2014-06-25 10:02 ` Igor Mammedov
2014-06-29 15:20 ` Michael S. Tsirkin
0 siblings, 2 replies; 11+ messages in thread
From: Hu Tao @ 2014-06-25 9:04 UTC (permalink / raw)
To: qemu-devel; +Cc: Yasunori Goto, Igor Mammedov, Michael S. Tsirkin
..to prevent one memory backend from being used by more than one numa
node.
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
numa.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/numa.c b/numa.c
index e471afe..6c1c554 100644
--- a/numa.c
+++ b/numa.c
@@ -279,6 +279,13 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
exit(1);
}
+ if (memory_region_is_mapped(seg)) {
+ char *path = object_get_canonical_path_component(OBJECT(backend));
+ error_report("memory backend %s is busy", path);
+ g_free(path);
+ exit(1);
+ }
+
memory_region_add_subregion(mr, addr, seg);
vmstate_register_ram_global(seg);
addr += size;
--
1.9.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: check for busy memory backend
2014-06-25 9:04 [Qemu-devel] [PATCH] numa: check for busy memory backend Hu Tao
@ 2014-06-25 10:02 ` Igor Mammedov
2014-06-25 11:48 ` Markus Armbruster
2014-06-29 15:20 ` Michael S. Tsirkin
1 sibling, 1 reply; 11+ messages in thread
From: Igor Mammedov @ 2014-06-25 10:02 UTC (permalink / raw)
To: Hu Tao; +Cc: Yasunori Goto, qemu-devel, Michael S. Tsirkin
On Wed, 25 Jun 2014 17:04:14 +0800
Hu Tao <hutao@cn.fujitsu.com> wrote:
> ..to prevent one memory backend from being used by more than one numa
> node.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
> numa.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/numa.c b/numa.c
> index e471afe..6c1c554 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -279,6 +279,13 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
> exit(1);
> }
>
> + if (memory_region_is_mapped(seg)) {
> + char *path = object_get_canonical_path_component(OBJECT(backend));
> + error_report("memory backend %s is busy", path);
> + g_free(path);
> + exit(1);
s/1/EXIT_FAILURE/ please
> + }
> +
> memory_region_add_subregion(mr, addr, seg);
> vmstate_register_ram_global(seg);
> addr += size;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: check for busy memory backend
2014-06-25 10:02 ` Igor Mammedov
@ 2014-06-25 11:48 ` Markus Armbruster
0 siblings, 0 replies; 11+ messages in thread
From: Markus Armbruster @ 2014-06-25 11:48 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Hu Tao, Michael S. Tsirkin, qemu-devel, Yasunori Goto
Igor Mammedov <imammedo@redhat.com> writes:
> On Wed, 25 Jun 2014 17:04:14 +0800
> Hu Tao <hutao@cn.fujitsu.com> wrote:
>
>> ..to prevent one memory backend from being used by more than one numa
>> node.
>>
>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>> ---
>> numa.c | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/numa.c b/numa.c
>> index e471afe..6c1c554 100644
>> --- a/numa.c
>> +++ b/numa.c
>> @@ -279,6 +279,13 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
>> exit(1);
>> }
>>
>> + if (memory_region_is_mapped(seg)) {
>> + char *path = object_get_canonical_path_component(OBJECT(backend));
>> + error_report("memory backend %s is busy", path);
>> + g_free(path);
>> + exit(1);
> s/1/EXIT_FAILURE/ please
I count >600 instances of exit() with a numeric argument (not counting
some 50 sys.exit() in Python code), but less than 40 using EXIT_SUCCESS
or EXIT_FAILURE.
The abstraction provided by EXIT_SUCCESS / EXIT_FAILURE is basically
worthless anyway.
>
>> + }
>> +
>> memory_region_add_subregion(mr, addr, seg);
>> vmstate_register_ram_global(seg);
>> addr += size;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: check for busy memory backend
2014-06-25 9:04 [Qemu-devel] [PATCH] numa: check for busy memory backend Hu Tao
2014-06-25 10:02 ` Igor Mammedov
@ 2014-06-29 15:20 ` Michael S. Tsirkin
2014-06-30 5:33 ` Hu Tao
1 sibling, 1 reply; 11+ messages in thread
From: Michael S. Tsirkin @ 2014-06-29 15:20 UTC (permalink / raw)
To: Hu Tao; +Cc: Igor Mammedov, qemu-devel, Yasunori Goto
On Wed, Jun 25, 2014 at 05:04:14PM +0800, Hu Tao wrote:
> ..to prevent one memory backend from being used by more than one numa
> node.
Thanks, but please always make the msg content self-contained
so it can be understood without the subject.
E.g. here, just drop "..to".
Are you sure we want this? Is there a chance sharing a backend
can be useful?
Igor, what's your take?
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
> numa.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/numa.c b/numa.c
> index e471afe..6c1c554 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -279,6 +279,13 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
> exit(1);
> }
>
> + if (memory_region_is_mapped(seg)) {
> + char *path = object_get_canonical_path_component(OBJECT(backend));
> + error_report("memory backend %s is busy", path);
> + g_free(path);
> + exit(1);
> + }
> +
> memory_region_add_subregion(mr, addr, seg);
> vmstate_register_ram_global(seg);
> addr += size;
> --
> 1.9.3
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: check for busy memory backend
2014-06-29 15:20 ` Michael S. Tsirkin
@ 2014-06-30 5:33 ` Hu Tao
2014-06-30 6:53 ` Michael S. Tsirkin
0 siblings, 1 reply; 11+ messages in thread
From: Hu Tao @ 2014-06-30 5:33 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Igor Mammedov, qemu-devel, Yasunori Goto
On Sun, Jun 29, 2014 at 06:20:22PM +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 25, 2014 at 05:04:14PM +0800, Hu Tao wrote:
> > ..to prevent one memory backend from being used by more than one numa
> > node.
>
> Thanks, but please always make the msg content self-contained
> so it can be understood without the subject.
> E.g. here, just drop "..to".
>
> Are you sure we want this? Is there a chance sharing a backend
> can be useful?
This patch is actually a bug fix. Even if we will want backend sharing, we
can do it after.
>
> Igor, what's your take?
>
> >
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> > numa.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/numa.c b/numa.c
> > index e471afe..6c1c554 100644
> > --- a/numa.c
> > +++ b/numa.c
> > @@ -279,6 +279,13 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
> > exit(1);
> > }
> >
> > + if (memory_region_is_mapped(seg)) {
> > + char *path = object_get_canonical_path_component(OBJECT(backend));
> > + error_report("memory backend %s is busy", path);
> > + g_free(path);
> > + exit(1);
> > + }
> > +
> > memory_region_add_subregion(mr, addr, seg);
> > vmstate_register_ram_global(seg);
> > addr += size;
> > --
> > 1.9.3
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: check for busy memory backend
2014-06-30 5:33 ` Hu Tao
@ 2014-06-30 6:53 ` Michael S. Tsirkin
2014-06-30 7:46 ` Hu Tao
0 siblings, 1 reply; 11+ messages in thread
From: Michael S. Tsirkin @ 2014-06-30 6:53 UTC (permalink / raw)
To: Hu Tao; +Cc: Igor Mammedov, qemu-devel, Yasunori Goto
On Mon, Jun 30, 2014 at 01:33:42PM +0800, Hu Tao wrote:
> On Sun, Jun 29, 2014 at 06:20:22PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 25, 2014 at 05:04:14PM +0800, Hu Tao wrote:
> > > ..to prevent one memory backend from being used by more than one numa
> > > node.
> >
> > Thanks, but please always make the msg content self-contained
> > so it can be understood without the subject.
> > E.g. here, just drop "..to".
> >
> > Are you sure we want this? Is there a chance sharing a backend
> > can be useful?
>
> This patch is actually a bug fix.
It is? What is the bug and how to reproduce it?
I am not sure we should write a ton of code to validate qemu
configuration, as long as qemu does not assert.
> Even if we will want backend sharing, we
> can do it after.
By reverting this patch? So why merge it?
> >
> > Igor, what's your take?
> >
> > >
> > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > > ---
> > > numa.c | 7 +++++++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/numa.c b/numa.c
> > > index e471afe..6c1c554 100644
> > > --- a/numa.c
> > > +++ b/numa.c
> > > @@ -279,6 +279,13 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
> > > exit(1);
> > > }
> > >
> > > + if (memory_region_is_mapped(seg)) {
> > > + char *path = object_get_canonical_path_component(OBJECT(backend));
> > > + error_report("memory backend %s is busy", path);
> > > + g_free(path);
> > > + exit(1);
> > > + }
> > > +
> > > memory_region_add_subregion(mr, addr, seg);
> > > vmstate_register_ram_global(seg);
> > > addr += size;
> > > --
> > > 1.9.3
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: check for busy memory backend
2014-06-30 6:53 ` Michael S. Tsirkin
@ 2014-06-30 7:46 ` Hu Tao
2014-06-30 8:28 ` Michael S. Tsirkin
0 siblings, 1 reply; 11+ messages in thread
From: Hu Tao @ 2014-06-30 7:46 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Igor Mammedov, qemu-devel, Yasunori Goto
On Mon, Jun 30, 2014 at 09:53:20AM +0300, Michael S. Tsirkin wrote:
> On Mon, Jun 30, 2014 at 01:33:42PM +0800, Hu Tao wrote:
> > On Sun, Jun 29, 2014 at 06:20:22PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 25, 2014 at 05:04:14PM +0800, Hu Tao wrote:
> > > > ..to prevent one memory backend from being used by more than one numa
> > > > node.
> > >
> > > Thanks, but please always make the msg content self-contained
> > > so it can be understood without the subject.
> > > E.g. here, just drop "..to".
> > >
> > > Are you sure we want this? Is there a chance sharing a backend
> > > can be useful?
> >
> > This patch is actually a bug fix.
>
> It is? What is the bug and how to reproduce it?
If user specifies the same memory backend for two numa nodes:
./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -m 512M \
-qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm \
-object memory-backend-ram,size=256M,id=ram0 \
-numa node,nodeid=0,memdev=ram0 \
-numa node,nodeid=1,memdev=ram0
> I am not sure we should write a ton of code to validate qemu
> configuration, as long as qemu does not assert.
It seems qemu does not provide a way to disable assert currently.
Even if I removed asserts on the code path in my test, there is another
problem that it hits an infinite in render_memory_region().
>
> > Even if we will want backend sharing, we
> > can do it after.
>
> By reverting this patch? So why merge it?
The point is qemu doesn't fire a bug no matter what user inputs.
>
> > >
> > > Igor, what's your take?
> > >
> > > >
> > > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > > > ---
> > > > numa.c | 7 +++++++
> > > > 1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/numa.c b/numa.c
> > > > index e471afe..6c1c554 100644
> > > > --- a/numa.c
> > > > +++ b/numa.c
> > > > @@ -279,6 +279,13 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
> > > > exit(1);
> > > > }
> > > >
> > > > + if (memory_region_is_mapped(seg)) {
> > > > + char *path = object_get_canonical_path_component(OBJECT(backend));
> > > > + error_report("memory backend %s is busy", path);
> > > > + g_free(path);
> > > > + exit(1);
> > > > + }
> > > > +
> > > > memory_region_add_subregion(mr, addr, seg);
> > > > vmstate_register_ram_global(seg);
> > > > addr += size;
> > > > --
> > > > 1.9.3
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: check for busy memory backend
2014-06-30 7:46 ` Hu Tao
@ 2014-06-30 8:28 ` Michael S. Tsirkin
2014-06-30 8:48 ` Igor Mammedov
0 siblings, 1 reply; 11+ messages in thread
From: Michael S. Tsirkin @ 2014-06-30 8:28 UTC (permalink / raw)
To: Hu Tao; +Cc: Igor Mammedov, qemu-devel, Yasunori Goto
On Mon, Jun 30, 2014 at 03:46:56PM +0800, Hu Tao wrote:
> On Mon, Jun 30, 2014 at 09:53:20AM +0300, Michael S. Tsirkin wrote:
> > On Mon, Jun 30, 2014 at 01:33:42PM +0800, Hu Tao wrote:
> > > On Sun, Jun 29, 2014 at 06:20:22PM +0300, Michael S. Tsirkin wrote:
> > > > On Wed, Jun 25, 2014 at 05:04:14PM +0800, Hu Tao wrote:
> > > > > ..to prevent one memory backend from being used by more than one numa
> > > > > node.
> > > >
> > > > Thanks, but please always make the msg content self-contained
> > > > so it can be understood without the subject.
> > > > E.g. here, just drop "..to".
> > > >
> > > > Are you sure we want this? Is there a chance sharing a backend
> > > > can be useful?
> > >
> > > This patch is actually a bug fix.
> >
> > It is? What is the bug and how to reproduce it?
>
> If user specifies the same memory backend for two numa nodes:
>
> ./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -m 512M \
> -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm \
> -object memory-backend-ram,size=256M,id=ram0 \
> -numa node,nodeid=0,memdev=ram0 \
> -numa node,nodeid=1,memdev=ram0
>
> > I am not sure we should write a ton of code to validate qemu
> > configuration, as long as qemu does not assert.
>
> It seems qemu does not provide a way to disable assert currently.
> Even if I removed asserts on the code path in my test, there is another
> problem that it hits an infinite in render_memory_region().
OK so this is what commit log should say:
--->
Specifying the same memory region twice leads to an assert:
./x86_64-softmmu/qemu-system-x86_64 -m 512M -enable-kvm -object
memory-backend-ram,size=256M,id=ram0 -numa node,nodeid=0,memdev=ram0
-numa node,nodeid=1,memdev=ram0
qemu-system-x86_64: /scm/qemu/memory.c:1506:
memory_region_add_subregion_common: Assertion `!subregion->container'
failed.
Aborted (core dumped)
Detect and exit with an error message instead.
<---
See? Explain why your patch makes sense, don't just repeat what it does.
> >
> > > Even if we will want backend sharing, we
> > > can do it after.
> >
> > By reverting this patch? So why merge it?
>
> The point is qemu doesn't fire a bug no matter what user inputs.
>
> >
> > > >
> > > > Igor, what's your take?
> > > >
> > > > >
> > > > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > > > > ---
> > > > > numa.c | 7 +++++++
> > > > > 1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/numa.c b/numa.c
> > > > > index e471afe..6c1c554 100644
> > > > > --- a/numa.c
> > > > > +++ b/numa.c
> > > > > @@ -279,6 +279,13 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
> > > > > exit(1);
> > > > > }
> > > > >
> > > > > + if (memory_region_is_mapped(seg)) {
> > > > > + char *path = object_get_canonical_path_component(OBJECT(backend));
> > > > > + error_report("memory backend %s is busy", path);
That's not very clear. How about:
memory backend %s is used multiple times. Each -numa option must use a different memdev value.
> > > > > + g_free(path);
As we are going to exit anyway, it does not make sense to bother with this.
> > > > > + exit(1);
> > > > > + }
> > > > > +
> > > > > memory_region_add_subregion(mr, addr, seg);
> > > > > vmstate_register_ram_global(seg);
> > > > > addr += size;
> > > > > --
> > > > > 1.9.3
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: check for busy memory backend
2014-06-30 8:28 ` Michael S. Tsirkin
@ 2014-06-30 8:48 ` Igor Mammedov
2014-06-30 9:12 ` Michael S. Tsirkin
0 siblings, 1 reply; 11+ messages in thread
From: Igor Mammedov @ 2014-06-30 8:48 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Hu Tao, qemu-devel, Yasunori Goto
On Mon, 30 Jun 2014 11:28:07 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Jun 30, 2014 at 03:46:56PM +0800, Hu Tao wrote:
> > On Mon, Jun 30, 2014 at 09:53:20AM +0300, Michael S. Tsirkin wrote:
> > > On Mon, Jun 30, 2014 at 01:33:42PM +0800, Hu Tao wrote:
> > > > On Sun, Jun 29, 2014 at 06:20:22PM +0300, Michael S. Tsirkin wrote:
> > > > > On Wed, Jun 25, 2014 at 05:04:14PM +0800, Hu Tao wrote:
> > > > > > ..to prevent one memory backend from being used by more than one numa
> > > > > > node.
> > > > >
> > > > > Thanks, but please always make the msg content self-contained
> > > > > so it can be understood without the subject.
> > > > > E.g. here, just drop "..to".
> > > > >
> > > > > Are you sure we want this? Is there a chance sharing a backend
> > > > > can be useful?
> > > >
> > > > This patch is actually a bug fix.
> > >
> > > It is? What is the bug and how to reproduce it?
> >
> > If user specifies the same memory backend for two numa nodes:
> >
> > ./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -m 512M \
> > -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm \
> > -object memory-backend-ram,size=256M,id=ram0 \
> > -numa node,nodeid=0,memdev=ram0 \
> > -numa node,nodeid=1,memdev=ram0
> >
> > > I am not sure we should write a ton of code to validate qemu
> > > configuration, as long as qemu does not assert.
> >
> > It seems qemu does not provide a way to disable assert currently.
> > Even if I removed asserts on the code path in my test, there is another
> > problem that it hits an infinite in render_memory_region().
>
> OK so this is what commit log should say:
> --->
> Specifying the same memory region twice leads to an assert:
>
> ./x86_64-softmmu/qemu-system-x86_64 -m 512M -enable-kvm -object
> memory-backend-ram,size=256M,id=ram0 -numa node,nodeid=0,memdev=ram0
> -numa node,nodeid=1,memdev=ram0
> qemu-system-x86_64: /scm/qemu/memory.c:1506:
> memory_region_add_subregion_common: Assertion `!subregion->container'
> failed.
> Aborted (core dumped)
>
> Detect and exit with an error message instead.
> <---
with fixed-up commit message:
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>
> See? Explain why your patch makes sense, don't just repeat what it does.
>
> > >
> > > > Even if we will want backend sharing, we
> > > > can do it after.
> > >
> > > By reverting this patch? So why merge it?
> >
> > The point is qemu doesn't fire a bug no matter what user inputs.
> >
> > >
> > > > >
> > > > > Igor, what's your take?
> > > > >
> > > > > >
> > > > > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > > > > > ---
> > > > > > numa.c | 7 +++++++
> > > > > > 1 file changed, 7 insertions(+)
> > > > > >
> > > > > > diff --git a/numa.c b/numa.c
> > > > > > index e471afe..6c1c554 100644
> > > > > > --- a/numa.c
> > > > > > +++ b/numa.c
> > > > > > @@ -279,6 +279,13 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
> > > > > > exit(1);
> > > > > > }
> > > > > >
> > > > > > + if (memory_region_is_mapped(seg)) {
> > > > > > + char *path = object_get_canonical_path_component(OBJECT(backend));
> > > > > > + error_report("memory backend %s is busy", path);
>
> That's not very clear. How about:
> memory backend %s is used multiple times. Each -numa option must use a different memdev value.
>
> > > > > > + g_free(path);
>
> As we are going to exit anyway, it does not make sense to bother with this.
>
> > > > > > + exit(1);
> > > > > > + }
> > > > > > +
> > > > > > memory_region_add_subregion(mr, addr, seg);
> > > > > > vmstate_register_ram_global(seg);
> > > > > > addr += size;
> > > > > > --
> > > > > > 1.9.3
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: check for busy memory backend
2014-06-30 8:48 ` Igor Mammedov
@ 2014-06-30 9:12 ` Michael S. Tsirkin
2014-06-30 10:16 ` Hu Tao
0 siblings, 1 reply; 11+ messages in thread
From: Michael S. Tsirkin @ 2014-06-30 9:12 UTC (permalink / raw)
To: Igor Mammedov; +Cc: Hu Tao, qemu-devel, Yasunori Goto
On Mon, Jun 30, 2014 at 10:48:22AM +0200, Igor Mammedov wrote:
> On Mon, 30 Jun 2014 11:28:07 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Mon, Jun 30, 2014 at 03:46:56PM +0800, Hu Tao wrote:
> > > On Mon, Jun 30, 2014 at 09:53:20AM +0300, Michael S. Tsirkin wrote:
> > > > On Mon, Jun 30, 2014 at 01:33:42PM +0800, Hu Tao wrote:
> > > > > On Sun, Jun 29, 2014 at 06:20:22PM +0300, Michael S. Tsirkin wrote:
> > > > > > On Wed, Jun 25, 2014 at 05:04:14PM +0800, Hu Tao wrote:
> > > > > > > ..to prevent one memory backend from being used by more than one numa
> > > > > > > node.
> > > > > >
> > > > > > Thanks, but please always make the msg content self-contained
> > > > > > so it can be understood without the subject.
> > > > > > E.g. here, just drop "..to".
> > > > > >
> > > > > > Are you sure we want this? Is there a chance sharing a backend
> > > > > > can be useful?
> > > > >
> > > > > This patch is actually a bug fix.
> > > >
> > > > It is? What is the bug and how to reproduce it?
> > >
> > > If user specifies the same memory backend for two numa nodes:
> > >
> > > ./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -m 512M \
> > > -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm \
> > > -object memory-backend-ram,size=256M,id=ram0 \
> > > -numa node,nodeid=0,memdev=ram0 \
> > > -numa node,nodeid=1,memdev=ram0
> > >
> > > > I am not sure we should write a ton of code to validate qemu
> > > > configuration, as long as qemu does not assert.
> > >
> > > It seems qemu does not provide a way to disable assert currently.
> > > Even if I removed asserts on the code path in my test, there is another
> > > problem that it hits an infinite in render_memory_region().
> >
> > OK so this is what commit log should say:
> > --->
> > Specifying the same memory region twice leads to an assert:
> >
> > ./x86_64-softmmu/qemu-system-x86_64 -m 512M -enable-kvm -object
> > memory-backend-ram,size=256M,id=ram0 -numa node,nodeid=0,memdev=ram0
> > -numa node,nodeid=1,memdev=ram0
> > qemu-system-x86_64: /scm/qemu/memory.c:1506:
> > memory_region_add_subregion_common: Assertion `!subregion->container'
> > failed.
> > Aborted (core dumped)
> >
> > Detect and exit with an error message instead.
> > <---
> with fixed-up commit message:
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Sorry I want the error message fixed up too.
> >
> > See? Explain why your patch makes sense, don't just repeat what it does.
> >
> > > >
> > > > > Even if we will want backend sharing, we
> > > > > can do it after.
> > > >
> > > > By reverting this patch? So why merge it?
> > >
> > > The point is qemu doesn't fire a bug no matter what user inputs.
> > >
> > > >
> > > > > >
> > > > > > Igor, what's your take?
> > > > > >
> > > > > > >
> > > > > > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > > > > > > ---
> > > > > > > numa.c | 7 +++++++
> > > > > > > 1 file changed, 7 insertions(+)
> > > > > > >
> > > > > > > diff --git a/numa.c b/numa.c
> > > > > > > index e471afe..6c1c554 100644
> > > > > > > --- a/numa.c
> > > > > > > +++ b/numa.c
> > > > > > > @@ -279,6 +279,13 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
> > > > > > > exit(1);
> > > > > > > }
> > > > > > >
> > > > > > > + if (memory_region_is_mapped(seg)) {
> > > > > > > + char *path = object_get_canonical_path_component(OBJECT(backend));
> > > > > > > + error_report("memory backend %s is busy", path);
> >
> > That's not very clear. How about:
> > memory backend %s is used multiple times. Each -numa option must use a different memdev value.
> >
> > > > > > > + g_free(path);
> >
> > As we are going to exit anyway, it does not make sense to bother with this.
> >
> > > > > > > + exit(1);
> > > > > > > + }
> > > > > > > +
> > > > > > > memory_region_add_subregion(mr, addr, seg);
> > > > > > > vmstate_register_ram_global(seg);
> > > > > > > addr += size;
> > > > > > > --
> > > > > > > 1.9.3
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] numa: check for busy memory backend
2014-06-30 9:12 ` Michael S. Tsirkin
@ 2014-06-30 10:16 ` Hu Tao
0 siblings, 0 replies; 11+ messages in thread
From: Hu Tao @ 2014-06-30 10:16 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Igor Mammedov, qemu-devel, Yasunori Goto
On Mon, Jun 30, 2014 at 12:12:20PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jun 30, 2014 at 10:48:22AM +0200, Igor Mammedov wrote:
> > On Mon, 30 Jun 2014 11:28:07 +0300
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Mon, Jun 30, 2014 at 03:46:56PM +0800, Hu Tao wrote:
> > > > On Mon, Jun 30, 2014 at 09:53:20AM +0300, Michael S. Tsirkin wrote:
> > > > > On Mon, Jun 30, 2014 at 01:33:42PM +0800, Hu Tao wrote:
> > > > > > On Sun, Jun 29, 2014 at 06:20:22PM +0300, Michael S. Tsirkin wrote:
> > > > > > > On Wed, Jun 25, 2014 at 05:04:14PM +0800, Hu Tao wrote:
> > > > > > > > ..to prevent one memory backend from being used by more than one numa
> > > > > > > > node.
> > > > > > >
> > > > > > > Thanks, but please always make the msg content self-contained
> > > > > > > so it can be understood without the subject.
> > > > > > > E.g. here, just drop "..to".
> > > > > > >
> > > > > > > Are you sure we want this? Is there a chance sharing a backend
> > > > > > > can be useful?
> > > > > >
> > > > > > This patch is actually a bug fix.
> > > > >
> > > > > It is? What is the bug and how to reproduce it?
> > > >
> > > > If user specifies the same memory backend for two numa nodes:
> > > >
> > > > ./x86_64-softmmu/qemu-system-x86_64 -hda /home/data/libvirt-images/f18.img -m 512M \
> > > > -qmp unix:/tmp/m,server,nowait -monitor stdio -enable-kvm \
> > > > -object memory-backend-ram,size=256M,id=ram0 \
> > > > -numa node,nodeid=0,memdev=ram0 \
> > > > -numa node,nodeid=1,memdev=ram0
> > > >
> > > > > I am not sure we should write a ton of code to validate qemu
> > > > > configuration, as long as qemu does not assert.
> > > >
> > > > It seems qemu does not provide a way to disable assert currently.
> > > > Even if I removed asserts on the code path in my test, there is another
> > > > problem that it hits an infinite in render_memory_region().
> > >
> > > OK so this is what commit log should say:
> > > --->
> > > Specifying the same memory region twice leads to an assert:
> > >
> > > ./x86_64-softmmu/qemu-system-x86_64 -m 512M -enable-kvm -object
> > > memory-backend-ram,size=256M,id=ram0 -numa node,nodeid=0,memdev=ram0
> > > -numa node,nodeid=1,memdev=ram0
> > > qemu-system-x86_64: /scm/qemu/memory.c:1506:
> > > memory_region_add_subregion_common: Assertion `!subregion->container'
> > > failed.
> > > Aborted (core dumped)
> > >
> > > Detect and exit with an error message instead.
> > > <---
> > with fixed-up commit message:
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
>
> Sorry I want the error message fixed up too.
Yes your error message is more clear. I'll send v2. Thanks for review.
Regards,
Hu
>
> > >
> > > See? Explain why your patch makes sense, don't just repeat what it does.
> > >
> > > > >
> > > > > > Even if we will want backend sharing, we
> > > > > > can do it after.
> > > > >
> > > > > By reverting this patch? So why merge it?
> > > >
> > > > The point is qemu doesn't fire a bug no matter what user inputs.
> > > >
> > > > >
> > > > > > >
> > > > > > > Igor, what's your take?
> > > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > > > > > > > ---
> > > > > > > > numa.c | 7 +++++++
> > > > > > > > 1 file changed, 7 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/numa.c b/numa.c
> > > > > > > > index e471afe..6c1c554 100644
> > > > > > > > --- a/numa.c
> > > > > > > > +++ b/numa.c
> > > > > > > > @@ -279,6 +279,13 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
> > > > > > > > exit(1);
> > > > > > > > }
> > > > > > > >
> > > > > > > > + if (memory_region_is_mapped(seg)) {
> > > > > > > > + char *path = object_get_canonical_path_component(OBJECT(backend));
> > > > > > > > + error_report("memory backend %s is busy", path);
> > >
> > > That's not very clear. How about:
> > > memory backend %s is used multiple times. Each -numa option must use a different memdev value.
> > >
> > > > > > > > + g_free(path);
> > >
> > > As we are going to exit anyway, it does not make sense to bother with this.
> > >
> > > > > > > > + exit(1);
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > memory_region_add_subregion(mr, addr, seg);
> > > > > > > > vmstate_register_ram_global(seg);
> > > > > > > > addr += size;
> > > > > > > > --
> > > > > > > > 1.9.3
> > >
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-06-30 10:18 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-06-25 9:04 [Qemu-devel] [PATCH] numa: check for busy memory backend Hu Tao
2014-06-25 10:02 ` Igor Mammedov
2014-06-25 11:48 ` Markus Armbruster
2014-06-29 15:20 ` Michael S. Tsirkin
2014-06-30 5:33 ` Hu Tao
2014-06-30 6:53 ` Michael S. Tsirkin
2014-06-30 7:46 ` Hu Tao
2014-06-30 8:28 ` Michael S. Tsirkin
2014-06-30 8:48 ` Igor Mammedov
2014-06-30 9:12 ` Michael S. Tsirkin
2014-06-30 10:16 ` Hu Tao
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.