Re: [Xen-devel] [Qemu-devel] [PATCH] increase maxmem before calling xc_domain_populate_physmap

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 26 Nov 2014, Don Slutz wrote:
> 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 <stefano.stabellini@xxxxxxxxxxxxx>
> > > > 
> > > > 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.

Right


> > > ~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.

I don't understand: are you aware that setmaxmem doesn't allocate any
memory, just raises the maximum amount of memory allowed for the domain
to have?

But you are right that we would raise the limit more than it could be,
specifically the videoram would get accounted for twice and we wouldn't
need LIBXL_MAXMEM_CONSTANT. I guess we would have to write a patch for
that.



> 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)) {

That's an interesting idea, but I am not sure if it is safe in all
configurations.

It could make QEMU work better with older libxl and avoid increasing
maxmem more than necessary.
On the other hand I guess it could break things when PoD is used, or in
general when the user purposely sets maxmem on the vm config file.


> 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.

I think that raising maxmem a bit higher than necessary is not too bad.
If we really care about it, we could lower the limit after QEMU's
initialization is completed.


>    -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@xxxxxxxxxxxxx
> > > > http://lists.xen.org/xen-devel
> > > 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel