diff mbox

[Xen-devel,for-4.5] libxl: do not load roms for any NICs except the first to avoid wasting memory

Message ID alpine.DEB.2.02.1411211706080.2675@kaball.uk.xensource.com
State New
Headers show

Commit Message

Stefano Stabellini Nov. 21, 2014, 5:11 p.m. UTC
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
memory and as a matter of fact breaks configurations with more than 4
NICs as QEMU fails to allocate memory on behalf of the guest.

With this fix, it is possible to assign more than 4 rtl8139 NICs to the
guest.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

Comments

Konrad Rzeszutek Wilk Nov. 21, 2014, 5:34 p.m. UTC | #1
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?

> memory and as a matter of fact breaks configurations with more than 4
> NICs as QEMU fails to allocate memory on behalf of the guest.

What if you have four different type of NICs? Say 1 rlt8193, 1 e1000, one eepro,
and ne2k ?

Don't you want to load the ROM for each one?
> 
> With this fix, it is possible to assign more than 4 rtl8139 NICs to the
> guest.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 3e191c3..f907ca9 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -674,9 +674,10 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
>                                                  LIBXL_NIC_TYPE_VIF_IOEMU);
>                  flexarray_append(dm_args, "-device");
>                  flexarray_append(dm_args,
> -                   libxl__sprintf(gc, "%s,id=nic%d,netdev=net%d,mac=%s",
> +                   libxl__sprintf(gc, "%s,id=nic%d,netdev=net%d,mac=%s%s",
>                                                  nics[i].model, nics[i].devid,
> -                                                nics[i].devid, smac));
> +                                                nics[i].devid, smac,
> +                                                i ? ",romfile=\"\"" : ""));
>                  flexarray_append(dm_args, "-netdev");
>                  flexarray_append(dm_args, GCSPRINTF(
>                                            "type=tap,id=net%d,ifname=%s,"
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
Stefano Stabellini Nov. 21, 2014, 6:48 p.m. UTC | #2
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).

TBH not all the emulated nics need a romfile but I wouldn't want to go
down at that level of details beause they become QEMU implementation
details. I wouldn't want to tie libxl to a certain version of QEMU in
any way.

A better way would be handling memory allocation errors in QEMU but QEMU
doesn't do that at the moment and the change there would be certainly
more invasive that a couple of lines in libxl.


> > memory and as a matter of fact breaks configurations with more than 4
> > NICs as QEMU fails to allocate memory on behalf of the guest.
> 
> What if you have four different type of NICs? Say 1 rlt8193, 1 e1000, one eepro,
> and ne2k ?
> 
> Don't you want to load the ROM for each one?

The rom cannot be reused in QEMU among multiple instances of the same
nic, so having four different types of nics or just one type doesn't
change anything.


> > With this fix, it is possible to assign more than 4 rtl8139 NICs to the
> > guest.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > index 3e191c3..f907ca9 100644
> > --- a/tools/libxl/libxl_dm.c
> > +++ b/tools/libxl/libxl_dm.c
> > @@ -674,9 +674,10 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
> >                                                  LIBXL_NIC_TYPE_VIF_IOEMU);
> >                  flexarray_append(dm_args, "-device");
> >                  flexarray_append(dm_args,
> > -                   libxl__sprintf(gc, "%s,id=nic%d,netdev=net%d,mac=%s",
> > +                   libxl__sprintf(gc, "%s,id=nic%d,netdev=net%d,mac=%s%s",
> >                                                  nics[i].model, nics[i].devid,
> > -                                                nics[i].devid, smac));
> > +                                                nics[i].devid, smac,
> > +                                                i ? ",romfile=\"\"" : ""));
> >                  flexarray_append(dm_args, "-netdev");
> >                  flexarray_append(dm_args, GCSPRINTF(
> >                                            "type=tap,id=net%d,ifname=%s,"
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
>
Konrad Rzeszutek Wilk Nov. 21, 2014, 7:07 p.m. UTC | #3
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? What if you
also do PCI passthrough? Does that figure in this calculation?
> 
> TBH not all the emulated nics need a romfile but I wouldn't want to go
> down at that level of details beause they become QEMU implementation
> details. I wouldn't want to tie libxl to a certain version of QEMU in
> any way.
> 
> A better way would be handling memory allocation errors in QEMU but QEMU
> doesn't do that at the moment and the change there would be certainly
> more invasive that a couple of lines in libxl.
> 
> 
> > > memory and as a matter of fact breaks configurations with more than 4
> > > NICs as QEMU fails to allocate memory on behalf of the guest.
> > 
> > What if you have four different type of NICs? Say 1 rlt8193, 1 e1000, one eepro,
> > and ne2k ?
> > 
> > Don't you want to load the ROM for each one?
> 
> The rom cannot be reused in QEMU among multiple instances of the same
> nic, so having four different types of nics or just one type doesn't
> change anything.
> 
> 
> > > With this fix, it is possible to assign more than 4 rtl8139 NICs to the
> > > guest.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > 
> > > diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> > > index 3e191c3..f907ca9 100644
> > > --- a/tools/libxl/libxl_dm.c
> > > +++ b/tools/libxl/libxl_dm.c
> > > @@ -674,9 +674,10 @@ static char ** libxl__build_device_model_args_new(libxl__gc *gc,
> > >                                                  LIBXL_NIC_TYPE_VIF_IOEMU);
> > >                  flexarray_append(dm_args, "-device");
> > >                  flexarray_append(dm_args,
> > > -                   libxl__sprintf(gc, "%s,id=nic%d,netdev=net%d,mac=%s",
> > > +                   libxl__sprintf(gc, "%s,id=nic%d,netdev=net%d,mac=%s%s",
> > >                                                  nics[i].model, nics[i].devid,
> > > -                                                nics[i].devid, smac));
> > > +                                                nics[i].devid, smac,
> > > +                                                i ? ",romfile=\"\"" : ""));
> > >                  flexarray_append(dm_args, "-netdev");
> > >                  flexarray_append(dm_args, GCSPRINTF(
> > >                                            "type=tap,id=net%d,ifname=%s,"
> > > 
> > > _______________________________________________
> > > Xen-devel mailing list
> > > Xen-devel@lists.xen.org
> > > http://lists.xen.org/xen-devel
> >
diff mbox

Patch

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 3e191c3..f907ca9 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -674,9 +674,10 @@  static char ** libxl__build_device_model_args_new(libxl__gc *gc,
                                                 LIBXL_NIC_TYPE_VIF_IOEMU);
                 flexarray_append(dm_args, "-device");
                 flexarray_append(dm_args,
-                   libxl__sprintf(gc, "%s,id=nic%d,netdev=net%d,mac=%s",
+                   libxl__sprintf(gc, "%s,id=nic%d,netdev=net%d,mac=%s%s",
                                                 nics[i].model, nics[i].devid,
-                                                nics[i].devid, smac));
+                                                nics[i].devid, smac,
+                                                i ? ",romfile=\"\"" : ""));
                 flexarray_append(dm_args, "-netdev");
                 flexarray_append(dm_args, GCSPRINTF(
                                           "type=tap,id=net%d,ifname=%s,"