Re: [Qemu-devel] [PATCH] numa: check for busy memory backend

[Hu Tao]

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" <address@hidden> 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 <address@hidden>
> 
> 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 <address@hidden>
> > > > > > > > ---
> > > > > > > >  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
> > >