[PATCH] xl: Update memory info in xenstore when use 'xl mem-set'

[PATCH] xl: Update memory info in xenstore when use 'xl mem-set'

[Yu Zhiguo]

'xl mem-set' should update memory info in xenstore's vm info.

Signed-off-by: Yu Zhiguo <yuzg@cn.fujitsu.com>

diff -r ccae861f52f7 -r 5847ce53f5dc tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Thu May 06 11:59:55 2010 +0100
+++ b/tools/libxl/libxl.c	Sat May 08 01:38:51 2010 +0800
@@ -2290,11 +2290,22 @@
     uint32_t videoram;
     char *videoram_s = NULL;
     char *dompath = libxl_xs_get_dompath(ctx, domid);
+    xc_domaininfo_t info;
+    struct libxl_dominfo ptr;
+    char *uuid;
 
     videoram_s = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/memory/videoram", dompath));
     videoram = videoram_s ? atoi(videoram_s) : 0;
 
     libxl_xs_write(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/memory/target", dompath), "%lu", target_memkb);
+
+    rc = xc_domain_getinfolist(ctx->xch, domid, 1, &info);
+    if (rc != 1 || info.domain != domid)
+        return rc;
+    xcinfo2xlinfo(&info, &ptr);
+    uuid = libxl_uuid2string(ctx, ptr.uuid);
+    libxl_xs_write(ctx, XBT_NULL, libxl_sprintf(ctx, "/vm/%s/memory", uuid), "%lu", target_memkb / 1024);
+
     rc = xc_domain_setmaxmem(ctx->xch, domid, target_memkb + LIBXL_MAXMEM_CONSTANT);
     if (rc != 0)
         return rc;

Re: [PATCH] xl: Update memory info in xenstore when use 'xl mem-set'

[Ian Jackson]

Yu Zhiguo writes ("[Xen-devel] [PATCH] xl: Update memory info in xenstore when use 'xl mem-set'"):
> 'xl mem-set' should update memory info in xenstore's vm info.

Your patch looks sane (although I haven't tested it - I guess you
have).  So:

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

However, looking at this, it's far from clear why
libxl_set_memory_target calls xc_domain_setmaxmem at all.  Can anyone
explain ?

Ian.

Re: [PATCH] xl: Update memory info in xenstore when use 'xl mem-set'

[Yu Zhiguo]

Hi Ian,

Ian Jackson wrote:
> 
> However, looking at this, it's far from clear why
> libxl_set_memory_target calls xc_domain_setmaxmem at all.  Can anyone
> explain ?

I'm trying to add subcommand 'mem-max', I think xc_domain_setmaxmem should
be used in it but not here.

I'll move this code to 'mem-max', and in 'mem-set', a check should be added
because setting memory larger than max memory is invalid.

1. Add 'mem-max'
  Add libxl_domain_setmaxmem, it calls xc_domain_setmaxmem.
  /local/domain/$domid/memory/static-max should be updated when set max memory,
it is missing now.

2. fix 'mem-set'
  Delete xc_domain_setmaxmem.
  Get max memory from /local/domain/$domid/memory/static-max, and then do value check.
It seems that we cannot get max memory use libxc routines.

Do you agree with me?



Add, I found another problem about mem-set.

I used 'xl mem-set' set the memory, but the size displayed in output of 'xm list'
did not change.
It seems that 'xm' get the memory size from xend, xend maintains some info of VM.
I'm not sure how to resolve this problem? Get info from xenstore?
What's your opinion?

# xl mem-set 3 204800
setting domid 3 memory to : 204800
# xl list
Name                                        ID   Mem VCPUs      State   Time(s)
...
pvguest1                                     3   200     2        r--      0.2
# xm list
Name                                        ID   Mem VCPUs      State   Time(s)
...
pvguest1                                     3   256     2     -b----      0.2



Regards
Yu Zhiguo

[PATCH 0/2] xl: Add subcommand mem-max and fix mem-set

[Yu Zhiguo]

Yu Zhiguo wrote:
> 
> I'm trying to add subcommand 'mem-max', I think xc_domain_setmaxmem should
> be used in it but not here.
> 
> I'll move this code to 'mem-max', and in 'mem-set', a check should be added
> because setting memory larger than max memory is invalid.
> 
> 1. Add 'mem-max'
>   Add libxl_domain_setmaxmem, it calls xc_domain_setmaxmem.
>   /local/domain/$domid/memory/static-max should be updated when set max memory,
> it is missing now.
> 
> 2. fix 'mem-set'
>   Delete xc_domain_setmaxmem.
>   Get max memory from /local/domain/$domid/memory/static-max, and then do value check.
> It seems that we cannot get max memory use libxc routines.
> 

I fixed it, please check.

[PATCH 1/2] xl: Add command 'xl mem-max'
[PATCH 2/2] xl: Add check for command 'xl mem-set'


Regards
Yu Zhiguo

Re: [PATCH 0/2] xl: Add subcommand mem-max and fix mem-set

[Yu Zhiguo]

Yu Zhiguo wrote:
> Yu Zhiguo wrote:
>> I'm trying to add subcommand 'mem-max', I think xc_domain_setmaxmem should
>> be used in it but not here.
>>
>> I'll move this code to 'mem-max', and in 'mem-set', a check should be added
>> because setting memory larger than max memory is invalid.
>>
>> 1. Add 'mem-max'
>>   Add libxl_domain_setmaxmem, it calls xc_domain_setmaxmem.
>>   /local/domain/$domid/memory/static-max should be updated when set max memory,
>> it is missing now.
>>
>> 2. fix 'mem-set'
>>   Delete xc_domain_setmaxmem.
>>   Get max memory from /local/domain/$domid/memory/static-max, and then do value check.
>> It seems that we cannot get max memory use libxc routines.
>>
> 
> I fixed it, please check.
> 
> [PATCH 1/2] xl: Add command 'xl mem-max'
> [PATCH 2/2] xl: Add check for command 'xl mem-set'
> 

It seems than /local/domain/$domid/memory/target cannot be created when use 'xl create',
only /local/domain/0/memory/static-max is existent.
Isn't it a bug? I'll check the code.

I think it should be created like static-max.
* memory/target can be created when use 'xm create'


> 
> Regards
> Yu Zhiguo
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 
> 
> 


-- 
Regards
Yu Zhiguo
--------------------------------------------------
Yu Zhiguo
Development Dept.I
Nanjing Fujitsu Nanda Software Tech. Co., Ltd.(FNST)
8/F., Civil Defense Building, No.189 Guangzhou Road,
Nanjing, 210029, China
TEL: +86+25-86630566-853
COINS: 79955-853
FAX: +86+25-83317685
MAIL: yuzg@cn.fujitsu.com
--------------------------------------------------
This communication is for use by the intended recipient(s) only and may contain information that is privileged, confidential and exempt from disclosure under applicable law. If you are not an intended recipient of this communication, you are hereby notified that any dissemination, distribution or copying hereof is strictly prohibited.  If you have received this communication in error, please notify me by reply e-mail, permanently delete this communication from your system, and destroy any hard copies you may have printed.

Re: [PATCH 0/2] xl: Add subcommand mem-max and fix mem-set

[Stefano Stabellini]

On Wed, 12 May 2010, Yu Zhiguo wrote:
> Yu Zhiguo wrote:
> > Yu Zhiguo wrote:
> >> I'm trying to add subcommand 'mem-max', I think xc_domain_setmaxmem should
> >> be used in it but not here.
> >>
> >> I'll move this code to 'mem-max', and in 'mem-set', a check should be added
> >> because setting memory larger than max memory is invalid.
> >>
> >> 1. Add 'mem-max'
> >>   Add libxl_domain_setmaxmem, it calls xc_domain_setmaxmem.
> >>   /local/domain/$domid/memory/static-max should be updated when set max memory,
> >> it is missing now.
> >>
> >> 2. fix 'mem-set'
> >>   Delete xc_domain_setmaxmem.
> >>   Get max memory from /local/domain/$domid/memory/static-max, and then do value check.
> >> It seems that we cannot get max memory use libxc routines.
> >>
> > 
> > I fixed it, please check.
> > 
> > [PATCH 1/2] xl: Add command 'xl mem-max'
> > [PATCH 2/2] xl: Add check for command 'xl mem-set'
> > 
> 
> It seems than /local/domain/$domid/memory/target cannot be created when use 'xl create',
> only /local/domain/0/memory/static-max is existent.
> Isn't it a bug? I'll check the code.
> 
> I think it should be created like static-max.
> * memory/target can be created when use 'xm create'
> 

Actually sorry if I didn't reply in time before you sent your patches,
but I asked the xapi memory management expert (CC'ed) an
opinion on the memset/memmax commands, I have the impression
that he has something interesting to say about them.

Keir, could you please wait before applying these patches?

Re: [PATCH 0/2] xl: Add subcommand mem-max and fix mem-set

[Stefano Stabellini]

On Wed, 12 May 2010, Stefano Stabellini wrote:
> On Wed, 12 May 2010, Yu Zhiguo wrote:
> > Yu Zhiguo wrote:
> > > Yu Zhiguo wrote:
> > >> I'm trying to add subcommand 'mem-max', I think xc_domain_setmaxmem should
> > >> be used in it but not here.
> > >>
> > >> I'll move this code to 'mem-max', and in 'mem-set', a check should be added
> > >> because setting memory larger than max memory is invalid.
> > >>
> > >> 1. Add 'mem-max'
> > >>   Add libxl_domain_setmaxmem, it calls xc_domain_setmaxmem.
> > >>   /local/domain/$domid/memory/static-max should be updated when set max memory,
> > >> it is missing now.
> > >>
> > >> 2. fix 'mem-set'
> > >>   Delete xc_domain_setmaxmem.
> > >>   Get max memory from /local/domain/$domid/memory/static-max, and then do value check.
> > >> It seems that we cannot get max memory use libxc routines.
> > >>
> > > 
> > > I fixed it, please check.
> > > 
> > > [PATCH 1/2] xl: Add command 'xl mem-max'
> > > [PATCH 2/2] xl: Add check for command 'xl mem-set'
> > > 
> > 
> > It seems than /local/domain/$domid/memory/target cannot be created when use 'xl create',
> > only /local/domain/0/memory/static-max is existent.
> > Isn't it a bug? I'll check the code.
> > 
> > I think it should be created like static-max.
> > * memory/target can be created when use 'xm create'
> > 
> 
> Actually sorry if I didn't reply in time before you sent your patches,
> but I asked the xapi memory management expert (CC'ed) an
> opinion on the memset/memmax commands, I have the impression
> that he has something interesting to say about them.
> 
> Keir, could you please wait before applying these patches?
> 

Oops I see now is too late :)
Jonathan, what do you think about them anyway?

Re: [PATCH 0/2] xl: Add subcommand mem-max and fix mem-set

[Jonathan Knowles]

Hi all

As one of the maintainers of the Xen API tool-stack [1], I have
a couple of comments about these patches.

Currently, the Xen API tool-stack does not use libxenlight, but
we're interested in moving to it in the near future.

Yu Zhiguo wrote:
> I'm trying to add subcommand 'mem-max', I think
> xc_domain_setmaxmem should be used in it but not here.
>
> I'll move this code to 'mem-max', and in 'mem-set', a check
> should be added because setting memory larger than max memory
> is invalid.
>
> 1. Add 'mem-max' Add libxl_domain_setmaxmem, it calls
> xc_domain_setmaxmem. /local/domain/$domid/memory/static-max
> should be updated when set max memory, it is missing now.

 From the point of view of the Xen API tool-stack, these values
have different purposes.

1. The XenStore value: /local/domain/$domid/memory/static-max

    This value reflects the maximum amount of physical host memory
    that's addressable by the guest OS.

    The Xen API tool-stack writes this value into XenStore at the
    time of domain construction, and leaves the value alone until
    the domain is destroyed.

    I think it would make sense for libxenlight to do the same.

    Many balloon drivers read this value, along with the "target"
    field, in order to determine how large they should make their
    balloons:

    balloon size = (static-max - target)

    In the absence of hotplug support, balloon drivers expect the
    value of "static-max" to remain constant for the lifetime of
    the domain.

2. The Xen value: maxmem

   Xen uses this value as a sort of "ratchet", to prevent a domain
   that's currently ballooned down from growing too large.

   Xen will reject any call to domain_memory_increase_reservation
   that would increase the reservation beyond the value of maxmex.

   It's possible to change the value of "maxmex" while a domain is
   running. Indeed, whenever the Xen API tool-stack writes a new
   balloon target into XenStore, it also calls xc_domain_setmaxmem
   with the same value.

   This mechanism allows the tool-stack to prevent a balloon driver
   from claiming back more memory if the guest is already using more
   than its target memory allocation.

So, I think it's fair to say that locking these two values
together does not make sense from the point of view of the Xen API
tool-stack.

>>>>> 2. fix 'mem-set' Delete xc_domain_setmaxmem. Get max
>>>>>  memory from /local/domain/$domid/memory/static-max,
>>>>>  and then do value check. It seems that we cannot get
>>>>>  max memory use libxc routines.

If the purpose of "mem-set" is to set a balloon target for a
guest, then it might also make sense to call xc_domain_setmaxmem
with the same value, whenever the target changes, for the reasons
stated above (preventing a domain that's currently ballooned down
from growing too large).

>>>> I fixed it, please check.
>>>>
>>>> [PATCH 1/2] xl: Add command 'xl mem-max' [PATCH 2/2]
>>>> xl: Add check for command 'xl mem-set'

Cheers,
Jonathan

[1] http://www.xen.org/products/cloud_source.html

Re: [PATCH 0/2] xl: Add subcommand mem-max and fix mem-set

[Stefano Stabellini]

On Wed, 12 May 2010, Jonathan Knowles wrote:
> Yu Zhiguo wrote:
> > I'm trying to add subcommand 'mem-max', I think
> > xc_domain_setmaxmem should be used in it but not here.
> >
> > I'll move this code to 'mem-max', and in 'mem-set', a check
> > should be added because setting memory larger than max memory
> > is invalid.
> >
> > 1. Add 'mem-max' Add libxl_domain_setmaxmem, it calls
> > xc_domain_setmaxmem. /local/domain/$domid/memory/static-max
> > should be updated when set max memory, it is missing now.
> 
>  From the point of view of the Xen API tool-stack, these values
> have different purposes.
> 
> 1. The XenStore value: /local/domain/$domid/memory/static-max
> 
>     This value reflects the maximum amount of physical host memory
>     that's addressable by the guest OS.
> 
>     The Xen API tool-stack writes this value into XenStore at the
>     time of domain construction, and leaves the value alone until
>     the domain is destroyed.
> 
>     I think it would make sense for libxenlight to do the same.
> 
>     Many balloon drivers read this value, along with the "target"
>     field, in order to determine how large they should make their
>     balloons:
> 
>     balloon size = (static-max - target)
> 
>     In the absence of hotplug support, balloon drivers expect the
>     value of "static-max" to remain constant for the lifetime of
>     the domain.
> 
> 2. The Xen value: maxmem
> 
>    Xen uses this value as a sort of "ratchet", to prevent a domain
>    that's currently ballooned down from growing too large.
> 
>    Xen will reject any call to domain_memory_increase_reservation
>    that would increase the reservation beyond the value of maxmex.
> 
>    It's possible to change the value of "maxmex" while a domain is
>    running. Indeed, whenever the Xen API tool-stack writes a new
>    balloon target into XenStore, it also calls xc_domain_setmaxmem
>    with the same value.
> 
>    This mechanism allows the tool-stack to prevent a balloon driver
>    from claiming back more memory if the guest is already using more
>    than its target memory allocation.
> 
> So, I think it's fair to say that locking these two values
> together does not make sense from the point of view of the Xen API
> tool-stack.


I think xl memset should change the memory currently used by the guest
and xl memmax should change the size of the guest's address space and
not the population.
For this reason libxl_set_memory_target should provide a way to enforce
the memory target, calling xc_domain_setmaxmem.
On the other hand xl memmax shouldn't call xc_domain_setmaxmem because
that is the upper bound of the memory reservation, it should just change
static-max, that at the moment wouldn't do much, but we can imagine that
in the future could trigger something useful in the guest.


Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Acked-by: Jonathan Knowles <Jonathan.Knowles@eu.citrix.com> 

---


diff -r 9d53864d7be6 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Thu May 13 12:21:16 2010 +0100
+++ b/tools/libxl/libxl.c	Thu May 13 13:17:51 2010 +0100
@@ -2468,7 +2468,6 @@
     char *mem, *endptr;
     uint32_t memorykb;
     char *dompath = libxl_xs_get_dompath(ctx, domid);
-    int rc;
 
     mem = libxl_xs_read(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/memory/target", dompath));
     if (!mem) {
@@ -2486,20 +2485,16 @@
         return 1;
     }
 
-    rc = xc_domain_setmaxmem(ctx->xch, domid, max_memkb);
-    if (rc != 0)
-        return rc;
-
     if (domid != 0)
         libxl_xs_write(ctx, XBT_NULL, libxl_sprintf(ctx, "%s/memory/static-max", dompath), "%lu", max_memkb);
 
-    return rc;
+    return 0;
 }
 
-int libxl_set_memory_target(struct libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb)
+int libxl_set_memory_target(struct libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb, int enforce)
 {
     int rc = 0;
-    uint32_t memorykb, videoram;
+    uint32_t memorykb = 0, videoram = 0;
     char *memmax, *endptr, *videoram_s = NULL;
     char *dompath = libxl_xs_get_dompath(ctx, domid);
     xc_domaininfo_t info;
@@ -2539,6 +2533,11 @@
     uuid = libxl_uuid2string(ctx, ptr.uuid);
     libxl_xs_write(ctx, XBT_NULL, libxl_sprintf(ctx, "/vm/%s/memory", uuid), "%lu", target_memkb / 1024);
 
+    if (enforce || !domid)
+        memorykb = target_memkb;
+    rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb + LIBXL_MAXMEM_CONSTANT);
+    if (rc != 0)
+        return rc;
     rc = xc_domain_memory_set_pod_target(ctx->xch, domid, (target_memkb - videoram) / 4, NULL, NULL, NULL);
     return rc;
 }
diff -r 9d53864d7be6 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Thu May 13 12:21:16 2010 +0100
+++ b/tools/libxl/libxl.h	Thu May 13 13:17:51 2010 +0100
@@ -340,7 +340,7 @@
 int libxl_domain_unpause(struct libxl_ctx *ctx, uint32_t domid);
 
 int libxl_domain_setmaxmem(struct libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb);
-int libxl_set_memory_target(struct libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb);
+int libxl_set_memory_target(struct libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb, int enforce);
 
 int libxl_console_attach(struct libxl_ctx *ctx, uint32_t domid, int cons_num);
 
diff -r 9d53864d7be6 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Thu May 13 12:21:16 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Thu May 13 13:17:51 2010 +0100
@@ -1262,7 +1262,7 @@
         exit(3);
     }
     printf("setting domid %d memory to : %d\n", domid, memorykb);
-    libxl_set_memory_target(&ctx, domid, memorykb);
+    libxl_set_memory_target(&ctx, domid, memorykb, /* enforce */ 1);
 }
 
 int main_memset(int argc, char **argv)

[PATCH] xl: Fix missing memory target in xenstore

[Yu Zhiguo]

Yu Zhiguo wrote:
> 
> It seems than /local/domain/$domid/memory/target cannot be created when use 'xl create',
> only /local/domain/0/memory/static-max is existent.
> Isn't it a bug? I'll check the code.
> 
> I think it should be created like static-max.
> * memory/target can be created when use 'xm create'


/local/domain/$domid/memory/target should be
created in xenstore when domain is created.

Signed-off-by: Yu Zhiguo <yuzg@cn.fujitsu.com>

diff -r d77a88f938c6 -r 3b473b9417e9 tools/libxl/libxl_dom.c
--- a/tools/libxl/libxl_dom.c	Tue May 11 14:05:28 2010 +0100
+++ b/tools/libxl/libxl_dom.c	Fri May 14 18:15:02 2010 +0800
@@ -100,22 +100,22 @@
     xc_cpuid_apply_policy(ctx->xch, domid);
 #endif
 
-    ents = libxl_calloc(ctx, (10 + info->max_vcpus) * 2, sizeof(char *));
+    ents = libxl_calloc(ctx, (12 + info->max_vcpus) * 2, sizeof(char *));
     ents[0] = "memory/static-max";
     ents[1] = libxl_sprintf(ctx, "%d", info->max_memkb);
     ents[2] = "memory/target";
     ents[3] = libxl_sprintf(ctx, "%d", info->target_memkb);
-    ents[2] = "memory/videoram";
-    ents[3] = libxl_sprintf(ctx, "%d", info->video_memkb);
-    ents[4] = "domid";
-    ents[5] = libxl_sprintf(ctx, "%d", domid);
-    ents[6] = "store/port";
-    ents[7] = libxl_sprintf(ctx, "%"PRIu32, state->store_port);
-    ents[8] = "store/ring-ref";
-    ents[9] = libxl_sprintf(ctx, "%lu", state->store_mfn);
+    ents[4] = "memory/videoram";
+    ents[5] = libxl_sprintf(ctx, "%d", info->video_memkb);
+    ents[6] = "domid";
+    ents[7] = libxl_sprintf(ctx, "%d", domid);
+    ents[8] = "store/port";
+    ents[9] = libxl_sprintf(ctx, "%"PRIu32, state->store_port);
+    ents[10] = "store/ring-ref";
+    ents[11] = libxl_sprintf(ctx, "%lu", state->store_mfn);
     for (i = 0; i < info->max_vcpus; i++) {
-        ents[10+(i*2)]   = libxl_sprintf(ctx, "cpu/%d/availability", i);
-        ents[10+(i*2)+1] = (i && info->cur_vcpus && (i >= info->cur_vcpus))
+        ents[12+(i*2)]   = libxl_sprintf(ctx, "cpu/%d/availability", i);
+        ents[12+(i*2)+1] = (i && info->cur_vcpus && (i >= info->cur_vcpus))
                             ? "offline" : "online";
     }