[PATCH V2 0/1] libxl: set stub domain size based on VRAM size

[PATCH V2 0/1] libxl: set stub domain size based on VRAM size

[Eric Shelton]

I ran into crashes with qemu-traditional stub domain when 16 MB was
assigned to the stdvga virtual video adapter.  These were occurring due
to an out of memory condition arising from stub domains having a fixed
size of 32 MB, with half of that being demanded for video.  Assuming the
the original value of 32 MB was based on having a 4 MB video adapter,
this increases the stub domain size by however much the VRAM exceeds 4 MB. 

Revised per Ian Jackson's suggestion.

Eric Shelton (1):
  libxl: set stub domain size based on VRAM size

 tools/libxl/libxl_dm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.1.0

[PATCH V2 1/1] libxl: set stub domain size based on VRAM size

[Eric Shelton]

Allocate additional memory to the stub domain for qemu-traditional if
more than 4 MB is assigned to the video adapter to avoid out of memory
condition for QEMU.

Signed-off-by: Eric Shelton <eshelton@pobox.com>
---
 tools/libxl/libxl_dm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 317a8eb..9a5a937 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1087,6 +1087,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
 
     dm_config->b_info.max_vcpus = 1;
-    dm_config->b_info.max_memkb = 32 * 1024;
+    dm_config->b_info.max_memkb = 28 * 1024 +
+        max(guest_config->b_info.video_memkb, 4096);
     dm_config->b_info.target_memkb = dm_config->b_info.max_memkb;
 
     dm_config->b_info.u.pv.features = "";
-- 
2.1.0

Re: [PATCH V2 1/1] libxl: set stub domain size based on VRAM size

[Samuel Thibault]

Eric Shelton, le Fri 10 Jul 2015 18:14:32 -0400, a écrit :
> -    dm_config->b_info.max_memkb = 32 * 1024;
> +    dm_config->b_info.max_memkb = 28 * 1024 +
> +        max(guest_config->b_info.video_memkb, 4096);

I'm actually wondering whether just using

> +    dm_config->b_info.max_memkb = 28 * 1024 +
> +        guest_config->b_info.video_memkb;

and I don't see why not.

Samuel

Re: [PATCH V2 1/1] libxl: set stub domain size based on VRAM size

[Ian Jackson]

Eric Shelton writes ("[PATCH V2 1/1] libxl: set stub domain size based on VRAM size"):
> Allocate additional memory to the stub domain for qemu-traditional if
> more than 4 MB is assigned to the video adapter to avoid out of memory
> condition for QEMU.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

This is IMO a bugfix so I am queueing it for 4.6.

Ian.

Re: [PATCH V2 1/1] libxl: set stub domain size based on VRAM size

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH V2 1/1] libxl: set stub domain size based on VRAM size"):
> Eric Shelton writes ("[PATCH V2 1/1] libxl: set stub domain size based on VRAM size"):
> > Allocate additional memory to the stub domain for qemu-traditional if
> > more than 4 MB is assigned to the video adapter to avoid out of memory
> > condition for QEMU.
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> This is IMO a bugfix so I am queueing it for 4.6.

My build test failed.  It turns out that max() is no good because the
types of `4096' and `guest_config->b_info.video_memkb' are not the
same.

In a moment I am going to send a v3 which uses max_t and uint64_t
(which is the type of the memkb fields and also obviously correct).

Ian.

[PATCH for-4.6 v3] libxl: set stub domain size based on VRAM size

[Ian Jackson]

From: Eric Shelton <eshelton@pobox.com>

Allocate additional memory to the stub domain for qemu-traditional if
more than 4 MB is assigned to the video adapter to avoid out of memory
condition for QEMU.

Signed-off-by: Eric Shelton <eshelton@pobox.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
v3: Use max_t() instead
v2: Use max()
---
 tools/libxl/libxl_dm.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index ad434f0..f700f9a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -1095,7 +1095,8 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
     libxl_domain_build_info_init_type(&dm_config->b_info, LIBXL_DOMAIN_TYPE_PV);
 
     dm_config->b_info.max_vcpus = 1;
-    dm_config->b_info.max_memkb = 32 * 1024;
+    dm_config->b_info.max_memkb = 28 * 1024 +
+        max_t(uint64_t, guest_config->b_info.video_memkb, 4096);
     dm_config->b_info.target_memkb = dm_config->b_info.max_memkb;
 
     dm_config->b_info.u.pv.features = "";
-- 
1.7.10.4

Re: [PATCH V2 1/1] libxl: set stub domain size based on VRAM size

[Ian Campbell]

On Mon, 2015-07-13 at 11:22 +0100, Ian Jackson wrote:
> Ian Jackson writes ("Re: [PATCH V2 1/1] libxl: set stub domain size based on VRAM size"):
> > Eric Shelton writes ("[PATCH V2 1/1] libxl: set stub domain size based on VRAM size"):
> > > Allocate additional memory to the stub domain for qemu-traditional if
> > > more than 4 MB is assigned to the video adapter to avoid out of memory
> > > condition for QEMU.
> > 
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> > This is IMO a bugfix so I am queueing it for 4.6.
> 
> My build test failed.  It turns out that max() is no good because the
> types of `4096' and `guest_config->b_info.video_memkb' are not the
> same.
> 
> In a moment I am going to send a v3 which uses max_t and uint64_t
> (which is the type of the memkb fields and also obviously correct).

Eric already sent a v3 in
<1436650242-1067-2-git-send-email-eshelton@pobox.com> which avoids the
use of max in a different way.

I think his approach looked fine.

Ian.

Re: [PATCH V2 1/1] libxl: set stub domain size based on VRAM size

[Ian Jackson]

Ian Campbell writes ("Re: [PATCH V2 1/1] libxl: set stub domain size based on VRAM size"):
> On Mon, 2015-07-13 at 11:22 +0100, Ian Jackson wrote:
> > In a moment I am going to send a v3 which uses max_t and uint64_t
> > (which is the type of the memkb fields and also obviously correct).
> 
> Eric already sent a v3 in
> <1436650242-1067-2-git-send-email-eshelton@pobox.com> which avoids the
> use of max in a different way.
> 
> I think his approach looked fine.

Well, it does also reduce the stubdom memory when the video memory is
<4Mby.  That sounds plausible to me but at the very least the commit
message needs changing, and maybe we should wait a bit to see if one
of the qemu experts objects.

Ian.