[PATCH] libxl_set_memory_target: only remove videoram from absolute targets

[PATCH] libxl_set_memory_target: only remove videoram from absolute targets

[Stefano Stabellini]

If the new target is relative to the current target, do not remove
videoram again: it has already been removed from the current target.

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

---

Changes in v2:
- add videoram when setting maxmem.
---
 tools/libxl/libxl.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 82227e8..cd6f42c 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4764,13 +4764,17 @@ retry_transaction:
         goto out;
     }
 
+    videoram_s = libxl__xs_read(gc, t, libxl__sprintf(gc,
+                "%s/memory/videoram", dompath));
+    videoram = videoram_s ? atoi(videoram_s) : 0;
+
     if (relative) {
         if (target_memkb < 0 && abs(target_memkb) > current_target_memkb)
             new_target_memkb = 0;
         else
             new_target_memkb = current_target_memkb + target_memkb;
     } else
-        new_target_memkb = target_memkb;
+        new_target_memkb = target_memkb - videoram;
     if (new_target_memkb > memorykb) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
                 "memory_dynamic_max must be less than or equal to"
@@ -4786,12 +4790,9 @@ retry_transaction:
         abort_transaction = 1;
         goto out;
     }
-    videoram_s = libxl__xs_read(gc, t, libxl__sprintf(gc,
-                "%s/memory/videoram", dompath));
-    videoram = videoram_s ? atoi(videoram_s) : 0;
 
     if (enforce) {
-        memorykb = new_target_memkb;
+        memorykb = new_target_memkb + videoram;
         rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
                 LIBXL_MAXMEM_CONSTANT);
         if (rc != 0) {
@@ -4803,7 +4804,6 @@ retry_transaction:
         }
     }
 
-    new_target_memkb -= videoram;
     rc = xc_domain_set_pod_target(ctx->xch, domid,
             new_target_memkb / 4, NULL, NULL, NULL);
     if (rc != 0) {
-- 
1.7.10.4

Re: [PATCH] libxl_set_memory_target: only remove videoram from absolute targets

[Ian Campbell]

On Mon, 2015-01-26 at 16:47 +0000, Stefano Stabellini wrote:
> If the new target is relative to the current target, do not remove
> videoram again: it has already been removed from the current target.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

How does this relate to "libxl_set_memory_target: retain the same maxmem
offset on top of the current target"? Doesn't that also achieve the same
aim, since maxmem accounts for vram but target doesn't?

Ian.

Re: [PATCH] libxl_set_memory_target: only remove videoram from absolute targets

[Stefano Stabellini]

On Wed, 28 Jan 2015, Ian Campbell wrote:
> On Mon, 2015-01-26 at 16:47 +0000, Stefano Stabellini wrote:
> > If the new target is relative to the current target, do not remove
> > videoram again: it has already been removed from the current target.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> How does this relate to "libxl_set_memory_target: retain the same maxmem
> offset on top of the current target"? Doesn't that also achieve the same
> aim, since maxmem accounts for vram but target doesn't?

The two patches are different: this one affects the memory target while
the other one affects maxmem. The only relationship is that this issue
was found out by looking at the other patch.

This patch didn't make it into 4.5 because although it is a bug fix, it
doesn't fix a regression compared to 4.4.

Re: [PATCH] libxl_set_memory_target: only remove videoram from absolute targets

[Ian Campbell]

On Wed, 2015-01-28 at 14:24 +0000, Stefano Stabellini wrote:
> On Wed, 28 Jan 2015, Ian Campbell wrote:
> > On Mon, 2015-01-26 at 16:47 +0000, Stefano Stabellini wrote:
> > > If the new target is relative to the current target, do not remove
> > > videoram again: it has already been removed from the current target.
> > > 
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > How does this relate to "libxl_set_memory_target: retain the same maxmem
> > offset on top of the current target"? Doesn't that also achieve the same
> > aim, since maxmem accounts for vram but target doesn't?
> 
> The two patches are different: this one affects the memory target while
> the other one affects maxmem. The only relationship is that this issue
> was found out by looking at the other patch.

Not even a textual relationship? (i.e. one needs to go first to avoid
conflicts)

> This patch didn't make it into 4.5 because although it is a bug fix, it
> doesn't fix a regression compared to 4.4.

Re: [PATCH] libxl_set_memory_target: only remove videoram from absolute targets

[Stefano Stabellini]

On Wed, 28 Jan 2015, Ian Campbell wrote:
> On Wed, 2015-01-28 at 14:24 +0000, Stefano Stabellini wrote:
> > On Wed, 28 Jan 2015, Ian Campbell wrote:
> > > On Mon, 2015-01-26 at 16:47 +0000, Stefano Stabellini wrote:
> > > > If the new target is relative to the current target, do not remove
> > > > videoram again: it has already been removed from the current target.
> > > > 
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > 
> > > How does this relate to "libxl_set_memory_target: retain the same maxmem
> > > offset on top of the current target"? Doesn't that also achieve the same
> > > aim, since maxmem accounts for vram but target doesn't?
> > 
> > The two patches are different: this one affects the memory target while
> > the other one affects maxmem. The only relationship is that this issue
> > was found out by looking at the other patch.
> 
> Not even a textual relationship? (i.e. one needs to go first to avoid
> conflicts)

Yep, there is a textual relationship. Sorry I failed to mention it.

This one needs to go first.


> > This patch didn't make it into 4.5 because although it is a bug fix, it
> > doesn't fix a regression compared to 4.4.
> 
> 

Re: [PATCH] libxl_set_memory_target: only remove videoram from absolute targets

[Ian Campbell]

On Wed, 2015-01-28 at 14:47 +0000, Stefano Stabellini wrote:
> On Wed, 28 Jan 2015, Ian Campbell wrote:
> > On Wed, 2015-01-28 at 14:24 +0000, Stefano Stabellini wrote:
> > > On Wed, 28 Jan 2015, Ian Campbell wrote:
> > > > On Mon, 2015-01-26 at 16:47 +0000, Stefano Stabellini wrote:
> > > > > If the new target is relative to the current target, do not remove
> > > > > videoram again: it has already been removed from the current target.
> > > > > 
> > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > > > 
> > > > How does this relate to "libxl_set_memory_target: retain the same maxmem
> > > > offset on top of the current target"? Doesn't that also achieve the same
> > > > aim, since maxmem accounts for vram but target doesn't?
> > > 
> > > The two patches are different: this one affects the memory target while
> > > the other one affects maxmem. The only relationship is that this issue
> > > was found out by looking at the other patch.
> > 
> > Not even a textual relationship? (i.e. one needs to go first to avoid
> > conflicts)
> 
> Yep, there is a textual relationship. Sorry I failed to mention it.
> 
> This one needs to go first.

Acked + applied then, thanks.