[Qemu-devel] [PATCH] numa: set the memory backend "is_mapped" field

[Qemu-devel] [PATCH] numa: set the memory backend "is_mapped" field

[Greg Kurz]

Commit 2aece63 "hostmem: detect host backend memory is being used properly"
added a way to know if a memory backend is busy or available for use. It
caused a slight regression if we pass the same backend to a NUMA node and
to a pc-dimm device:

-m 1G,slots=2,maxmem=2G \
-object memory-backend-ram,size=1G,id=mem-mem1 \
-device pc-dimm,id=dimm-mem1,memdev=mem-mem1 \
-numa node,nodeid=0,memdev=mem-mem1

Before commit 2aece63, this would cause QEMU to print an error message and
to exit gracefully:

qemu-system-ppc64: -device pc-dimm,id=dimm-mem1,memdev=mem-mem1:
    can't use already busy memdev: mem-mem1

Since commit 2aece63, QEMU hits an assertion in the memory code:

qemu-system-ppc64: memory.c:1934: memory_region_add_subregion_common:
    Assertion `!subregion->container' failed.
Aborted

This happens because pc-dimm devices don't use memory_region_is_mapped()
anymore and cannot guess the backend is already used by a NUMA node.

Let's revert to the previous behavior by turning the NUMA code to also
call host_memory_backend_set_mapped() when it uses a backend.

Fixes: 2aece63c8a9d2c3a8ff41d2febc4cdeff2633331
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 numa.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/numa.c b/numa.c
index cbae430e3644..72861713e5fd 100644
--- a/numa.c
+++ b/numa.c
@@ -463,6 +463,7 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
             exit(1);
         }
 
+        host_memory_backend_set_mapped(backend, true);
         memory_region_add_subregion(mr, addr, seg);
         vmstate_register_ram_global(seg);
         addr += size;

Re: [Qemu-devel] [PATCH] numa: set the memory backend "is_mapped" field

[Igor Mammedov]

On Tue, 19 Jul 2016 10:28:35 +0200
Greg Kurz <groug@kaod.org> wrote:

> Commit 2aece63 "hostmem: detect host backend memory is being used properly"
> added a way to know if a memory backend is busy or available for use. It
> caused a slight regression if we pass the same backend to a NUMA node and
> to a pc-dimm device:
> 
> -m 1G,slots=2,maxmem=2G \
> -object memory-backend-ram,size=1G,id=mem-mem1 \
> -device pc-dimm,id=dimm-mem1,memdev=mem-mem1 \
> -numa node,nodeid=0,memdev=mem-mem1
> 
> Before commit 2aece63, this would cause QEMU to print an error message and
> to exit gracefully:
> 
> qemu-system-ppc64: -device pc-dimm,id=dimm-mem1,memdev=mem-mem1:
>     can't use already busy memdev: mem-mem1
> 
> Since commit 2aece63, QEMU hits an assertion in the memory code:
> 
> qemu-system-ppc64: memory.c:1934: memory_region_add_subregion_common:
>     Assertion `!subregion->container' failed.
> Aborted
> 
> This happens because pc-dimm devices don't use memory_region_is_mapped()
> anymore and cannot guess the backend is already used by a NUMA node.
> 
> Let's revert to the previous behavior by turning the NUMA code to also
> call host_memory_backend_set_mapped() when it uses a backend.
> 
> Fixes: 2aece63c8a9d2c3a8ff41d2febc4cdeff2633331
> Signed-off-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  numa.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/numa.c b/numa.c
> index cbae430e3644..72861713e5fd 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -463,6 +463,7 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
>              exit(1);
>          }
>  
> +        host_memory_backend_set_mapped(backend, true);
>          memory_region_add_subregion(mr, addr, seg);
>          vmstate_register_ram_global(seg);
>          addr += size;
> 
> 

Re: [Qemu-devel] [PATCH] numa: set the memory backend "is_mapped" field

[Eduardo Habkost]

On Tue, Jul 19, 2016 at 12:07:53PM +0200, Igor Mammedov wrote:
> On Tue, 19 Jul 2016 10:28:35 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > Commit 2aece63 "hostmem: detect host backend memory is being used properly"
> > added a way to know if a memory backend is busy or available for use. It
> > caused a slight regression if we pass the same backend to a NUMA node and
> > to a pc-dimm device:
> > 
> > -m 1G,slots=2,maxmem=2G \
> > -object memory-backend-ram,size=1G,id=mem-mem1 \
> > -device pc-dimm,id=dimm-mem1,memdev=mem-mem1 \
> > -numa node,nodeid=0,memdev=mem-mem1
> > 
> > Before commit 2aece63, this would cause QEMU to print an error message and
> > to exit gracefully:
> > 
> > qemu-system-ppc64: -device pc-dimm,id=dimm-mem1,memdev=mem-mem1:
> >     can't use already busy memdev: mem-mem1
> > 
> > Since commit 2aece63, QEMU hits an assertion in the memory code:
> > 
> > qemu-system-ppc64: memory.c:1934: memory_region_add_subregion_common:
> >     Assertion `!subregion->container' failed.
> > Aborted
> > 
> > This happens because pc-dimm devices don't use memory_region_is_mapped()
> > anymore and cannot guess the backend is already used by a NUMA node.
> > 
> > Let's revert to the previous behavior by turning the NUMA code to also
> > call host_memory_backend_set_mapped() when it uses a backend.
> > 
> > Fixes: 2aece63c8a9d2c3a8ff41d2febc4cdeff2633331
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Igor Mammedov <imammedo@redhat.com>

I have just noticed that this fell through the cracks, sorry.
Applied to numa-next. Thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH] numa: set the memory backend "is_mapped" field

[Paolo Bonzini]

----- Original Message -----
> From: "Eduardo Habkost" <ehabkost@redhat.com>
> To: "Igor Mammedov" <imammedo@redhat.com>
> Cc: "Greg Kurz" <groug@kaod.org>, "Thomas Huth" <thuth@redhat.com>, qemu-devel@nongnu.org, pbonzini@redhat.com
> Sent: Tuesday, August 2, 2016 8:00:06 PM
> Subject: Re: [Qemu-devel] [PATCH] numa: set the memory backend "is_mapped" field
> 
> On Tue, Jul 19, 2016 at 12:07:53PM +0200, Igor Mammedov wrote:
> > On Tue, 19 Jul 2016 10:28:35 +0200
> > Greg Kurz <groug@kaod.org> wrote:
> > 
> > > Commit 2aece63 "hostmem: detect host backend memory is being used
> > > properly"
> > > added a way to know if a memory backend is busy or available for use. It
> > > caused a slight regression if we pass the same backend to a NUMA node and
> > > to a pc-dimm device:
> > > 
> > > -m 1G,slots=2,maxmem=2G \
> > > -object memory-backend-ram,size=1G,id=mem-mem1 \
> > > -device pc-dimm,id=dimm-mem1,memdev=mem-mem1 \
> > > -numa node,nodeid=0,memdev=mem-mem1
> > > 
> > > Before commit 2aece63, this would cause QEMU to print an error message
> > > and
> > > to exit gracefully:
> > > 
> > > qemu-system-ppc64: -device pc-dimm,id=dimm-mem1,memdev=mem-mem1:
> > >     can't use already busy memdev: mem-mem1
> > > 
> > > Since commit 2aece63, QEMU hits an assertion in the memory code:
> > > 
> > > qemu-system-ppc64: memory.c:1934: memory_region_add_subregion_common:
> > >     Assertion `!subregion->container' failed.
> > > Aborted
> > > 
> > > This happens because pc-dimm devices don't use memory_region_is_mapped()
> > > anymore and cannot guess the backend is already used by a NUMA node.
> > > 
> > > Let's revert to the previous behavior by turning the NUMA code to also
> > > call host_memory_backend_set_mapped() when it uses a backend.
> > > 
> > > Fixes: 2aece63c8a9d2c3a8ff41d2febc4cdeff2633331
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > Reviewed-by: Igor Mammedov <imammedo@redhat.com>
> 
> I have just noticed that this fell through the cracks, sorry.
> Applied to numa-next. Thanks!

Actually I am just finishing tests of a pull request that included it
(because I was the one that caused the regression).  Ok for me to just
handle this one patch?

Paolo

Re: [Qemu-devel] [PATCH] numa: set the memory backend "is_mapped" field

[Eduardo Habkost]

On Tue, Aug 02, 2016 at 03:20:41PM -0400, Paolo Bonzini wrote:
[...]
> > I have just noticed that this fell through the cracks, sorry.
> > Applied to numa-next. Thanks!
> 
> Actually I am just finishing tests of a pull request that included it
> (because I was the one that caused the regression).  Ok for me to just
> handle this one patch?

Absolutely. It will save me the work of sending a pull request
just for this patch. Thanks!

-- 
Eduardo