Xen devel - Mar 2013 - [PATCH v2] tools/tests/mem-sharing/memshrtool share-all test

Update memshrtool test program to allow sharing of all pages of two domains
with identical memory sizes. Currently the tool only allows sharing of
specific pages. With this patch we can quickly share all pages between clones
and check how many pages were successfully deduplicated. The pages''
content
are not checked, therefore this mode is only safe for clone domains.

v2: fix typo of source_info

Signed-off-by: Tamas Lengyel <tamas.lengyel@zentific.com>
---
 tools/tests/mem-sharing/memshrtool.c |   58 ++++++++++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)

diff --git a/tools/tests/mem-sharing/memshrtool.c
b/tools/tests/mem-sharing/memshrtool.c
index db44294..b3fd415 100644
--- a/tools/tests/mem-sharing/memshrtool.c
+++ b/tools/tests/mem-sharing/memshrtool.c
@@ -10,9 +10,12 @@
 #include <errno.h>
 #include <string.h>
 #include <sys/mman.h>
+#include <inttypes.h>
 
 #include "xenctrl.h"
 
+#define PAGE_SIZE_KB (XC_PAGE_SIZE/1024)
+
 static int usage(const char* prog)
 {
     printf("usage: %s <command> [args...]\n", prog);
@@ -24,6 +27,8 @@ static int usage(const char* prog)
     printf("  share <domid> <gfn> <handle>
<source> <source-gfn> <source-handle>\n");
     printf("                          - Share two pages.\n");
     printf("  unshare <domid> <gfn>   - Unshare a page by
grabbing a writable map.\n");
+    printf("  share-all <domid> <source>\n");
+    printf("                          - Share all pages.\n");
     printf("  add-to-physmap <domid> <gfn> <source>
<source-gfn> <source-handle>\n");
     printf("                          - Populate a page in a domain with a
shared page.\n");
     printf("  debug-gfn <domid> <gfn> - Debug a particular
domain and gfn.\n");
@@ -131,6 +136,59 @@ int main(int argc, const char** argv)
             munmap(map, 4096);
         R((int)!map);
     }
+    else if( !strcasecmp(cmd, "share-all") )
+    {
+        domid_t domid;
+        uint64_t handle;
+        domid_t source_domid;
+        uint64_t source_handle;
+        xc_dominfo_t info, source_info;
+        uint64_t pages, source_pages;
+        uint64_t total_shared=0;
+        int ret;
+        uint64_t share_page;
+
+        if( argc != 4 )
+            return usage(argv[0]);
+
+        domid = strtol(argv[2], NULL, 0);
+        source_domid = strtol(argv[3], NULL, 0);
+
+        ret=xc_domain_getinfo(xch, domid, 1, &info);
+        if(ret!=1 || info.domid != domid)
+            return usage(argv[0]);
+        pages=info.max_memkb/PAGE_SIZE_KB;
+
+        ret=xc_domain_getinfo(xch, source_domid, 1, &source_info);
+        if(ret!=1 || source_info.domid != source_domid)
+            return usage(argv[0]);
+        source_pages=source_info.max_memkb/PAGE_SIZE_KB;
+
+        if(pages != source_pages) {
+            printf("Page count in source and destination domain
doesn''t match "
+                   "(source: %"PRIu64", destination
%"PRIu64")\n", source_pages, pages);
+            return 1;
+        }
+
+        for(share_page=0;share_page<=pages;++share_page) {
+
+            ret=xc_memshr_nominate_gfn(xch, domid, share_page, &handle);
+            if(ret<0) {
+                continue;
+            }
+
+            ret=xc_memshr_nominate_gfn(xch, source_domid, share_page,
&source_handle);
+            if(ret<0) {
+                continue;
+            }
+
+            ret=xc_memshr_share_gfns(xch, source_domid, share_page,
source_handle, domid, share_page, handle);
+            if(ret>=0) total_shared++;
+        }
+
+        printf("Shared pages: %"PRIu64" out of
%"PRIu64"\n", total_shared, pages);
+
+    }
     else if( !strcasecmp(cmd, "add-to-physmap") )
     {
         domid_t domid;
-- 
1.7.2.5

At 09:34 -0400 on 18 Mar (1363599276), Tamas Lengyel
wrote:
> Update memshrtool test program to allow sharing of all pages of two domains
> with identical memory sizes. Currently the tool only allows sharing of
> specific pages. With this patch we can quickly share all pages between
clones
> and check how many pages were successfully deduplicated. The
pages'' content
> are not checked, therefore this mode is only safe for clone domains.
Cc''ing Andres, who wrote the original tool. 

Tim.
> v2: fix typo of source_info
> 
> Signed-off-by: Tamas Lengyel <tamas.lengyel@zentific.com>
> ---
>  tools/tests/mem-sharing/memshrtool.c |   58
++++++++++++++++++++++++++++++++++
>  1 files changed, 58 insertions(+), 0 deletions(-)
> 
> diff --git a/tools/tests/mem-sharing/memshrtool.c
b/tools/tests/mem-sharing/memshrtool.c
> index db44294..b3fd415 100644
> --- a/tools/tests/mem-sharing/memshrtool.c
> +++ b/tools/tests/mem-sharing/memshrtool.c
> @@ -10,9 +10,12 @@
>  #include <errno.h>
>  #include <string.h>
>  #include <sys/mman.h>
> +#include <inttypes.h>
>  
>  #include "xenctrl.h"
>  
> +#define PAGE_SIZE_KB (XC_PAGE_SIZE/1024)
> +
>  static int usage(const char* prog)
>  {
>      printf("usage: %s <command> [args...]\n", prog);
> @@ -24,6 +27,8 @@ static int usage(const char* prog)
>      printf("  share <domid> <gfn> <handle>
<source> <source-gfn> <source-handle>\n");
>      printf("                          - Share two pages.\n");
>      printf("  unshare <domid> <gfn>   - Unshare a page by
grabbing a writable map.\n");
> +    printf("  share-all <domid> <source>\n");
> +    printf("                          - Share all pages.\n");
>      printf("  add-to-physmap <domid> <gfn> <source>
<source-gfn> <source-handle>\n");
>      printf("                          - Populate a page in a domain
with a shared page.\n");
>      printf("  debug-gfn <domid> <gfn> - Debug a
particular domain and gfn.\n");
> @@ -131,6 +136,59 @@ int main(int argc, const char** argv)
>              munmap(map, 4096);
>          R((int)!map);
>      }
> +    else if( !strcasecmp(cmd, "share-all") )
> +    {
> +        domid_t domid;
> +        uint64_t handle;
> +        domid_t source_domid;
> +        uint64_t source_handle;
> +        xc_dominfo_t info, source_info;
> +        uint64_t pages, source_pages;
> +        uint64_t total_shared=0;
> +        int ret;
> +        uint64_t share_page;
> +
> +        if( argc != 4 )
> +            return usage(argv[0]);
> +
> +        domid = strtol(argv[2], NULL, 0);
> +        source_domid = strtol(argv[3], NULL, 0);
> +
> +        ret=xc_domain_getinfo(xch, domid, 1, &info);
> +        if(ret!=1 || info.domid != domid)
> +            return usage(argv[0]);
> +        pages=info.max_memkb/PAGE_SIZE_KB;
> +
> +        ret=xc_domain_getinfo(xch, source_domid, 1, &source_info);
> +        if(ret!=1 || source_info.domid != source_domid)
> +            return usage(argv[0]);
> +        source_pages=source_info.max_memkb/PAGE_SIZE_KB;
> +
> +        if(pages != source_pages) {
> +            printf("Page count in source and destination domain
doesn''t match "
> +                   "(source: %"PRIu64", destination
%"PRIu64")\n", source_pages, pages);
> +            return 1;
> +        }
> +
> +        for(share_page=0;share_page<=pages;++share_page) {
> +
> +            ret=xc_memshr_nominate_gfn(xch, domid, share_page,
&handle);
> +            if(ret<0) {
> +                continue;
> +            }
> +
> +            ret=xc_memshr_nominate_gfn(xch, source_domid, share_page,
&source_handle);
> +            if(ret<0) {
> +                continue;
> +            }
> +
> +            ret=xc_memshr_share_gfns(xch, source_domid, share_page,
source_handle, domid, share_page, handle);
> +            if(ret>=0) total_shared++;
> +        }
> +
> +        printf("Shared pages: %"PRIu64" out of
%"PRIu64"\n", total_shared, pages);
> +
> +    }
>      else if( !strcasecmp(cmd, "add-to-physmap") )
>      {
>          domid_t domid;
> -- 
> 1.7.2.5
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Mar 21, 2013, at 8:17 AM, Tim Deegan <tim@xen.org> wrote:
> At 09:34 -0400 on 18 Mar (1363599276), Tamas Lengyel wrote:
>> Update memshrtool test program to allow sharing of all pages of two
domains
>> with identical memory sizes. Currently the tool only allows sharing of
>> specific pages. With this patch we can quickly share all pages between
clones
>> and check how many pages were successfully deduplicated. The
pages'' content
>> are not checked, therefore this mode is only safe for clone domains.
> 
> Cc''ing Andres, who wrote the original tool. 
> 
> Tim.
> 
>> v2: fix typo of source_info
>> 
>> Signed-off-by: Tamas Lengyel <tamas.lengyel@zentific.com>
Just a few minute comments.

The code in itself is correct as a first attempt. I am tempted to ack it on the
basis of being a useful thing.

However, I have  concerns of a higher level, and I see one important problem
outlined below.

In terms of higher level:
- Are these really clone VMs? In order to nominate gfns, they must be allocated
… so, what was allocated in the target VM before this? How would you share two
16GB domains if you have 2GB free before allocating the target domain (setting
aside how do you deal with CoW overflow, which is a separate issue). You may
consider revisiting the add to physmap sharing memop.
- Can you document when should one call this? Or at least your envisioned
scenario. Ties in with the question before.
- I think it''s high time we batch sharing calls. I have not been able
to do this, but it should be relatively simple to submit a hypervisor patch to
achieve this. For what you are trying to do, it will give you a very nice boost
in performance.
>> ---
>> tools/tests/mem-sharing/memshrtool.c |   58
++++++++++++++++++++++++++++++++++
>> 1 files changed, 58 insertions(+), 0 deletions(-)
>> 
>> diff --git a/tools/tests/mem-sharing/memshrtool.c
b/tools/tests/mem-sharing/memshrtool.c
>> index db44294..b3fd415 100644
>> --- a/tools/tests/mem-sharing/memshrtool.c
>> +++ b/tools/tests/mem-sharing/memshrtool.c
>> @@ -10,9 +10,12 @@
>> #include <errno.h>
>> #include <string.h>
>> #include <sys/mman.h>
>> +#include <inttypes.h>
>> 
>> #include "xenctrl.h"
>> 
>> +#define PAGE_SIZE_KB (XC_PAGE_SIZE/1024)
A matter of style, but in my view this is unneeded, see
below.
>> +
>> static int usage(const char* prog)
>> {
>>     printf("usage: %s <command> [args...]\n", prog);
>> @@ -24,6 +27,8 @@ static int usage(const char* prog)
>>     printf("  share <domid> <gfn> <handle>
<source> <source-gfn> <source-handle>\n");
>>     printf("                          - Share two pages.\n");
>>     printf("  unshare <domid> <gfn>   - Unshare a page
by grabbing a writable map.\n");
>> +    printf("  share-all <domid> <source>\n");
>> +    printf("                          - Share all
pages.\n");
>>     printf("  add-to-physmap <domid> <gfn>
<source> <source-gfn> <source-handle>\n");
>>     printf("                          - Populate a page in a
domain with a shared page.\n");
>>     printf("  debug-gfn <domid> <gfn> - Debug a
particular domain and gfn.\n");
>> @@ -131,6 +136,59 @@ int main(int argc, const char** argv)
>>             munmap(map, 4096);
>>         R((int)!map);
>>     }
>> +    else if( !strcasecmp(cmd, "share-all") )
>> +    {
>> +        domid_t domid;
>> +        uint64_t handle;
>> +        domid_t source_domid;
>> +        uint64_t source_handle;
>> +        xc_dominfo_t info, source_info;
>> +        uint64_t pages, source_pages;
>> +        uint64_t total_shared=0;
>> +        int ret;
>> +        uint64_t share_page;
>> +
>> +        if( argc != 4 )
>> +            return usage(argv[0]);
>> +
>> +        domid = strtol(argv[2], NULL, 0);
>> +        source_domid = strtol(argv[3], NULL, 0);
>> +
>> +        ret=xc_domain_getinfo(xch, domid, 1, &info);
>> +        if(ret!=1 || info.domid != domid)
>> +            return usage(argv[0]);
>> +        pages=info.max_memkb/PAGE_SIZE_KB;
>> 2, cleaner code, more inline with the code base.
>> +
>> +        ret=xc_domain_getinfo(xch, source_domid, 1, &source_info);
>> +        if(ret!=1 || source_info.domid != source_domid)
>> +            return usage(argv[0]);
>> +        source_pages=source_info.max_memkb/PAGE_SIZE_KB;
In most scenarios you would need to pause this, particularly as VMs may
self-modify their physmap (balloon, mmio, etc)
>> +
>> +        if(pages != source_pages) {
>> +            printf("Page count in source and destination domain
doesn''t match "
to stderr.
>> +                   "(source: %"PRIu64", destination
%"PRIu64")\n", source_pages, pages);
>> +            return 1;
>> +        }
>> +
>> +        for(share_page=0;share_page<=pages;++share_page) {
The memory layout of an hvm is sparse. While tot pages will get you a lot of
sharing, it will not get you all. For example, for a VM with nominal 4GB of RAM,
the max gfn is around 4.25GB. Even for small VMs, you have gfns in the 3.75-4GB
range. You should check equality of max gfn, which might be a very difficult
thing to achieve depending on the stage of a VM''s lifetime at which you
call this. And you should have a policy for dealing with physmap holes (for
example, is there any point in sharing the VGA mmio? yes/no, your call, argue
for it, document it, etc)

Andres
>> +
>> +            ret=xc_memshr_nominate_gfn(xch, domid, share_page,
&handle);
>> +            if(ret<0) {
>> +                continue;
>> +            }
>> +
>> +            ret=xc_memshr_nominate_gfn(xch, source_domid, share_page,
&source_handle);
>> +            if(ret<0) {
>> +                continue;
>> +            }
>> +
>> +            ret=xc_memshr_share_gfns(xch, source_domid, share_page,
source_handle, domid, share_page, handle);
>> +            if(ret>=0) total_shared++;
>> +        }
>> +
>> +        printf("Shared pages: %"PRIu64" out of
%"PRIu64"\n", total_shared, pages);
>> +
>> +    }
>>     else if( !strcasecmp(cmd, "add-to-physmap") )
>>     {
>>         domid_t domid;
>> -- 
>> 1.7.2.5
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel

Hi Andres,
thanks for taking a look at this patch!
> In terms of higher level:
> - Are these really clone VMs? In order to nominate gfns, they must be
allocated … so, what was allocated in the target VM before this? How would you
share two 16GB domains if you have 2GB free before allocating the target domain
(setting aside how do you deal with CoW overflow, which is a separate issue).
You may consider revisiting the add to physmap sharing memop.
> - Can you document when should one call this? Or at least your envisioned
scenario. Ties in with the question before.
While add_to_physmap would be ideal to quickly clone VMs, I haven''t
found anything useful (documentation/code sample) on doing it that
way. The only way I found to clone a VM right now is using XL
save/restore and than deduplicating the pages using nominate/share. I
do this in the following order:

1. Retrieve origin VMs configuration.
2. Parse and modify the config by changing the VM''s name, disk and
network interface. The disk assigned to the clone is a CoW disk (qcow2
or LVM), the network bridge is a new bridge as to avoid MAC/IP
collision with the origin VM.
3. Create a FIFO pipe on the filesystem (mkfifo /tmp/cloning)
4. Use XL to clone the VM''s execution state and memory without
deduplication: xl pause <origin> && xl save -c <origin>
/tmp/cloning |
xl restore -p <modified config> /tmp/cloning
5. Use the routine in this patch to deduplicate the memory
6. Unpause clone.

This is quite wasteful as you and Patrick pointed it out in
http://lists.xen.org/archives/html/xen-devel/2012-02/msg00259.html.
Unfortunately, I haven''t found a straight forward way to duplicate
only the execution state of a VM without duplicating it''s entire
memory to allow me to use add_to_physmap. If XL would have an option
in it''s xl save routine to do a partial save, that would be great. I
did scan through the XL code to determine how one would do that but
I''m not even close to understanding the internals of XL.
> - I think it''s high time we batch sharing calls. I have not been
able to do this, but it should be relatively simple to submit a hypervisor patch
to achieve this. For what you are trying to do, it will give you a very nice
boost in performance.
That sounds like something that would be very useful.
>>> +#define PAGE_SIZE_KB (XC_PAGE_SIZE/1024)
> A matter of style, but in my view this is unneeded, see below.
>>> +        pages=info.max_memkb/PAGE_SIZE_KB;
>>> 2, cleaner code, more inline with the code base.
Sure.
>>> +        source_pages=source_info.max_memkb/PAGE_SIZE_KB;
> In most scenarios you would need to pause this, particularly as VMs may
self-modify their physmap (balloon, mmio, etc)
See above my intended usage (origin and clone should both be paused
during this operation).
>>> +
>>> +        if(pages != source_pages) {
>>> +            printf("Page count in source and destination
domain doesn''t match "
> to stderr.
OK.
>>> +        for(share_page=0;share_page<=pages;++share_page) {
> The memory layout of an hvm is sparse. While tot pages will get you a lot
of sharing, it will not get you all. For example, for a VM with nominal 4GB of
RAM, the max gfn is around 4.25GB.
This is something that lacks documentation (or did I just failed
finding it?) so thanks for shedding some light on it! =) I did spend
days trying to figure out the best way of getting the list of valid
gfn''s of a domain, without success. This approach did seem to work OK,
although the number of pages shared this way was never 100% as parts
of the memory fail at the nominate call, hence my continue; in the
code for those cases.
> Even for small VMs, you have gfns in the 3.75-4GB range. You should check
equality of max gfn, which might be a very difficult thing to achieve depending
on the stage of a VM''s lifetime at which you call this.
Can you elaborate on this? (Is this documented anywhere? How would you
determine the max gfn of a domain?)
> And you should have a policy for dealing with physmap holes (for example,
is there any point in sharing the VGA mmio? yes/no, your call, argue for it,
document it, etc)
I guess this depends on the intended usage of the clone. For my
purposes the closer the clone is to the origin the better. Of course,
there are situations where this is simply not possible (for example
cloning a VM with PCI passthrough devices).

Tamas

On Fri, 2013-03-22 at 19:25 +0000, Andres Lagar-Cavilla
wrote:
> On Mar 21, 2013, at 8:17 AM, Tim Deegan <tim@xen.org> wrote:
> 
> > At 09:34 -0400 on 18 Mar (1363599276), Tamas Lengyel wrote:
> >> Update memshrtool test program to allow sharing of all pages of
two domains
> >> with identical memory sizes. Currently the tool only allows
sharing of
> >> specific pages. With this patch we can quickly share all pages
between clones
> >> and check how many pages were successfully deduplicated. The
pages' content
> >> are not checked, therefore this mode is only safe for clone
domains.
> > 
> > Cc'ing Andres, who wrote the original tool. 
> > 
> > Tim.
> > 
> >> v2: fix typo of source_info
> >> 
> >> Signed-off-by: Tamas Lengyel <tamas.lengyel@zentific.com>
> Just a few minute comments.
> 
> The code in itself is correct as a first attempt. I am tempted to ack
> it on the basis of being a useful thing.
Did you conclude that you would ack it in the end or not?

WRT the freeze it seems this is new standalone functionality in a test
tool, which ought to be pretty safe. George CCd.

Ian.
> 
> However, I have  concerns of a higher level, and I see one important
problem outlined below.
> 
> In terms of higher level:
> - Are these really clone VMs? In order to nominate gfns, they must be
allocated … so, what was allocated in the target VM before this? How would you
share two 16GB domains if you have 2GB free before allocating the target domain
(setting aside how do you deal with CoW overflow, which is a separate issue).
You may consider revisiting the add to physmap sharing memop.
> - Can you document when should one call this? Or at least your envisioned
scenario. Ties in with the question before.
> - I think it's high time we batch sharing calls. I have not been able
to do this, but it should be relatively simple to submit a hypervisor patch to
achieve this. For what you are trying to do, it will give you a very nice boost
in performance.
> 
> >> ---
> >> tools/tests/mem-sharing/memshrtool.c |   58
++++++++++++++++++++++++++++++++++
> >> 1 files changed, 58 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/tools/tests/mem-sharing/memshrtool.c
b/tools/tests/mem-sharing/memshrtool.c
> >> index db44294..b3fd415 100644
> >> --- a/tools/tests/mem-sharing/memshrtool.c
> >> +++ b/tools/tests/mem-sharing/memshrtool.c
> >> @@ -10,9 +10,12 @@
> >> #include <errno.h>
> >> #include <string.h>
> >> #include <sys/mman.h>
> >> +#include <inttypes.h>
> >> 
> >> #include "xenctrl.h"
> >> 
> >> +#define PAGE_SIZE_KB (XC_PAGE_SIZE/1024)
> A matter of style, but in my view this is unneeded, see below.
> >> +
> >> static int usage(const char* prog)
> >> {
> >>     printf("usage: %s <command> [args...]\n",
prog);
> >> @@ -24,6 +27,8 @@ static int usage(const char* prog)
> >>     printf("  share <domid> <gfn> <handle>
<source> <source-gfn> <source-handle>\n");
> >>     printf("                          - Share two
pages.\n");
> >>     printf("  unshare <domid> <gfn>   - Unshare a
page by grabbing a writable map.\n");
> >> +    printf("  share-all <domid>
<source>\n");
> >> +    printf("                          - Share all
pages.\n");
> >>     printf("  add-to-physmap <domid> <gfn>
<source> <source-gfn> <source-handle>\n");
> >>     printf("                          - Populate a page in a
domain with a shared page.\n");
> >>     printf("  debug-gfn <domid> <gfn> - Debug a
particular domain and gfn.\n");
> >> @@ -131,6 +136,59 @@ int main(int argc, const char** argv)
> >>             munmap(map, 4096);
> >>         R((int)!map);
> >>     }
> >> +    else if( !strcasecmp(cmd, "share-all") )
> >> +    {
> >> +        domid_t domid;
> >> +        uint64_t handle;
> >> +        domid_t source_domid;
> >> +        uint64_t source_handle;
> >> +        xc_dominfo_t info, source_info;
> >> +        uint64_t pages, source_pages;
> >> +        uint64_t total_shared=0;
> >> +        int ret;
> >> +        uint64_t share_page;
> >> +
> >> +        if( argc != 4 )
> >> +            return usage(argv[0]);
> >> +
> >> +        domid = strtol(argv[2], NULL, 0);
> >> +        source_domid = strtol(argv[3], NULL, 0);
> >> +
> >> +        ret=xc_domain_getinfo(xch, domid, 1, &info);
> >> +        if(ret!=1 || info.domid != domid)
> >> +            return usage(argv[0]);
> >> +        pages=info.max_memkb/PAGE_SIZE_KB;
> >> 2, cleaner code, more inline with the code base.
> >> +
> >> +        ret=xc_domain_getinfo(xch, source_domid, 1,
&source_info);
> >> +        if(ret!=1 || source_info.domid != source_domid)
> >> +            return usage(argv[0]);
> >> +        source_pages=source_info.max_memkb/PAGE_SIZE_KB;
> In most scenarios you would need to pause this, particularly as VMs may
self-modify their physmap (balloon, mmio, etc)
> >> +
> >> +        if(pages != source_pages) {
> >> +            printf("Page count in source and destination
domain doesn't match "
> to stderr.
> >> +                   "(source: %"PRIu64",
destination %"PRIu64")\n", source_pages, pages);
> >> +            return 1;
> >> +        }
> >> +
> >> +        for(share_page=0;share_page<=pages;++share_page) {
> The memory layout of an hvm is sparse. While tot pages will get you a lot
of sharing, it will not get you all. For example, for a VM with nominal 4GB of
RAM, the max gfn is around 4.25GB. Even for small VMs, you have gfns in the
3.75-4GB range. You should check equality of max gfn, which might be a very
difficult thing to achieve depending on the stage of a VM's lifetime at
which you call this. And you should have a policy for dealing with physmap holes
(for example, is there any point in sharing the VGA mmio? yes/no, your call,
argue for it, document it, etc)
> 
> Andres
> >> +
> >> +            ret=xc_memshr_nominate_gfn(xch, domid, share_page,
&handle);
> >> +            if(ret<0) {
> >> +                continue;
> >> +            }
> >> +
> >> +            ret=xc_memshr_nominate_gfn(xch, source_domid,
share_page, &source_handle);
> >> +            if(ret<0) {
> >> +                continue;
> >> +            }
> >> +
> >> +            ret=xc_memshr_share_gfns(xch, source_domid,
share_page, source_handle, domid, share_page, handle);
> >> +            if(ret>=0) total_shared++;
> >> +        }
> >> +
> >> +        printf("Shared pages: %"PRIu64" out of
%"PRIu64"\n", total_shared, pages);
> >> +
> >> +    }
> >>     else if( !strcasecmp(cmd, "add-to-physmap") )
> >>     {
> >>         domid_t domid;
> >> -- 
> >> 1.7.2.5
> >> 
> >> 
> >> _______________________________________________
> >> Xen-devel mailing list
> >> Xen-devel@lists.xen.org
> >> http://lists.xen.org/xen-devel
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


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

On 22/04/13 13:07, Ian Campbell wrote:
> On Fri, 2013-03-22 at 19:25 +0000, Andres Lagar-Cavilla wrote:
>> On Mar 21, 2013, at 8:17 AM, Tim Deegan <tim@xen.org> wrote:
>>
>>> At 09:34 -0400 on 18 Mar (1363599276), Tamas Lengyel wrote:
>>>> Update memshrtool test program to allow sharing of all pages of
two domains
>>>> with identical memory sizes. Currently the tool only allows
sharing of
>>>> specific pages. With this patch we can quickly share all pages
between clones
>>>> and check how many pages were successfully deduplicated. The
pages'' content
>>>> are not checked, therefore this mode is only safe for clone
domains.
>>> Cc''ing Andres, who wrote the original tool.
>>>
>>> Tim.
>>>
>>>> v2: fix typo of source_info
>>>>
>>>> Signed-off-by: Tamas Lengyel <tamas.lengyel@zentific.com>
>> Just a few minute comments.
>>
>> The code in itself is correct as a first attempt. I am tempted to ack
>> it on the basis of being a useful thing.
> Did you conclude that you would ack it in the end or not?
>
> WRT the freeze it seems this is new standalone functionality in a test
> tool, which ought to be pretty safe. George CCd.
I think this is a "just barely" in terms of timing / risk / benefits 
analysis, but:

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

On Apr 22, 2013, at 8:11 AM, George Dunlap <George.Dunlap@eu.citrix.com>
wrote:
> On 22/04/13 13:07, Ian Campbell wrote:
>> On Fri, 2013-03-22 at 19:25 +0000, Andres Lagar-Cavilla wrote:
>>> On Mar 21, 2013, at 8:17 AM, Tim Deegan <tim@xen.org> wrote:
>>> 
>>>> At 09:34 -0400 on 18 Mar (1363599276), Tamas Lengyel wrote:
>>>>> Update memshrtool test program to allow sharing of all
pages of two domains
>>>>> with identical memory sizes. Currently the tool only allows
sharing of
>>>>> specific pages. With this patch we can quickly share all
pages between clones
>>>>> and check how many pages were successfully deduplicated.
The pages'' content
>>>>> are not checked, therefore this mode is only safe for clone
domains.
>>>> Cc''ing Andres, who wrote the original tool.
>>>> 
>>>> Tim.
>>>> 
>>>>> v2: fix typo of source_info
>>>>> 
>>>>> Signed-off-by: Tamas Lengyel
<tamas.lengyel@zentific.com>
>>> Just a few minute comments.
>>> 
>>> The code in itself is correct as a first attempt. I am tempted to
ack
>>> it on the basis of being a useful thing.
>> Did you conclude that you would ack it in the end or not?
>> 
>> WRT the freeze it seems this is new standalone functionality in a test
>> tool, which ought to be pretty safe. George CCd.
> 
> I think this is a "just barely" in terms of timing / risk /
benefits analysis, but:
> 
> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
It is clearly over the acceptable threshold in terms of a demo testing tool.

Acked-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>

Having said that, Tamas, can you add a blob of comment stating the expected
operating conditions: both domains paused, sharing enabled, both domains able to
stand on their own feet (vcpu contexts, device model, PV backends if any). Also
clearly stating that this might corrupt domain memory if used carelessly. Just a
few lines of prose in there.

Thanks (and sorry for dropping the ball for a bit)
Andres