* [PATCH 1 of 8] Tools: Do not assume sharing target is dom0 in libxc wrappers
2011-12-09 23:15 [PATCH 0 of 8] Tools: Memory sharing interface updates Andres Lagar-Cavilla
@ 2011-12-09 23:15 ` Andres Lagar-Cavilla
2011-12-12 9:50 ` Ian Campbell
2011-12-09 23:15 ` [PATCH 2 of 8] Tools: Update libxc mem sharing interface Andres Lagar-Cavilla
` (6 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Andres Lagar-Cavilla @ 2011-12-09 23:15 UTC (permalink / raw)
To: xen-devel; +Cc: ian.jackson, andres, tim, ian.campbell, adin
tools/libxc/xc_memshr.c | 3 ++-
tools/libxc/xenctrl.h | 1 +
2 files changed, 3 insertions(+), 1 deletions(-)
The libxc wrapper that shares two pages always assumed one
of the pages was mapped by dom0. Expand the API to specify
both sharing domains.
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
diff -r 835be7c121b7 -r 6f58de995103 tools/libxc/xc_memshr.c
--- a/tools/libxc/xc_memshr.c
+++ b/tools/libxc/xc_memshr.c
@@ -87,6 +87,7 @@ int xc_memshr_nominate_gref(xc_interface
}
int xc_memshr_share(xc_interface *xch,
+ uint32_t source_domain,
uint64_t source_handle,
uint64_t client_handle)
{
@@ -95,7 +96,7 @@ int xc_memshr_share(xc_interface *xch,
domctl.cmd = XEN_DOMCTL_mem_sharing_op;
domctl.interface_version = XEN_DOMCTL_INTERFACE_VERSION;
- domctl.domain = 0;
+ domctl.domain = (domid_t) source_domain;
op = &(domctl.u.mem_sharing_op);
op->op = XEN_DOMCTL_MEM_EVENT_OP_SHARING_SHARE;
op->u.share.source_handle = source_handle;
diff -r 835be7c121b7 -r 6f58de995103 tools/libxc/xenctrl.h
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1892,6 +1892,7 @@ int xc_memshr_nominate_gref(xc_interface
grant_ref_t gref,
uint64_t *handle);
int xc_memshr_share(xc_interface *xch,
+ uint32_t source_domain,
uint64_t source_handle,
uint64_t client_handle);
int xc_memshr_domain_resume(xc_interface *xch,
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 1 of 8] Tools: Do not assume sharing target is dom0 in libxc wrappers
2011-12-09 23:15 ` [PATCH 1 of 8] Tools: Do not assume sharing target is dom0 in libxc wrappers Andres Lagar-Cavilla
@ 2011-12-12 9:50 ` Ian Campbell
0 siblings, 0 replies; 24+ messages in thread
From: Ian Campbell @ 2011-12-12 9:50 UTC (permalink / raw)
To: Andres Lagar-Cavilla
Cc: Ian Jackson, andres@gridcentric.ca, xen-devel@lists.xensource.com,
Tim (Xen.org), adin@gridcentric.ca
On Fri, 2011-12-09 at 23:15 +0000, Andres Lagar-Cavilla wrote:
> tools/libxc/xc_memshr.c | 3 ++-
> tools/libxc/xenctrl.h | 1 +
> 2 files changed, 3 insertions(+), 1 deletions(-)
>
>
> The libxc wrapper that shares two pages always assumed one
> of the pages was mapped by dom0. Expand the API to specify
> both sharing domains.
>
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>
> diff -r 835be7c121b7 -r 6f58de995103 tools/libxc/xc_memshr.c
> --- a/tools/libxc/xc_memshr.c
> +++ b/tools/libxc/xc_memshr.c
> @@ -87,6 +87,7 @@ int xc_memshr_nominate_gref(xc_interface
> }
>
> int xc_memshr_share(xc_interface *xch,
> + uint32_t source_domain,
> uint64_t source_handle,
> uint64_t client_handle)
> {
> @@ -95,7 +96,7 @@ int xc_memshr_share(xc_interface *xch,
>
> domctl.cmd = XEN_DOMCTL_mem_sharing_op;
> domctl.interface_version = XEN_DOMCTL_INTERFACE_VERSION;
> - domctl.domain = 0;
> + domctl.domain = (domid_t) source_domain;
domid_t is fair game in xenctrl.h, why not just use it?
There is an existing caller of this function in tools/memshr/interface.c
so this change will break the build. I suspect your 3/8 patch fixes this
but you must update the users at the same time, at least with the
minimal "no semantic change" patch, e.g. pass source_domain as 0, such
that everything still works at each step.
The changelog says "specify both sharing domains" yet you only add the
source domain here, where is the tgt domain?
> op = &(domctl.u.mem_sharing_op);
> op->op = XEN_DOMCTL_MEM_EVENT_OP_SHARING_SHARE;
> op->u.share.source_handle = source_handle;
> diff -r 835be7c121b7 -r 6f58de995103 tools/libxc/xenctrl.h
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1892,6 +1892,7 @@ int xc_memshr_nominate_gref(xc_interface
> grant_ref_t gref,
> uint64_t *handle);
> int xc_memshr_share(xc_interface *xch,
> + uint32_t source_domain,
> uint64_t source_handle,
> uint64_t client_handle);
> int xc_memshr_domain_resume(xc_interface *xch,
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2 of 8] Tools: Update libxc mem sharing interface
2011-12-09 23:15 [PATCH 0 of 8] Tools: Memory sharing interface updates Andres Lagar-Cavilla
2011-12-09 23:15 ` [PATCH 1 of 8] Tools: Do not assume sharing target is dom0 in libxc wrappers Andres Lagar-Cavilla
@ 2011-12-09 23:15 ` Andres Lagar-Cavilla
2011-12-12 9:56 ` Ian Campbell
2011-12-09 23:15 ` [PATCH 3 of 8] Tools: Update memshr tool to use new sharing API Andres Lagar-Cavilla
` (5 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Andres Lagar-Cavilla @ 2011-12-09 23:15 UTC (permalink / raw)
To: xen-devel; +Cc: ian.jackson, andres, tim, ian.campbell, adin
tools/libxc/xc_memshr.c | 19 ++++++++++++++++++-
tools/libxc/xenctrl.h | 7 ++++++-
2 files changed, 24 insertions(+), 2 deletions(-)
Previosuly, the mem sharing code would return an opaque handle
to index shared pages (and nominees) in its global hash table.
By removing the hash table, the handle becomes a version, to
avoid sharing a stale version of a page. Thus, libxc wrappers
need to be updated accordingly.
Signed-off-by: Adin Scannell <adin@scannell.ca>
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
diff -r 6f58de995103 -r 892049dfc1c9 tools/libxc/xc_memshr.c
--- a/tools/libxc/xc_memshr.c
+++ b/tools/libxc/xc_memshr.c
@@ -88,8 +88,13 @@ int xc_memshr_nominate_gref(xc_interface
int xc_memshr_share(xc_interface *xch,
uint32_t source_domain,
+ uint64_t source_gfn,
uint64_t source_handle,
- uint64_t client_handle)
+ int source_is_gref,
+ uint32_t client_domain,
+ uint64_t client_gfn,
+ uint64_t client_handle,
+ int client_is_gref)
{
DECLARE_DOMCTL;
struct xen_domctl_mem_sharing_op *op;
@@ -100,8 +105,20 @@ int xc_memshr_share(xc_interface *xch,
op = &(domctl.u.mem_sharing_op);
op->op = XEN_DOMCTL_MEM_EVENT_OP_SHARING_SHARE;
op->u.share.source_handle = source_handle;
+ op->u.share.client_domain = (uint64_t) client_domain;
op->u.share.client_handle = client_handle;
+ if (source_is_gref)
+ XEN_DOMCTL_MEM_SHARING_FIELD_MAKE_GREF(
+ op->u.share.source_gfn, source_gfn);
+ else
+ op->u.share.source_gfn = source_gfn;
+ if (client_is_gref)
+ XEN_DOMCTL_MEM_SHARING_FIELD_MAKE_GREF(
+ op->u.share.client_gfn, client_gfn);
+ else
+ op->u.share.client_gfn = client_gfn;
+
return do_domctl(xch, &domctl);
}
diff -r 6f58de995103 -r 892049dfc1c9 tools/libxc/xenctrl.h
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1893,8 +1893,13 @@ int xc_memshr_nominate_gref(xc_interface
uint64_t *handle);
int xc_memshr_share(xc_interface *xch,
uint32_t source_domain,
+ uint64_t source_gfn,
uint64_t source_handle,
- uint64_t client_handle);
+ int source_is_gref,
+ uint32_t client_domain,
+ uint64_t client_gfn,
+ uint64_t client_handle,
+ int dest_is_gref);
int xc_memshr_domain_resume(xc_interface *xch,
uint32_t domid);
int xc_memshr_debug_gfn(xc_interface *xch,
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 2 of 8] Tools: Update libxc mem sharing interface
2011-12-09 23:15 ` [PATCH 2 of 8] Tools: Update libxc mem sharing interface Andres Lagar-Cavilla
@ 2011-12-12 9:56 ` Ian Campbell
0 siblings, 0 replies; 24+ messages in thread
From: Ian Campbell @ 2011-12-12 9:56 UTC (permalink / raw)
To: Andres Lagar-Cavilla
Cc: Ian Jackson, andres@gridcentric.ca, xen-devel@lists.xensource.com,
Tim (Xen.org), adin@gridcentric.ca
On Fri, 2011-12-09 at 23:15 +0000, Andres Lagar-Cavilla wrote:
> tools/libxc/xc_memshr.c | 19 ++++++++++++++++++-
> tools/libxc/xenctrl.h | 7 ++++++-
> 2 files changed, 24 insertions(+), 2 deletions(-)
>
>
> Previosuly, the mem sharing code would return an opaque handle
> to index shared pages (and nominees) in its global hash table.
> By removing the hash table, the handle becomes a version, to
> avoid sharing a stale version of a page. Thus, libxc wrappers
> need to be updated accordingly.
Same comment wrt breaking the build as the previous patch.
>
> Signed-off-by: Adin Scannell <adin@scannell.ca>
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>
> diff -r 6f58de995103 -r 892049dfc1c9 tools/libxc/xc_memshr.c
> --- a/tools/libxc/xc_memshr.c
> +++ b/tools/libxc/xc_memshr.c
> @@ -88,8 +88,13 @@ int xc_memshr_nominate_gref(xc_interface
>
> int xc_memshr_share(xc_interface *xch,
> uint32_t source_domain,
> + uint64_t source_gfn,
> uint64_t source_handle,
> - uint64_t client_handle)
> + int source_is_gref,
> + uint32_t client_domain,
> + uint64_t client_gfn,
> + uint64_t client_handle,
> + int client_is_gref)
> {
> DECLARE_DOMCTL;
> struct xen_domctl_mem_sharing_op *op;
> @@ -100,8 +105,20 @@ int xc_memshr_share(xc_interface *xch,
> op = &(domctl.u.mem_sharing_op);
> op->op = XEN_DOMCTL_MEM_EVENT_OP_SHARING_SHARE;
> op->u.share.source_handle = source_handle;
> + op->u.share.client_domain = (uint64_t) client_domain;
Why this disconnect between xc and hypercall API?
> op->u.share.client_handle = client_handle;
>
> + if (source_is_gref)
> + XEN_DOMCTL_MEM_SHARING_FIELD_MAKE_GREF(
Is there an outstanding patch somewhere which adds this macro? I don't
see it in staging/xen-unstable.hg. Ah, yes, found it in another mail:
+#define XEN_DOMCTL_MEM_SHARING_FIELD_MAKE_GREF(field, val) \
+ (field) = (XEN_DOMCTL_MEM_SHARING_FIELD_IS_GREF_FLAG | val)
Since this just adds a flag to the gfn why not just expose that to the
caller of this function rather than adding a separate flag as a
parameter?
> + op->u.share.source_gfn, source_gfn);
> + else
> + op->u.share.source_gfn = source_gfn;
> + if (client_is_gref)
> + XEN_DOMCTL_MEM_SHARING_FIELD_MAKE_GREF(
> + op->u.share.client_gfn, client_gfn);
> + else
> + op->u.share.client_gfn = client_gfn;
> +
> return do_domctl(xch, &domctl);
> }
>
> diff -r 6f58de995103 -r 892049dfc1c9 tools/libxc/xenctrl.h
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1893,8 +1893,13 @@ int xc_memshr_nominate_gref(xc_interface
> uint64_t *handle);
> int xc_memshr_share(xc_interface *xch,
> uint32_t source_domain,
> + uint64_t source_gfn,
> uint64_t source_handle,
> - uint64_t client_handle);
> + int source_is_gref,
> + uint32_t client_domain,
> + uint64_t client_gfn,
> + uint64_t client_handle,
> + int dest_is_gref);
> int xc_memshr_domain_resume(xc_interface *xch,
> uint32_t domid);
> int xc_memshr_debug_gfn(xc_interface *xch,
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3 of 8] Tools: Update memshr tool to use new sharing API
2011-12-09 23:15 [PATCH 0 of 8] Tools: Memory sharing interface updates Andres Lagar-Cavilla
2011-12-09 23:15 ` [PATCH 1 of 8] Tools: Do not assume sharing target is dom0 in libxc wrappers Andres Lagar-Cavilla
2011-12-09 23:15 ` [PATCH 2 of 8] Tools: Update libxc mem sharing interface Andres Lagar-Cavilla
@ 2011-12-09 23:15 ` Andres Lagar-Cavilla
2011-12-12 10:13 ` Ian Campbell
2011-12-12 12:14 ` Ian Jackson
2011-12-09 23:15 ` [PATCH 4 of 8] Tools: Add a sharing command to xl for information about shared pages Andres Lagar-Cavilla
` (4 subsequent siblings)
7 siblings, 2 replies; 24+ messages in thread
From: Andres Lagar-Cavilla @ 2011-12-09 23:15 UTC (permalink / raw)
To: xen-devel; +Cc: ian.jackson, andres, tim, ian.campbell, adin
tools/blktap2/drivers/Makefile | 2 +-
tools/blktap2/drivers/tapdisk-image.c | 2 +-
tools/blktap2/drivers/tapdisk-vbd.c | 6 +++---
tools/blktap2/drivers/tapdisk.h | 6 +++++-
tools/memshr/bidir-daemon.c | 34 ++++++++++++++++++++++++----------
tools/memshr/bidir-hash.h | 13 ++++++++-----
tools/memshr/interface.c | 30 ++++++++++++++++++------------
tools/memshr/memshr.h | 11 +++++++++--
8 files changed, 69 insertions(+), 35 deletions(-)
The only (in-tree, that we know of) consumer of the mem sharing API
is the memshr tool (conditionally linked into blktap2). Update it to
use the new API.
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
diff -r 892049dfc1c9 -r e16f5d2643c9 tools/blktap2/drivers/Makefile
--- a/tools/blktap2/drivers/Makefile
+++ b/tools/blktap2/drivers/Makefile
@@ -43,7 +43,7 @@ MEMSHR_DIR = $(XEN_ROOT)/tools/memshr
MEMSHRLIBS :=
ifeq ($(CONFIG_Linux), __fixme__)
CFLAGS += -DMEMSHR
-MEMSHRLIBS += $(MEMSHR_DIR)/libmemshr.a
+MEMSHRLIBS += -L$(XEN_ROOT)/tools/libxc -lxenctrl $(MEMSHR_DIR)/libmemshr.a
endif
ifeq ($(VHD_STATIC),y)
diff -r 892049dfc1c9 -r e16f5d2643c9 tools/blktap2/drivers/tapdisk-image.c
--- a/tools/blktap2/drivers/tapdisk-image.c
+++ b/tools/blktap2/drivers/tapdisk-image.c
@@ -60,7 +60,7 @@ tapdisk_image_allocate(const char *file,
image->storage = storage;
image->private = private;
#ifdef MEMSHR
- image->memshr_id = memshr_vbd_image_get(file);
+ image->memshr_id = memshr_vbd_image_get((char *)file);
#endif
INIT_LIST_HEAD(&image->next);
diff -r 892049dfc1c9 -r e16f5d2643c9 tools/blktap2/drivers/tapdisk-vbd.c
--- a/tools/blktap2/drivers/tapdisk-vbd.c
+++ b/tools/blktap2/drivers/tapdisk-vbd.c
@@ -1218,14 +1218,14 @@ __tapdisk_vbd_complete_td_request(td_vbd
#ifdef MEMSHR
if (treq.op == TD_OP_READ
&& td_flag_test(image->flags, TD_OPEN_RDONLY)) {
- uint64_t hnd = treq.memshr_hnd;
+ share_tuple_t hnd = treq.memshr_hnd;
uint16_t uid = image->memshr_id;
blkif_request_t *breq = &vreq->req;
uint64_t sec = tapdisk_vbd_breq_get_sector(breq, treq);
int secs = breq->seg[treq.sidx].last_sect -
breq->seg[treq.sidx].first_sect + 1;
- if (hnd != 0)
+ if (hnd.handle != 0)
memshr_vbd_complete_ro_request(hnd, uid,
sec, secs);
}
@@ -1297,7 +1297,7 @@ __tapdisk_vbd_reissue_td_request(td_vbd_
/* Reset memshr handle. This'll prevent
* memshr_vbd_complete_ro_request being called
*/
- treq.memshr_hnd = 0;
+ treq.memshr_hnd.handle = 0;
td_complete_request(treq, 0);
} else
td_queue_read(parent, treq);
diff -r 892049dfc1c9 -r e16f5d2643c9 tools/blktap2/drivers/tapdisk.h
--- a/tools/blktap2/drivers/tapdisk.h
+++ b/tools/blktap2/drivers/tapdisk.h
@@ -64,6 +64,10 @@
#include "tapdisk-log.h"
#include "tapdisk-utils.h"
+#ifdef MEMSHR
+#include "memshr.h"
+#endif
+
#define DPRINTF(_f, _a...) syslog(LOG_INFO, _f, ##_a)
#define EPRINTF(_f, _a...) syslog(LOG_ERR, "tap-err:%s: " _f, __func__, ##_a)
#define PERROR(_f, _a...) EPRINTF(_f ": %s", ##_a, strerror(errno))
@@ -136,7 +140,7 @@ struct td_request {
void *private;
#ifdef MEMSHR
- uint64_t memshr_hnd;
+ share_tuple_t memshr_hnd;
#endif
};
diff -r 892049dfc1c9 -r e16f5d2643c9 tools/memshr/bidir-daemon.c
--- a/tools/memshr/bidir-daemon.c
+++ b/tools/memshr/bidir-daemon.c
@@ -19,16 +19,25 @@
#include <pthread.h>
#include <inttypes.h>
#include <unistd.h>
+#include <errno.h>
#include "bidir-hash.h"
#include "memshr-priv.h"
static struct blockshr_hash *blks_hash;
+/* Callback in the iterator, remember this value, and leave */
+int find_one(vbdblk_t k, share_tuple_t v, void *priv)
+{
+ share_tuple_t *rv = (share_tuple_t *) priv;
+ *rv = v;
+ /* Break out of iterator loop */
+ return 1;
+}
+
void* bidir_daemon(void *unused)
{
uint32_t nr_ent, max_nr_ent, tab_size, max_load, min_load;
- static uint64_t shrhnd = 1;
while(1)
{
@@ -41,20 +50,30 @@ void* bidir_daemon(void *unused)
/* Remove some hints as soon as we get to 90% capacity */
if(10 * nr_ent > 9 * max_nr_ent)
{
- uint64_t next_remove = shrhnd;
+ share_tuple_t next_remove;
int to_remove;
int ret;
to_remove = 0.1 * max_nr_ent;
while(to_remove > 0)
{
- ret = blockshr_shrhnd_remove(blks_hash, next_remove, NULL);
- if(ret < 0)
+ /* We use the iterator to get one entry */
+ next_remove.handle = 0;
+ ret = blockshr_hash_iterator(blks_hash, find_one, &next_remove);
+
+ if ( !ret )
+ if ( next_remove.handle == 0 )
+ ret = -ESRCH;
+
+ if ( !ret )
+ ret = blockshr_shrhnd_remove(blks_hash, next_remove, NULL);
+
+ if(ret <= 0)
{
/* We failed to remove an entry, because of a serious hash
* table error */
DPRINTF("Could not remove handle %"PRId64", error: %d\n",
- next_remove, ret);
+ next_remove.handle, ret);
/* Force to exit the loop early */
to_remove = 0;
} else
@@ -62,12 +81,7 @@ void* bidir_daemon(void *unused)
{
/* Managed to remove the entry. Note next_remove not
* incremented, in case there are duplicates */
- shrhnd = next_remove;
to_remove--;
- } else
- {
- /* Failed to remove, because there is no such handle */
- next_remove++;
}
}
}
diff -r 892049dfc1c9 -r e16f5d2643c9 tools/memshr/bidir-hash.h
--- a/tools/memshr/bidir-hash.h
+++ b/tools/memshr/bidir-hash.h
@@ -81,15 +81,16 @@ static int fgprtshr_mfn_cmp(uint32_t m1,
#undef BIDIR_VALUE
#undef BIDIR_KEY_T
#undef BIDIR_VALUE_T
+
/* TODO better hashes! */
static inline uint32_t blockshr_block_hash(vbdblk_t block)
{
return (uint32_t)(block.sec) ^ (uint32_t)(block.disk_id);
}
-static inline uint32_t blockshr_shrhnd_hash(uint64_t shrhnd)
+static inline uint32_t blockshr_shrhnd_hash(share_tuple_t shrhnd)
{
- return (uint32_t)shrhnd;
+ return ((uint32_t) shrhnd.handle);
}
static inline int blockshr_block_cmp(vbdblk_t b1, vbdblk_t b2)
@@ -97,15 +98,17 @@ static inline int blockshr_block_cmp(vbd
return (b1.sec == b2.sec) && (b1.disk_id == b2.disk_id);
}
-static inline int blockshr_shrhnd_cmp(uint64_t h1, uint64_t h2)
+static inline int blockshr_shrhnd_cmp(share_tuple_t h1, share_tuple_t h2)
{
- return (h1 == h2);
+ return ( (h1.domain == h2.domain) &&
+ (h1.frame == h2.frame) &&
+ (h1.handle == h2.handle) );
}
#define BIDIR_NAME_PREFIX blockshr
#define BIDIR_KEY block
#define BIDIR_VALUE shrhnd
#define BIDIR_KEY_T vbdblk_t
-#define BIDIR_VALUE_T uint64_t
+#define BIDIR_VALUE_T share_tuple_t
#include "bidir-namedefs.h"
#endif /* BLOCK_MAP */
diff -r 892049dfc1c9 -r e16f5d2643c9 tools/memshr/interface.c
--- a/tools/memshr/interface.c
+++ b/tools/memshr/interface.c
@@ -145,16 +145,17 @@ void memshr_vbd_image_put(uint16_t memsh
int memshr_vbd_issue_ro_request(char *buf,
grant_ref_t gref,
- uint16_t file_id,
+ uint16_t file_id,
uint64_t sec,
int secs,
- uint64_t *hnd)
+ share_tuple_t *hnd)
{
vbdblk_t blk;
- uint64_t s_hnd, c_hnd;
+ share_tuple_t source_st, client_st;
+ uint64_t c_hnd;
int ret;
- *hnd = 0;
+ *hnd = (share_tuple_t){ 0, 0, 0 };
if(!vbd_info.enabled)
return -1;
@@ -169,26 +170,31 @@ int memshr_vbd_issue_ro_request(char *bu
/* If page couldn't be made sharable, we cannot do anything about it */
if(ret != 0)
return -3;
- *hnd = c_hnd;
+
+ *(&client_st) = (share_tuple_t){ vbd_info.domid, gref, c_hnd };
+ *hnd = client_st;
/* Check if we've read matching disk block previously */
blk.sec = sec;
blk.disk_id = file_id;
- if(blockshr_block_lookup(memshr.blks, blk, &s_hnd) > 0)
+ if(blockshr_block_lookup(memshr.blks, blk, &source_st) > 0)
{
- ret = xc_memshr_share(vbd_info.xc_handle, s_hnd, c_hnd);
+ ret = xc_memshr_share(vbd_info.xc_handle, source_st.domain, source_st.frame,
+ source_st.handle, 1, vbd_info.domid, gref, c_hnd, 1);
if(!ret) return 0;
/* Handles failed to be shared => at least one of them must be invalid,
remove the relevant ones from the map */
switch(ret)
{
case XEN_DOMCTL_MEM_SHARING_S_HANDLE_INVALID:
- ret = blockshr_shrhnd_remove(memshr.blks, s_hnd, NULL);
- if(ret) DPRINTF("Could not rm invl s_hnd: %"PRId64"\n", s_hnd);
+ ret = blockshr_shrhnd_remove(memshr.blks, source_st, NULL);
+ if(ret) DPRINTF("Could not rm invl s_hnd: %u %"PRId64" %"PRId64"\n",
+ source_st.domain, source_st.frame, source_st.handle);
break;
case XEN_DOMCTL_MEM_SHARING_C_HANDLE_INVALID:
- ret = blockshr_shrhnd_remove(memshr.blks, c_hnd, NULL);
- if(ret) DPRINTF("Could not rm invl c_hnd: %"PRId64"\n", c_hnd);
+ ret = blockshr_shrhnd_remove(memshr.blks, client_st, NULL);
+ if(ret) DPRINTF("Could not rm invl c_hnd: %u %"PRId64" %"PRId64"\n",
+ client_st.domain, client_st.frame, client_st.handle);
break;
default:
break;
@@ -199,7 +205,7 @@ int memshr_vbd_issue_ro_request(char *bu
return -4;
}
-void memshr_vbd_complete_ro_request(uint64_t hnd,
+void memshr_vbd_complete_ro_request(share_tuple_t hnd,
uint16_t file_id,
uint64_t sec,
int secs)
diff -r 892049dfc1c9 -r e16f5d2643c9 tools/memshr/memshr.h
--- a/tools/memshr/memshr.h
+++ b/tools/memshr/memshr.h
@@ -25,6 +25,13 @@
typedef uint64_t xen_mfn_t;
+typedef struct share_tuple
+{
+ uint32_t domain;
+ uint64_t frame;
+ uint64_t handle;
+} share_tuple_t;
+
extern void memshr_set_domid(int domid);
extern void memshr_daemon_initialize(void);
extern void memshr_vbd_initialize(void);
@@ -35,9 +42,9 @@ extern int memshr_vbd_issue_ro_request(c
uint16_t file_id,
uint64_t sec,
int secs,
- uint64_t *hnd);
+ share_tuple_t *hnd);
extern void memshr_vbd_complete_ro_request(
- uint64_t hnd,
+ share_tuple_t hnd,
uint16_t file_id,
uint64_t sec,
int secs);
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 3 of 8] Tools: Update memshr tool to use new sharing API
2011-12-09 23:15 ` [PATCH 3 of 8] Tools: Update memshr tool to use new sharing API Andres Lagar-Cavilla
@ 2011-12-12 10:13 ` Ian Campbell
2011-12-13 4:18 ` Andres Lagar-Cavilla
2011-12-12 12:14 ` Ian Jackson
1 sibling, 1 reply; 24+ messages in thread
From: Ian Campbell @ 2011-12-12 10:13 UTC (permalink / raw)
To: Andres Lagar-Cavilla
Cc: Ian Jackson, andres@gridcentric.ca, xen-devel@lists.xensource.com,
Tim (Xen.org), adin@gridcentric.ca
On Fri, 2011-12-09 at 23:15 +0000, Andres Lagar-Cavilla wrote:
> tools/blktap2/drivers/Makefile | 2 +-
> tools/blktap2/drivers/tapdisk-image.c | 2 +-
> tools/blktap2/drivers/tapdisk-vbd.c | 6 +++---
> tools/blktap2/drivers/tapdisk.h | 6 +++++-
> tools/memshr/bidir-daemon.c | 34 ++++++++++++++++++++++++----------
> tools/memshr/bidir-hash.h | 13 ++++++++-----
> tools/memshr/interface.c | 30 ++++++++++++++++++------------
> tools/memshr/memshr.h | 11 +++++++++--
> 8 files changed, 69 insertions(+), 35 deletions(-)
>
>
> The only (in-tree, that we know of) consumer of the mem sharing API
> is the memshr tool (conditionally linked into blktap2). Update it to
> use the new API.
At least some of this must happen in the previous 2 patches so that the
build is not broken.
Is there any benefit to conditionally linking this stuff? Is it lots of
code or does it have some other undesirable impact when built even when
it is quiescent? Seems like a recipe for bit rot to not build it, plus
it makes it harder for people to consume via distro packages etc.
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>
> diff -r 892049dfc1c9 -r e16f5d2643c9 tools/blktap2/drivers/Makefile
> --- a/tools/blktap2/drivers/Makefile
> +++ b/tools/blktap2/drivers/Makefile
> @@ -43,7 +43,7 @@ MEMSHR_DIR = $(XEN_ROOT)/tools/memshr
> MEMSHRLIBS :=
> ifeq ($(CONFIG_Linux), __fixme__)
> CFLAGS += -DMEMSHR
> -MEMSHRLIBS += $(MEMSHR_DIR)/libmemshr.a
> +MEMSHRLIBS += -L$(XEN_ROOT)/tools/libxc -lxenctrl $(MEMSHR_DIR)/libmemshr.a
Please use $(LDLIBS_libxenctrl).
Is there a reason libmemshr is not a shared library?
> endif
>
> ifeq ($(VHD_STATIC),y)
> diff -r 892049dfc1c9 -r e16f5d2643c9 tools/blktap2/drivers/tapdisk-image.c
> --- a/tools/blktap2/drivers/tapdisk-image.c
> +++ b/tools/blktap2/drivers/tapdisk-image.c
> @@ -60,7 +60,7 @@ tapdisk_image_allocate(const char *file,
> image->storage = storage;
> image->private = private;
> #ifdef MEMSHR
> - image->memshr_id = memshr_vbd_image_get(file);
> + image->memshr_id = memshr_vbd_image_get((char *)file);
Casting away the const here raises a big red flag. Why is this
necessary? You should either fix the API of memshr_vbd_image_get or
tapdisk_image_allocate IMHO and propagate the change as necessary.
AFAICT there is no reason not to push the const'ness down into the
memshr API.
> #endif
> INIT_LIST_HEAD(&image->next);
>
[...]
> diff -r 892049dfc1c9 -r e16f5d2643c9 tools/memshr/bidir-hash.h
> --- a/tools/memshr/bidir-hash.h
> +++ b/tools/memshr/bidir-hash.h
> @@ -81,15 +81,16 @@ static int fgprtshr_mfn_cmp(uint32_t m1,
> #undef BIDIR_VALUE
> #undef BIDIR_KEY_T
> #undef BIDIR_VALUE_T
> +
> /* TODO better hashes! */
> static inline uint32_t blockshr_block_hash(vbdblk_t block)
> {
> return (uint32_t)(block.sec) ^ (uint32_t)(block.disk_id);
> }
>
> -static inline uint32_t blockshr_shrhnd_hash(uint64_t shrhnd)
> +static inline uint32_t blockshr_shrhnd_hash(share_tuple_t shrhnd)
> {
> - return (uint32_t)shrhnd;
> + return ((uint32_t) shrhnd.handle);
> }
>
> static inline int blockshr_block_cmp(vbdblk_t b1, vbdblk_t b2)
> @@ -97,15 +98,17 @@ static inline int blockshr_block_cmp(vbd
> return (b1.sec == b2.sec) && (b1.disk_id == b2.disk_id);
> }
>
> -static inline int blockshr_shrhnd_cmp(uint64_t h1, uint64_t h2)
> +static inline int blockshr_shrhnd_cmp(share_tuple_t h1, share_tuple_t h2)
> {
> - return (h1 == h2);
> + return ( (h1.domain == h2.domain) &&
> + (h1.frame == h2.frame) &&
> + (h1.handle == h2.handle) );
memcmp?
> }
> #define BIDIR_NAME_PREFIX blockshr
> #define BIDIR_KEY block
> #define BIDIR_VALUE shrhnd
> #define BIDIR_KEY_T vbdblk_t
> -#define BIDIR_VALUE_T uint64_t
> +#define BIDIR_VALUE_T share_tuple_t
> #include "bidir-namedefs.h"
>
> #endif /* BLOCK_MAP */
> diff -r 892049dfc1c9 -r e16f5d2643c9 tools/memshr/interface.c
> --- a/tools/memshr/interface.c
> +++ b/tools/memshr/interface.c
> @@ -145,16 +145,17 @@ void memshr_vbd_image_put(uint16_t memsh
>
> int memshr_vbd_issue_ro_request(char *buf,
> grant_ref_t gref,
> - uint16_t file_id,
> + uint16_t file_id,
> uint64_t sec,
> int secs,
> - uint64_t *hnd)
> + share_tuple_t *hnd)
> {
> vbdblk_t blk;
> - uint64_t s_hnd, c_hnd;
> + share_tuple_t source_st, client_st;
> + uint64_t c_hnd;
> int ret;
>
> - *hnd = 0;
> + *hnd = (share_tuple_t){ 0, 0, 0 };
> if(!vbd_info.enabled)
> return -1;
>
> @@ -169,26 +170,31 @@ int memshr_vbd_issue_ro_request(char *bu
> /* If page couldn't be made sharable, we cannot do anything about it */
> if(ret != 0)
> return -3;
> - *hnd = c_hnd;
> +
> + *(&client_st) = (share_tuple_t){ vbd_info.domid, gref, c_hnd };
Is "*(&thing)" somehow different to "thing"?
> + *hnd = client_st;
>
> /* Check if we've read matching disk block previously */
> blk.sec = sec;
> blk.disk_id = file_id;
> - if(blockshr_block_lookup(memshr.blks, blk, &s_hnd) > 0)
> + if(blockshr_block_lookup(memshr.blks, blk, &source_st) > 0)
> {
> - ret = xc_memshr_share(vbd_info.xc_handle, s_hnd, c_hnd);
> + ret = xc_memshr_share(vbd_info.xc_handle, source_st.domain, source_st.frame,
> + source_st.handle, 1, vbd_info.domid, gref, c_hnd, 1);
These 1's == is a gref but I only know that because I was just reading
that patch. If you decide you do need to keep the flag separate then
this would be better as a flags argument with named values
XC_MEMSHR_SOURCE_IS_GREF etc.
> diff -r 892049dfc1c9 -r e16f5d2643c9 tools/memshr/memshr.h
> --- a/tools/memshr/memshr.h
> +++ b/tools/memshr/memshr.h
> @@ -25,6 +25,13 @@
>
> typedef uint64_t xen_mfn_t;
>
> +typedef struct share_tuple
> +{
> + uint32_t domain;
> + uint64_t frame;
> + uint64_t handle;
> +} share_tuple_t;
Didn't you just add these as three separate parameters to
xc_memshr_share? Seems like this could be xc_memshr_tuple (or some
better name with more meaning than "tuple") and used throughout.
Ian.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 3 of 8] Tools: Update memshr tool to use new sharing API
2011-12-12 10:13 ` Ian Campbell
@ 2011-12-13 4:18 ` Andres Lagar-Cavilla
0 siblings, 0 replies; 24+ messages in thread
From: Andres Lagar-Cavilla @ 2011-12-13 4:18 UTC (permalink / raw)
To: Ian Campbell
Cc: Ian Jackson, andres@gridcentric.ca, xen-devel@lists.xensource.com,
Tim (Xen.org), adin@gridcentric.ca
> On Fri, 2011-12-09 at 23:15 +0000, Andres Lagar-Cavilla wrote:
>> tools/blktap2/drivers/Makefile | 2 +-
>> tools/blktap2/drivers/tapdisk-image.c | 2 +-
>> tools/blktap2/drivers/tapdisk-vbd.c | 6 +++---
>> tools/blktap2/drivers/tapdisk.h | 6 +++++-
>> tools/memshr/bidir-daemon.c | 34
>> ++++++++++++++++++++++++----------
>> tools/memshr/bidir-hash.h | 13 ++++++++-----
>> tools/memshr/interface.c | 30
>> ++++++++++++++++++------------
>> tools/memshr/memshr.h | 11 +++++++++--
>> 8 files changed, 69 insertions(+), 35 deletions(-)
>>
>>
>> The only (in-tree, that we know of) consumer of the mem sharing API
>> is the memshr tool (conditionally linked into blktap2). Update it to
>> use the new API.
>
> At least some of this must happen in the previous 2 patches so that the
> build is not broken.
>
> Is there any benefit to conditionally linking this stuff? Is it lots of
> code or does it have some other undesirable impact when built even when
> it is quiescent? Seems like a recipe for bit rot to not build it, plus
> it makes it harder for people to consume via distro packages etc.
Let me clarify this a bit: our interest in memshr is in not breaking it
with our changes. We don't know why it's built that way. It was like that
wen we arrived. We don't change that. We certainly don't intend to own it.
I would frame line 44 at
http://xenbits.xensource.com/hg/xen-unstable.hg/file/1c58bb664d8d/tools/blktap2/drivers/Makefile
as the definition of bit rot
Andres
>
>> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>>
>> diff -r 892049dfc1c9 -r e16f5d2643c9 tools/blktap2/drivers/Makefile
>> --- a/tools/blktap2/drivers/Makefile
>> +++ b/tools/blktap2/drivers/Makefile
>> @@ -43,7 +43,7 @@ MEMSHR_DIR = $(XEN_ROOT)/tools/memshr
>> MEMSHRLIBS :=
>> ifeq ($(CONFIG_Linux), __fixme__)
>> CFLAGS += -DMEMSHR
>> -MEMSHRLIBS += $(MEMSHR_DIR)/libmemshr.a
>> +MEMSHRLIBS += -L$(XEN_ROOT)/tools/libxc -lxenctrl
>> $(MEMSHR_DIR)/libmemshr.a
>
> Please use $(LDLIBS_libxenctrl).
>
> Is there a reason libmemshr is not a shared library?
>
>> endif
>>
>> ifeq ($(VHD_STATIC),y)
>> diff -r 892049dfc1c9 -r e16f5d2643c9
>> tools/blktap2/drivers/tapdisk-image.c
>> --- a/tools/blktap2/drivers/tapdisk-image.c
>> +++ b/tools/blktap2/drivers/tapdisk-image.c
>> @@ -60,7 +60,7 @@ tapdisk_image_allocate(const char *file,
>> image->storage = storage;
>> image->private = private;
>> #ifdef MEMSHR
>> - image->memshr_id = memshr_vbd_image_get(file);
>> + image->memshr_id = memshr_vbd_image_get((char *)file);
>
> Casting away the const here raises a big red flag. Why is this
> necessary? You should either fix the API of memshr_vbd_image_get or
> tapdisk_image_allocate IMHO and propagate the change as necessary.
> AFAICT there is no reason not to push the const'ness down into the
> memshr API.
>
>> #endif
>> INIT_LIST_HEAD(&image->next);
>>
> [...]
>
>> diff -r 892049dfc1c9 -r e16f5d2643c9 tools/memshr/bidir-hash.h
>> --- a/tools/memshr/bidir-hash.h
>> +++ b/tools/memshr/bidir-hash.h
>> @@ -81,15 +81,16 @@ static int fgprtshr_mfn_cmp(uint32_t m1,
>> #undef BIDIR_VALUE
>> #undef BIDIR_KEY_T
>> #undef BIDIR_VALUE_T
>> +
>> /* TODO better hashes! */
>> static inline uint32_t blockshr_block_hash(vbdblk_t block)
>> {
>> return (uint32_t)(block.sec) ^ (uint32_t)(block.disk_id);
>> }
>>
>> -static inline uint32_t blockshr_shrhnd_hash(uint64_t shrhnd)
>> +static inline uint32_t blockshr_shrhnd_hash(share_tuple_t shrhnd)
>> {
>> - return (uint32_t)shrhnd;
>> + return ((uint32_t) shrhnd.handle);
>> }
>>
>> static inline int blockshr_block_cmp(vbdblk_t b1, vbdblk_t b2)
>> @@ -97,15 +98,17 @@ static inline int blockshr_block_cmp(vbd
>> return (b1.sec == b2.sec) && (b1.disk_id == b2.disk_id);
>> }
>>
>> -static inline int blockshr_shrhnd_cmp(uint64_t h1, uint64_t h2)
>> +static inline int blockshr_shrhnd_cmp(share_tuple_t h1, share_tuple_t
>> h2)
>> {
>> - return (h1 == h2);
>> + return ( (h1.domain == h2.domain) &&
>> + (h1.frame == h2.frame) &&
>> + (h1.handle == h2.handle) );
>
> memcmp?
>
>> }
>> #define BIDIR_NAME_PREFIX blockshr
>> #define BIDIR_KEY block
>> #define BIDIR_VALUE shrhnd
>> #define BIDIR_KEY_T vbdblk_t
>> -#define BIDIR_VALUE_T uint64_t
>> +#define BIDIR_VALUE_T share_tuple_t
>> #include "bidir-namedefs.h"
>>
>> #endif /* BLOCK_MAP */
>> diff -r 892049dfc1c9 -r e16f5d2643c9 tools/memshr/interface.c
>> --- a/tools/memshr/interface.c
>> +++ b/tools/memshr/interface.c
>> @@ -145,16 +145,17 @@ void memshr_vbd_image_put(uint16_t memsh
>>
>> int memshr_vbd_issue_ro_request(char *buf,
>> grant_ref_t gref,
>> - uint16_t file_id,
>> + uint16_t file_id,
>> uint64_t sec,
>> int secs,
>> - uint64_t *hnd)
>> + share_tuple_t *hnd)
>> {
>> vbdblk_t blk;
>> - uint64_t s_hnd, c_hnd;
>> + share_tuple_t source_st, client_st;
>> + uint64_t c_hnd;
>> int ret;
>>
>> - *hnd = 0;
>> + *hnd = (share_tuple_t){ 0, 0, 0 };
>> if(!vbd_info.enabled)
>> return -1;
>>
>> @@ -169,26 +170,31 @@ int memshr_vbd_issue_ro_request(char *bu
>> /* If page couldn't be made sharable, we cannot do anything about
>> it */
>> if(ret != 0)
>> return -3;
>> - *hnd = c_hnd;
>> +
>> + *(&client_st) = (share_tuple_t){ vbd_info.domid, gref, c_hnd };
>
> Is "*(&thing)" somehow different to "thing"?
>
>> + *hnd = client_st;
>>
>> /* Check if we've read matching disk block previously */
>> blk.sec = sec;
>> blk.disk_id = file_id;
>> - if(blockshr_block_lookup(memshr.blks, blk, &s_hnd) > 0)
>> + if(blockshr_block_lookup(memshr.blks, blk, &source_st) > 0)
>> {
>> - ret = xc_memshr_share(vbd_info.xc_handle, s_hnd, c_hnd);
>> + ret = xc_memshr_share(vbd_info.xc_handle, source_st.domain,
>> source_st.frame,
>> + source_st.handle, 1, vbd_info.domid,
>> gref, c_hnd, 1);
>
> These 1's == is a gref but I only know that because I was just reading
> that patch. If you decide you do need to keep the flag separate then
> this would be better as a flags argument with named values
> XC_MEMSHR_SOURCE_IS_GREF etc.
>
>> diff -r 892049dfc1c9 -r e16f5d2643c9 tools/memshr/memshr.h
>> --- a/tools/memshr/memshr.h
>> +++ b/tools/memshr/memshr.h
>> @@ -25,6 +25,13 @@
>>
>> typedef uint64_t xen_mfn_t;
>>
>> +typedef struct share_tuple
>> +{
>> + uint32_t domain;
>> + uint64_t frame;
>> + uint64_t handle;
>> +} share_tuple_t;
>
> Didn't you just add these as three separate parameters to
> xc_memshr_share? Seems like this could be xc_memshr_tuple (or some
> better name with more meaning than "tuple") and used throughout.
>
> Ian.
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3 of 8] Tools: Update memshr tool to use new sharing API
2011-12-09 23:15 ` [PATCH 3 of 8] Tools: Update memshr tool to use new sharing API Andres Lagar-Cavilla
2011-12-12 10:13 ` Ian Campbell
@ 2011-12-12 12:14 ` Ian Jackson
1 sibling, 0 replies; 24+ messages in thread
From: Ian Jackson @ 2011-12-12 12:14 UTC (permalink / raw)
To: Andres Lagar-Cavilla
Cc: andres@gridcentric.ca, xen-devel@lists.xensource.com,
Tim (Xen.org), Ian Campbell, adin@gridcentric.ca
Andres Lagar-Cavilla writes ("[PATCH 3 of 8] Tools: Update memshr tool to use new sharing API"):
> #ifdef MEMSHR
> - image->memshr_id = memshr_vbd_image_get(file);
> + image->memshr_id = memshr_vbd_image_get((char *)file);
This case would be unnecessary if memshr_vbd_image_get was
const-correct, which I think would be better.
Ian.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4 of 8] Tools: Add a sharing command to xl for information about shared pages
2011-12-09 23:15 [PATCH 0 of 8] Tools: Memory sharing interface updates Andres Lagar-Cavilla
` (2 preceding siblings ...)
2011-12-09 23:15 ` [PATCH 3 of 8] Tools: Update memshr tool to use new sharing API Andres Lagar-Cavilla
@ 2011-12-09 23:15 ` Andres Lagar-Cavilla
2011-12-12 10:26 ` Ian Campbell
2011-12-09 23:15 ` [PATCH 5 of 8] Tools: Expose to libxc the total number of shared frames and space saved Andres Lagar-Cavilla
` (3 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Andres Lagar-Cavilla @ 2011-12-09 23:15 UTC (permalink / raw)
To: xen-devel; +Cc: ian.jackson, andres, tim, ian.campbell, adin
docs/man/xl.pod.1 | 18 +++++++++
tools/libxl/xl.h | 1 +
tools/libxl/xl_cmdimpl.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++
tools/libxl/xl_cmdtable.c | 6 +++
4 files changed, 110 insertions(+), 0 deletions(-)
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Signed-off-by: Adin Scannell <adin@scannell.ca>
diff -r e16f5d2643c9 -r a22140d92931 docs/man/xl.pod.1
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -280,6 +280,24 @@ B<Warning:> There is no good way to know
mem-set will make a domain unstable and cause it to crash. Be very
careful when using this command on running domains.
+=item B<sharing> [I<-t> I<--totals>] [I<domain-id>]
+
+List count of shared pages.
+
+B<OPTIONS>
+
+=over 4
+
+=item I<domain_id>
+
+List specifically for that domain. Otherwise, list for all domains.
+
+=item I<-t>, I<--totals>
+
+Also print global count of shared pages for this host.
+
+=back
+
=item B<migrate> [I<OPTIONS>] I<domain-id> I<host>
Migrate a domain to another host machine. By default B<xl> relies on ssh as a
diff -r e16f5d2643c9 -r a22140d92931 tools/libxl/xl.h
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -28,6 +28,7 @@ struct cmd_spec {
int main_vcpulist(int argc, char **argv);
int main_info(int argc, char **argv);
+int main_sharing(int argc, char **argv);
int main_cd_eject(int argc, char **argv);
int main_cd_insert(int argc, char **argv);
int main_console(int argc, char **argv);
diff -r e16f5d2643c9 -r a22140d92931 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3755,6 +3755,91 @@ int main_info(int argc, char **argv)
return 0;
}
+static void sharing(int totals, const libxl_dominfo *info, int nb_domain)
+{
+ int i;
+
+ printf("Name ID Mem Shared\n");
+
+ for (i = 0; i < nb_domain; i++) {
+ char *domname;
+ unsigned shutdown_reason;
+ domname = libxl_domid_to_name(ctx, info[i].domid);
+ shutdown_reason = info[i].shutdown ? info[i].shutdown_reason : 0;
+ printf("%-40s %5d %5lu %5lu\n",
+ domname,
+ info[i].domid,
+ (unsigned long) (info[i].current_memkb / 1024),
+ (unsigned long) (info[i].shared_memkb / 1024));
+ free(domname);
+ }
+
+ if (totals)
+ {
+ /* To be added with a future patch. */
+ }
+}
+
+int main_sharing(int argc, char **argv)
+{
+ int opt;
+ int option_index = 0;
+ static struct option long_options[] = {
+ {"help", 0, 0, 'h'},
+ {"totals", 0, 0, 't'},
+ {0, 0, 0, 0}
+ };
+ int totals = 0;
+
+ libxl_dominfo info_buf;
+ libxl_dominfo *info, *info_free=0;
+ int nb_domain, rc;
+
+ while ((opt = getopt_long(argc, argv, "ht", long_options, &option_index)) != -1) {
+ switch (opt) {
+ case 'h':
+ help("sharing");
+ return 0;
+ case 't':
+ totals = 1;
+ break;
+ default:
+ fprintf(stderr, "option `%c' not supported.\n", optopt);
+ break;
+ }
+ }
+
+ if (optind >= argc) {
+ info = libxl_list_domain(ctx, &nb_domain);
+ if (!info) {
+ fprintf(stderr, "libxl_domain_infolist failed.\n");
+ return 1;
+ }
+ info_free = info;
+ } else if (optind == argc-1) {
+ find_domain(argv[optind]);
+ rc = libxl_domain_info(ctx, &info_buf, domid);
+ if (rc == ERROR_INVAL) {
+ fprintf(stderr, "Error: Domain \'%s\' does not exist.\n",
+ argv[optind]);
+ return -rc;
+ }
+ if (rc) {
+ fprintf(stderr, "libxl_domain_info failed (code %d).\n", rc);
+ return -rc;
+ }
+ info = &info_buf;
+ nb_domain = 1;
+ } else {
+ help("sharing");
+ return 2;
+ }
+
+ sharing(totals, info, nb_domain);
+
+ return 0;
+}
+
static int sched_credit_domain_get(
int domid, libxl_sched_credit *scinfo)
{
diff -r e16f5d2643c9 -r a22140d92931 tools/libxl/xl_cmdtable.c
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -189,6 +189,12 @@ struct cmd_spec cmd_table[] = {
"Get information about Xen host",
"-n, --numa List host NUMA topology information",
},
+ { "sharing",
+ &main_sharing, 0,
+ "Get information about page sharing",
+ "[options] [Domain]",
+ "-t, --totals Include host totals in the output",
+ },
{ "sched-credit",
&main_sched_credit, 0,
"Get/set credit scheduler parameters",
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4 of 8] Tools: Add a sharing command to xl for information about shared pages
2011-12-09 23:15 ` [PATCH 4 of 8] Tools: Add a sharing command to xl for information about shared pages Andres Lagar-Cavilla
@ 2011-12-12 10:26 ` Ian Campbell
0 siblings, 0 replies; 24+ messages in thread
From: Ian Campbell @ 2011-12-12 10:26 UTC (permalink / raw)
To: Andres Lagar-Cavilla
Cc: Ian Jackson, andres@gridcentric.ca, xen-devel@lists.xensource.com,
Tim (Xen.org), adin@gridcentric.ca
On Fri, 2011-12-09 at 23:15 +0000, Andres Lagar-Cavilla wrote:
> docs/man/xl.pod.1 | 18 +++++++++
> tools/libxl/xl.h | 1 +
> tools/libxl/xl_cmdimpl.c | 85 +++++++++++++++++++++++++++++++++++++++++++++++
> tools/libxl/xl_cmdtable.c | 6 +++
> 4 files changed, 110 insertions(+), 0 deletions(-)
>
>
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
> Signed-off-by: Adin Scannell <adin@scannell.ca>
>
> diff -r e16f5d2643c9 -r a22140d92931 docs/man/xl.pod.1
> --- a/docs/man/xl.pod.1
> +++ b/docs/man/xl.pod.1
> @@ -280,6 +280,24 @@ B<Warning:> There is no good way to know
> mem-set will make a domain unstable and cause it to crash. Be very
> careful when using this command on running domains.
>
> +=item B<sharing> [I<-t> I<--totals>] [I<domain-id>]
[...]
> =item B<migrate> [I<OPTIONS>] I<domain-id> I<host>
The commands are (supposed to be) alphabetical within each section.
[...]
> diff -r e16f5d2643c9 -r a22140d92931 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -3755,6 +3755,91 @@ int main_info(int argc, char **argv)
[...]
> +int main_sharing(int argc, char **argv)
> +{
> + int opt;
> + int option_index = 0;
> + static struct option long_options[] = {
> + {"help", 0, 0, 'h'},
> + {"totals", 0, 0, 't'},
> + {0, 0, 0, 0}
> + };
> + int totals = 0;
> +
> + libxl_dominfo info_buf;
> + libxl_dominfo *info, *info_free=0;
> + int nb_domain, rc;
> +
> + while ((opt = getopt_long(argc, argv, "ht", long_options, &option_index)) != -1) {
Please use def_getopt().
Ian.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 5 of 8] Tools: Expose to libxc the total number of shared frames and space saved
2011-12-09 23:15 [PATCH 0 of 8] Tools: Memory sharing interface updates Andres Lagar-Cavilla
` (3 preceding siblings ...)
2011-12-09 23:15 ` [PATCH 4 of 8] Tools: Add a sharing command to xl for information about shared pages Andres Lagar-Cavilla
@ 2011-12-09 23:15 ` Andres Lagar-Cavilla
2011-12-12 10:39 ` Ian Campbell
2011-12-09 23:15 ` [PATCH 6 of 8] Tools: Allow libxl/xl to expose " Andres Lagar-Cavilla
` (2 subsequent siblings)
7 siblings, 1 reply; 24+ messages in thread
From: Andres Lagar-Cavilla @ 2011-12-09 23:15 UTC (permalink / raw)
To: xen-devel; +Cc: ian.jackson, andres, tim, ian.campbell, adin
tools/libxc/xc_private.c | 10 ++++++++++
tools/libxc/xenctrl.h | 4 ++++
2 files changed, 14 insertions(+), 0 deletions(-)
Signed-off-by: Adin Scannell <adin@scannell.ca>
diff -r a22140d92931 -r 6e8b8fb2c8b2 tools/libxc/xc_private.c
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -533,6 +533,16 @@ long xc_maximum_ram_page(xc_interface *x
return do_memory_op(xch, XENMEM_maximum_ram_page, NULL, 0);
}
+long xc_sharing_freed_pages(xc_interface *xch)
+{
+ return do_memory_op(xch, XENMEM_get_sharing_freed_pages, NULL, 0);
+}
+
+long xc_sharing_used_frames(xc_interface *xch)
+{
+ return do_memory_op(xch, XENMEM_get_sharing_shared_pages, NULL, 0);
+}
+
long long xc_domain_get_cpu_usage( xc_interface *xch, domid_t domid, int vcpu )
{
DECLARE_DOMCTL;
diff -r a22140d92931 -r 6e8b8fb2c8b2 tools/libxc/xenctrl.h
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1227,6 +1227,10 @@ int xc_mmuext_op(xc_interface *xch, stru
/* System wide memory properties */
long xc_maximum_ram_page(xc_interface *xch);
+long xc_sharing_freed_pages(xc_interface *xch);
+
+long xc_sharing_used_frames(xc_interface *xch);
+
/* Get current total pages allocated to a domain. */
long xc_get_tot_pages(xc_interface *xch, uint32_t domid);
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 5 of 8] Tools: Expose to libxc the total number of shared frames and space saved
2011-12-09 23:15 ` [PATCH 5 of 8] Tools: Expose to libxc the total number of shared frames and space saved Andres Lagar-Cavilla
@ 2011-12-12 10:39 ` Ian Campbell
2011-12-13 3:43 ` Andres Lagar-Cavilla
0 siblings, 1 reply; 24+ messages in thread
From: Ian Campbell @ 2011-12-12 10:39 UTC (permalink / raw)
To: Andres Lagar-Cavilla
Cc: Ian Jackson, andres@gridcentric.ca, xen-devel@lists.xensource.com,
Tim (Xen.org), adin@gridcentric.ca
On Fri, 2011-12-09 at 23:15 +0000, Andres Lagar-Cavilla wrote:
> tools/libxc/xc_private.c | 10 ++++++++++
> tools/libxc/xenctrl.h | 4 ++++
> 2 files changed, 14 insertions(+), 0 deletions(-)
>
>
> Signed-off-by: Adin Scannell <adin@scannell.ca>
If you pass on someone elses work I think it is normal to add your own
Signed-off-by (per DCO clause (c)).
[...]
> diff -r a22140d92931 -r 6e8b8fb2c8b2 tools/libxc/xenctrl.h
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1227,6 +1227,10 @@ int xc_mmuext_op(xc_interface *xch, stru
> /* System wide memory properties */
> long xc_maximum_ram_page(xc_interface *xch);
>
> +long xc_sharing_freed_pages(xc_interface *xch);
> +
> +long xc_sharing_used_frames(xc_interface *xch);
A quick comment on what each function returns might be worthwhile, maybe
it's obvious to someone who knows the sharing stuff, but e.g. does
"used" mean used right now or does it mean have ever been used? I
suppose "freed" must be historical.
Also why pages vs frames? The underlying domctl is always pages.
Ian.
> +
> /* Get current total pages allocated to a domain. */
> long xc_get_tot_pages(xc_interface *xch, uint32_t domid);
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5 of 8] Tools: Expose to libxc the total number of shared frames and space saved
2011-12-12 10:39 ` Ian Campbell
@ 2011-12-13 3:43 ` Andres Lagar-Cavilla
2011-12-13 17:08 ` Adin Scannell
0 siblings, 1 reply; 24+ messages in thread
From: Andres Lagar-Cavilla @ 2011-12-13 3:43 UTC (permalink / raw)
To: Ian Campbell
Cc: Ian Jackson, andres@gridcentric.ca, xen-devel@lists.xensource.com,
Tim (Xen.org), adin@gridcentric.ca
> On Fri, 2011-12-09 at 23:15 +0000, Andres Lagar-Cavilla wrote:
>> tools/libxc/xc_private.c | 10 ++++++++++
>> tools/libxc/xenctrl.h | 4 ++++
>> 2 files changed, 14 insertions(+), 0 deletions(-)
>>
>>
>> Signed-off-by: Adin Scannell <adin@scannell.ca>
>
> If you pass on someone elses work I think it is normal to add your own
> Signed-off-by (per DCO clause (c)).
>
> [...]
>> diff -r a22140d92931 -r 6e8b8fb2c8b2 tools/libxc/xenctrl.h
>> --- a/tools/libxc/xenctrl.h
>> +++ b/tools/libxc/xenctrl.h
>> @@ -1227,6 +1227,10 @@ int xc_mmuext_op(xc_interface *xch, stru
>> /* System wide memory properties */
>> long xc_maximum_ram_page(xc_interface *xch);
>>
>> +long xc_sharing_freed_pages(xc_interface *xch);
>> +
>> +long xc_sharing_used_frames(xc_interface *xch);
We'll comment, certainly. For the record:
used_frames indicates the number of frames of memory that are shared,
each, by two or more domains. We use the term "frame" because it's
commonly used in textbooks for MMUs, in the sense that "a frame hosts a
page" or "a page maps to a frame". Thus our use of shared frames,
representing a different page (albeit with identical contents) to
different domains.
freed_pages indicates the number of individual pages freed as a result of
sharing. These are pages, one per domain.
Andres
>
> A quick comment on what each function returns might be worthwhile, maybe
> it's obvious to someone who knows the sharing stuff, but e.g. does
> "used" mean used right now or does it mean have ever been used? I
> suppose "freed" must be historical.
>
> Also why pages vs frames? The underlying domctl is always pages.
>
> Ian.
>
>> +
>> /* Get current total pages allocated to a domain. */
>> long xc_get_tot_pages(xc_interface *xch, uint32_t domid);
>>
>
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5 of 8] Tools: Expose to libxc the total number of shared frames and space saved
2011-12-13 3:43 ` Andres Lagar-Cavilla
@ 2011-12-13 17:08 ` Adin Scannell
2011-12-14 10:47 ` Ian Campbell
0 siblings, 1 reply; 24+ messages in thread
From: Adin Scannell @ 2011-12-13 17:08 UTC (permalink / raw)
To: andres
Cc: Ian Jackson, andres@gridcentric.ca, xen-devel@lists.xensource.com,
Tim (Xen.org), Ian Campbell
On Mon, Dec 12, 2011 at 10:43 PM, Andres Lagar-Cavilla
<andres@lagarcavilla.org> wrote:
> used_frames indicates the number of frames of memory that are shared,
> each, by two or more domains. We use the term "frame" because it's
> commonly used in textbooks for MMUs, in the sense that "a frame hosts a
> page" or "a page maps to a frame". Thus our use of shared frames,
> representing a different page (albeit with identical contents) to
> different domains.
As Andres has pointed out, I named it frames to clearly differentiate
it from other page-based domctls (it worked!). With the unusual
condition of multiple domains pointing to the same frame, the
nomenclature gets a bit hairy. Any suggestions on this front?
> freed_pages indicates the number of individual pages freed as a result of
> sharing. These are pages, one per domain.
Here's a quick explanation and reason for these stats:
The way I see it, there are three easy-to-track, non-historical stats
about sharing.
X - The total number of domain pages that map to shared frames.
Y - The number of frames saved with sharing.
Z - The total number of shared frames (i.e. pages owned by dom_cow).
Any two of these will give you the third (via X = Y + Z). In this
case, we used Y & Z because I think that they are the simplest to
reason about (and therefore the most likely to be wanted). An
argument could easily be made for others though, if someone feels
strongly.
There are certainly some historical stats (# of unshares, etc.) and
lots of per-page/frame stats that would be of interest as well. But,
bit by bit...
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5 of 8] Tools: Expose to libxc the total number of shared frames and space saved
2011-12-13 17:08 ` Adin Scannell
@ 2011-12-14 10:47 ` Ian Campbell
0 siblings, 0 replies; 24+ messages in thread
From: Ian Campbell @ 2011-12-14 10:47 UTC (permalink / raw)
To: Adin Scannell
Cc: Ian Jackson, andres@gridcentric.ca, xen-devel@lists.xensource.com,
Tim (Xen.org), andres@lagarcavilla.org
On Tue, 2011-12-13 at 17:08 +0000, Adin Scannell wrote:
> On Mon, Dec 12, 2011 at 10:43 PM, Andres Lagar-Cavilla
> <andres@lagarcavilla.org> wrote:
> > used_frames indicates the number of frames of memory that are shared,
> > each, by two or more domains. We use the term "frame" because it's
> > commonly used in textbooks for MMUs, in the sense that "a frame hosts a
> > page" or "a page maps to a frame". Thus our use of shared frames,
> > representing a different page (albeit with identical contents) to
> > different domains.
>
> As Andres has pointed out, I named it frames to clearly differentiate
> it from other page-based domctls (it worked!). With the unusual
> condition of multiple domains pointing to the same frame, the
> nomenclature gets a bit hairy. Any suggestions on this front?
Nothing useful, your explanation makes sense and I can't think of a
better way of putting it.
It would be good if you described these semantics in the section of
xl(1) dealing with whichever command displays them (perhaps xl info if
you add the numbers to physinfo?).
> > freed_pages indicates the number of individual pages freed as a result of
> > sharing. These are pages, one per domain.
>
> Here's a quick explanation and reason for these stats:
>
> The way I see it, there are three easy-to-track, non-historical stats
> about sharing.
> X - The total number of domain pages that map to shared frames.
> Y - The number of frames saved with sharing.
> Z - The total number of shared frames (i.e. pages owned by dom_cow).
>
> Any two of these will give you the third (via X = Y + Z). In this
> case, we used Y & Z because I think that they are the simplest to
> reason about (and therefore the most likely to be wanted). An
> argument could easily be made for others though, if someone feels
> strongly.
>
> There are certainly some historical stats (# of unshares, etc.) and
> lots of per-page/frame stats that would be of interest as well. But,
> bit by bit...
Sure...
Ian.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 6 of 8] Tools: Allow libxl/xl to expose the total number of shared frames and space saved
2011-12-09 23:15 [PATCH 0 of 8] Tools: Memory sharing interface updates Andres Lagar-Cavilla
` (4 preceding siblings ...)
2011-12-09 23:15 ` [PATCH 5 of 8] Tools: Expose to libxc the total number of shared frames and space saved Andres Lagar-Cavilla
@ 2011-12-09 23:15 ` Andres Lagar-Cavilla
2011-12-12 10:43 ` Ian Campbell
2011-12-09 23:15 ` [PATCH 7 of 8] Tools: Libxc wrappers to add shared pages to physmap Andres Lagar-Cavilla
2011-12-09 23:15 ` [PATCH 8 of 8] Tools: Libxc wrapper for the new sharing audit domctl Andres Lagar-Cavilla
7 siblings, 1 reply; 24+ messages in thread
From: Andres Lagar-Cavilla @ 2011-12-09 23:15 UTC (permalink / raw)
To: xen-devel; +Cc: ian.jackson, andres, tim, ian.campbell, adin
tools/libxl/libxl.c | 10 ++++++++++
tools/libxl/libxl.h | 3 +++
tools/libxl/xl_cmdimpl.c | 12 +++++++++---
3 files changed, 22 insertions(+), 3 deletions(-)
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
Signed-off-by: Adin Scannell <adin@scannell.ca>
diff -r 6e8b8fb2c8b2 -r 3fc7875c8d98 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -499,6 +499,16 @@ int libxl_domain_pause(libxl_ctx *ctx, u
return 0;
}
+long libxl_sharing_used_frames(libxl_ctx *ctx)
+{
+ return xc_sharing_used_frames(ctx->xch);
+}
+
+long libxl_sharing_freed_pages(libxl_ctx *ctx)
+{
+ return xc_sharing_freed_pages(ctx->xch);
+}
+
int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
const char *filename)
{
diff -r 6e8b8fb2c8b2 -r 3fc7875c8d98 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -350,6 +350,9 @@ int libxl_domain_rename(libxl_ctx *ctx,
int libxl_domain_pause(libxl_ctx *ctx, uint32_t domid);
int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid);
+long libxl_sharing_used_frames(libxl_ctx *ctx);
+long libxl_sharing_freed_pages(libxl_ctx *ctx);
+
int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid, const char *filename);
int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb);
diff -r 6e8b8fb2c8b2 -r 3fc7875c8d98 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -3758,6 +3758,7 @@ int main_info(int argc, char **argv)
static void sharing(int totals, const libxl_dominfo *info, int nb_domain)
{
int i;
+ const libxl_version_info *vinfo;
printf("Name ID Mem Shared\n");
@@ -3774,9 +3775,14 @@ static void sharing(int totals, const li
free(domname);
}
- if (totals)
- {
- /* To be added with a future patch. */
+ if (totals) {
+ vinfo = libxl_get_version_info(ctx);
+ if (vinfo) {
+ i = (1 << 20) / vinfo->pagesize;
+ printf("Freed with sharing: %ld Total physical shared: %ld\n",
+ libxl_sharing_freed_pages(ctx) / i,
+ libxl_sharing_used_frames(ctx) / i);
+ }
}
}
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 6 of 8] Tools: Allow libxl/xl to expose the total number of shared frames and space saved
2011-12-09 23:15 ` [PATCH 6 of 8] Tools: Allow libxl/xl to expose " Andres Lagar-Cavilla
@ 2011-12-12 10:43 ` Ian Campbell
2011-12-13 17:13 ` Adin Scannell
0 siblings, 1 reply; 24+ messages in thread
From: Ian Campbell @ 2011-12-12 10:43 UTC (permalink / raw)
To: Andres Lagar-Cavilla
Cc: Ian Jackson, andres@gridcentric.ca, xen-devel@lists.xensource.com,
Tim (Xen.org), adin@gridcentric.ca
On Fri, 2011-12-09 at 23:15 +0000, Andres Lagar-Cavilla wrote:
> diff -r 6e8b8fb2c8b2 -r 3fc7875c8d98 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -350,6 +350,9 @@ int libxl_domain_rename(libxl_ctx *ctx,
> int libxl_domain_pause(libxl_ctx *ctx, uint32_t domid);
> int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid);
>
> +long libxl_sharing_used_frames(libxl_ctx *ctx);
> +long libxl_sharing_freed_pages(libxl_ctx *ctx);
I'm undecided but perhaps it would be appropriate to add these to
libxl_physinfo? In which case the output of "xl physinfo" or "xl info"
would be a nice home for these. Or perhaps a new meminfo or hostinfo
struct?
Ian.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 6 of 8] Tools: Allow libxl/xl to expose the total number of shared frames and space saved
2011-12-12 10:43 ` Ian Campbell
@ 2011-12-13 17:13 ` Adin Scannell
0 siblings, 0 replies; 24+ messages in thread
From: Adin Scannell @ 2011-12-13 17:13 UTC (permalink / raw)
To: Ian Campbell
Cc: Ian Jackson, andres@gridcentric.ca, xen-devel@lists.xensource.com,
Tim (Xen.org), Andres Lagar-Cavilla
On Mon, Dec 12, 2011 at 5:43 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2011-12-09 at 23:15 +0000, Andres Lagar-Cavilla wrote:
>> diff -r 6e8b8fb2c8b2 -r 3fc7875c8d98 tools/libxl/libxl.h
>> --- a/tools/libxl/libxl.h
>> +++ b/tools/libxl/libxl.h
>> @@ -350,6 +350,9 @@ int libxl_domain_rename(libxl_ctx *ctx,
>> int libxl_domain_pause(libxl_ctx *ctx, uint32_t domid);
>> int libxl_domain_unpause(libxl_ctx *ctx, uint32_t domid);
>>
>> +long libxl_sharing_used_frames(libxl_ctx *ctx);
>> +long libxl_sharing_freed_pages(libxl_ctx *ctx);
>
> I'm undecided but perhaps it would be appropriate to add these to
> libxl_physinfo? In which case the output of "xl physinfo" or "xl info"
> would be a nice home for these. Or perhaps a new meminfo or hostinfo
> struct?
Sure, libxl_physinfo makes sense.
Cheers!
-Adin
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 7 of 8] Tools: Libxc wrappers to add shared pages to physmap
2011-12-09 23:15 [PATCH 0 of 8] Tools: Memory sharing interface updates Andres Lagar-Cavilla
` (5 preceding siblings ...)
2011-12-09 23:15 ` [PATCH 6 of 8] Tools: Allow libxl/xl to expose " Andres Lagar-Cavilla
@ 2011-12-09 23:15 ` Andres Lagar-Cavilla
2011-12-12 10:48 ` Ian Campbell
2011-12-12 10:50 ` Ian Campbell
2011-12-09 23:15 ` [PATCH 8 of 8] Tools: Libxc wrapper for the new sharing audit domctl Andres Lagar-Cavilla
7 siblings, 2 replies; 24+ messages in thread
From: Andres Lagar-Cavilla @ 2011-12-09 23:15 UTC (permalink / raw)
To: xen-devel; +Cc: ian.jackson, andres, tim, ian.campbell, adin
tools/libxc/xc_memshr.c | 23 +++++++++++++++++++++++
tools/libxc/xenctrl.h | 6 ++++++
2 files changed, 29 insertions(+), 0 deletions(-)
Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
diff -r 3fc7875c8d98 -r 33935769e257 tools/libxc/xc_memshr.c
--- a/tools/libxc/xc_memshr.c
+++ b/tools/libxc/xc_memshr.c
@@ -86,6 +86,29 @@ int xc_memshr_nominate_gref(xc_interface
return ret;
}
+int xc_memshr_add_to_physmap(xc_interface *xch,
+ uint32_t source_domain,
+ uint64_t source_gfn,
+ uint64_t source_handle,
+ uint32_t client_domain,
+ uint64_t client_gfn)
+{
+ DECLARE_DOMCTL;
+ struct xen_domctl_mem_sharing_op *op;
+
+ domctl.cmd = XEN_DOMCTL_mem_sharing_op;
+ domctl.interface_version = XEN_DOMCTL_INTERFACE_VERSION;
+ domctl.domain = (domid_t) source_domain;
+ op = &(domctl.u.mem_sharing_op);
+ op->op = XEN_DOMCTL_MEM_EVENT_OP_SHARING_ADD_PHYSMAP;
+ op->u.share.source_gfn = source_gfn;
+ op->u.share.source_handle = source_handle;
+ op->u.share.client_gfn = client_gfn;
+ op->u.share.client_domain = (uint64_t) client_domain;
+
+ return do_domctl(xch, &domctl);
+}
+
int xc_memshr_share(xc_interface *xch,
uint32_t source_domain,
uint64_t source_gfn,
diff -r 3fc7875c8d98 -r 33935769e257 tools/libxc/xenctrl.h
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1895,6 +1895,12 @@ int xc_memshr_nominate_gref(xc_interface
uint32_t domid,
grant_ref_t gref,
uint64_t *handle);
+int xc_memshr_add_to_physmap(xc_interface *xch,
+ uint32_t source_domain,
+ uint64_t source_gfn,
+ uint64_t source_handle,
+ uint32_t client_domain,
+ uint64_t client_gfn);
int xc_memshr_share(xc_interface *xch,
uint32_t source_domain,
uint64_t source_gfn,
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 7 of 8] Tools: Libxc wrappers to add shared pages to physmap
2011-12-09 23:15 ` [PATCH 7 of 8] Tools: Libxc wrappers to add shared pages to physmap Andres Lagar-Cavilla
@ 2011-12-12 10:48 ` Ian Campbell
2011-12-12 10:50 ` Ian Campbell
1 sibling, 0 replies; 24+ messages in thread
From: Ian Campbell @ 2011-12-12 10:48 UTC (permalink / raw)
To: Andres Lagar-Cavilla
Cc: Ian Jackson, andres@gridcentric.ca, xen-devel@lists.xensource.com,
Tim (Xen.org), adin@gridcentric.ca
On Fri, 2011-12-09 at 23:15 +0000, Andres Lagar-Cavilla wrote:
> tools/libxc/xc_memshr.c | 23 +++++++++++++++++++++++
> tools/libxc/xenctrl.h | 6 ++++++
> 2 files changed, 29 insertions(+), 0 deletions(-)
>
>
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>
> diff -r 3fc7875c8d98 -r 33935769e257 tools/libxc/xc_memshr.c
> --- a/tools/libxc/xc_memshr.c
> +++ b/tools/libxc/xc_memshr.c
> @@ -86,6 +86,29 @@ int xc_memshr_nominate_gref(xc_interface
> return ret;
> }
>
> +int xc_memshr_add_to_physmap(xc_interface *xch,
Who uses this?
> + uint32_t source_domain,
> + uint64_t source_gfn,
> + uint64_t source_handle,
> + uint32_t client_domain,
> + uint64_t client_gfn)
> +{
> + DECLARE_DOMCTL;
> + struct xen_domctl_mem_sharing_op *op;
> +
> + domctl.cmd = XEN_DOMCTL_mem_sharing_op;
> + domctl.interface_version = XEN_DOMCTL_INTERFACE_VERSION;
do_domctl already takes care of this, although this seems to be the
prevalent form in xc_memchr.c.
> + domctl.domain = (domid_t) source_domain;
> + op = &(domctl.u.mem_sharing_op);
> + op->op = XEN_DOMCTL_MEM_EVENT_OP_SHARING_ADD_PHYSMAP;
> + op->u.share.source_gfn = source_gfn;
> + op->u.share.source_handle = source_handle;
> + op->u.share.client_gfn = client_gfn;
> + op->u.share.client_domain = (uint64_t) client_domain;
> +
> + return do_domctl(xch, &domctl);
> +}
> +
> int xc_memshr_share(xc_interface *xch,
> uint32_t source_domain,
> uint64_t source_gfn,
> diff -r 3fc7875c8d98 -r 33935769e257 tools/libxc/xenctrl.h
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1895,6 +1895,12 @@ int xc_memshr_nominate_gref(xc_interface
> uint32_t domid,
> grant_ref_t gref,
> uint64_t *handle);
> +int xc_memshr_add_to_physmap(xc_interface *xch,
> + uint32_t source_domain,
> + uint64_t source_gfn,
> + uint64_t source_handle,
> + uint32_t client_domain,
> + uint64_t client_gfn);
> int xc_memshr_share(xc_interface *xch,
> uint32_t source_domain,
> uint64_t source_gfn,
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 7 of 8] Tools: Libxc wrappers to add shared pages to physmap
2011-12-09 23:15 ` [PATCH 7 of 8] Tools: Libxc wrappers to add shared pages to physmap Andres Lagar-Cavilla
2011-12-12 10:48 ` Ian Campbell
@ 2011-12-12 10:50 ` Ian Campbell
1 sibling, 0 replies; 24+ messages in thread
From: Ian Campbell @ 2011-12-12 10:50 UTC (permalink / raw)
To: Andres Lagar-Cavilla
Cc: Ian Jackson, andres@gridcentric.ca, xen-devel@lists.xensource.com,
Tim (Xen.org), adin@gridcentric.ca
On Fri, 2011-12-09 at 23:15 +0000, Andres Lagar-Cavilla wrote:
> tools/libxc/xc_memshr.c | 23 +++++++++++++++++++++++
> tools/libxc/xenctrl.h | 6 ++++++
> 2 files changed, 29 insertions(+), 0 deletions(-)
>
>
> Signed-off-by: Andres Lagar-Cavilla <andres@lagarcavilla.org>
>
> diff -r 3fc7875c8d98 -r 33935769e257 tools/libxc/xc_memshr.c
> --- a/tools/libxc/xc_memshr.c
> +++ b/tools/libxc/xc_memshr.c
> @@ -86,6 +86,29 @@ int xc_memshr_nominate_gref(xc_interface
> return ret;
> }
>
> +int xc_memshr_add_to_physmap(xc_interface *xch,
> + uint32_t source_domain,
> + uint64_t source_gfn,
> + uint64_t source_handle,
> + uint32_t client_domain,
> + uint64_t client_gfn)
> +{
> + DECLARE_DOMCTL;
> + struct xen_domctl_mem_sharing_op *op;
> +
> + domctl.cmd = XEN_DOMCTL_mem_sharing_op;
> + domctl.interface_version = XEN_DOMCTL_INTERFACE_VERSION;
> + domctl.domain = (domid_t) source_domain;
Why not use domid_t in the interface?
> + op = &(domctl.u.mem_sharing_op);
> + op->op = XEN_DOMCTL_MEM_EVENT_OP_SHARING_ADD_PHYSMAP;
> + op->u.share.source_gfn = source_gfn;
> + op->u.share.source_handle = source_handle;
> + op->u.share.client_gfn = client_gfn;
> + op->u.share.client_domain = (uint64_t) client_domain;
Why not uint64_t in the interface?
> +
> + return do_domctl(xch, &domctl);
> +}
> +
> int xc_memshr_share(xc_interface *xch,
> uint32_t source_domain,
> uint64_t source_gfn,
> diff -r 3fc7875c8d98 -r 33935769e257 tools/libxc/xenctrl.h
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1895,6 +1895,12 @@ int xc_memshr_nominate_gref(xc_interface
> uint32_t domid,
> grant_ref_t gref,
> uint64_t *handle);
> +int xc_memshr_add_to_physmap(xc_interface *xch,
> + uint32_t source_domain,
> + uint64_t source_gfn,
> + uint64_t source_handle,
> + uint32_t client_domain,
> + uint64_t client_gfn);
> int xc_memshr_share(xc_interface *xch,
> uint32_t source_domain,
> uint64_t source_gfn,
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 8 of 8] Tools: Libxc wrapper for the new sharing audit domctl
2011-12-09 23:15 [PATCH 0 of 8] Tools: Memory sharing interface updates Andres Lagar-Cavilla
` (6 preceding siblings ...)
2011-12-09 23:15 ` [PATCH 7 of 8] Tools: Libxc wrappers to add shared pages to physmap Andres Lagar-Cavilla
@ 2011-12-09 23:15 ` Andres Lagar-Cavilla
2011-12-12 10:52 ` Ian Campbell
7 siblings, 1 reply; 24+ messages in thread
From: Andres Lagar-Cavilla @ 2011-12-09 23:15 UTC (permalink / raw)
To: xen-devel; +Cc: ian.jackson, andres, tim, ian.campbell, adin
tools/libxc/xc_memshr.c | 14 ++++++++++++++
tools/libxc/xenctrl.h | 2 ++
2 files changed, 16 insertions(+), 0 deletions(-)
Signed-off-by: Adin Scannell <adin@scannell.ca>
diff -r 33935769e257 -r 6514004012c7 tools/libxc/xc_memshr.c
--- a/tools/libxc/xc_memshr.c
+++ b/tools/libxc/xc_memshr.c
@@ -211,3 +211,17 @@ int xc_memshr_debug_gref(xc_interface *x
return do_domctl(xch, &domctl);
}
+int xc_memshr_audit(xc_interface *xch,
+ uint32_t domid)
+{
+ DECLARE_DOMCTL;
+ struct xen_domctl_mem_sharing_op *op;
+
+ domctl.cmd = XEN_DOMCTL_mem_sharing_op;
+ domctl.interface_version = XEN_DOMCTL_INTERFACE_VERSION;
+ domctl.domain = (domid_t)domid;
+ op = &(domctl.u.mem_sharing_op);
+ op->op = XEN_DOMCTL_MEM_EVENT_OP_SHARING_AUDIT;
+
+ return do_domctl(xch, &domctl);
+}
diff -r 33935769e257 -r 6514004012c7 tools/libxc/xenctrl.h
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1921,6 +1921,8 @@ int xc_memshr_debug_mfn(xc_interface *xc
int xc_memshr_debug_gref(xc_interface *xch,
uint32_t domid,
grant_ref_t gref);
+int xc_memshr_audit(xc_interface *xch,
+ uint32_t domid);
int xc_flask_load(xc_interface *xc_handle, char *buf, uint32_t size);
int xc_flask_context_to_sid(xc_interface *xc_handle, char *buf, uint32_t size, uint32_t *sid);
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 8 of 8] Tools: Libxc wrapper for the new sharing audit domctl
2011-12-09 23:15 ` [PATCH 8 of 8] Tools: Libxc wrapper for the new sharing audit domctl Andres Lagar-Cavilla
@ 2011-12-12 10:52 ` Ian Campbell
0 siblings, 0 replies; 24+ messages in thread
From: Ian Campbell @ 2011-12-12 10:52 UTC (permalink / raw)
To: Andres Lagar-Cavilla
Cc: Ian Jackson, andres@gridcentric.ca, xen-devel@lists.xensource.com,
Tim (Xen.org), adin@gridcentric.ca
On Fri, 2011-12-09 at 23:15 +0000, Andres Lagar-Cavilla wrote:
> tools/libxc/xc_memshr.c | 14 ++++++++++++++
> tools/libxc/xenctrl.h | 2 ++
> 2 files changed, 16 insertions(+), 0 deletions(-)
This is something you'd trigger manually as a debug aid? Or would the
memshr daemon do it occasionally?
>
>
> Signed-off-by: Adin Scannell <adin@scannell.ca>
>
> diff -r 33935769e257 -r 6514004012c7 tools/libxc/xc_memshr.c
> --- a/tools/libxc/xc_memshr.c
> +++ b/tools/libxc/xc_memshr.c
> @@ -211,3 +211,17 @@ int xc_memshr_debug_gref(xc_interface *x
> return do_domctl(xch, &domctl);
> }
>
> +int xc_memshr_audit(xc_interface *xch,
> + uint32_t domid)
> +{
> + DECLARE_DOMCTL;
> + struct xen_domctl_mem_sharing_op *op;
> +
> + domctl.cmd = XEN_DOMCTL_mem_sharing_op;
> + domctl.interface_version = XEN_DOMCTL_INTERFACE_VERSION;
> + domctl.domain = (domid_t)domid;
> + op = &(domctl.u.mem_sharing_op);
> + op->op = XEN_DOMCTL_MEM_EVENT_OP_SHARING_AUDIT;
> +
> + return do_domctl(xch, &domctl);
> +}
> diff -r 33935769e257 -r 6514004012c7 tools/libxc/xenctrl.h
> --- a/tools/libxc/xenctrl.h
> +++ b/tools/libxc/xenctrl.h
> @@ -1921,6 +1921,8 @@ int xc_memshr_debug_mfn(xc_interface *xc
> int xc_memshr_debug_gref(xc_interface *xch,
> uint32_t domid,
> grant_ref_t gref);
> +int xc_memshr_audit(xc_interface *xch,
> + uint32_t domid);
>
> int xc_flask_load(xc_interface *xc_handle, char *buf, uint32_t size);
> int xc_flask_context_to_sid(xc_interface *xc_handle, char *buf, uint32_t size, uint32_t *sid);
^ permalink raw reply [flat|nested] 24+ messages in thread