* [PATCH v2 for-4.6] libxl_set_memory_target: retain the same maxmem offset on top of the current target @ 2014-12-02 11:53 Stefano Stabellini 2014-12-02 13:17 ` Don Slutz 0 siblings, 1 reply; 9+ messages in thread From: Stefano Stabellini @ 2014-12-02 11:53 UTC (permalink / raw) To: xen-devel Cc: Ian Jackson, Wei Liu (Intern), Ian Campbell, dslutz, Stefano Stabellini 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@eu.citrix.com> --- 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; + rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb); if (rc != 0) { LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "xc_domain_setmaxmem domid=%d memkb=%d failed " @@ -4800,8 +4802,6 @@ retry_transaction: goto out; } - libxl_dominfo_init(&ptr); - xcinfo2xlinfo(ctx, &info, &ptr); uuid = libxl__uuid2string(gc, ptr.uuid); libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid), "%"PRIu32, new_target_memkb / 1024); ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 for-4.6] libxl_set_memory_target: retain the same maxmem offset on top of the current target 2014-12-02 11:53 [PATCH v2 for-4.6] libxl_set_memory_target: retain the same maxmem offset on top of the current target Stefano Stabellini @ 2014-12-02 13:17 ` Don Slutz 2014-12-02 14:26 ` Stefano Stabellini 0 siblings, 1 reply; 9+ messages in thread From: Don Slutz @ 2014-12-02 13:17 UTC (permalink / raw) To: Stefano Stabellini Cc: Ian Jackson, xen-devel, Wei Liu (Intern), Ian Campbell, dslutz 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@eu.citrix.com> > > --- > > 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; > + rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb); > if (rc != 0) { > LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, > "xc_domain_setmaxmem domid=%d memkb=%d failed " You need to remove LIBXL_MAXMEM_CONSTANT here also. -Don Slutz > @@ -4800,8 +4802,6 @@ retry_transaction: > goto out; > } > > - libxl_dominfo_init(&ptr); > - xcinfo2xlinfo(ctx, &info, &ptr); > uuid = libxl__uuid2string(gc, ptr.uuid); > libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid), > "%"PRIu32, new_target_memkb / 1024); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 for-4.6] libxl_set_memory_target: retain the same maxmem offset on top of the current target 2014-12-02 13:17 ` Don Slutz @ 2014-12-02 14:26 ` Stefano Stabellini 2014-12-02 14:59 ` Don Slutz 0 siblings, 1 reply; 9+ messages in thread From: Stefano Stabellini @ 2014-12-02 14:26 UTC (permalink / raw) To: Don Slutz Cc: Ian Jackson, xen-devel, Wei Liu (Intern), Ian Campbell, Stefano Stabellini 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@eu.citrix.com> > > > > --- > > > > 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; > > + rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb); > > if (rc != 0) { > > LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, > > "xc_domain_setmaxmem domid=%d memkb=%d failed " > > You need to remove LIBXL_MAXMEM_CONSTANT here also. I don't think so: LIBXL_MAXMEM_CONSTANT is supposed to be a safety buffer and we should keep it as is across libxl_set_memory_target calls. Arguably LIBXL_MAXMEM_CONSTANT could be removed entirely with the proposed QEMU changes but that is a separate matter. > > @@ -4800,8 +4802,6 @@ retry_transaction: > > goto out; > > } > > - libxl_dominfo_init(&ptr); > > - xcinfo2xlinfo(ctx, &info, &ptr); > > uuid = libxl__uuid2string(gc, ptr.uuid); > > libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid), > > "%"PRIu32, new_target_memkb / 1024); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 for-4.6] libxl_set_memory_target: retain the same maxmem offset on top of the current target 2014-12-02 14:26 ` Stefano Stabellini @ 2014-12-02 14:59 ` Don Slutz 2014-12-02 15:04 ` Stefano Stabellini 2014-12-02 15:27 ` Don Slutz 0 siblings, 2 replies; 9+ messages in thread From: Don Slutz @ 2014-12-02 14:59 UTC (permalink / raw) To: Stefano Stabellini, Don Slutz Cc: Ian Jackson, xen-devel, Wei Liu (Intern), Ian Campbell 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@eu.citrix.com> >>> >>> --- >>> >>> 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; >>> + rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb); >>> if (rc != 0) { >>> LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, >>> "xc_domain_setmaxmem domid=%d memkb=%d failed " >> You need to remove LIBXL_MAXMEM_CONSTANT here also. > I don't think so: LIBXL_MAXMEM_CONSTANT is supposed to be a safety > buffer and we should keep it as is across libxl_set_memory_target calls. > Arguably LIBXL_MAXMEM_CONSTANT could be removed entirely with the > proposed QEMU changes but that is a separate matter. > I was talking about the line: "rc=%d\n", domid, memorykb + LIBXL_MAXMEM_CONSTANT, rc); (which is missing from the diff but is part of the LIBXL__LOG_ERRNO call). The error message no longer matches what xc_domain_setmaxmem() was called with. -Don Slutz > >>> @@ -4800,8 +4802,6 @@ retry_transaction: >>> goto out; >>> } >>> - libxl_dominfo_init(&ptr); >>> - xcinfo2xlinfo(ctx, &info, &ptr); >>> uuid = libxl__uuid2string(gc, ptr.uuid); >>> libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid), >>> "%"PRIu32, new_target_memkb / 1024); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 for-4.6] libxl_set_memory_target: retain the same maxmem offset on top of the current target 2014-12-02 14:59 ` Don Slutz @ 2014-12-02 15:04 ` Stefano Stabellini 2014-12-02 15:27 ` Don Slutz 1 sibling, 0 replies; 9+ messages in thread From: Stefano Stabellini @ 2014-12-02 15:04 UTC (permalink / raw) To: Don Slutz Cc: Ian Jackson, xen-devel, Wei Liu (Intern), Ian Campbell, Stefano Stabellini On Tue, 2 Dec 2014, 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@eu.citrix.com> > > > > > > > > --- > > > > > > > > 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; > > > > + rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb); > > > > if (rc != 0) { > > > > LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, > > > > "xc_domain_setmaxmem domid=%d memkb=%d failed " > > > You need to remove LIBXL_MAXMEM_CONSTANT here also. > > I don't think so: LIBXL_MAXMEM_CONSTANT is supposed to be a safety > > buffer and we should keep it as is across libxl_set_memory_target calls. > > Arguably LIBXL_MAXMEM_CONSTANT could be removed entirely with the > > proposed QEMU changes but that is a separate matter. > > > > I was talking about the line: > > "rc=%d\n", domid, memorykb + LIBXL_MAXMEM_CONSTANT, rc); > > (which is missing from the diff but is part of the LIBXL__LOG_ERRNO call). > The > error message no longer matches what xc_domain_setmaxmem() was called > with. Yep, you are right, I'll fix. > > > > > > @@ -4800,8 +4802,6 @@ retry_transaction: > > > > goto out; > > > > } > > > > - libxl_dominfo_init(&ptr); > > > > - xcinfo2xlinfo(ctx, &info, &ptr); > > > > uuid = libxl__uuid2string(gc, ptr.uuid); > > > > libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid), > > > > "%"PRIu32, new_target_memkb / 1024); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 for-4.6] libxl_set_memory_target: retain the same maxmem offset on top of the current target 2014-12-02 14:59 ` Don Slutz 2014-12-02 15:04 ` Stefano Stabellini @ 2014-12-02 15:27 ` Don Slutz 2014-12-03 17:31 ` Stefano Stabellini 1 sibling, 1 reply; 9+ messages in thread From: Don Slutz @ 2014-12-02 15:27 UTC (permalink / raw) To: Don Slutz, Stefano Stabellini Cc: Ian Jackson, xen-devel, Wei Liu (Intern), Ian Campbell 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@eu.citrix.com> >>>> >>>> --- >>>> >>>> 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; -Don Slutz >>>> + rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb); >>>> if (rc != 0) { >>>> LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, >>>> "xc_domain_setmaxmem domid=%d memkb=%d failed " >>> You need to remove LIBXL_MAXMEM_CONSTANT here also. >> I don't think so: LIBXL_MAXMEM_CONSTANT is supposed to be a safety >> buffer and we should keep it as is across libxl_set_memory_target calls. >> Arguably LIBXL_MAXMEM_CONSTANT could be removed entirely with the >> proposed QEMU changes but that is a separate matter. >> > > I was talking about the line: > > "rc=%d\n", domid, memorykb + > LIBXL_MAXMEM_CONSTANT, rc); > > (which is missing from the diff but is part of the LIBXL__LOG_ERRNO > call). The > error message no longer matches what xc_domain_setmaxmem() was called > with. > > -Don Slutz > >>>> @@ -4800,8 +4802,6 @@ retry_transaction: >>>> goto out; >>>> } >>>> - libxl_dominfo_init(&ptr); >>>> - xcinfo2xlinfo(ctx, &info, &ptr); >>>> uuid = libxl__uuid2string(gc, ptr.uuid); >>>> libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", >>>> uuid), >>>> "%"PRIu32, new_target_memkb / 1024); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 for-4.6] libxl_set_memory_target: retain the same maxmem offset on top of the current target 2014-12-02 15:27 ` Don Slutz @ 2014-12-03 17:31 ` Stefano Stabellini 2014-12-03 18:07 ` Don Slutz 0 siblings, 1 reply; 9+ messages in thread From: Stefano Stabellini @ 2014-12-03 17:31 UTC (permalink / raw) To: Don Slutz Cc: Ian Jackson, xen-devel, Wei Liu (Intern), Ian Campbell, Stefano Stabellini 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@eu.citrix.com> > > > > > > > > > > --- > > > > > > > > > > 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. 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) { ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 for-4.6] libxl_set_memory_target: retain the same maxmem offset on top of the current target 2014-12-03 17:31 ` Stefano Stabellini @ 2014-12-03 18:07 ` Don Slutz 2014-12-03 18:18 ` Stefano Stabellini 0 siblings, 1 reply; 9+ messages in thread From: Don Slutz @ 2014-12-03 18:07 UTC (permalink / raw) To: Stefano Stabellini, Don Slutz Cc: Ian Jackson, xen-devel, Wei Liu (Intern), Ian Campbell 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@eu.citrix.com> >>>>>> >>>>>> --- >>>>>> >>>>>> 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. -Don Slutz ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 for-4.6] libxl_set_memory_target: retain the same maxmem offset on top of the current target 2014-12-03 18:07 ` Don Slutz @ 2014-12-03 18:18 ` Stefano Stabellini 0 siblings, 0 replies; 9+ messages in thread From: Stefano Stabellini @ 2014-12-03 18:18 UTC (permalink / raw) To: Don Slutz Cc: Ian Jackson, xen-devel, Wei Liu (Intern), Ian Campbell, Stefano Stabellini 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@eu.citrix.com> > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-12-03 18:18 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-12-02 11:53 [PATCH v2 for-4.6] libxl_set_memory_target: retain the same maxmem offset on top of the current target Stefano Stabellini 2014-12-02 13:17 ` Don Slutz 2014-12-02 14:26 ` Stefano Stabellini 2014-12-02 14:59 ` Don Slutz 2014-12-02 15:04 ` Stefano Stabellini 2014-12-02 15:27 ` Don Slutz 2014-12-03 17:31 ` Stefano Stabellini 2014-12-03 18:07 ` Don Slutz 2014-12-03 18:18 ` Stefano Stabellini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.