[PATCH for-4.5] libxl_set_memory_target: only remove videoram from absolute targets

[PATCH for-4.5] 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>

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index de23fec..2aa83bd 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4741,13 +4741,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"
@@ -4763,9 +4767,6 @@ 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;
@@ -4780,7 +4781,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) {

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

[Konrad Rzeszutek Wilk]

On Wed, Dec 03, 2014 at 06:20:41PM +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.

Please explain:
 - Is this an regression?
 - How often does it occur?
 - Is it fatal?
 - Are there work-arounds?

Thanks!
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index de23fec..2aa83bd 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4741,13 +4741,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"
> @@ -4763,9 +4767,6 @@ 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;
> @@ -4780,7 +4781,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) {

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

[Don Slutz]

On 12/03/14 13:20, 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>
>
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index de23fec..2aa83bd 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4741,13 +4741,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"
> @@ -4763,9 +4767,6 @@ 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;

Since new_target_memkb is now adjusted before this line, you
need to change this to:

          memorykb = new_target_memkb + videoram;

    -Don Slutz

> @@ -4780,7 +4781,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) {

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

[Stefano Stabellini]

On Wed, 3 Dec 2014, Konrad Rzeszutek Wilk wrote:
> On Wed, Dec 03, 2014 at 06:20:41PM +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.
> 
> Please explain:
>  - Is this an regression?

No, it is not.


>  - How often does it occur?

Every time the user creates a domain and memory needs to be freed in
dom0 for the allocation.


>  - Is it fatal?

No, it causes the new target for dom0 to be lower than it actually
needs to be by videram amount. However I realize now that the videoram
for dom0 is 0 so the bug wouldn't cause any troubles at the moment.

I guess it might be best to postpone this fix to 4.6.


>  - Are there work-arounds?
> 
> Thanks!
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index de23fec..2aa83bd 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -4741,13 +4741,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"
> > @@ -4763,9 +4767,6 @@ 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;
> > @@ -4780,7 +4781,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) {
> 

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

[Stefano Stabellini]

On Wed, 3 Dec 2014, Don Slutz wrote:
> On 12/03/14 13:20, 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>
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index de23fec..2aa83bd 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -4741,13 +4741,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"
> > @@ -4763,9 +4767,6 @@ 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;
> 
> Since new_target_memkb is now adjusted before this line, you
> need to change this to:
> 
>          memorykb = new_target_memkb + videoram;

You are right, I'll make the change.