[PATCH/GIT PULL] Fixes for Xen v4.3 (v1)

[PATCH/GIT PULL] Fixes for Xen v4.3 (v1)

[Konrad Rzeszutek Wilk]

Please review the patches attached. Four of them have been Acked/Reviewed-by.
They are:

 [PATCH 1/5] MAINTAINERS: Change tmem maintainer.
 [PATCH 2/5] hypervisor/xen/tools: Remove the
 [PATCH 3/5] libxl: Change claim_mode from bool to int.
 [PATCH 4/5] libxl: claim: Print the values in 'xl info'

The last patch:
 [PATCH 5/5] libxl: Make 'xl vcpu-set' work properly on overcommited

has not yet been reviewed (well, v0 had been).

If they are OK please pull the following branch:

git://xenbits.xen.org/people/konradwilk/xen.git stable/for-xen-4.3

which has the following fixes:

 MAINTAINERS                         |  2 +-
 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.c                    |  6 ++--
 tools/libxl/xl.h                    |  2 +-
 tools/libxl/xl_cmdimpl.c            | 57 +++++++++++++++++++++----------------
 tools/libxl/xl_cmdtable.c           |  3 +-
 xen/common/memory.c                 |  8 ------
 xen/common/page_alloc.c             |  7 +++--
 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               |  6 ----
 xen/xsm/dummy.c                     |  1 -
 xen/xsm/flask/hooks.c               |  7 -----
 xen/xsm/flask/policy/access_vectors |  2 +-
 21 files changed, 53 insertions(+), 97 deletions(-)



Konrad Rzeszutek Wilk (5):
      MAINTAINERS: Change tmem maintainer.
      hypervisor/xen/tools: Remove the XENMEM_get_oustanding_pages and provide the data via xc_phys_info
      libxl: Change claim_mode from bool to int.
      libxl: claim: Print the values in 'xl info' unconditionally
      libxl: Make 'xl vcpu-set' work properly on overcommited hosts with an override.

Thanks!

[PATCH 1/5] MAINTAINERS: Change tmem maintainer.

[Konrad Rzeszutek Wilk]

Konrad has graduated to becoming an maintainer in the Xen hypervisor.

Acked-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Dan Magenheimer <dan.magenheimer@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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.8.1.4

[PATCH 2/5] hypervisor/xen/tools: Remove the XENMEM_get_oustanding_pages and provide the data via xc_phys_info

[Konrad Rzeszutek Wilk]

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.

Acked-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Reviewed-by: Tim Deegan <tim@xen.org>
Acked-by: Keir Fraser <keir.xen@gmail.com>
[v1: Fix missing XSM hooks cleanups, fixed get_outstanding_claims fnc,
  add Acked/Reviewed-by]
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             |  7 +++++--
 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               |  6 ------
 xen/xsm/dummy.c                     |  1 -
 xen/xsm/flask/hooks.c               |  7 -------
 xen/xsm/flask/policy/access_vectors |  2 +-
 17 files changed, 16 insertions(+), 80 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 87bda72..dfba755 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -3941,6 +3941,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;
@@ -4104,20 +4105,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 c1a969b..f2bb3dc 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..2162ef1 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -380,9 +380,12 @@ out:
     return ret;
 }
 
-long get_outstanding_claims(void)
+void 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);
 }
 
 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 28512fb..ac6e4eb 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -52,7 +52,7 @@ void free_xenheap_pages(void *v, unsigned int order);
 /* 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);
+void 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..1939453 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);
 
@@ -360,11 +359,6 @@ static inline int xsm_claim_pages(xsm_default_t def, struct domain *d)
     return xsm_ops->claim_pages(d);
 }
 
-static inline int xsm_xenmem_get_outstanding_pages(xsm_default_t def)
-{
-    return xsm_ops->xenmem_get_outstanding_pages();
-}
-
 static inline int xsm_console_io (xsm_default_t def, struct domain *d, int cmd)
 {
     return xsm_ops->console_io(d, 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..fa0589a 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;
@@ -1504,7 +1498,6 @@ static struct xsm_operations flask_ops = {
     .memory_stat_reservation = flask_memory_stat_reservation,
     .memory_pin_page = flask_memory_pin_page,
     .claim_pages = flask_claim_pages,
-    .xenmem_get_outstanding_pages = flask_xenmem_get_outstanding_pages,
 
     .console_io = flask_console_io,
 
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 544c3ba..5dfe13b 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -54,7 +54,7 @@ class xen
     debug
 # XEN_SYSCTL_getcpuinfo, XENPF_get_cpu_version, XENPF_get_cpuinfo
     getcpuinfo
-# XEN_SYSCTL_availheap, XENMEM_get_outstanding_pages
+# XEN_SYSCTL_availheap
     heap
 # XEN_SYSCTL_get_pmstat, XEN_SYSCTL_pm_op, XENPF_set_processor_pminfo,
 # XENPF_core_parking
-- 
1.8.1.4

[PATCH 3/5] libxl: Change claim_mode from bool to int.

[Konrad Rzeszutek Wilk]

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.

Acked-by: Ian Campbell <Ian.Campbell@citrix.com>
[v1: Make by default b_info->claim_mode false to allow callers of
 libxl to not care about claim_mode]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/xl.c         | 6 +++---
 tools/libxl/xl.h         | 2 +-
 tools/libxl/xl_cmdimpl.c | 6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

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 f2bb3dc..c3e1183 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.8.1.4

[PATCH 4/5] libxl: claim: Print the values in 'xl info' unconditionally

[Konrad Rzeszutek Wilk]

During the review of "libxl: Change claim_mode from bool to int."
Ian Campbell suggested that the xl info should print the
claim information irregardless of the global claim_mode value.

Suggested-by: Ian Campbell <Ian.Campbell@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/xl_cmdimpl.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c3e1183..bb7a7af 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4604,11 +4604,7 @@ static void output_physinfo(void)
         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 (claim_mode || info.outstanding_pages)
-        printf("outstanding_claims     : %ld\n", info.outstanding_pages / i);
+    printf("outstanding_claims     : %ld\n", info.outstanding_pages / i);
 
     if (!libxl_get_freecpus(ctx, &cpumap)) {
         libxl_for_each_bit(i, cpumap)
-- 
1.8.1.4

[PATCH 5/5] libxl: Make 'xl vcpu-set' work properly on overcommited hosts with an override.

[Konrad Rzeszutek Wilk]

The libxl_cpu_bitmap_alloc(..) function, if provided with a zero
value for max CPUs will call xc_get_max_cpus() which will retrieve
the number of physical CPUs the host has. This is usually
OK if the guest's maxvcpus <= host pcpus. But if the value
is different, then the bitmap for VCPUs is limited by the
number of CPUs the host has.

This is incorrect as what we want is to hotplug in the guest
the amount of CPUs that the user specified on the command line
and not be limited by the amount of physical CPUs.

This means that a guest config like this:

vcpus=8
maxvcpus=32

and on a 4 PCPU machine doing

xl vcpu-set <guest name> 16

won't work. This is b/c the the size of the bitmap is one byte
so it can only hold up to 8 VCPUs. Hence anything above that
is going to be ignored.

Note that this patch also fixes the bitmap setting - as it
would set all of the bits allowed. Meaning if the user had a 4PCPU
host we would still allow the user to set 8VCPUs. This second
iteration of the patch fixes this.

Note that all of the libxl_cpu_bitmap_[test|set] silently ignore
any test or sets above its size:

     if (bit >= bitmap->size * 8)
         return 0;

so we were never notified off this bug.

This patch warns the user if they are trying to do this. If the
user really wants to do this they have to provide the --ignore-host
parameter to bypass this check.

[v1: Add --ignore-host and also fix the bitmap being updated to
the full word instead of to the count of physical CPUs]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/libxl/xl_cmdimpl.c  | 35 +++++++++++++++++++++++++++++------
 tools/libxl/xl_cmdtable.c |  3 ++-
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index bb7a7af..1630e72 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -4487,7 +4487,7 @@ int main_vcpupin(int argc, char **argv)
     return 0;
 }
 
-static void vcpuset(uint32_t domid, const char* nr_vcpus)
+static void vcpuset(uint32_t domid, const char* nr_vcpus, int check_host)
 {
     char *endptr;
     unsigned int max_vcpus, i;
@@ -4499,7 +4499,22 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus)
         return;
     }
 
-    if (libxl_cpu_bitmap_alloc(ctx, &cpumap, 0)) {
+    /*
+     * Maximum amount of vCPUS the guest is allowed to set is limited
+     * by the host's amount of pCPUs.
+     */
+    if (check_host) {
+        unsigned int host_cpu = libxl_get_max_cpus(ctx);
+        if (max_vcpus > host_cpu) {
+            fprintf(stderr, "You are overocmmiting! You have %d physical CPUs" \
+                    " and want %d vCPUs! Aborting, use --ignore-host to " \
+                    " continue\n", host_cpu, max_vcpus);
+            return;
+        }
+        /* NB: This also limits how many are set in the bitmap */
+        max_vcpus = (max_vcpus > host_cpu ? host_cpu : max_vcpus);
+    }
+    if (libxl_cpu_bitmap_alloc(ctx, &cpumap, max_vcpus)) {
         fprintf(stderr, "libxl_cpu_bitmap_alloc failed\n");
         return;
     }
@@ -4514,13 +4529,21 @@ static void vcpuset(uint32_t domid, const char* nr_vcpus)
 
 int main_vcpuset(int argc, char **argv)
 {
-    int opt;
+    static struct option opts[] = {
+        {"ignore-host", 0, 0, 'i'},
+        {0, 0, 0, 0}
+    };
+    int opt, check_host = 1;
 
-    SWITCH_FOREACH_OPT(opt, "", NULL, "vcpu-set", 2) {
-        /* No options */
+    SWITCH_FOREACH_OPT(opt, "i", opts, "vcpu-set", 2) {
+    case 'i':
+        check_host = 0;
+        break;
+    default:
+        break;
     }
 
-    vcpuset(find_domain(argv[optind]), argv[optind+1]);
+    vcpuset(find_domain(argv[optind]), argv[optind + 1], check_host);
     return 0;
 }
 
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 347302c..c98d9bb 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -217,7 +217,8 @@ struct cmd_spec cmd_table[] = {
     { "vcpu-set",
       &main_vcpuset, 0, 1,
       "Set the number of active VCPUs allowed for the domain",
-      "<Domain> <vCPUs>",
+      "[option] <Domain> <vCPUs>",
+      "-i, --ignore-host  Don't limit the vCPU based on the host CPU count",
     },
     { "vm-list",
       &main_vm_list, 0, 0,
-- 
1.8.1.4

Re: [PATCH 4/5] libxl: claim: Print the values in 'xl info' unconditionally

[Ian Campbell]

On Fri, 2013-05-10 at 22:00 +0100, Konrad Rzeszutek Wilk wrote:
> During the review of "libxl: Change claim_mode from bool to int."
> Ian Campbell suggested that the xl info should print the
> claim information irregardless of the global claim_mode value.
> 
> Suggested-by: Ian Campbell <Ian.Campbell@citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxl/xl_cmdimpl.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index c3e1183..bb7a7af 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -4604,11 +4604,7 @@ static void output_physinfo(void)
>          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 (claim_mode || info.outstanding_pages)
> -        printf("outstanding_claims     : %ld\n", info.outstanding_pages / i);
> +    printf("outstanding_claims     : %ld\n", info.outstanding_pages / i);

I here is only initialised within the previous "if (vinfo)" (the tail of
which is right above).

This printf should probably therefore have always been inside that same
block. (the horrible use of the variable i as something other than a
loop iterator is probably at least partly to blame for the confusion)

This patch isn't making this any worse so I'll Ack + apply but perhaps
you could send a follow up to fix this?

>  
>      if (!libxl_get_freecpus(ctx, &cpumap)) {
>          libxl_for_each_bit(i, cpumap)

Re: [PATCH 4/5] libxl: claim: Print the values in 'xl info' unconditionally

[Ian Campbell]

On Mon, 2013-05-13 at 11:17 +0100, Ian Campbell wrote:
> On Fri, 2013-05-10 at 22:00 +0100, Konrad Rzeszutek Wilk wrote:
> > During the review of "libxl: Change claim_mode from bool to int."
> > Ian Campbell suggested that the xl info should print the
> > claim information irregardless of the global claim_mode value.
> > 
> > Suggested-by: Ian Campbell <Ian.Campbell@citrix.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  tools/libxl/xl_cmdimpl.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index c3e1183..bb7a7af 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -4604,11 +4604,7 @@ static void output_physinfo(void)
> >          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 (claim_mode || info.outstanding_pages)
> > -        printf("outstanding_claims     : %ld\n", info.outstanding_pages / i);
> > +    printf("outstanding_claims     : %ld\n", info.outstanding_pages / i);
> 
> I here is only initialised within the previous "if (vinfo)" (the tail of
> which is right above).
> 
> This printf should probably therefore have always been inside that same
> block. (the horrible use of the variable i as something other than a
> loop iterator is probably at least partly to blame for the confusion)
> 
> This patch isn't making this any worse so I'll Ack + apply but perhaps
> you could send a follow up to fix this?

For reason I don't understand this seems to cause the 32-bit build to
fail with:
        
        xl_cmdimpl.c: In function ‘output_physinfo’:
        xl_cmdimpl.c:4607:5: error: format ‘%ld’ expects argument of type ‘long int’, but argument 2 has type ‘uint64_t’ [-Werror=format]
        cc1: all warnings being treated as errors
        
But given that you've changed neither the format string nor the types
involved I've no idea why...

Ian.


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

Re: [PATCH 4/5] libxl: claim: Print the values in 'xl info' unconditionally

[Ian Campbell]

On Mon, 2013-05-13 at 11:22 +0100, Ian Campbell wrote:
> For reason I don't understand this seems to cause the 32-bit build to
> fail with:
>         
>         xl_cmdimpl.c: In function ‘output_physinfo’:
>         xl_cmdimpl.c:4607:5: error: format ‘%ld’ expects argument of type ‘long int’, but argument 2 has type ‘uint64_t’ [-Werror=format]
>         cc1: all warnings being treated as errors
>         
> But given that you've changed neither the format string nor the types
> involved I've no idea why...

Ah, this was actually "hypervisor/xen/tools: Remove the
XENMEM_get_oustanding_pages and provide ..." which changed the value
being printed from an unsigned long to a uint64_t. Can you switch to
PRIu64 please.

Ian.


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

Re: [PATCH 4/5] libxl: claim: Print the values in 'xl info' unconditionally

[Ian Campbell]

On Mon, 2013-05-13 at 11:17 +0100, Ian Campbell wrote:
> On Fri, 2013-05-10 at 22:00 +0100, Konrad Rzeszutek Wilk wrote:
> > During the review of "libxl: Change claim_mode from bool to int."
> > Ian Campbell suggested that the xl info should print the
> > claim information irregardless of the global claim_mode value.
> > 
> > Suggested-by: Ian Campbell <Ian.Campbell@citrix.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  tools/libxl/xl_cmdimpl.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index c3e1183..bb7a7af 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -4604,11 +4604,7 @@ static void output_physinfo(void)
> >          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 (claim_mode || info.outstanding_pages)
> > -        printf("outstanding_claims     : %ld\n", info.outstanding_pages / i);
> > +    printf("outstanding_claims     : %ld\n", info.outstanding_pages / i);
> 
> I here is only initialised within the previous "if (vinfo)" (the tail of
> which is right above).
> 
> This printf should probably therefore have always been inside that same
> block. (the horrible use of the variable i as something other than a
> loop iterator is probably at least partly to blame for the confusion)
> 
> This patch isn't making this any worse so I'll Ack + apply but perhaps
> you could send a follow up to fix this?

Given that you now need to resend anyway (see the build issue  sent
separately), you may as well fold this aspect in too...

Ian.

Re: [PATCH 5/5] libxl: Make 'xl vcpu-set' work properly on overcommited hosts with an override.

[Ian Campbell]

On Fri, 2013-05-10 at 22:00 +0100, Konrad Rzeszutek Wilk wrote:
> The libxl_cpu_bitmap_alloc(..) function, if provided with a zero
> value for max CPUs will call xc_get_max_cpus() which will retrieve
> the number of physical CPUs the host has. This is usually
> OK if the guest's maxvcpus <= host pcpus. But if the value
> is different, then the bitmap for VCPUs is limited by the
> number of CPUs the host has.
> 
> This is incorrect as what we want is to hotplug in the guest
> the amount of CPUs that the user specified on the command line
> and not be limited by the amount of physical CPUs.
> 
> This means that a guest config like this:
> 
> vcpus=8
> maxvcpus=32
> 
> and on a 4 PCPU machine doing
> 
> xl vcpu-set <guest name> 16
> 
> won't work. This is b/c the the size of the bitmap is one byte
> so it can only hold up to 8 VCPUs. Hence anything above that
> is going to be ignored.
> 
> Note that this patch also fixes the bitmap setting - as it
> would set all of the bits allowed. Meaning if the user had a 4PCPU
> host we would still allow the user to set 8VCPUs. This second
> iteration of the patch fixes this.
> 
> Note that all of the libxl_cpu_bitmap_[test|set] silently ignore
> any test or sets above its size:
> 
>      if (bit >= bitmap->size * 8)
>          return 0;
> 
> so we were never notified off this bug.
> 
> This patch warns the user if they are trying to do this. If the
> user really wants to do this they have to provide the --ignore-host
> parameter to bypass this check.
> 
> [v1: Add --ignore-host and also fix the bitmap being updated to
> the full word instead of to the count of physical CPUs]
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

This looks good to me, thanks:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Re: [PATCH 4/5] libxl: claim: Print the values in 'xl info' unconditionally

[Konrad Rzeszutek Wilk]

On Mon, May 13, 2013 at 11:17:31AM +0100, Ian Campbell wrote:
> On Fri, 2013-05-10 at 22:00 +0100, Konrad Rzeszutek Wilk wrote:
> > During the review of "libxl: Change claim_mode from bool to int."
> > Ian Campbell suggested that the xl info should print the
> > claim information irregardless of the global claim_mode value.
> > 
> > Suggested-by: Ian Campbell <Ian.Campbell@citrix.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  tools/libxl/xl_cmdimpl.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index c3e1183..bb7a7af 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -4604,11 +4604,7 @@ static void output_physinfo(void)
> >          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 (claim_mode || info.outstanding_pages)
> > -        printf("outstanding_claims     : %ld\n", info.outstanding_pages / i);
> > +    printf("outstanding_claims     : %ld\n", info.outstanding_pages / i);
> 
> I here is only initialised within the previous "if (vinfo)" (the tail of
> which is right above).
> 
> This printf should probably therefore have always been inside that same
> block. (the horrible use of the variable i as something other than a
> loop iterator is probably at least partly to blame for the confusion)
> 
> This patch isn't making this any worse so I'll Ack + apply but perhaps
> you could send a follow up to fix this?

Naturally!

> 
> >  
> >      if (!libxl_get_freecpus(ctx, &cpumap)) {
> >          libxl_for_each_bit(i, cpumap)
> 
>