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 <stefano.stabell...@eu.citrix.com>
>
> 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);
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
