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: Tue, 02 Dec 2014 10:27:24 -0500 Message-ID: <547DDA5C.8080803@terremark.com> References: <547DBC05.9070904@terremark.com> <547DD3E5.9090303@terremark.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <547DD3E5.9090303@terremark.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Don Slutz , Stefano Stabellini Cc: Ian Jackson , xen-devel@lists.xensource.com, "Wei Liu (Intern)" , Ian Campbell List-Id: xen-devel@lists.xenproject.org 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; -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); >