[PATCH v2 0/5] enable Memory Bandwidth Monitoring (MBM) for VMs

[PATCH v2 0/5] enable Memory Bandwidth Monitoring (MBM) for VMs

[Chao Peng]

Changes from v1:
* Move event type check from xc to xl;
* Add retry capability for MBM sampling;
* Fix Coding style/docs;

Intel Memory Bandwidth Monitoring(MBM) is a new hardware feature
which builds on the CMT infrastructure to allow monitoring of system
memory bandwidth. Event codes are provided to monitor both "total"
and "local" bandwidth, meaning bandwidth over QPI and other external
links can be monitored.

For XEN, MBM is used to monitor memory bandwidth for VMs. Due to its
dependency on CMT, the software also makes use of most of CMT codes.
Actually, besides introducing two additional events and some cpuid
feature bits, there are no extra changes compared to cache occupancy
monitoring in CMT. Due to this, CMT should be enabled first to use
this feature.

For interface changes, the patch serial only introduces a new command
"XEN_SYSCTL_PSR_CMT_get_l3_event_mask" which exposes MBM feature
capability to user space and introduces two additional options for
"xl psr-cmt-show":
total_mem_bandwidth:     Show total memory bandwidth
local_mem_bandwidth:     Show local memory bandwidth

The usage flow keeps the same with CMT.

Chao Peng (5):
  x86: expose CMT L3 event mask to user space
  tools: add routine to get CMT L3 event mask
  tools: correct coding style for psr
  tools: code refactoring for MBM
  tools: add total/local memory bandwith monitoring

 docs/man/xl.pod.1             |    9 +++
 tools/libxc/include/xenctrl.h |   11 +--
 tools/libxc/xc_psr.c          |   42 ++++++++++--
 tools/libxl/libxl.h           |   20 ++++--
 tools/libxl/libxl_psr.c       |  147 +++++++++++++++++++++++++++++++++++------
 tools/libxl/libxl_types.idl   |    2 +
 tools/libxl/xl_cmdimpl.c      |   69 +++++++++++++------
 tools/libxl/xl_cmdtable.c     |    4 +-
 xen/arch/x86/sysctl.c         |    3 +
 xen/include/public/sysctl.h   |    1 +
 10 files changed, 254 insertions(+), 54 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/5] x86: expose CMT L3 event mask to user space

[Chao Peng]

L3 event mask indicates the event types supported in host, including
cache occupancy event as well as local/total memory bandwidth events
for Memory Bandwidth Monitoring(MBM). Expose it so all these events
can be monitored in user space.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/sysctl.c       |    3 +++
 xen/include/public/sysctl.h |    1 +
 2 files changed, 4 insertions(+)

diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 57ad992..611a291 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -157,6 +157,9 @@ long arch_do_sysctl(
             sysctl->u.psr_cmt_op.u.data = (ret ? 0 : info.size);
             break;
         }
+        case XEN_SYSCTL_PSR_CMT_get_l3_event_mask:
+            sysctl->u.psr_cmt_op.u.data = psr_cmt->l3.features;
+            break;
         default:
             sysctl->u.psr_cmt_op.u.data = 0;
             ret = -ENOSYS;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index b3713b3..8552dc6 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -641,6 +641,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_sysctl_coverage_op_t);
 /* The L3 cache size is returned in KB unit */
 #define XEN_SYSCTL_PSR_CMT_get_l3_cache_size         2
 #define XEN_SYSCTL_PSR_CMT_enabled                   3
+#define XEN_SYSCTL_PSR_CMT_get_l3_event_mask         4
 struct xen_sysctl_psr_cmt_op {
     uint32_t cmd;       /* IN: XEN_SYSCTL_PSR_CMT_* */
     uint32_t flags;     /* padding variable, may be extended for future use */
-- 
1.7.9.5

[PATCH v2 2/5] tools: add routine to get CMT L3 event mask

[Chao Peng]

This is the tools side wrapper for XEN_SYSCTL_PSR_CMT_get_l3_event_mask
of XEN_SYSCTL_psr_cmt_op.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
 tools/libxc/include/xenctrl.h |    1 +
 tools/libxc/xc_psr.c          |   24 ++++++++++++++++++++++++
 tools/libxl/libxl.h           |    1 +
 tools/libxl/libxl_psr.c       |   18 ++++++++++++++++++
 4 files changed, 44 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 0ad8b8d..96b357c 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2697,6 +2697,7 @@ int xc_psr_cmt_get_domain_rmid(xc_interface *xch, uint32_t domid,
 int xc_psr_cmt_get_total_rmid(xc_interface *xch, uint32_t *total_rmid);
 int xc_psr_cmt_get_l3_upscaling_factor(xc_interface *xch,
     uint32_t *upscaling_factor);
+int xc_psr_cmt_get_l3_event_mask(xc_interface *xch, uint32_t *event_mask);
 int xc_psr_cmt_get_l3_cache_size(xc_interface *xch, uint32_t cpu,
     uint32_t *l3_cache_size);
 int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid,
diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c
index 872e6dc..e76a0f9 100644
--- a/tools/libxc/xc_psr.c
+++ b/tools/libxc/xc_psr.c
@@ -112,6 +112,30 @@ int xc_psr_cmt_get_l3_upscaling_factor(xc_interface *xch,
     return rc;
 }
 
+int xc_psr_cmt_get_l3_event_mask(xc_interface *xch, uint32_t *event_mask)
+{
+    static int val = 0;
+    int rc;
+    DECLARE_SYSCTL;
+
+    if ( val )
+    {
+        *event_mask = val;
+        return 0;
+    }
+
+    sysctl.cmd = XEN_SYSCTL_psr_cmt_op;
+    sysctl.u.psr_cmt_op.cmd =
+        XEN_SYSCTL_PSR_CMT_get_l3_event_mask;
+    sysctl.u.psr_cmt_op.flags = 0;
+
+    rc = xc_sysctl(xch, &sysctl);
+    if ( !rc )
+        val = *event_mask = sysctl.u.psr_cmt_op.u.data;
+
+    return rc;
+}
+
 int xc_psr_cmt_get_l3_cache_size(xc_interface *xch, uint32_t cpu,
                                       uint32_t *l3_cache_size)
 {
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 0a123f1..42ace76 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1453,6 +1453,7 @@ int libxl_psr_cmt_attach(libxl_ctx *ctx, uint32_t domid);
 int libxl_psr_cmt_detach(libxl_ctx *ctx, uint32_t domid);
 int libxl_psr_cmt_domain_attached(libxl_ctx *ctx, uint32_t domid);
 int libxl_psr_cmt_enabled(libxl_ctx *ctx);
+int libxl_psr_cmt_type_supported(libxl_ctx *ctx, libxl_psr_cmt_type type);
 int libxl_psr_cmt_get_total_rmid(libxl_ctx *ctx, uint32_t *total_rmid);
 int libxl_psr_cmt_get_l3_cache_size(libxl_ctx *ctx, uint32_t socketid,
     uint32_t *l3_cache_size);
diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index 0437465..3018a0d 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -120,6 +120,24 @@ int libxl_psr_cmt_enabled(libxl_ctx *ctx)
     return xc_psr_cmt_enabled(ctx->xch);
 }
 
+int libxl_psr_cmt_type_supported(libxl_ctx *ctx, libxl_psr_cmt_type type)
+{
+    GC_INIT(ctx);
+    uint32_t event_mask;
+    int ret;
+
+    ret = xc_psr_cmt_get_l3_event_mask(ctx->xch, &event_mask);
+    if (ret < 0) {
+        libxl__psr_cmt_log_err_msg(gc, errno);
+        ret = 0;
+    } else {
+        ret = event_mask & (1 << (type - 1));
+    }
+
+    GC_FREE;
+    return ret;
+}
+
 int libxl_psr_cmt_get_total_rmid(libxl_ctx *ctx, uint32_t *total_rmid)
 {
     GC_INIT(ctx);
-- 
1.7.9.5

[PATCH v2 3/5] tools: correct coding style for psr

[Chao Peng]

- space: remove space after '(' or before ')' in 'if' condition;
- indention: align function definition/call arguments;

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
 tools/libxc/include/xenctrl.h |    8 ++++----
 tools/libxc/xc_psr.c          |   10 +++++-----
 tools/libxl/libxl.h           |   11 +++++++----
 tools/libxl/libxl_psr.c       |   11 +++++++----
 tools/libxl/xl_cmdimpl.c      |   11 ++++++-----
 5 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 96b357c..c6e9e3e 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2693,15 +2693,15 @@ typedef enum xc_psr_cmt_type xc_psr_cmt_type;
 int xc_psr_cmt_attach(xc_interface *xch, uint32_t domid);
 int xc_psr_cmt_detach(xc_interface *xch, uint32_t domid);
 int xc_psr_cmt_get_domain_rmid(xc_interface *xch, uint32_t domid,
-    uint32_t *rmid);
+                               uint32_t *rmid);
 int xc_psr_cmt_get_total_rmid(xc_interface *xch, uint32_t *total_rmid);
 int xc_psr_cmt_get_l3_upscaling_factor(xc_interface *xch,
-    uint32_t *upscaling_factor);
+                                       uint32_t *upscaling_factor);
 int xc_psr_cmt_get_l3_event_mask(xc_interface *xch, uint32_t *event_mask);
 int xc_psr_cmt_get_l3_cache_size(xc_interface *xch, uint32_t cpu,
     uint32_t *l3_cache_size);
-int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid,
-    uint32_t cpu, uint32_t psr_cmt_type, uint64_t *monitor_data);
+int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
+                        uint32_t psr_cmt_type, uint64_t *monitor_data);
 int xc_psr_cmt_enabled(xc_interface *xch);
 #endif
 
diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c
index e76a0f9..e3ecc41 100644
--- a/tools/libxc/xc_psr.c
+++ b/tools/libxc/xc_psr.c
@@ -47,7 +47,7 @@ int xc_psr_cmt_detach(xc_interface *xch, uint32_t domid)
 }
 
 int xc_psr_cmt_get_domain_rmid(xc_interface *xch, uint32_t domid,
-                                    uint32_t *rmid)
+                               uint32_t *rmid)
 {
     int rc;
     DECLARE_DOMCTL;
@@ -88,7 +88,7 @@ int xc_psr_cmt_get_total_rmid(xc_interface *xch, uint32_t *total_rmid)
 }
 
 int xc_psr_cmt_get_l3_upscaling_factor(xc_interface *xch,
-                                            uint32_t *upscaling_factor)
+                                       uint32_t *upscaling_factor)
 {
     static int val = 0;
     int rc;
@@ -137,7 +137,7 @@ int xc_psr_cmt_get_l3_event_mask(xc_interface *xch, uint32_t *event_mask)
 }
 
 int xc_psr_cmt_get_l3_cache_size(xc_interface *xch, uint32_t cpu,
-                                      uint32_t *l3_cache_size)
+                                 uint32_t *l3_cache_size)
 {
     static int val = 0;
     int rc;
@@ -162,8 +162,8 @@ int xc_psr_cmt_get_l3_cache_size(xc_interface *xch, uint32_t cpu,
     return rc;
 }
 
-int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid,
-    uint32_t cpu, xc_psr_cmt_type type, uint64_t *monitor_data)
+int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
+                        xc_psr_cmt_type type, uint64_t *monitor_data)
 {
     xc_resource_op_t op;
     xc_resource_entry_t entries[2];
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 42ace76..d84ff7f 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1455,10 +1455,13 @@ int libxl_psr_cmt_domain_attached(libxl_ctx *ctx, uint32_t domid);
 int libxl_psr_cmt_enabled(libxl_ctx *ctx);
 int libxl_psr_cmt_type_supported(libxl_ctx *ctx, libxl_psr_cmt_type type);
 int libxl_psr_cmt_get_total_rmid(libxl_ctx *ctx, uint32_t *total_rmid);
-int libxl_psr_cmt_get_l3_cache_size(libxl_ctx *ctx, uint32_t socketid,
-    uint32_t *l3_cache_size);
-int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx, uint32_t domid,
-    uint32_t socketid, uint32_t *l3_cache_occupancy);
+int libxl_psr_cmt_get_l3_cache_size(libxl_ctx *ctx,
+                                    uint32_t socketid,
+                                    uint32_t *l3_cache_size);
+int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
+                                      uint32_t domid,
+                                      uint32_t socketid,
+                                      uint32_t *l3_cache_occupancy);
 #endif
 
 /* misc */
diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index 3018a0d..0f2c7e0 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -153,8 +153,9 @@ int libxl_psr_cmt_get_total_rmid(libxl_ctx *ctx, uint32_t *total_rmid)
     return rc;
 }
 
-int libxl_psr_cmt_get_l3_cache_size(libxl_ctx *ctx, uint32_t socketid,
-                                         uint32_t *l3_cache_size)
+int libxl_psr_cmt_get_l3_cache_size(libxl_ctx *ctx,
+                                    uint32_t socketid,
+                                    uint32_t *l3_cache_size)
 {
     GC_INIT(ctx);
 
@@ -178,8 +179,10 @@ out:
     return rc;
 }
 
-int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx, uint32_t domid,
-    uint32_t socketid, uint32_t *l3_cache_occupancy)
+int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
+                                      uint32_t domid,
+                                      uint32_t socketid,
+                                      uint32_t *l3_cache_occupancy)
 {
     GC_INIT(ctx);
 
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index ed0d478..24f3c8d 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7847,7 +7847,7 @@ out:
 
 #ifdef LIBXL_HAVE_PSR_CMT
 static void psr_cmt_print_domain_cache_occupancy(libxl_dominfo *dominfo,
-                                                    uint32_t nr_sockets)
+                                                 uint32_t nr_sockets)
 {
     char *domain_name;
     uint32_t socketid;
@@ -7861,8 +7861,8 @@ static void psr_cmt_print_domain_cache_occupancy(libxl_dominfo *dominfo,
     free(domain_name);
 
     for (socketid = 0; socketid < nr_sockets; socketid++) {
-        if ( !libxl_psr_cmt_get_cache_occupancy(ctx, dominfo->domid,
-                 socketid, &l3_cache_occupancy) )
+        if (!libxl_psr_cmt_get_cache_occupancy(ctx, dominfo->domid, socketid,
+                                               &l3_cache_occupancy))
             printf("%13u KB", l3_cache_occupancy);
     }
 
@@ -7910,8 +7910,9 @@ static int psr_cmt_show_cache_occupancy(uint32_t domid)
     for (socketid = 0; socketid < nr_sockets; socketid++) {
         rc = libxl_psr_cmt_get_l3_cache_size(ctx, socketid, &l3_cache_size);
         if (rc < 0) {
-            fprintf(stderr, "Failed to get system l3 cache size for socket:%d\n",
-                            socketid);
+            fprintf(stderr,
+                    "Failed to get system l3 cache size for socket:%d\n",
+                    socketid);
             return -1;
         }
         printf("%13u KB", l3_cache_size);
-- 
1.7.9.5

[PATCH v2 4/5] tools: code refactoring for MBM

[Chao Peng]

Make some internal routines common so that total/local memory bandwidth
monitoring in the next patch can make use of them.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
 tools/libxl/libxl_psr.c  |   51 ++++++++++++++++++++++++++-----------------
 tools/libxl/xl_cmdimpl.c |   54 +++++++++++++++++++++++++++-------------------
 2 files changed, 63 insertions(+), 42 deletions(-)

diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index 0f2c7e0..42e74d2 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -179,42 +179,53 @@ out:
     return rc;
 }
 
-int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
-                                      uint32_t domid,
-                                      uint32_t socketid,
-                                      uint32_t *l3_cache_occupancy)
+static int libxl__psr_cmt_get_l3_monitoring_data(libxl__gc *gc,
+                                                 uint32_t domid,
+                                                 xc_psr_cmt_type type,
+                                                 uint32_t socketid,
+                                                 uint64_t *data)
 {
-    GC_INIT(ctx);
-
     unsigned int rmid;
-    uint32_t upscaling_factor;
-    uint64_t monitor_data;
     int cpu, rc;
-    xc_psr_cmt_type type;
 
-    rc = xc_psr_cmt_get_domain_rmid(ctx->xch, domid, &rmid);
+    rc = xc_psr_cmt_get_domain_rmid(CTX->xch, domid, &rmid);
     if (rc < 0 || rmid == 0) {
         LOGE(ERROR, "fail to get the domain rmid, "
             "or domain is not attached with platform QoS monitoring service");
-        rc = ERROR_FAIL;
-        goto out;
+        return ERROR_FAIL;
     }
 
     cpu = libxl__pick_socket_cpu(gc, socketid);
     if (cpu < 0) {
         LOGE(ERROR, "failed to get socket cpu");
-        rc = ERROR_FAIL;
-        goto out;
+        return ERROR_FAIL;
     }
 
-    type = XC_PSR_CMT_L3_OCCUPANCY;
-    rc = xc_psr_cmt_get_data(ctx->xch, rmid, cpu, type, &monitor_data);
+    rc = xc_psr_cmt_get_data(CTX->xch, rmid, cpu, type, data);
     if (rc < 0) {
         LOGE(ERROR, "failed to get monitoring data");
-        rc = ERROR_FAIL;
-        goto out;
+        return ERROR_FAIL;
     }
 
+    return rc;
+}
+
+int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
+                                      uint32_t domid,
+                                      uint32_t socketid,
+                                      uint32_t *l3_cache_occupancy)
+{
+    GC_INIT(ctx);
+    uint64_t data;
+    uint32_t upscaling_factor;
+    int rc;
+
+    rc= libxl__psr_cmt_get_l3_monitoring_data(gc, domid,
+                                              XC_PSR_CMT_L3_OCCUPANCY,
+                                              socketid, &data);
+    if (rc < 0)
+            goto out;
+
     rc = xc_psr_cmt_get_l3_upscaling_factor(ctx->xch, &upscaling_factor);
     if (rc < 0) {
         LOGE(ERROR, "failed to get L3 upscaling factor");
@@ -222,8 +233,8 @@ int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
         goto out;
     }
 
-    *l3_cache_occupancy = upscaling_factor * monitor_data / 1024;
-    rc = 0;
+    *l3_cache_occupancy = upscaling_factor * data / 1024;
+
 out:
     GC_FREE;
     return rc;
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 24f3c8d..09ca73e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7846,12 +7846,13 @@ out:
 }
 
 #ifdef LIBXL_HAVE_PSR_CMT
-static void psr_cmt_print_domain_cache_occupancy(libxl_dominfo *dominfo,
-                                                 uint32_t nr_sockets)
+static void psr_cmt_print_domain_l3_info(libxl_dominfo *dominfo,
+                                         libxl_psr_cmt_type type,
+                                         uint32_t nr_sockets)
 {
     char *domain_name;
     uint32_t socketid;
-    uint32_t l3_cache_occupancy;
+    uint32_t data;
 
     if (!libxl_psr_cmt_domain_attached(ctx, dominfo->domid))
         return;
@@ -7861,15 +7862,21 @@ static void psr_cmt_print_domain_cache_occupancy(libxl_dominfo *dominfo,
     free(domain_name);
 
     for (socketid = 0; socketid < nr_sockets; socketid++) {
-        if (!libxl_psr_cmt_get_cache_occupancy(ctx, dominfo->domid, socketid,
-                                               &l3_cache_occupancy))
-            printf("%13u KB", l3_cache_occupancy);
+        switch (type) {
+        case LIBXL_PSR_CMT_TYPE_CACHE_OCCUPANCY:
+            if (!libxl_psr_cmt_get_cache_occupancy(ctx, dominfo->domid,
+                                                   socketid, &data))
+                printf("%13u KB", data);
+            break;
+        default:
+            return;
+        }
     }
 
     printf("\n");
 }
 
-static int psr_cmt_show_cache_occupancy(uint32_t domid)
+static int psr_cmt_show_l3_info(libxl_psr_cmt_type type, uint32_t domid)
 {
     uint32_t i, socketid, nr_sockets, total_rmid;
     uint32_t l3_cache_size;
@@ -7905,19 +7912,22 @@ static int psr_cmt_show_cache_occupancy(uint32_t domid)
         printf("%14s %d", "Socket", socketid);
     printf("\n");
 
-    /* Total L3 cache size */
-    printf("%-46s", "Total L3 Cache Size");
-    for (socketid = 0; socketid < nr_sockets; socketid++) {
-        rc = libxl_psr_cmt_get_l3_cache_size(ctx, socketid, &l3_cache_size);
-        if (rc < 0) {
-            fprintf(stderr,
-                    "Failed to get system l3 cache size for socket:%d\n",
-                    socketid);
-            return -1;
-        }
-        printf("%13u KB", l3_cache_size);
+    if (type == LIBXL_PSR_CMT_TYPE_CACHE_OCCUPANCY) {
+            /* Total L3 cache size */
+            printf("%-46s", "Total L3 Cache Size");
+            for (socketid = 0; socketid < nr_sockets; socketid++) {
+                rc = libxl_psr_cmt_get_l3_cache_size(ctx, socketid,
+                                                     &l3_cache_size);
+                if (rc < 0) {
+                    fprintf(stderr,
+                            "Failed to get system l3 cache size for socket:%d\n",
+                            socketid);
+                    return -1;
+                }
+                printf("%13u KB", l3_cache_size);
+            }
+            printf("\n");
     }
-    printf("\n");
 
     /* Each domain */
     if (domid != INVALID_DOMID) {
@@ -7926,7 +7936,7 @@ static int psr_cmt_show_cache_occupancy(uint32_t domid)
             fprintf(stderr, "Failed to get domain info for %d\n", domid);
             return -1;
         }
-        psr_cmt_print_domain_cache_occupancy(&dominfo, nr_sockets);
+        psr_cmt_print_domain_l3_info(&dominfo, type, nr_sockets);
     }
     else
     {
@@ -7936,7 +7946,7 @@ static int psr_cmt_show_cache_occupancy(uint32_t domid)
             return -1;
         }
         for (i = 0; i < nr_domains; i++)
-            psr_cmt_print_domain_cache_occupancy(list + i, nr_sockets);
+            psr_cmt_print_domain_l3_info(list + i, type, nr_sockets);
         libxl_dominfo_list_free(list, nr_domains);
     }
     return 0;
@@ -7995,7 +8005,7 @@ int main_psr_cmt_show(int argc, char **argv)
 
     switch (type) {
     case LIBXL_PSR_CMT_TYPE_CACHE_OCCUPANCY:
-        ret = psr_cmt_show_cache_occupancy(domid);
+        ret = psr_cmt_show_l3_info(type, domid);
         break;
     default:
         help("psr-cmt-show");
-- 
1.7.9.5

[PATCH v2 5/5] tools: add total/local memory bandwith monitoring

[Chao Peng]

Add Memory Bandwidth Monitoring(MBM) for VMs. Two types of monitoring
are supported: total and local memory bandwidth monitoring. To use it,
CMT should be enabled in hypervisor.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
 docs/man/xl.pod.1             |    9 +++++
 tools/libxc/include/xenctrl.h |    2 ++
 tools/libxc/xc_psr.c          |    8 +++++
 tools/libxl/libxl.h           |    8 +++++
 tools/libxl/libxl_psr.c       |   75 +++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_types.idl   |    2 ++
 tools/libxl/xl_cmdimpl.c      |   18 ++++++++++
 tools/libxl/xl_cmdtable.c     |    4 ++-
 8 files changed, 125 insertions(+), 1 deletion(-)

diff --git a/docs/man/xl.pod.1 b/docs/man/xl.pod.1
index 6b89ba8..0370625 100644
--- a/docs/man/xl.pod.1
+++ b/docs/man/xl.pod.1
@@ -1461,6 +1461,13 @@ is domain level. To monitor a specific domain, just attach the domain id with
 the monitoring service. When the domain doesn't need to be monitored any more,
 detach the domain id from the monitoring service.
 
+Intel Broadwell and later server platforms also offer total/local memory
+bandwidth monitoring. Xen supports per-domain monitoring for these two
+additional monitoring types. Both memory bandwidth monitoring and L3 cache
+occupancy monitoring share the same set of underground monitoring service. Once
+a domain is attached to the monitoring service, monitoring data can be showed
+for any of these monitoring types.
+
 =over 4
 
 =item B<psr-cmt-attach> [I<domain-id>]
@@ -1476,6 +1483,8 @@ detach: Detach the platform shared resource monitoring service from a domain.
 Show monitoring data for a certain domain or all domains. Current supported
 monitor types are:
  - "cache-occupancy": showing the L3 cache occupancy.
+ - "total-mem-bandwidth": showing the total memory bandwidth.
+ - "local-mem-bandwidth": showing the local memory bandwidth.
 
 =back
 
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index c6e9e3e..06366b5 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2688,6 +2688,8 @@ int xc_resource_op(xc_interface *xch, uint32_t nr_ops, xc_resource_op_t *ops);
 #if defined(__i386__) || defined(__x86_64__)
 enum xc_psr_cmt_type {
     XC_PSR_CMT_L3_OCCUPANCY,
+    XC_PSR_CMT_TOTAL_MEM_BANDWIDTH,
+    XC_PSR_CMT_LOCAL_MEM_BANDWIDTH,
 };
 typedef enum xc_psr_cmt_type xc_psr_cmt_type;
 int xc_psr_cmt_attach(xc_interface *xch, uint32_t domid);
diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c
index e3ecc41..99cb754 100644
--- a/tools/libxc/xc_psr.c
+++ b/tools/libxc/xc_psr.c
@@ -23,6 +23,8 @@
 #define IA32_CMT_CTR_ERROR_MASK         (0x3ull << 62)
 
 #define EVTID_L3_OCCUPANCY             0x1
+#define EVTID_TOTAL_MEM_BANDWIDTH      0x2
+#define EVTID_LOCAL_MEM_BANDWIDTH      0x3
 
 int xc_psr_cmt_attach(xc_interface *xch, uint32_t domid)
 {
@@ -175,6 +177,12 @@ int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid, uint32_t cpu,
     case XC_PSR_CMT_L3_OCCUPANCY:
         evtid = EVTID_L3_OCCUPANCY;
         break;
+    case XC_PSR_CMT_TOTAL_MEM_BANDWIDTH:
+        evtid = EVTID_TOTAL_MEM_BANDWIDTH;
+        break;
+    case XC_PSR_CMT_LOCAL_MEM_BANDWIDTH:
+        evtid = EVTID_LOCAL_MEM_BANDWIDTH;
+        break;
     default:
         return -1;
     }
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index d84ff7f..5ab9d0c 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1462,6 +1462,14 @@ int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
                                       uint32_t domid,
                                       uint32_t socketid,
                                       uint32_t *l3_cache_occupancy);
+int libxl_psr_cmt_get_total_mem_bandwidth(libxl_ctx *ctx,
+                                          uint32_t domid,
+                                          uint32_t socketid,
+                                          uint32_t *bandwidth);
+int libxl_psr_cmt_get_local_mem_bandwidth(libxl_ctx *ctx,
+                                          uint32_t domid,
+                                          uint32_t socketid,
+                                          uint32_t *bandwidth);
 #endif
 
 /* misc */
diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index 42e74d2..a0cda89 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -18,6 +18,7 @@
 
 
 #define IA32_QM_CTR_ERROR_MASK         (0x3ul << 62)
+#define MBM_SAMPLE_RETRY_MAX 4
 
 static void libxl__psr_cmt_log_err_msg(libxl__gc *gc, int err)
 {
@@ -240,6 +241,80 @@ out:
     return rc;
 }
 
+static int libxl__psr_cmt_get_mem_bandwidth(libxl__gc *gc,
+                                            uint32_t domid,
+                                            xc_psr_cmt_type type,
+                                            uint32_t socketid,
+                                            uint32_t *bandwidth)
+{
+    uint64_t sample1, sample2;
+    uint32_t upscaling_factor;
+    int retry_attempts = 0;
+    int rc;
+
+retry:
+    rc = libxl__psr_cmt_get_l3_monitoring_data(gc, domid, type, socketid,
+                                               &sample1);
+    if (rc < 0)
+        return ERROR_FAIL;
+
+    usleep(10000);
+
+    rc = libxl__psr_cmt_get_l3_monitoring_data(gc, domid, type, socketid,
+                                               &sample2);
+    if (rc < 0)
+       return ERROR_FAIL;
+
+    if (sample2 < sample1) {
+         if (retry_attempts < MBM_SAMPLE_RETRY_MAX) {
+             retry_attempts++;
+             goto retry;
+         } else {
+             LOGE(ERROR, "event counter overflowed");
+             return ERROR_FAIL;
+         }
+    }
+
+    rc = xc_psr_cmt_get_l3_upscaling_factor(CTX->xch, &upscaling_factor);
+    if (rc < 0) {
+        LOGE(ERROR, "failed to get L3 upscaling factor");
+        return ERROR_FAIL;
+    }
+
+    *bandwidth = (sample2 - sample1) * 100 *  upscaling_factor / 1024;
+    return rc;
+}
+
+int libxl_psr_cmt_get_total_mem_bandwidth(libxl_ctx *ctx,
+                                          uint32_t domid,
+                                          uint32_t socketid,
+                                          uint32_t *bandwidth)
+{
+    GC_INIT(ctx);
+    int rc;
+
+    rc = libxl__psr_cmt_get_mem_bandwidth(gc, domid,
+                                          XC_PSR_CMT_TOTAL_MEM_BANDWIDTH,
+                                          socketid, bandwidth);
+    GC_FREE;
+    return rc;
+}
+
+int libxl_psr_cmt_get_local_mem_bandwidth(libxl_ctx *ctx,
+                                          uint32_t domid,
+                                          uint32_t socketid,
+                                          uint32_t *bandwidth)
+{
+    GC_INIT(ctx);
+    int rc;
+
+    rc = libxl__psr_cmt_get_mem_bandwidth(gc, domid,
+                                          XC_PSR_CMT_LOCAL_MEM_BANDWIDTH,
+                                          socketid, bandwidth);
+    GC_FREE;
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index f7fc695..8029a39 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -693,4 +693,6 @@ libxl_event = Struct("event",[
 
 libxl_psr_cmt_type = Enumeration("psr_cmt_type", [
     (1, "CACHE_OCCUPANCY"),
+    (2, "TOTAL_MEM_BANDWIDTH"),
+    (3, "LOCAL_MEM_BANDWIDTH"),
     ])
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 09ca73e..73a591e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -7868,6 +7868,16 @@ static void psr_cmt_print_domain_l3_info(libxl_dominfo *dominfo,
                                                    socketid, &data))
                 printf("%13u KB", data);
             break;
+        case LIBXL_PSR_CMT_TYPE_TOTAL_MEM_BANDWIDTH:
+            if (!libxl_psr_cmt_get_total_mem_bandwidth(ctx, dominfo->domid,
+                                                       socketid, &data))
+                printf("%11u KB/s", data);
+            break;
+        case LIBXL_PSR_CMT_TYPE_LOCAL_MEM_BANDWIDTH:
+            if (!libxl_psr_cmt_get_local_mem_bandwidth(ctx, dominfo->domid,
+                                                       socketid, &data))
+                printf("%11u KB/s", data);
+            break;
         default:
             return;
         }
@@ -7888,6 +7898,12 @@ static int psr_cmt_show_l3_info(libxl_psr_cmt_type type, uint32_t domid)
         return -1;
     }
 
+    if (!libxl_psr_cmt_type_supported(ctx, type)) {
+        fprintf(stderr, "Monitor type '%s' is not supported in the system\n",
+                libxl_psr_cmt_type_to_string(type));
+        return -1;
+    }
+
     libxl_physinfo_init(&info);
     rc = libxl_get_physinfo(ctx, &info);
     if (rc < 0) {
@@ -8005,6 +8021,8 @@ int main_psr_cmt_show(int argc, char **argv)
 
     switch (type) {
     case LIBXL_PSR_CMT_TYPE_CACHE_OCCUPANCY:
+    case LIBXL_PSR_CMT_TYPE_TOTAL_MEM_BANDWIDTH:
+    case LIBXL_PSR_CMT_TYPE_LOCAL_MEM_BANDWIDTH:
         ret = psr_cmt_show_l3_info(type, domid);
         break;
     default:
diff --git a/tools/libxl/xl_cmdtable.c b/tools/libxl/xl_cmdtable.c
index 4b30d3d..2d8f272 100644
--- a/tools/libxl/xl_cmdtable.c
+++ b/tools/libxl/xl_cmdtable.c
@@ -538,7 +538,9 @@ struct cmd_spec cmd_table[] = {
       "Show Cache Monitoring Technology information",
       "<PSR-CMT-Type> <Domain>",
       "Available monitor types:\n"
-      "\"cache_occupancy\":         Show L3 cache occupancy\n",
+      "\"cache_occupancy\":         Show L3 cache occupancy\n"
+      "\"total_mem_bandwidth\":     Show total memory bandwidth\n"
+      "\"local_mem_bandwidth\":     Show local memory bandwidth\n",
     },
 #endif
 };
-- 
1.7.9.5

Re: [PATCH v2 3/5] tools: correct coding style for psr

[Wei Liu]

On Wed, Jan 07, 2015 at 07:12:03PM +0800, Chao Peng wrote:
> - space: remove space after '(' or before ')' in 'if' condition;
> - indention: align function definition/call arguments;
> 
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

Re: [PATCH v2 2/5] tools: add routine to get CMT L3 event mask

[Andrew Cooper]

On 07/01/15 11:12, Chao Peng wrote:
> This is the tools side wrapper for XEN_SYSCTL_PSR_CMT_get_l3_event_mask
> of XEN_SYSCTL_psr_cmt_op.
>
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> ---
>  tools/libxc/include/xenctrl.h |    1 +
>  tools/libxc/xc_psr.c          |   24 ++++++++++++++++++++++++
>  tools/libxl/libxl.h           |    1 +
>  tools/libxl/libxl_psr.c       |   18 ++++++++++++++++++
>  4 files changed, 44 insertions(+)
>
> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> index 0ad8b8d..96b357c 100644
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -2697,6 +2697,7 @@ int xc_psr_cmt_get_domain_rmid(xc_interface *xch, uint32_t domid,
>  int xc_psr_cmt_get_total_rmid(xc_interface *xch, uint32_t *total_rmid);
>  int xc_psr_cmt_get_l3_upscaling_factor(xc_interface *xch,
>      uint32_t *upscaling_factor);
> +int xc_psr_cmt_get_l3_event_mask(xc_interface *xch, uint32_t *event_mask);
>  int xc_psr_cmt_get_l3_cache_size(xc_interface *xch, uint32_t cpu,
>      uint32_t *l3_cache_size);
>  int xc_psr_cmt_get_data(xc_interface *xch, uint32_t rmid,
> diff --git a/tools/libxc/xc_psr.c b/tools/libxc/xc_psr.c
> index 872e6dc..e76a0f9 100644
> --- a/tools/libxc/xc_psr.c
> +++ b/tools/libxc/xc_psr.c
> @@ -112,6 +112,30 @@ int xc_psr_cmt_get_l3_upscaling_factor(xc_interface *xch,
>      return rc;
>  }
>  
> +int xc_psr_cmt_get_l3_event_mask(xc_interface *xch, uint32_t *event_mask)
> +{
> +    static int val = 0;

This should be uint32_t rather than int.

I am somewhat concerned about multithreaded use of libxc, but this is
not the first issue in libxc, and probably shouldn't be held against
this patch.  As the result of the hypercall is going to be the same, the
worse that a race could achieve is a wasted hypercall.

> +    int rc;
> +    DECLARE_SYSCTL;
> +
> +    if ( val )
> +    {
> +        *event_mask = val;
> +        return 0;
> +    }
> +
> +    sysctl.cmd = XEN_SYSCTL_psr_cmt_op;
> +    sysctl.u.psr_cmt_op.cmd =
> +        XEN_SYSCTL_PSR_CMT_get_l3_event_mask;
> +    sysctl.u.psr_cmt_op.flags = 0;
> +
> +    rc = xc_sysctl(xch, &sysctl);
> +    if ( !rc )
> +        val = *event_mask = sysctl.u.psr_cmt_op.u.data;
> +
> +    return rc;
> +}
> +
>  int xc_psr_cmt_get_l3_cache_size(xc_interface *xch, uint32_t cpu,
>                                        uint32_t *l3_cache_size)
>  {
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 0a123f1..42ace76 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -1453,6 +1453,7 @@ int libxl_psr_cmt_attach(libxl_ctx *ctx, uint32_t domid);
>  int libxl_psr_cmt_detach(libxl_ctx *ctx, uint32_t domid);
>  int libxl_psr_cmt_domain_attached(libxl_ctx *ctx, uint32_t domid);
>  int libxl_psr_cmt_enabled(libxl_ctx *ctx);
> +int libxl_psr_cmt_type_supported(libxl_ctx *ctx, libxl_psr_cmt_type type);
>  int libxl_psr_cmt_get_total_rmid(libxl_ctx *ctx, uint32_t *total_rmid);
>  int libxl_psr_cmt_get_l3_cache_size(libxl_ctx *ctx, uint32_t socketid,
>      uint32_t *l3_cache_size);
> diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> index 0437465..3018a0d 100644
> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
> @@ -120,6 +120,24 @@ int libxl_psr_cmt_enabled(libxl_ctx *ctx)
>      return xc_psr_cmt_enabled(ctx->xch);
>  }
>  
> +int libxl_psr_cmt_type_supported(libxl_ctx *ctx, libxl_psr_cmt_type type)
> +{
> +    GC_INIT(ctx);
> +    uint32_t event_mask;
> +    int ret;

The libxl CODING_SYTLE states that this "ret" should be "rc"

~Andrew

> +
> +    ret = xc_psr_cmt_get_l3_event_mask(ctx->xch, &event_mask);
> +    if (ret < 0) {
> +        libxl__psr_cmt_log_err_msg(gc, errno);
> +        ret = 0;
> +    } else {
> +        ret = event_mask & (1 << (type - 1));
> +    }
> +
> +    GC_FREE;
> +    return ret;
> +}
> +
>  int libxl_psr_cmt_get_total_rmid(libxl_ctx *ctx, uint32_t *total_rmid)
>  {
>      GC_INIT(ctx);

Re: [PATCH v2 4/5] tools: code refactoring for MBM

[Wei Liu]

On Wed, Jan 07, 2015 at 07:12:04PM +0800, Chao Peng wrote:
[...]
> -int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
> -                                      uint32_t domid,
> -                                      uint32_t socketid,
> -                                      uint32_t *l3_cache_occupancy)
> +static int libxl__psr_cmt_get_l3_monitoring_data(libxl__gc *gc,
> +                                                 uint32_t domid,
> +                                                 xc_psr_cmt_type type,
> +                                                 uint32_t socketid,
> +                                                 uint64_t *data)
>  {
> -    GC_INIT(ctx);
> -
>      unsigned int rmid;
> -    uint32_t upscaling_factor;
> -    uint64_t monitor_data;
>      int cpu, rc;
> -    xc_psr_cmt_type type;
>  
> -    rc = xc_psr_cmt_get_domain_rmid(ctx->xch, domid, &rmid);
> +    rc = xc_psr_cmt_get_domain_rmid(CTX->xch, domid, &rmid);
>      if (rc < 0 || rmid == 0) {
>          LOGE(ERROR, "fail to get the domain rmid, "
>              "or domain is not attached with platform QoS monitoring service");
> -        rc = ERROR_FAIL;
> -        goto out;
> +        return ERROR_FAIL;

Please retain the "goto out" idiom if possible.

>      }
>  
>      cpu = libxl__pick_socket_cpu(gc, socketid);
>      if (cpu < 0) {
>          LOGE(ERROR, "failed to get socket cpu");
> -        rc = ERROR_FAIL;
> -        goto out;
> +        return ERROR_FAIL;
>      }
>  
> -    type = XC_PSR_CMT_L3_OCCUPANCY;
> -    rc = xc_psr_cmt_get_data(ctx->xch, rmid, cpu, type, &monitor_data);
> +    rc = xc_psr_cmt_get_data(CTX->xch, rmid, cpu, type, data);
>      if (rc < 0) {
>          LOGE(ERROR, "failed to get monitoring data");
> -        rc = ERROR_FAIL;
> -        goto out;
> +        return ERROR_FAIL;
>      }
>  
> +    return rc;
> +}
> +
> +int libxl_psr_cmt_get_cache_occupancy(libxl_ctx *ctx,
> +                                      uint32_t domid,
> +                                      uint32_t socketid,
> +                                      uint32_t *l3_cache_occupancy)
> +{
> +    GC_INIT(ctx);
> +    uint64_t data;
> +    uint32_t upscaling_factor;
> +    int rc;
> +
> +    rc= libxl__psr_cmt_get_l3_monitoring_data(gc, domid,

"rc ="

Wei.

Re: [PATCH v2 5/5] tools: add total/local memory bandwith monitoring

[Wei Liu]

On Wed, Jan 07, 2015 at 07:12:05PM +0800, Chao Peng wrote:
[...]
> +static int libxl__psr_cmt_get_mem_bandwidth(libxl__gc *gc,
> +                                            uint32_t domid,
> +                                            xc_psr_cmt_type type,
> +                                            uint32_t socketid,
> +                                            uint32_t *bandwidth)
> +{
> +    uint64_t sample1, sample2;
> +    uint32_t upscaling_factor;
> +    int retry_attempts = 0;
> +    int rc;
> +
> +retry:
> +    rc = libxl__psr_cmt_get_l3_monitoring_data(gc, domid, type, socketid,
> +                                               &sample1);
> +    if (rc < 0)
> +        return ERROR_FAIL;
> +
> +    usleep(10000);
> +
> +    rc = libxl__psr_cmt_get_l3_monitoring_data(gc, domid, type, socketid,
> +                                               &sample2);
> +    if (rc < 0)
> +       return ERROR_FAIL;
> +
> +    if (sample2 < sample1) {
> +         if (retry_attempts < MBM_SAMPLE_RETRY_MAX) {
> +             retry_attempts++;
> +             goto retry;
> +         } else {
> +             LOGE(ERROR, "event counter overflowed");
> +             return ERROR_FAIL;
> +         }
> +    }
> +

Two things:

1. Use for/while for retry loop, don't use goto.
2. Use "goto out" idiom for error handling.

The "goto out" idiom is part of documented libxl error handling section
in tools/libxl/CODING_STYLE. You might find that document useful
reference.

Wei.

Re: [PATCH v2 2/5] tools: add routine to get CMT L3 event mask

[Ian Jackson]

Andrew Cooper writes ("Re: [Xen-devel] [PATCH v2 2/5] tools: add routine to get CMT L3 event mask"):
> On 07/01/15 11:12, Chao Peng wrote:
> > +int xc_psr_cmt_get_l3_event_mask(xc_interface *xch, uint32_t *event_mask)
> > +{
> > +    static int val = 0;
> 
> This should be uint32_t rather than int.
> 
> I am somewhat concerned about multithreaded use of libxc, but this is
> not the first issue in libxc, and probably shouldn't be held against
> this patch.

On the contrary, this is quite bad.  libxc should be properly
reentrant and I think it generally is.  If you are aware of other
global variables of this kind please do ...

... I have just seen that the existing version of xc_psr.c has this
problem already.

IMO this is a serious bug.  Why was it made static before ?

>  As the result of the hypercall is going to be the same, the
> worse that a race could achieve is a wasted hypercall.

This kind of analysis is unfounded in the presence of modern compilers
with aggressive optimisations.  At the very least, if you're going to
do some caching like this, it needs a lock around it.

Ian.

Re: [PATCH v2 2/5] tools: add routine to get CMT L3 event mask

[Wei Liu]

On Wed, Jan 07, 2015 at 04:37:40PM +0000, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [PATCH v2 2/5] tools: add routine to get CMT L3 event mask"):
> > On 07/01/15 11:12, Chao Peng wrote:
> > > +int xc_psr_cmt_get_l3_event_mask(xc_interface *xch, uint32_t *event_mask)
> > > +{
> > > +    static int val = 0;
> > 
> > This should be uint32_t rather than int.
> > 
> > I am somewhat concerned about multithreaded use of libxc, but this is
> > not the first issue in libxc, and probably shouldn't be held against
> > this patch.
> 
> On the contrary, this is quite bad.  libxc should be properly
> reentrant and I think it generally is.  If you are aware of other
> global variables of this kind please do ...
> 

xc_misc.c has two similar variables.

> ... I have just seen that the existing version of xc_psr.c has this
> problem already.
> 
> IMO this is a serious bug.  Why was it made static before ?
> 
> >  As the result of the hypercall is going to be the same, the
> > worse that a race could achieve is a wasted hypercall.
> 
> This kind of analysis is unfounded in the presence of modern compilers
> with aggressive optimisations.  At the very least, if you're going to
> do some caching like this, it needs a lock around it.
> 
> Ian.

Re: [PATCH v2 2/5] tools: add routine to get CMT L3 event mask

[Ian Campbell]

On Wed, 2015-01-07 at 16:37 +0000, Ian Jackson wrote:
> >  As the result of the hypercall is going to be the same, the
> > worse that a race could achieve is a wasted hypercall.
> 
> This kind of analysis is unfounded in the presence of modern compilers
> with aggressive optimisations.  At the very least, if you're going to
> do some caching like this, it needs a lock around it.

Even for a hypercall which is just "return 42" (at least between boots)?

Ian.

Re: [PATCH v2 2/5] tools: add routine to get CMT L3 event mask

[Andrew Cooper]

On 07/01/15 16:37, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [PATCH v2 2/5] tools: add routine to get CMT L3 event mask"):
>> On 07/01/15 11:12, Chao Peng wrote:
>>> +int xc_psr_cmt_get_l3_event_mask(xc_interface *xch, uint32_t *event_mask)
>>> +{
>>> +    static int val = 0;
>> This should be uint32_t rather than int.
>>
>> I am somewhat concerned about multithreaded use of libxc, but this is
>> not the first issue in libxc, and probably shouldn't be held against
>> this patch.
> On the contrary, this is quite bad.  libxc should be properly
> reentrant and I think it generally is.  If you are aware of other
> global variables of this kind please do ...
>
> ... I have just seen that the existing version of xc_psr.c has this
> problem already.
>
> IMO this is a serious bug.  Why was it made static before ?

The idea is to caching a constant value from Xen.

>From a quick grep (and not very thorough):

Other culprits are xc_get_max_nodes(), xc_get_max_cpus(), 4 instances in
xc_psr.c and most things in xc_offline_page.c which appears to have
static structures for domain context.  The "pluggable loader"
infrastructure in xc_dom.c also appears to be thread-unsafe.

xc_dom_decompress_unsafe.c also uses static data, but "unsafe" in the
name might be a sufficient guard?

There are quite a few files which have static data structures which
appear to be able to get away with being static const, and should
probably move in that direction.

>
>>  As the result of the hypercall is going to be the same, the
>> worse that a race could achieve is a wasted hypercall.
> This kind of analysis is unfounded in the presence of modern compilers
> with aggressive optimisations.  At the very least, if you're going to
> do some caching like this, it needs a lock around it.

No aggressively optimising compiler is going to perform partial writes
on a naturally aligned integer, so I stand by my comment when applied to
the common case.

A dumb compiler on the other hand might, and C/POSIX make no guarantees
in this regard, so the issue is indeed more serious than my initial
analysis.

~Andrew

Re: [PATCH v2 2/5] tools: add routine to get CMT L3 event mask

[Ian Jackson]

Andrew Cooper writes ("Re: [Xen-devel] [PATCH v2 2/5] tools: add routine to get CMT L3 event mask"):
> Other culprits are xc_get_max_nodes(), xc_get_max_cpus(), 4 instances in
> xc_psr.c and most things in xc_offline_page.c which appears to have
> static structures for domain context.  The "pluggable loader"
> infrastructure in xc_dom.c also appears to be thread-unsafe.
>
> xc_dom_decompress_unsafe.c also uses static data, but "unsafe" in the
> name might be a sufficient guard?

I will look at these tomorrow.

> No aggressively optimising compiler is going to perform partial writes
> on a naturally aligned integer, so I stand by my comment when applied to
> the common case.

You misunderstand.  An aggressively optimising compiler might be able
to "prove" (perhaps through whole program analysis - we have link-time
optimisation nowadays) various falsehoods about the way these
variables are used.

The resulting generated machine code might be arbitrarily bad, up to
and including missing important parts of the whole program.

I'm not aware of any compilers which currently take "advantge" of
thread safety "bugs" (really, just spec-violations) but I think this
is just a matter of time.

Ian.

Re: [PATCH v2 2/5] tools: add routine to get CMT L3 event mask

[Chao Peng]

On Wed, Jan 07, 2015 at 09:43:44PM +0000, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [PATCH v2 2/5] tools: add routine to get CMT L3 event mask"):
> > Other culprits are xc_get_max_nodes(), xc_get_max_cpus(), 4 instances in
> > xc_psr.c and most things in xc_offline_page.c which appears to have
> > static structures for domain context.  The "pluggable loader"
> > infrastructure in xc_dom.c also appears to be thread-unsafe.
> >
> > xc_dom_decompress_unsafe.c also uses static data, but "unsafe" in the
> > name might be a sufficient guard?
> 
> I will look at these tomorrow.
> 
> > No aggressively optimising compiler is going to perform partial writes
> > on a naturally aligned integer, so I stand by my comment when applied to
> > the common case.
> 
> You misunderstand.  An aggressively optimising compiler might be able
> to "prove" (perhaps through whole program analysis - we have link-time
> optimisation nowadays) various falsehoods about the way these
> variables are used.
> 
> The resulting generated machine code might be arbitrarily bad, up to
> and including missing important parts of the whole program.
Does this really happen, as for this example?
> 
> I'm not aware of any compilers which currently take "advantge" of
> thread safety "bugs" (really, just spec-violations) but I think this
> is just a matter of time.
I think this can be the issue.  At least we should be very careful.
Or to build a common mechanism in libxc to cache something safely, using
that to replace all the static places current we have.


While I personally agree to make xc re-enterable. The cache
responsibility then can be move to top caller.

For this example, remove static in libxc and the caching for const
hypercall return value is then moved to xl. It can just 'cache' the value
in local variable. While this is not the most performance way as at least
one hypercall is needed for each xl command invocation.

Chao

> 
> Ian.