From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v2 for-4.6] libxl_set_memory_target: retain the same maxmem offset on top of the current target Date: Wed, 03 Dec 2014 13:07:33 -0500 Message-ID: <547F5165.4040804@terremark.com> References: <547DBC05.9070904@terremark.com> <547DD3E5.9090303@terremark.com> <547DDA5C.8080803@terremark.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini , Don Slutz Cc: Ian Jackson , xen-devel@lists.xensource.com, "Wei Liu (Intern)" , Ian Campbell List-Id: xen-devel@lists.xenproject.org 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 >>>>>> >>>>>> --- >>>>>> >>>>>> 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