Home
Reading
Searching
Subscribe
Sponsors
Statistics
Posting
Contact
Spam
Lists
Links
About
Hosting
Filtering
Features Download
Marketing
Archives
FAQ
Blog
 
Gmane

From: Stefano Stabellini <stefano.stabellini <at> eu.citrix.com>
Subject: Re: [PATCH for-4.5] libxl: do not load roms for any NICs except the first to avoid wasting memory
Newsgroups: gmane.comp.emulators.xen.devel
Date: Tuesday 25th November 2014 12:21:42 UTC (over 4 years ago)
On Mon, 24 Nov 2014, Don Slutz wrote:
> On 11/24/14 12:07, Stefano Stabellini wrote:
> > On Mon, 24 Nov 2014, Don Slutz wrote:
> > > On 11/24/14 10:26, Stefano Stabellini wrote:
> > > > On Mon, 24 Nov 2014, Konrad Rzeszutek Wilk wrote:
> > > > > On Mon, Nov 24, 2014 at 12:17:12PM +0000, Stefano Stabellini
wrote:
> > > > > > On Fri, 21 Nov 2014, Konrad Rzeszutek Wilk wrote:
> > > > > > > On Fri, Nov 21, 2014 at 06:48:53PM +0000, Stefano Stabellini
> > > > > > > wrote:
> > > > > > > > On Fri, 21 Nov 2014, Konrad Rzeszutek Wilk wrote:
> > > > > > > > > On Fri, Nov 21, 2014 at 05:11:09PM +0000, Stefano
Stabellini
> > > > > > > > > wrote:
> > > > > > > > > > The rom is used for pxebooting. We don't need to allow
> > > > > > > > > > pxebooting from
> > > > > > > > > > more than one network card.  Loading a romfile for
every NIC
> > > > > > > > > > wastes
> > > > > > > > > Why not? Why can't we PXE boot from each network card?
> > > > > > > > I take it back: you are right, at the moment it is actually
> > > > > > > > possible
> > > > > > > > to
> > > > > > > > pxe boot from the fourth nic for example. Maybe we could
just
> > > > > > > > load
> > > > > > > > the
> > > > > > > > first four romfiles and skip the others (four is the limit
> > > > > > > > before
> > > > > > > > QEMU
> > > > > > > > fails to allocate any more memory).
> > > > > > > The limit is in the count of ROMs or the memory consumption?
> > > > > > The limit is memory consumption.
> > > > > Um, how big are those ROMs? Gigs?
> > > > I think they are 60K each in the case of rtl8139.
> > > > However they are accounted on top of the ram of the guest, that's
why
> > > > the allocation fails. Strictly speaking I guess it shouldn't work
even
> > > > the first time around.
> > > My extra QEMU debug says:
> > > 
> > > xen_ram_alloc: alloc 40000 bytes (256 Kib) of ram at 42010000
> > > mr.name=rtl8139.rom
> > > e1000 vhdr enabled
> > > xen_ram_alloc: alloc 40000 bytes (256 Kib) of ram at 42050000
> > > mr.name=e1000.rom
> > > 
> > > And that matches the max of 4.
> > > 
> > > #define LIBXL_MAXMEM_CONSTANT 1024
> > > 
> > > is what controls this.
> > > 
> > > > Maybe the bug is in libxl memory allocation, that doesn't account
for
> > > > rom sizes.
> > > Yes.
> > Here is a better patch.
> > 
> > ---
> > 
> > libxl: account for romfile memory
> > 
> > Account for memory needed for emulated network card rom files.
> > Assume 256K for each romfile.
> > 
> > Signed-off-by: Stefano Stabellini 
> 
> This looks to be a good idea, just not fully done.
> 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index de23fec..2edb194 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -4527,6 +4527,33 @@ out:
> >    
/******************************************************************************/
> >   +int libxl__get_rom_memory_kb(libxl__gc *gc, uint32_t domid,
> > libxl_domain_config *d_config)
> > +{
> > +    int i, count_rom, 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;
> > +    }
> > +
> > +    if (d_config->c_info.type == LIBXL_DOMAIN_TYPE_PV)
> > +        return 0;
> > +
> > +    for (i = 0, count_rom = 0; i < d_config->num_nics; i++) {
> > +        if (d_config->nics[i].nictype == LIBXL_NIC_TYPE_VIF_IOEMU)
> > +            count_rom++;
> > +    }
> > +
> > +    return count_rom*LIBXL_ROMSIZE_KB;
> > +
> > +    return 0;
> 
> 2 return statements?
> 
> > +}
> > +
> >   int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t
> > max_memkb)
> >   {
> >       GC_INIT(ctx);
> > @@ -4550,11 +4577,14 @@ int libxl_domain_setmaxmem(libxl_ctx *ctx,
uint32_t
> > domid, uint32_t max_memkb)
> >           LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "memory_static_max
must be
> > greater than or or equal to memory_dynamic_max\n");
> >           goto out;
> >       }
> > -    rc = xc_domain_setmaxmem(ctx->xch, domid, max_memkb +
> > LIBXL_MAXMEM_CONSTANT);
> > +    rc = xc_domain_setmaxmem(ctx->xch, domid,
> > +                             max_memkb + LIBXL_MAXMEM_CONSTANT
> > +                             + libxl__get_rom_memory_kb(gc, domid,
NULL));
> 
> Here (and the rest) need to check for error's.

Good suggestion.


> Also I think that LIBXL_MAXMEM_CONSTANT can be dropped here.

Probably, but I don't know whether there are any other "hidden"
allocations that would begin to fail if I removed LIBXL_MAXMEM_CONSTANT.
I think it is best to leave it as is for the moment and maybe remove it
at the beginning of the next release cycle?


> I found out that
> 
> #define LIBXL_HVM_EXTRA_MEMORY 2048
> 
> Should be
> 
> #define LIBXL_HVM_EXTRA_MEMORY (LIBXL_MAXMEM_CONSTANT + 1024)

OK


> if you change the size of the ROM memory for QEMU.  I have only done the
> static
> change.

I don't understand what you mean here. Yes, if the ROM size changes in
QEMU we'll have an issue, specifically if it increases. However I don't
know how to solve that. I guess it would be another reason to keep
LIBXL_MAXMEM_CONSTANT: have an extra little buffer.


>    -Don Slutz
> 
> 
> >       if (rc != 0) {
> >           LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> >                   "xc_domain_setmaxmem domid=%d memkb=%d failed "
> > -                "rc=%d\n", domid, max_memkb + LIBXL_MAXMEM_CONSTANT,
rc);
> > +                "rc=%d\n", domid, max_memkb + LIBXL_MAXMEM_CONSTANT +
> > +                libxl__get_rom_memory_kb(gc, domid, NULL), rc);
> >           goto out;
> >       }
> >   @@ -4770,11 +4800,12 @@ retry_transaction:
> >       if (enforce) {
> >           memorykb = new_target_memkb;
> >           rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
> > -                LIBXL_MAXMEM_CONSTANT);
> > +                LIBXL_MAXMEM_CONSTANT + libxl__get_rom_memory_kb(gc,
domid,
> > NULL));
> >           if (rc != 0) {
> >               LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> >                       "xc_domain_setmaxmem domid=%d memkb=%d failed "
> > -                    "rc=%d\n", domid, memorykb +
LIBXL_MAXMEM_CONSTANT,
> > rc);
> > +                    "rc=%d\n", domid, memorykb + LIBXL_MAXMEM_CONSTANT
+
> > +                    libxl__get_rom_memory_kb(gc, domid, NULL), rc);
> >               abort_transaction = 1;
> >               goto out;
> >           }
> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index 74ea84b..ca254f9 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -406,7 +406,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> >       }
> >         if (xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb +
> > -        LIBXL_MAXMEM_CONSTANT) < 0) {
> > +        LIBXL_MAXMEM_CONSTANT + libxl__get_rom_memory_kb(gc, domid,
> > d_config)) < 0) {
> >           LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Couldn't set max
> > memory");
> >           return ERROR_FAIL;
> >       }
> > diff --git a/tools/libxl/libxl_internal.h
b/tools/libxl/libxl_internal.h
> > index 4361421..33826ea 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -90,6 +90,7 @@
> >   #define LIBXL_XENCONSOLE_LIMIT 1048576
> >   #define LIBXL_XENCONSOLE_PROTOCOL "vt100"
> >   #define LIBXL_MAXMEM_CONSTANT 1024
> > +#define LIBXL_ROMSIZE_KB 256
> >   #define LIBXL_PV_EXTRA_MEMORY 1024
> >   #define LIBXL_HVM_EXTRA_MEMORY 2048
> >   #define LIBXL_MIN_DOM0_MEM (128*1024)
> > @@ -1023,6 +1024,12 @@ _hidden char *
libxl__domain_pvcontrol_read(libxl__gc
> > *gc,
> >   _hidden int libxl__domain_pvcontrol_write(libxl__gc *gc,
xs_transaction_t
> > t,
> >                                             uint32_t domid, const char
> > *cmd);
> >   +/* Returns the amount of extra mem required to allocate roms or an
libxl
> > + * error code on error.
> > + * The *d_config parameter is optional.
> > + */
> > +_hidden int libxl__get_rom_memory_kb(libxl__gc *gc, uint32_t domid,
> > libxl_domain_config *d_config);
> > +
> >   /* from xl_device */
> >   _hidden char *libxl__device_disk_string_of_backend(libxl_disk_backend
> > backend);
> >   _hidden char *libxl__device_disk_string_of_format(libxl_disk_format
> > format);
>
 
CD: 131ms