From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33447) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xtp2D-0004sv-Hk for qemu-devel@nongnu.org; Wed, 26 Nov 2014 21:42:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xtp24-0005o3-CW for qemu-devel@nongnu.org; Wed, 26 Nov 2014 21:42:21 -0500 Received: from omzsmtpe04.verizonbusiness.com ([199.249.25.207]:16804) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xtp24-0005mI-77 for qemu-devel@nongnu.org; Wed, 26 Nov 2014 21:42:12 -0500 From: Don Slutz Message-ID: <54768F7F.2030602@terremark.com> Date: Wed, 26 Nov 2014 21:42:07 -0500 MIME-Version: 1.0 References: <5474C96A.6090506@citrix.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Xen-devel] [PATCH] increase maxmem before calling xc_domain_populate_physmap List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: Andrew Cooper , xen-devel@lists.xensource.com, "Wei Liu (Intern)" , Ian Campbell , qemu-devel@nongnu.org On 11/26/14 13:17, Stefano Stabellini wrote: > On Tue, 25 Nov 2014, Andrew Cooper wrote: >> On 25/11/14 17:45, Stefano Stabellini wrote: >>> Increase maxmem before calling xc_domain_populate_physmap_exact to avoid >>> the risk of running out of guest memory. This way we can also avoid >>> complex memory calculations in libxl at domain construction time. >>> >>> This patch fixes an abort() when assigning more than 4 NICs to a VM. >>> >>> Signed-off-by: Stefano Stabellini >>> >>> diff --git a/xen-hvm.c b/xen-hvm.c >>> index 5c69a8d..38e08c3 100644 >>> --- a/xen-hvm.c >>> +++ b/xen-hvm.c >>> @@ -218,6 +218,7 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr) >>> unsigned long nr_pfn; >>> xen_pfn_t *pfn_list; >>> int i; >>> + xc_dominfo_t info; >>> >>> if (runstate_check(RUN_STATE_INMIGRATE)) { >>> /* RAM already populated in Xen */ >>> @@ -240,6 +241,13 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr) >>> pfn_list[i] = (ram_addr >> TARGET_PAGE_BITS) + i; >>> } >>> >>> + if (xc_domain_getinfo(xen_xc, xen_domid, 1, &info) < 0) { >> xc_domain_getinfo()'s interface is mad, and provides no guarantee that >> it returns the information for the domain you requested. It also won't >> return -1 on error. The correct error handing is: >> >> (xc_domain_getinfo(xen_xc, xen_domid, 1, &info) != 1) || (info.domid != >> xen_domid) > It might be wiser to switch to xc_domain_getinfolist Either needs the same tests, since both return an vector of info. > >> ~Andrew >> >>> + hw_error("xc_domain_getinfo failed"); >>> + } >>> + if (xc_domain_setmaxmem(xen_xc, xen_domid, info.max_memkb + >>> + (nr_pfn * XC_PAGE_SIZE / 1024)) < 0) { There are two big issues and 1 minor one with this. 1) You will allocate the videoram again. 2) You will never use the 1 MB already allocated for option ROMs. And the minor one is that you can increase maxmem more then is needed. Here is a better if: - if (xc_domain_setmaxmem(xen_xc, xen_domid, info.max_memkb + - (nr_pfn * XC_PAGE_SIZE / 1024)) < 0) { + max_pages = info.max_memkb * 1024 / XC_PAGE_SIZE; + free_pages = max_pages - info.nr_pages; + need_pages = nr_pfn - free_pages; + if ((free_pages < nr_pfn) && + (xc_domain_setmaxmem(xen_xc, xen_domid, info.max_memkb + + (need_pages * XC_PAGE_SIZE / 1024)) < 0)) { My testing shows a free 32 pages that I am not sure where they come from. But the code about is passing my 8 nics of e1000. -Don Slutz >>> + hw_error("xc_domain_setmaxmem failed"); >>> + } >>> if (xc_domain_populate_physmap_exact(xen_xc, xen_domid, nr_pfn, 0, 0, pfn_list)) { >>> hw_error("xen: failed to populate ram at " RAM_ADDR_FMT, ram_addr); >>> } >>> >>> _______________________________________________ >>> Xen-devel mailing list >>> Xen-devel@lists.xen.org >>> http://lists.xen.org/xen-devel >> From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [Xen-devel] [PATCH] increase maxmem before calling xc_domain_populate_physmap Date: Wed, 26 Nov 2014 21:42:07 -0500 Message-ID: <54768F7F.2030602@terremark.com> References: <5474C96A.6090506@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org To: Stefano Stabellini Cc: Andrew Cooper , xen-devel@lists.xensource.com, "Wei Liu (Intern)" , Ian Campbell , qemu-devel@nongnu.org List-Id: xen-devel@lists.xenproject.org On 11/26/14 13:17, Stefano Stabellini wrote: > On Tue, 25 Nov 2014, Andrew Cooper wrote: >> On 25/11/14 17:45, Stefano Stabellini wrote: >>> Increase maxmem before calling xc_domain_populate_physmap_exact to avoid >>> the risk of running out of guest memory. This way we can also avoid >>> complex memory calculations in libxl at domain construction time. >>> >>> This patch fixes an abort() when assigning more than 4 NICs to a VM. >>> >>> Signed-off-by: Stefano Stabellini >>> >>> diff --git a/xen-hvm.c b/xen-hvm.c >>> index 5c69a8d..38e08c3 100644 >>> --- a/xen-hvm.c >>> +++ b/xen-hvm.c >>> @@ -218,6 +218,7 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr) >>> unsigned long nr_pfn; >>> xen_pfn_t *pfn_list; >>> int i; >>> + xc_dominfo_t info; >>> >>> if (runstate_check(RUN_STATE_INMIGRATE)) { >>> /* RAM already populated in Xen */ >>> @@ -240,6 +241,13 @@ void xen_ram_alloc(ram_addr_t ram_addr, ram_addr_t size, MemoryRegion *mr) >>> pfn_list[i] = (ram_addr >> TARGET_PAGE_BITS) + i; >>> } >>> >>> + if (xc_domain_getinfo(xen_xc, xen_domid, 1, &info) < 0) { >> xc_domain_getinfo()'s interface is mad, and provides no guarantee that >> it returns the information for the domain you requested. It also won't >> return -1 on error. The correct error handing is: >> >> (xc_domain_getinfo(xen_xc, xen_domid, 1, &info) != 1) || (info.domid != >> xen_domid) > It might be wiser to switch to xc_domain_getinfolist Either needs the same tests, since both return an vector of info. > >> ~Andrew >> >>> + hw_error("xc_domain_getinfo failed"); >>> + } >>> + if (xc_domain_setmaxmem(xen_xc, xen_domid, info.max_memkb + >>> + (nr_pfn * XC_PAGE_SIZE / 1024)) < 0) { There are two big issues and 1 minor one with this. 1) You will allocate the videoram again. 2) You will never use the 1 MB already allocated for option ROMs. And the minor one is that you can increase maxmem more then is needed. Here is a better if: - if (xc_domain_setmaxmem(xen_xc, xen_domid, info.max_memkb + - (nr_pfn * XC_PAGE_SIZE / 1024)) < 0) { + max_pages = info.max_memkb * 1024 / XC_PAGE_SIZE; + free_pages = max_pages - info.nr_pages; + need_pages = nr_pfn - free_pages; + if ((free_pages < nr_pfn) && + (xc_domain_setmaxmem(xen_xc, xen_domid, info.max_memkb + + (need_pages * XC_PAGE_SIZE / 1024)) < 0)) { My testing shows a free 32 pages that I am not sure where they come from. But the code about is passing my 8 nics of e1000. -Don Slutz >>> + hw_error("xc_domain_setmaxmem failed"); >>> + } >>> if (xc_domain_populate_physmap_exact(xen_xc, xen_domid, nr_pfn, 0, 0, pfn_list)) { >>> hw_error("xen: failed to populate ram at " RAM_ADDR_FMT, ram_addr); >>> } >>> >>> _______________________________________________ >>> Xen-devel mailing list >>> Xen-devel@lists.xen.org >>> http://lists.xen.org/xen-devel >>