Konrad Rzeszutek Wilk
2013-May-08 18:35 UTC
[PATCH 1/3] xen/tools: Remove the XENMEM_get_oustanding_pages and provide the data via xc_phys_info
During the review of the patches it was noticed that there exists
a race wherein the ''free_memory'' value consists of information
from
two hypercalls. That is the XEN_SYSCTL_physinfo and
XENMEM_get_outstanding_pages.
The free memory the host has available for guest is the difference between
the ''free_pages'' (from XEN_SYSCTL_physinfo) and
''outstanding_pages''. As they
are two hypercalls many things can happen in between the execution of them.
This patch resolves this by eliminating the XENMEM_get_outstanding_pages
hypercall and providing the free_pages and outstanding_pages information
via the xc_phys_info structure.
It also removes the XSM hooks and adds locking as needed.
CC: dgdera@tycho.nsa.gov
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
tools/libxc/xc_domain.c | 9 ---------
tools/libxc/xenctrl.h | 2 --
tools/libxl/libxl.c | 15 +--------------
tools/libxl/libxl.h | 1 -
tools/libxl/libxl_types.idl | 1 +
tools/libxl/xl_cmdimpl.c | 16 +++-------------
xen/common/memory.c | 8 --------
xen/common/page_alloc.c | 8 ++++++--
xen/common/sysctl.c | 3 ++-
xen/include/public/memory.h | 7 -------
xen/include/public/sysctl.h | 3 ++-
xen/include/xen/mm.h | 2 +-
xen/include/xsm/dummy.h | 6 ------
xen/include/xsm/xsm.h | 1 -
xen/xsm/dummy.c | 1 -
xen/xsm/flask/hooks.c | 6 ------
16 files changed, 16 insertions(+), 73 deletions(-)
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index bb71cca..3257e2a 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -873,15 +873,6 @@ int xc_domain_claim_pages(xc_interface *xch,
err = errno = 0;
return err;
}
-unsigned long xc_domain_get_outstanding_pages(xc_interface *xch)
-{
- long ret = do_memory_op(xch, XENMEM_get_outstanding_pages, NULL, 0);
-
- /* Ignore it if the hypervisor does not support the call. */
- if (ret == -1 && errno == ENOSYS)
- ret = errno = 0;
- return ret;
-}
int xc_domain_populate_physmap(xc_interface *xch,
uint32_t domid,
diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index c024af4..40ee8fc 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -1182,8 +1182,6 @@ int xc_domain_claim_pages(xc_interface *xch,
uint32_t domid,
unsigned long nr_pages);
-unsigned long xc_domain_get_outstanding_pages(xc_interface *xch);
-
int xc_domain_memory_exchange_pages(xc_interface *xch,
int domid,
unsigned long nr_in_extents,
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index bc91fd5..ee1fa9c 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3942,6 +3942,7 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo
*physinfo)
physinfo->total_pages = xcphysinfo.total_pages;
physinfo->free_pages = xcphysinfo.free_pages;
physinfo->scrub_pages = xcphysinfo.scrub_pages;
+ physinfo->outstanding_pages = xcphysinfo.outstanding_pages;
l = xc_sharing_freed_pages(ctx->xch);
if (l == -ENOSYS) {
l = 0;
@@ -4105,20 +4106,6 @@ libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx, int
*nr)
return ret;
}
-uint64_t libxl_get_claiminfo(libxl_ctx *ctx)
-{
- long l;
-
- l = xc_domain_get_outstanding_pages(ctx->xch);
- if (l < 0) {
- LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_WARNING, l,
- "xc_domain_get_outstanding_pages
failed.");
- return ERROR_FAIL;
- }
- /* In pages */
- return l;
-}
-
const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
{
union {
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index ef96bce..0bc005e 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -624,7 +624,6 @@ int libxl_wait_for_free_memory(libxl_ctx *ctx, uint32_t
domid, uint32_t memory_k
/* wait for the memory target of a domain to be reached */
int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int
wait_secs);
-uint64_t libxl_get_claiminfo(libxl_ctx *ctx);
int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass);
int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num,
libxl_console_type type);
/* libxl_primary_console_exec finds the domid and console number
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index ecf1f0b..8262cba 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -487,6 +487,7 @@ libxl_physinfo = Struct("physinfo", [
("total_pages", uint64),
("free_pages", uint64),
("scrub_pages", uint64),
+ ("outstanding_pages", uint64),
("sharing_freed_pages", uint64),
("sharing_used_frames", uint64),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index ef7f81b..5259b2e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4576,21 +4576,11 @@ static void output_physinfo(void)
unsigned int i;
libxl_bitmap cpumap;
int n = 0;
- long claims = 0;
if (libxl_get_physinfo(ctx, &info) != 0) {
fprintf(stderr, "libxl_physinfo failed.\n");
return;
}
- /*
- * Don''t bother checking "claim_mode" as user might have
turned it off
- * and we have outstanding claims.
- */
- if ((claims = libxl_get_claiminfo(ctx)) < 0){
- fprintf(stderr, "libxl_get_claiminfo failed. errno: %d
(%s)\n",
- errno, strerror(errno));
- return;
- }
printf("nr_cpus : %d\n", info.nr_cpus);
printf("max_cpu_id : %d\n", info.max_cpu_id);
printf("nr_nodes : %d\n", info.nr_nodes);
@@ -4610,15 +4600,15 @@ static void output_physinfo(void)
if (vinfo) {
i = (1 << 20) / vinfo->pagesize;
printf("total_memory : %"PRIu64"\n",
info.total_pages / i);
- printf("free_memory : %"PRIu64"\n",
(info.free_pages - claims) / i);
+ printf("free_memory : %"PRIu64"\n",
(info.free_pages - info.outstanding_pages) / i);
printf("sharing_freed_memory : %"PRIu64"\n",
info.sharing_freed_pages / i);
printf("sharing_used_memory : %"PRIu64"\n",
info.sharing_used_frames / i);
}
/*
* Only if enabled (claim_mode=1) or there are outstanding claims.
*/
- if (libxl_defbool_val(claim_mode) || claims)
- printf("outstanding_claims : %ld\n", claims / i);
+ if (libxl_defbool_val(claim_mode) || info.outstanding_pages)
+ printf("outstanding_claims : %ld\n",
info.outstanding_pages / i);
if (!libxl_get_freecpus(ctx, &cpumap)) {
libxl_for_each_bit(i, cpumap)
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 3239d53..06a0d0a 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -737,14 +737,6 @@ long do_memory_op(unsigned long cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)
break;
- case XENMEM_get_outstanding_pages:
- rc = xsm_xenmem_get_outstanding_pages(XSM_PRIV);
-
- if ( !rc )
- rc = get_outstanding_claims();
-
- break;
-
default:
rc = arch_memory_op(op, arg);
break;
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 203f77a..ede896c 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -380,9 +380,13 @@ out:
return ret;
}
-long get_outstanding_claims(void)
+int get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages)
{
- return outstanding_claims;
+ spin_lock(&heap_lock);
+ *outstanding_pages = outstanding_claims;
+ *free_pages = avail_domheap_pages();
+ spin_unlock(&heap_lock);
+ return 0;
}
static unsigned long init_node_heap(int node, unsigned long mfn,
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index 31f9650..117e095 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -264,7 +264,8 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t)
u_sysctl)
pi->max_node_id = MAX_NUMNODES-1;
pi->max_cpu_id = nr_cpu_ids - 1;
pi->total_pages = total_pages;
- pi->free_pages = avail_domheap_pages();
+ /* Protected by lock */
+ get_outstanding_claims(&pi->free_pages,
&pi->outstanding_pages);
pi->scrub_pages = 0;
pi->cpu_khz = cpu_khz;
arch_do_physinfo(pi);
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index 51d5254..7a26dee 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -459,13 +459,6 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t);
* The zero value is appropiate.
*/
-/*
- * Get the number of pages currently claimed (but not yet
"possessed")
- * across all domains. The caller must be privileged but otherwise
- * the call never fails.
- */
-#define XENMEM_get_outstanding_pages 25
-
#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
#endif /* __XEN_PUBLIC_MEMORY_H__ */
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 03710d8..8437d31 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -34,7 +34,7 @@
#include "xen.h"
#include "domctl.h"
-#define XEN_SYSCTL_INTERFACE_VERSION 0x00000009
+#define XEN_SYSCTL_INTERFACE_VERSION 0x0000000A
/*
* Read console content from Xen buffer ring.
@@ -101,6 +101,7 @@ struct xen_sysctl_physinfo {
uint64_aligned_t total_pages;
uint64_aligned_t free_pages;
uint64_aligned_t scrub_pages;
+ uint64_aligned_t outstanding_pages;
uint32_t hw_cap[8];
/* XEN_SYSCTL_PHYSCAP_??? */
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index efc45c7..e7c88a7 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -59,7 +59,7 @@ void destroy_xen_mappings(unsigned long v, unsigned long e);
/* Claim handling */
unsigned long domain_adjust_tot_pages(struct domain *d, long pages);
int domain_set_outstanding_pages(struct domain *d, unsigned long pages);
-long get_outstanding_claims(void);
+int get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages);
/* Domain suballocator. These functions are *not* interrupt-safe.*/
void init_domheap_pages(paddr_t ps, paddr_t pe);
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index a872056..cc0a5a8 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -259,12 +259,6 @@ static XSM_INLINE int xsm_claim_pages(XSM_DEFAULT_ARG
struct domain *d)
return xsm_default_action(action, current->domain, d);
}
-static XSM_INLINE int xsm_xenmem_get_outstanding_pages(XSM_DEFAULT_VOID)
-{
- XSM_ASSERT_ACTION(XSM_PRIV);
- return xsm_default_action(action, current->domain, NULL);
-}
-
static XSM_INLINE int xsm_evtchn_unbound(XSM_DEFAULT_ARG struct domain *d,
struct evtchn *chn,
domid_t id2)
{
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index 58a4fbb..65150a3 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -93,7 +93,6 @@ struct xsm_operations {
int (*add_to_physmap) (struct domain *d1, struct domain *d2);
int (*remove_from_physmap) (struct domain *d1, struct domain *d2);
int (*claim_pages) (struct domain *d);
- int (*xenmem_get_outstanding_pages) (void);
int (*console_io) (struct domain *d, int cmd);
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 937761f..31e4f73 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -67,7 +67,6 @@ void xsm_fixup_ops (struct xsm_operations *ops)
set_to_dummy_if_null(ops, memory_stat_reservation);
set_to_dummy_if_null(ops, memory_pin_page);
set_to_dummy_if_null(ops, claim_pages);
- set_to_dummy_if_null(ops, xenmem_get_outstanding_pages);
set_to_dummy_if_null(ops, console_io);
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index bb10de3..8e93cdb 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -422,12 +422,6 @@ static int flask_claim_pages(struct domain *d)
return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SETCLAIM);
}
-static int flask_xenmem_get_outstanding_pages(void)
-{
- return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN,
- XEN__HEAP, NULL);
-}
-
static int flask_console_io(struct domain *d, int cmd)
{
u32 perm;
--
1.7.7.6
Konrad Rzeszutek Wilk
2013-May-08 18:35 UTC
[PATCH 2/3] libxl: Change claim_mode from bool to int.
During the review it was noticed that it would be better if internally
the claim_mode was held as an ''int'' instead of a
''bool''. The reason
is that during the startup of xl, one has call the libxl_defbool_setdefault.
otherwise any usage of claim_mode would result in assert break.
The assert is due to the fact that using defbool without any set
values (either true of false) will cause it hit an assertion.
If we use an ''int'' we don''t have to worry about it
and by default
the value of zero will suffice for checks whether the claim is
enabled or disabled.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
tools/libxl/libxl_create.c | 2 --
tools/libxl/xl.c | 6 +++---
tools/libxl/xl.h | 2 +-
tools/libxl/xl_cmdimpl.c | 6 +++---
4 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index cb9c822..c116186 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -202,8 +202,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
if (b_info->target_memkb == LIBXL_MEMKB_DEFAULT)
b_info->target_memkb = b_info->max_memkb;
- libxl_defbool_setdefault(&b_info->claim_mode, false);
-
libxl_defbool_setdefault(&b_info->localtime, false);
libxl_defbool_setdefault(&b_info->disable_migrate, false);
diff --git a/tools/libxl/xl.c b/tools/libxl/xl.c
index 3c141bf..1ce820c 100644
--- a/tools/libxl/xl.c
+++ b/tools/libxl/xl.c
@@ -46,7 +46,7 @@ char *default_vifscript = NULL;
char *default_bridge = NULL;
char *default_gatewaydev = NULL;
enum output_format default_output_format = OUTPUT_FORMAT_JSON;
-libxl_defbool claim_mode;
+int claim_mode = 0;
static xentoollog_level minmsglevel = XTL_PROGRESS;
@@ -170,8 +170,8 @@ static void parse_global_config(const char *configfile,
if (!xlu_cfg_get_string (config, "blkdev_start", &buf, 0))
blkdev_start = strdup(buf);
- libxl_defbool_setdefault(&claim_mode, false);
- (void)xlu_cfg_get_defbool (config, "claim_mode", &claim_mode,
0);
+ if (!xlu_cfg_get_long (config, "claim_mode", &l, 0))
+ claim_mode = l;
xlu_cfg_destroy(config);
}
diff --git a/tools/libxl/xl.h b/tools/libxl/xl.h
index 771b4af..5ad3e17 100644
--- a/tools/libxl/xl.h
+++ b/tools/libxl/xl.h
@@ -146,7 +146,7 @@ int xl_child_pid(xlchildnum); /* returns 0 if child struct
is not in use */
extern int autoballoon;
extern int run_hotplug_scripts;
extern int dryrun_only;
-extern libxl_defbool claim_mode;
+extern int claim_mode;
extern char *lockfile;
extern char *default_vifscript;
extern char *default_bridge;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 5259b2e..76ee33a 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -734,7 +734,7 @@ static void parse_config_data(const char *config_source,
if (!xlu_cfg_get_long (config, "maxmem", &l, 0))
b_info->max_memkb = l * 1024;
- b_info->claim_mode = claim_mode;
+ libxl_defbool_set(&b_info->claim_mode, claim_mode);
if (xlu_cfg_get_string (config, "on_poweroff", &buf, 0))
buf = "destroy";
@@ -4607,7 +4607,7 @@ static void output_physinfo(void)
/*
* Only if enabled (claim_mode=1) or there are outstanding claims.
*/
- if (libxl_defbool_val(claim_mode) || info.outstanding_pages)
+ if (claim_mode || info.outstanding_pages)
printf("outstanding_claims : %ld\n",
info.outstanding_pages / i);
if (!libxl_get_freecpus(ctx, &cpumap)) {
@@ -5911,7 +5911,7 @@ int main_claims(int argc, char **argv)
/* No options */
}
- if (!libxl_defbool_val(claim_mode))
+ if (!claim_mode)
fprintf(stderr, "claim_mode not enabled (see man
xl.conf).\n");
info = libxl_list_domain(ctx, &nb_domain);
--
1.7.7.6
Konrad Rzeszutek Wilk
2013-May-08 18:35 UTC
[PATCH 3/3] MAINTAINERS: Change tmem maintainer.
Konrad has graduated to becoming an maintainer in the Xen hypervisor. Acked-by: Dan Magenheimer <dan.magenheimer@oracle.com> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- MAINTAINERS | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 7c672c8..b1e8d23 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -262,7 +262,7 @@ S: Supported F: tools/ TRANSCENDENT MEMORY (TMEM) -M: Dan Magenheimer <dan.magenheimer@oracle.com> +M: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> W: http://oss.oracle.com/projects/tmem S: Supported F: xen/common/tmem* -- 1.7.7.6
Ian Campbell
2013-May-09 07:44 UTC
Re: [PATCH 1/3] xen/tools: Remove the XENMEM_get_oustanding_pages and provide the data via xc_phys_info
On Wed, 2013-05-08 at 19:35 +0100, Konrad Rzeszutek Wilk wrote:> During the review of the patches it was noticed that there exists > a race wherein the ''free_memory'' value consists of information from > two hypercalls. That is the XEN_SYSCTL_physinfo and XENMEM_get_outstanding_pages. > > The free memory the host has available for guest is the difference between > the ''free_pages'' (from XEN_SYSCTL_physinfo) and ''outstanding_pages''. As they > are two hypercalls many things can happen in between the execution of them. > > This patch resolves this by eliminating the XENMEM_get_outstanding_pages > hypercall and providing the free_pages and outstanding_pages information > via the xc_phys_info structure. > > It also removes the XSM hooks and adds locking as needed. > > CC: dgdera@tycho.nsa.gov > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>For the tools side: Acked-by: Ian Campbell <ian.campbell@citrix.com> Needs a hypervisor ack though, since contrary to the subject line this isn''t just a tools change. Adding Keir, Tim & Jan (not sure which of them is the right one here). I''ve left the quotes untrimmed for the new CCs.> --- > tools/libxc/xc_domain.c | 9 --------- > tools/libxc/xenctrl.h | 2 -- > tools/libxl/libxl.c | 15 +-------------- > tools/libxl/libxl.h | 1 - > tools/libxl/libxl_types.idl | 1 + > tools/libxl/xl_cmdimpl.c | 16 +++------------- > xen/common/memory.c | 8 -------- > xen/common/page_alloc.c | 8 ++++++-- > xen/common/sysctl.c | 3 ++- > xen/include/public/memory.h | 7 ------- > xen/include/public/sysctl.h | 3 ++- > xen/include/xen/mm.h | 2 +- > xen/include/xsm/dummy.h | 6 ------ > xen/include/xsm/xsm.h | 1 - > xen/xsm/dummy.c | 1 - > xen/xsm/flask/hooks.c | 6 ------ > 16 files changed, 16 insertions(+), 73 deletions(-) > > diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c > index bb71cca..3257e2a 100644 > --- a/tools/libxc/xc_domain.c > +++ b/tools/libxc/xc_domain.c > @@ -873,15 +873,6 @@ int xc_domain_claim_pages(xc_interface *xch, > err = errno = 0; > return err; > } > -unsigned long xc_domain_get_outstanding_pages(xc_interface *xch) > -{ > - long ret = do_memory_op(xch, XENMEM_get_outstanding_pages, NULL, 0); > - > - /* Ignore it if the hypervisor does not support the call. */ > - if (ret == -1 && errno == ENOSYS) > - ret = errno = 0; > - return ret; > -} > > int xc_domain_populate_physmap(xc_interface *xch, > uint32_t domid, > diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h > index c024af4..40ee8fc 100644 > --- a/tools/libxc/xenctrl.h > +++ b/tools/libxc/xenctrl.h > @@ -1182,8 +1182,6 @@ int xc_domain_claim_pages(xc_interface *xch, > uint32_t domid, > unsigned long nr_pages); > > -unsigned long xc_domain_get_outstanding_pages(xc_interface *xch); > - > int xc_domain_memory_exchange_pages(xc_interface *xch, > int domid, > unsigned long nr_in_extents, > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c > index bc91fd5..ee1fa9c 100644 > --- a/tools/libxl/libxl.c > +++ b/tools/libxl/libxl.c > @@ -3942,6 +3942,7 @@ int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo) > physinfo->total_pages = xcphysinfo.total_pages; > physinfo->free_pages = xcphysinfo.free_pages; > physinfo->scrub_pages = xcphysinfo.scrub_pages; > + physinfo->outstanding_pages = xcphysinfo.outstanding_pages; > l = xc_sharing_freed_pages(ctx->xch); > if (l == -ENOSYS) { > l = 0; > @@ -4105,20 +4106,6 @@ libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx, int *nr) > return ret; > } > > -uint64_t libxl_get_claiminfo(libxl_ctx *ctx) > -{ > - long l; > - > - l = xc_domain_get_outstanding_pages(ctx->xch); > - if (l < 0) { > - LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_WARNING, l, > - "xc_domain_get_outstanding_pages failed."); > - return ERROR_FAIL; > - } > - /* In pages */ > - return l; > -} > - > const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx) > { > union { > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h > index ef96bce..0bc005e 100644 > --- a/tools/libxl/libxl.h > +++ b/tools/libxl/libxl.h > @@ -624,7 +624,6 @@ int libxl_wait_for_free_memory(libxl_ctx *ctx, uint32_t domid, uint32_t memory_k > /* wait for the memory target of a domain to be reached */ > int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int wait_secs); > > -uint64_t libxl_get_claiminfo(libxl_ctx *ctx); > int libxl_vncviewer_exec(libxl_ctx *ctx, uint32_t domid, int autopass); > int libxl_console_exec(libxl_ctx *ctx, uint32_t domid, int cons_num, libxl_console_type type); > /* libxl_primary_console_exec finds the domid and console number > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl > index ecf1f0b..8262cba 100644 > --- a/tools/libxl/libxl_types.idl > +++ b/tools/libxl/libxl_types.idl > @@ -487,6 +487,7 @@ libxl_physinfo = Struct("physinfo", [ > ("total_pages", uint64), > ("free_pages", uint64), > ("scrub_pages", uint64), > + ("outstanding_pages", uint64), > ("sharing_freed_pages", uint64), > ("sharing_used_frames", uint64), > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index ef7f81b..5259b2e 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -4576,21 +4576,11 @@ static void output_physinfo(void) > unsigned int i; > libxl_bitmap cpumap; > int n = 0; > - long claims = 0; > > if (libxl_get_physinfo(ctx, &info) != 0) { > fprintf(stderr, "libxl_physinfo failed.\n"); > return; > } > - /* > - * Don''t bother checking "claim_mode" as user might have turned it off > - * and we have outstanding claims. > - */ > - if ((claims = libxl_get_claiminfo(ctx)) < 0){ > - fprintf(stderr, "libxl_get_claiminfo failed. errno: %d (%s)\n", > - errno, strerror(errno)); > - return; > - } > printf("nr_cpus : %d\n", info.nr_cpus); > printf("max_cpu_id : %d\n", info.max_cpu_id); > printf("nr_nodes : %d\n", info.nr_nodes); > @@ -4610,15 +4600,15 @@ static void output_physinfo(void) > if (vinfo) { > i = (1 << 20) / vinfo->pagesize; > printf("total_memory : %"PRIu64"\n", info.total_pages / i); > - printf("free_memory : %"PRIu64"\n", (info.free_pages - claims) / i); > + printf("free_memory : %"PRIu64"\n", (info.free_pages - info.outstanding_pages) / i); > printf("sharing_freed_memory : %"PRIu64"\n", info.sharing_freed_pages / i); > printf("sharing_used_memory : %"PRIu64"\n", info.sharing_used_frames / i); > } > /* > * Only if enabled (claim_mode=1) or there are outstanding claims. > */ > - if (libxl_defbool_val(claim_mode) || claims) > - printf("outstanding_claims : %ld\n", claims / i); > + if (libxl_defbool_val(claim_mode) || info.outstanding_pages) > + printf("outstanding_claims : %ld\n", info.outstanding_pages / i); > > if (!libxl_get_freecpus(ctx, &cpumap)) { > libxl_for_each_bit(i, cpumap) > diff --git a/xen/common/memory.c b/xen/common/memory.c > index 3239d53..06a0d0a 100644 > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -737,14 +737,6 @@ long do_memory_op(unsigned long cmd, XEN_GUEST_HANDLE_PARAM(void) arg) > > break; > > - case XENMEM_get_outstanding_pages: > - rc = xsm_xenmem_get_outstanding_pages(XSM_PRIV); > - > - if ( !rc ) > - rc = get_outstanding_claims(); > - > - break; > - > default: > rc = arch_memory_op(op, arg); > break; > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 203f77a..ede896c 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -380,9 +380,13 @@ out: > return ret; > } > > -long get_outstanding_claims(void) > +int get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages) > { > - return outstanding_claims; > + spin_lock(&heap_lock); > + *outstanding_pages = outstanding_claims; > + *free_pages = avail_domheap_pages(); > + spin_unlock(&heap_lock); > + return 0; > } > > static unsigned long init_node_heap(int node, unsigned long mfn, > diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c > index 31f9650..117e095 100644 > --- a/xen/common/sysctl.c > +++ b/xen/common/sysctl.c > @@ -264,7 +264,8 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) > pi->max_node_id = MAX_NUMNODES-1; > pi->max_cpu_id = nr_cpu_ids - 1; > pi->total_pages = total_pages; > - pi->free_pages = avail_domheap_pages(); > + /* Protected by lock */ > + get_outstanding_claims(&pi->free_pages, &pi->outstanding_pages); > pi->scrub_pages = 0; > pi->cpu_khz = cpu_khz; > arch_do_physinfo(pi); > diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h > index 51d5254..7a26dee 100644 > --- a/xen/include/public/memory.h > +++ b/xen/include/public/memory.h > @@ -459,13 +459,6 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_sharing_op_t); > * The zero value is appropiate. > */ > > -/* > - * Get the number of pages currently claimed (but not yet "possessed") > - * across all domains. The caller must be privileged but otherwise > - * the call never fails. > - */ > -#define XENMEM_get_outstanding_pages 25 > - > #endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */ > > #endif /* __XEN_PUBLIC_MEMORY_H__ */ > diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h > index 03710d8..8437d31 100644 > --- a/xen/include/public/sysctl.h > +++ b/xen/include/public/sysctl.h > @@ -34,7 +34,7 @@ > #include "xen.h" > #include "domctl.h" > > -#define XEN_SYSCTL_INTERFACE_VERSION 0x00000009 > +#define XEN_SYSCTL_INTERFACE_VERSION 0x0000000A > > /* > * Read console content from Xen buffer ring. > @@ -101,6 +101,7 @@ struct xen_sysctl_physinfo { > uint64_aligned_t total_pages; > uint64_aligned_t free_pages; > uint64_aligned_t scrub_pages; > + uint64_aligned_t outstanding_pages; > uint32_t hw_cap[8]; > > /* XEN_SYSCTL_PHYSCAP_??? */ > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h > index efc45c7..e7c88a7 100644 > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -59,7 +59,7 @@ void destroy_xen_mappings(unsigned long v, unsigned long e); > /* Claim handling */ > unsigned long domain_adjust_tot_pages(struct domain *d, long pages); > int domain_set_outstanding_pages(struct domain *d, unsigned long pages); > -long get_outstanding_claims(void); > +int get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages); > > /* Domain suballocator. These functions are *not* interrupt-safe.*/ > void init_domheap_pages(paddr_t ps, paddr_t pe); > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h > index a872056..cc0a5a8 100644 > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -259,12 +259,6 @@ static XSM_INLINE int xsm_claim_pages(XSM_DEFAULT_ARG struct domain *d) > return xsm_default_action(action, current->domain, d); > } > > -static XSM_INLINE int xsm_xenmem_get_outstanding_pages(XSM_DEFAULT_VOID) > -{ > - XSM_ASSERT_ACTION(XSM_PRIV); > - return xsm_default_action(action, current->domain, NULL); > -} > - > static XSM_INLINE int xsm_evtchn_unbound(XSM_DEFAULT_ARG struct domain *d, struct evtchn *chn, > domid_t id2) > { > diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h > index 58a4fbb..65150a3 100644 > --- a/xen/include/xsm/xsm.h > +++ b/xen/include/xsm/xsm.h > @@ -93,7 +93,6 @@ struct xsm_operations { > int (*add_to_physmap) (struct domain *d1, struct domain *d2); > int (*remove_from_physmap) (struct domain *d1, struct domain *d2); > int (*claim_pages) (struct domain *d); > - int (*xenmem_get_outstanding_pages) (void); > > int (*console_io) (struct domain *d, int cmd); > > diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c > index 937761f..31e4f73 100644 > --- a/xen/xsm/dummy.c > +++ b/xen/xsm/dummy.c > @@ -67,7 +67,6 @@ void xsm_fixup_ops (struct xsm_operations *ops) > set_to_dummy_if_null(ops, memory_stat_reservation); > set_to_dummy_if_null(ops, memory_pin_page); > set_to_dummy_if_null(ops, claim_pages); > - set_to_dummy_if_null(ops, xenmem_get_outstanding_pages); > > set_to_dummy_if_null(ops, console_io); > > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c > index bb10de3..8e93cdb 100644 > --- a/xen/xsm/flask/hooks.c > +++ b/xen/xsm/flask/hooks.c > @@ -422,12 +422,6 @@ static int flask_claim_pages(struct domain *d) > return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SETCLAIM); > } > > -static int flask_xenmem_get_outstanding_pages(void) > -{ > - return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN, > - XEN__HEAP, NULL); > -} > - > static int flask_console_io(struct domain *d, int cmd) > { > u32 perm; > -- > 1.7.7.6 >
Ian Campbell
2013-May-09 08:10 UTC
Re: [PATCH 2/3] libxl: Change claim_mode from bool to int.
On Wed, 2013-05-08 at 19:35 +0100, Konrad Rzeszutek Wilk wrote:> During the review it was noticed that it would be better if internally > the claim_mode was held as an ''int'' instead of a ''bool''. The reason > is that during the startup of xl, one has call the libxl_defbool_setdefault. > otherwise any usage of claim_mode would result in assert break. > > The assert is due to the fact that using defbool without any set > values (either true of false) will cause it hit an assertion. > > If we use an ''int'' we don''t have to worry about it and by default > the value of zero will suffice for checks whether the claim is > enabled or disabled.In <1366102243.8399.118.camel@zakaz.uk.xensource.com> I mentioned that it doesn''t seem correct that this is a libxl_domain_build_info setting at all, in that thread you said that claim was a host global setting not a per domain one. Perhaps we want to consider moving claim_mode from libxl_domain_build_info to e.g. libxl_ctx now to save us some API churn in the future? Oh, except libxl_ctx is opaque to the callers of libxl so it''s not much use and we''re therefore back to having to add APIs to expose these settings and all the faff. OK, so I think I''ve convinced myself that this patch is OK for 4.3 (modulo the comment below on the libxl_create.c change) and we can have a rethink about libxl default handling for 4.4. Keeping the libxl interface for claim as a defbool in build_info will facilitate us implementing this rethink since it effectively then becomes a per-domain override of the host global setting, so it doesn''t paint us into too much of a corner.> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > tools/libxl/libxl_create.c | 2 -- > tools/libxl/xl.c | 6 +++--- > tools/libxl/xl.h | 2 +- > tools/libxl/xl_cmdimpl.c | 6 +++--- > 4 files changed, 7 insertions(+), 9 deletions(-) > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > index cb9c822..c116186 100644 > --- a/tools/libxl/libxl_create.c > +++ b/tools/libxl/libxl_create.c > @@ -202,8 +202,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc, > if (b_info->target_memkb == LIBXL_MEMKB_DEFAULT) > b_info->target_memkb = b_info->max_memkb; > > - libxl_defbool_setdefault(&b_info->claim_mode, false);This line should stay, since it allows callers of libxl to not care about claim if they don''t want to (xl does, others may not).> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > index 5259b2e..76ee33a 100644 > --- a/tools/libxl/xl_cmdimpl.c > +++ b/tools/libxl/xl_cmdimpl.c > @@ -734,7 +734,7 @@ static void parse_config_data(const char *config_source, > if (!xlu_cfg_get_long (config, "maxmem", &l, 0)) > b_info->max_memkb = l * 1024; > > - b_info->claim_mode = claim_mode; > + libxl_defbool_set(&b_info->claim_mode, claim_mode); > > if (xlu_cfg_get_string (config, "on_poweroff", &buf, 0)) > buf = "destroy"; > @@ -4607,7 +4607,7 @@ static void output_physinfo(void) > /* > * Only if enabled (claim_mode=1) or there are outstanding claims. > */ > - if (libxl_defbool_val(claim_mode) || info.outstanding_pages) > + if (claim_mode || info.outstanding_pages) > printf("outstanding_claims : %ld\n", info.outstanding_pages / i);There wouldn''t seem to be much harm in printing this unconditionally, it would print 0 if claim mode was disabled, which is ok.> > if (!libxl_get_freecpus(ctx, &cpumap)) { > @@ -5911,7 +5911,7 @@ int main_claims(int argc, char **argv) > /* No options */ > } > > - if (!libxl_defbool_val(claim_mode)) > + if (!claim_mode) > fprintf(stderr, "claim_mode not enabled (see man xl.conf).\n");You don''t exit here? Like above I think it would be actually OK for it to just list the domains with claims of zero. FWIW It occurs to me now that this could have just been "xl list --claims/-c", but it''s there now. (those last three comments are a bit orthogonal to the actual patch...) Ian.
On Wed, 2013-05-08 at 19:35 +0100, Konrad Rzeszutek Wilk wrote:> Konrad has graduated to becoming an maintainer in the Xen hypervisor.Congratulations! Or should that be commiserations? I''m never sure with these things, more bloody work! ;-)> Acked-by: Dan Magenheimer <dan.magenheimer@oracle.com> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>Acked-by: Ian Campbell <ian.campbell@citrix.com> Ian.
Tim Deegan
2013-May-09 11:16 UTC
Re: [PATCH 1/3] xen/tools: Remove the XENMEM_get_oustanding_pages and provide the data via xc_phys_info
At 08:44 +0100 on 09 May (1368089087), Ian Campbell wrote:> On Wed, 2013-05-08 at 19:35 +0100, Konrad Rzeszutek Wilk wrote: > > During the review of the patches it was noticed that there exists > > a race wherein the ''free_memory'' value consists of information from > > two hypercalls. That is the XEN_SYSCTL_physinfo and XENMEM_get_outstanding_pages. > > > > The free memory the host has available for guest is the difference between > > the ''free_pages'' (from XEN_SYSCTL_physinfo) and ''outstanding_pages''. As they > > are two hypercalls many things can happen in between the execution of them. > > > > This patch resolves this by eliminating the XENMEM_get_outstanding_pages > > hypercall and providing the free_pages and outstanding_pages information > > via the xc_phys_info structure. > > > > It also removes the XSM hooks and adds locking as needed. > > > > CC: dgdera@tycho.nsa.gov > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > For the tools side: > Acked-by: Ian Campbell <ian.campbell@citrix.com> > > Needs a hypervisor ack though, since contrary to the subject line this > isn''t just a tools change. Adding Keir, Tim & Jan (not sure which of > them is the right one here).> > -long get_outstanding_claims(void) > > +int get_outstanding_claims(uint64_t *free_pages, uint64_t *outstanding_pages) > > { > > - return outstanding_claims; > > + spin_lock(&heap_lock); > > + *outstanding_pages = outstanding_claims; > > + *free_pages = avail_domheap_pages(); > > + spin_unlock(&heap_lock); > > + return 0; > > }This function should return void -- its only caller ignores the return value anyway. Apart from that, Reviewed-by: Tim Deegan <tim@xen.org>
Daniel De Graaf
2013-May-09 13:02 UTC
Re: [PATCH 1/3] xen/tools: Remove the XENMEM_get_oustanding_pages and provide the data via xc_phys_info
On 05/08/2013 02:35 PM, Konrad Rzeszutek Wilk wrote:> During the review of the patches it was noticed that there exists > a race wherein the ''free_memory'' value consists of information from > two hypercalls. That is the XEN_SYSCTL_physinfo and XENMEM_get_outstanding_pages. > > The free memory the host has available for guest is the difference between > the ''free_pages'' (from XEN_SYSCTL_physinfo) and ''outstanding_pages''. As they > are two hypercalls many things can happen in between the execution of them. > > This patch resolves this by eliminating the XENMEM_get_outstanding_pages > hypercall and providing the free_pages and outstanding_pages information > via the xc_phys_info structure. > > It also removes the XSM hooks and adds locking as needed. > > CC: dgdera@tycho.nsa.govMy email address here is missing a "g" (dgdegra).> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > tools/libxc/xc_domain.c | 9 --------- > tools/libxc/xenctrl.h | 2 -- > tools/libxl/libxl.c | 15 +-------------- > tools/libxl/libxl.h | 1 - > tools/libxl/libxl_types.idl | 1 + > tools/libxl/xl_cmdimpl.c | 16 +++------------- > xen/common/memory.c | 8 -------- > xen/common/page_alloc.c | 8 ++++++-- > xen/common/sysctl.c | 3 ++- > xen/include/public/memory.h | 7 ------- > xen/include/public/sysctl.h | 3 ++- > xen/include/xen/mm.h | 2 +- > xen/include/xsm/dummy.h | 6 ------ > xen/include/xsm/xsm.h | 1 - > xen/xsm/dummy.c | 1 - > xen/xsm/flask/hooks.c | 6 ------ > 16 files changed, 16 insertions(+), 73 deletions(-) >[...]> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h > index a872056..cc0a5a8 100644 > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -259,12 +259,6 @@ static XSM_INLINE int xsm_claim_pages(XSM_DEFAULT_ARG struct domain *d) > return xsm_default_action(action, current->domain, d); > } > > -static XSM_INLINE int xsm_xenmem_get_outstanding_pages(XSM_DEFAULT_VOID) > -{ > - XSM_ASSERT_ACTION(XSM_PRIV); > - return xsm_default_action(action, current->domain, NULL); > -} > - > static XSM_INLINE int xsm_evtchn_unbound(XSM_DEFAULT_ARG struct domain *d, struct evtchn *chn, > domid_t id2) > { > diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h > index 58a4fbb..65150a3 100644 > --- a/xen/include/xsm/xsm.h > +++ b/xen/include/xsm/xsm.h > @@ -93,7 +93,6 @@ struct xsm_operations { > int (*add_to_physmap) (struct domain *d1, struct domain *d2); > int (*remove_from_physmap) (struct domain *d1, struct domain *d2); > int (*claim_pages) (struct domain *d); > - int (*xenmem_get_outstanding_pages) (void); > > int (*console_io) (struct domain *d, int cmd); >You also need to remove the XSM-enabled function from xsm/xsm.h: static inline int xsm_xenmem_get_outstanding_pages(xsm_default_t def) { return xsm_ops->xenmem_get_outstanding_pages(); }> diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c > index 937761f..31e4f73 100644 > --- a/xen/xsm/dummy.c > +++ b/xen/xsm/dummy.c > @@ -67,7 +67,6 @@ void xsm_fixup_ops (struct xsm_operations *ops) > set_to_dummy_if_null(ops, memory_stat_reservation); > set_to_dummy_if_null(ops, memory_pin_page); > set_to_dummy_if_null(ops, claim_pages); > - set_to_dummy_if_null(ops, xenmem_get_outstanding_pages); > > set_to_dummy_if_null(ops, console_io); > > diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c > index bb10de3..8e93cdb 100644 > --- a/xen/xsm/flask/hooks.c > +++ b/xen/xsm/flask/hooks.c > @@ -422,12 +422,6 @@ static int flask_claim_pages(struct domain *d) > return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SETCLAIM); > } > > -static int flask_xenmem_get_outstanding_pages(void) > -{ > - return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN, > - XEN__HEAP, NULL); > -} > - > static int flask_console_io(struct domain *d, int cmd) > { > u32 perm; >And the entry in xsm_operations from flask/hooks.c: .xenmem_get_outstanding_pages = flask_xenmem_get_outstanding_pages, With those two changes: Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> -- Daniel De Graaf National Security Agency
Konrad Rzeszutek Wilk
2013-May-09 13:23 UTC
Re: [PATCH 2/3] libxl: Change claim_mode from bool to int.
On Thu, May 09, 2013 at 09:10:00AM +0100, Ian Campbell wrote:> On Wed, 2013-05-08 at 19:35 +0100, Konrad Rzeszutek Wilk wrote: > > During the review it was noticed that it would be better if internally > > the claim_mode was held as an ''int'' instead of a ''bool''. The reason > > is that during the startup of xl, one has call the libxl_defbool_setdefault. > > otherwise any usage of claim_mode would result in assert break. > > > > The assert is due to the fact that using defbool without any set > > values (either true of false) will cause it hit an assertion. > > > > If we use an ''int'' we don''t have to worry about it and by default > > the value of zero will suffice for checks whether the claim is > > enabled or disabled. > > In <1366102243.8399.118.camel@zakaz.uk.xensource.com> I mentioned that > it doesn''t seem correct that this is a libxl_domain_build_info setting > at all, in that thread you said that claim was a host global setting not > a per domain one. > > Perhaps we want to consider moving claim_mode from > libxl_domain_build_info to e.g. libxl_ctx now to save us some API churn > in the future? Oh, except libxl_ctx is opaque to the callers of libxl so > it''s not much use and we''re therefore back to having to add APIs to > expose these settings and all the faff. > > OK, so I think I''ve convinced myself that this patch is OK for 4.3 > (modulo the comment below on the libxl_create.c change) and we can have > a rethink about libxl default handling for 4.4. Keeping the libxl > interface for claim as a defbool in build_info will facilitate us > implementing this rethink since it effectively then becomes a per-domain > override of the host global setting, so it doesn''t paint us into too > much of a corner. > > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- > > tools/libxl/libxl_create.c | 2 -- > > tools/libxl/xl.c | 6 +++--- > > tools/libxl/xl.h | 2 +- > > tools/libxl/xl_cmdimpl.c | 6 +++--- > > 4 files changed, 7 insertions(+), 9 deletions(-) > > > > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c > > index cb9c822..c116186 100644 > > --- a/tools/libxl/libxl_create.c > > +++ b/tools/libxl/libxl_create.c > > @@ -202,8 +202,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc, > > if (b_info->target_memkb == LIBXL_MEMKB_DEFAULT) > > b_info->target_memkb = b_info->max_memkb; > > > > - libxl_defbool_setdefault(&b_info->claim_mode, false); > > This line should stay, since it allows callers of libxl to not care > about claim if they don''t want to (xl does, others may not). > > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c > > index 5259b2e..76ee33a 100644 > > --- a/tools/libxl/xl_cmdimpl.c > > +++ b/tools/libxl/xl_cmdimpl.c > > @@ -734,7 +734,7 @@ static void parse_config_data(const char *config_source, > > if (!xlu_cfg_get_long (config, "maxmem", &l, 0)) > > b_info->max_memkb = l * 1024; > > > > - b_info->claim_mode = claim_mode; > > + libxl_defbool_set(&b_info->claim_mode, claim_mode); > > > > if (xlu_cfg_get_string (config, "on_poweroff", &buf, 0)) > > buf = "destroy"; > > @@ -4607,7 +4607,7 @@ static void output_physinfo(void) > > /* > > * Only if enabled (claim_mode=1) or there are outstanding claims. > > */ > > - if (libxl_defbool_val(claim_mode) || info.outstanding_pages) > > + if (claim_mode || info.outstanding_pages) > > printf("outstanding_claims : %ld\n", info.outstanding_pages / i); > > There wouldn''t seem to be much harm in printing this unconditionally, it > would print 0 if claim mode was disabled, which is ok.OK, making the change.> > > > > if (!libxl_get_freecpus(ctx, &cpumap)) { > > @@ -5911,7 +5911,7 @@ int main_claims(int argc, char **argv) > > /* No options */ > > } > > > > - if (!libxl_defbool_val(claim_mode)) > > + if (!claim_mode) > > fprintf(stderr, "claim_mode not enabled (see man xl.conf).\n"); > > You don''t exit here? Like above I think it would be actually OK for it > to just list the domains with claims of zero.Ian Jackson during the review suggested that it just print that warning and continue on. And now that I am using I actually like the warning. It reminds me that I forgot to enable or disable it.> > FWIW It occurs to me now that this could have just been "xl list > --claims/-c", but it''s there now.I can certainly try to redo it. I think I tried it two weeks ago and ran in the trouble of having to modify a bunch of extra print_* functions to have the extra claim information. And also not being sure how to expose it via the JSON or sxp. So I choose the less invasive method as it was close to the feature freeze (or already past it). I can certainly rework this for Xen 4.4 ? Or for Xen 4.3 if you think that George would be OK with that.> > (those last three comments are a bit orthogonal to the actual patch...)> > Ian. >
Ian Campbell
2013-May-09 13:40 UTC
Re: [PATCH 2/3] libxl: Change claim_mode from bool to int.
On Thu, 2013-05-09 at 14:23 +0100, Konrad Rzeszutek Wilk wrote:> > > > > > if (!libxl_get_freecpus(ctx, &cpumap)) { > > > @@ -5911,7 +5911,7 @@ int main_claims(int argc, char **argv) > > > /* No options */ > > > } > > > > > > - if (!libxl_defbool_val(claim_mode)) > > > + if (!claim_mode) > > > fprintf(stderr, "claim_mode not enabled (see man xl.conf).\n"); > > > > You don''t exit here? Like above I think it would be actually OK for it > > to just list the domains with claims of zero. > > Ian Jackson during the review suggested that it just print that warning > and continue on. And now that I am using I actually like the warning. It > reminds me that I forgot to enable or disable it.OK.> > > > FWIW It occurs to me now that this could have just been "xl list > > --claims/-c", but it''s there now. > > I can certainly try to redo it. I think I tried it two weeks ago and ran > in the trouble of having to modify a bunch of extra print_* functions to > have the extra claim information. And also not being sure how to expose > it via the JSON or sxp.Not sure what you mean, main_claims just calls list_domains with claims==1, so main_list could call it with claims == some flag set by a command line option. WRT sxp -- you don''t need to care, that is purely legacy and shouldn''t be updated with new features . Not sure which JSON exposes this, but you should get the field for free due to using the IDL for this stuff> I can certainly rework this for Xen 4.4 ? Or for Xen 4.3 if you think > that George would be OK with that.
Jan Beulich
2013-May-10 08:38 UTC
Re: [PATCH 1/3] xen/tools: Remove the XENMEM_get_oustanding_pages and provide the data via xc_phys_info
>>> On 09.05.13 at 09:44, Ian Campbell <Ian.Campbell@citrix.com> wrote: > On Wed, 2013-05-08 at 19:35 +0100, Konrad Rzeszutek Wilk wrote: >> During the review of the patches it was noticed that there exists >> a race wherein the ''free_memory'' value consists of information from >> two hypercalls. That is the XEN_SYSCTL_physinfo and > XENMEM_get_outstanding_pages. >> >> The free memory the host has available for guest is the difference between >> the ''free_pages'' (from XEN_SYSCTL_physinfo) and ''outstanding_pages''. As they >> are two hypercalls many things can happen in between the execution of them. >> >> This patch resolves this by eliminating the XENMEM_get_outstanding_pages >> hypercall and providing the free_pages and outstanding_pages information >> via the xc_phys_info structure. >> >> It also removes the XSM hooks and adds locking as needed. >> >> CC: dgdera@tycho.nsa.gov >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > For the tools side: > Acked-by: Ian Campbell <ian.campbell@citrix.com> > > Needs a hypervisor ack though, since contrary to the subject line this > isn''t just a tools change. Adding Keir, Tim & Jan (not sure which of > them is the right one here).Looks okay to me (albeit quite late in the game for a bump of XEN_SYSCTL_INTERFACE_VERSION), but from a formal perspective Keir needs to ack this for it to go in. Jan
Keir Fraser
2013-May-10 13:58 UTC
Re: [PATCH 1/3] xen/tools: Remove the XENMEM_get_oustanding_pages and provide the data via xc_phys_info
On 10/05/2013 09:38, "Jan Beulich" <JBeulich@suse.com> wrote:>>>> On 09.05.13 at 09:44, Ian Campbell <Ian.Campbell@citrix.com> wrote: >> On Wed, 2013-05-08 at 19:35 +0100, Konrad Rzeszutek Wilk wrote: >>> During the review of the patches it was noticed that there exists >>> a race wherein the ''free_memory'' value consists of information from >>> two hypercalls. That is the XEN_SYSCTL_physinfo and >> XENMEM_get_outstanding_pages. >>> >>> The free memory the host has available for guest is the difference between >>> the ''free_pages'' (from XEN_SYSCTL_physinfo) and ''outstanding_pages''. As they >>> are two hypercalls many things can happen in between the execution of them. >>> >>> This patch resolves this by eliminating the XENMEM_get_outstanding_pages >>> hypercall and providing the free_pages and outstanding_pages information >>> via the xc_phys_info structure. >>> >>> It also removes the XSM hooks and adds locking as needed. >>> >>> CC: dgdera@tycho.nsa.gov >>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> >> For the tools side: >> Acked-by: Ian Campbell <ian.campbell@citrix.com> >> >> Needs a hypervisor ack though, since contrary to the subject line this >> isn''t just a tools change. Adding Keir, Tim & Jan (not sure which of >> them is the right one here). > > Looks okay to me (albeit quite late in the game for a bump of > XEN_SYSCTL_INTERFACE_VERSION), but from a formal > perspective Keir needs to ack this for it to go in.Acked-by: Keir Fraser <keir@xen.org>> Jan >
Konrad Rzeszutek Wilk
2013-May-10 19:18 UTC
Re: [PATCH 1/3] xen/tools: Remove the XENMEM_get_oustanding_pages and provide the data via xc_phys_info
> With those two changes: > > Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>Thank you. I also seemed to have missed xsm/flask/policy/access_vectors :-( Resending it shortly.