From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53300) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1Yf9-0005hz-VV for qemu-devel@nongnu.org; Mon, 30 Jun 2014 06:18:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X1Yf5-0003oF-6v for qemu-devel@nongnu.org; Mon, 30 Jun 2014 06:18:15 -0400 Received: from [59.151.112.132] (port=22065 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X1Yf4-0003mC-FH for qemu-devel@nongnu.org; Mon, 30 Jun 2014 06:18:11 -0400 Date: Mon, 30 Jun 2014 18:16:08 +0800 From: Hu Tao Message-ID: <20140630101608.GB27778@G08FNSTD100614.fnst.cn.fujitsu.com> References: <1403687054-13682-1-git-send-email-hutao@cn.fujitsu.com> <20140629152022.GC29448@redhat.com> <20140630053342.GA8535@G08FNSTD100614.fnst.cn.fujitsu.com> <20140630065320.GC14491@redhat.com> <20140630074656.GA26311@G08FNSTD100614.fnst.cn.fujitsu.com> <20140630082807.GA15777@redhat.com> <20140630104822.0b43540e@nial.usersys.redhat.com> <20140630091220.GD15777@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline In-Reply-To: <20140630091220.GD15777@redhat.com> Subject: Re: [Qemu-devel] [PATCH] numa: check for busy memory backend List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: Igor Mammedov , qemu-devel@nongnu.org, 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" 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 > > 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 > > > > > > > > --- > > > > > > > > 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 > > >