Re: [Xen-devel] [PATCH v2 for-4.5] libxl: account for romfile memory

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 25 Nov 2014, Ian Campbell wrote:
> On Tue, 2014-11-25 at 16:49 +0000, Stefano Stabellini wrote:
> > On Tue, 25 Nov 2014, Ian Campbell wrote:
> > > On Tue, 2014-11-25 at 12:43 +0000, Stefano Stabellini wrote:
> > > > Account for the extra memory needed for the rom files of any emulated 
> > > > nics:
> > > > QEMU uses xc_domain_populate_physmap_exact to allocate the memory for
> > > > each them. Assume 256K each.
> > > 
> > > I suppose this will have to do for 4.5. Can we do something better in
> > > the future -- like figuring out a way for guests to have
> > > "not-really-RAM" allocations like this which are made by the toolstack
> > > and happen to be backed by RAM not count or something.
> > > 
> > > > 
> > > > This patch fixes a QEMU abort() when more than 4 emulated nics are
> > > > assigned to a VM.
> > > 
> > > Are you also going to fix qemu to fail gracefully if it cannot deploy
> > > option roms? abort() seems a bit extreme.
> > > 
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > > > CC: Don Slutz <dslutz@xxxxxxxxxxx>
> > > > CC: hanyandong <hanyandong@xxxxxxxxx>
> > > > CC: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> > > > CC: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
> > > > CC: Wei Liu <wei.liu2@xxxxxxxxxx>
> > > 
> > > You missed Ian J. I've added him.
> > 
> > Actually Wei suggested a better alternative: I could call
> > xc_domain_setmaxmem directly from QEMU. That makes much more sense.
> 
> xl mem-set would do it again, but not taking qemu's extras into account,
> unless you communicate the overhead somehow...

We could start reading the current maxmem and add to it in
libxl_set_memory_target. Or we could write the maxmem to xenstore and
read it back again.  Given that the allocations are only done by QEMU at
initialization time, I don't think we need to worry about concurrency
here.


> > 
> > > > 
> > > > ---
> > > > 
> > > > Changes in v2:
> > > > - remove double return statement;
> > > > - check for return errors;
> > > > - check for overflows.
> > > > ---
> > > >  tools/libxl/libxl.c          |   53 
> > > > +++++++++++++++++++++++++++++++++++++-----
> > > >  tools/libxl/libxl_dom.c      |    8 +++++--
> > > >  tools/libxl/libxl_internal.h |    7 ++++++
> > > >  3 files changed, 60 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > > index de23fec..2cdb768 100644
> > > > --- a/tools/libxl/libxl.c
> > > > +++ b/tools/libxl/libxl.c
> > > > @@ -4527,13 +4527,40 @@ out:
> > > >  
> > > >  
> > > > /******************************************************************************/
> > > >  
> > > > +int libxl__get_rom_memory_kb(libxl__gc *gc, uint32_t domid, 
> > > > libxl_domain_config *d_config)
> > > > +{
> > > > +    int i, romsize, rc;
> > > > +    libxl_domain_config local_d_config;
> > > > +    libxl_ctx *ctx = libxl__gc_owner(gc);
> > > > +
> > > > +    if (d_config == NULL) {
> > > > +        libxl_domain_config_init(&local_d_config);
> > > > +        rc = libxl_retrieve_domain_configuration(ctx, domid, 
> > > > &local_d_config);
> > > > +        if (rc < 0)
> > > > +            return rc;
> > > > +        d_config = &local_d_config;
> > > > +    }
> > > 
> > > Perhaps we could store the answer to this function in XS when we build
> > > the domain and simply read it back and account for it in the places
> > > which use it?
> > > 
> > > Apart from being rather costly reparsing the json every time is going to
> > > behave a bit strangely if NICs are plugged/unplugged at runtime and
> > > ballooning is going on.
> > > 
> > > > +    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV)
> > > > +        return 0;
> > > > +
> > > > +    for (i = 0, romsize = 0;
> > > > +         i < d_config->num_nics && romsize < INT_MAX;
> > > 
> > > I don't think that romsize < INT_MAX is useful except in the case that
> > > romsize+= results in romsize == INT_MAX. If you actually overflow then
> > > romsize becomes negative which satisfies the condition (and in any case
> > > you are into undefined behaviour territory there anyhow, I think).
> > > 
> > > Given that INT_MAX is a boat load of ROMs I'd be inclined to just limit
> > > it to INT_MAX/2 or /4 or something.
> > > 
> > > Or you could do romsize < (INT_MAX - LIBXL_ROMSIZE_KB) I suppose.
> > > 
> > > > +    rc = xc_domain_setmaxmem(ctx->xch, domid,
> > > > +                             max_memkb + LIBXL_MAXMEM_CONSTANT
> > > > +                             + romsize);
> > > 
> > > Seems like we ought to have a helper to return the memory overheads,
> > > which would be the constant + the romsize starting from now...
> > > 
> > > Ian.
> > > 
> 
> 

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