Re: [Xen-devel] [PATCH v2 for-4.6] libxl_set_memory_target: retain the same maxmem offset on top of the current target

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 3 Dec 2014, Don Slutz wrote:
> On 12/03/14 12:31, Stefano Stabellini wrote:
> > On Tue, 2 Dec 2014, Don Slutz wrote:
> > > On 12/02/14 09:59, Don Slutz wrote:
> > > > On 12/02/14 09:26, Stefano Stabellini wrote:
> > > > > On Tue, 2 Dec 2014, Don Slutz wrote:
> > > > > > On 12/02/14 06:53, Stefano Stabellini wrote:
> > > > > > > In libxl_set_memory_target when setting the new maxmem, retain the
> > > > > > > same
> > > > > > > offset on top of the current target. The offset includes memory
> > > > > > > allocated by QEMU for rom files.
> > > > > > > 
> > > > > > > Signed-off-by: Stefano
> > > > > > > Stabellini<stefano.stabellini@xxxxxxxxxxxxx>
> > > > > > > 
> > > > > > > ---
> > > > > > > 
> > > > > > > Changes in v2:
> > > > > > > - call libxl_domain_info instead of libxl_dominfo_init;
> > > > > > > - call libxl_domain_info before retry_transaction.
> > > > > > > 
> > > > > > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > > > > > index de23fec..569a32a 100644
> > > > > > > --- a/tools/libxl/libxl.c
> > > > > > > +++ b/tools/libxl/libxl.c
> > > > > > > @@ -4694,6 +4694,9 @@ int libxl_set_memory_target(libxl_ctx *ctx,
> > > > > > > uint32_t
> > > > > > > domid,
> > > > > > >         char *uuid;
> > > > > > >         xs_transaction_t t;
> > > > > > >     +    if (libxl_domain_info(ctx, &ptr, domid) < 0)
> > > > > > > +        goto out_no_transaction;
> > > > > > > +
> > > > > > >     retry_transaction:
> > > > > > >         t = xs_transaction_start(ctx->xsh);
> > > > > > >     @@ -4767,10 +4770,9 @@ retry_transaction:
> > > > > > >                     "%s/memory/videoram", dompath));
> > > > > > >         videoram = videoram_s ? atoi(videoram_s) : 0;
> > > > > > >     -    if (enforce) {
> > > > > > > -        memorykb = new_target_memkb;
> > > > > > > -        rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb +
> > > > > > > -                LIBXL_MAXMEM_CONSTANT);
> > > > > > > +    if (enforce && new_target_memkb > 0) {
> > > > > > > +        memorykb = ptr.max_memkb - current_target_memkb +
> > > > > > > new_target_memkb;
> > > My testing shows that this should be:
> > > 
> > >          memorykb = ptr.max_memkb - (current_target_memkb + videoram) +
> > >              new_target_memkb;
> > > 
> > > As far as I can tell the reason for this is that memory/target (aka
> > > current_target_memkb) was set based on:
> > > 
> > >      new_target_memkb -= videoram;
> > Thank you very much for testing and the suggestion!
> > 
> > I think that the right fix for this is to remove videoram from
> > new_target_memkb earlier and only when the new target is absolute,
> > otherwise we risk removing videoram twice (in case the new target is
> > relative). I wonder why we didn't notice this before.
> 
> Sounds like a good idea.  No clue, I have been looking real close at this
> stuff.
> and that my be why I tripped over it.
> 
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index d5d5204..4803cc4 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -4744,13 +4744,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"
> > @@ -4766,9 +4770,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 && new_target_memkb > 0) {
> >           memorykb = ptr.max_memkb - current_target_memkb +
> > new_target_memkb;
> > @@ -4782,7 +4783,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) {
> 
> This does look like a bugfix for just the videoram issue.  Not sure why
> you made this v2 since I do not see the original change.
> 
> Anyway if you want to post this change for 4.5 (?) I would be happy to review
> it.

Thanks, I'll do.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel