[PATCH for Xen 4.6 0/5] Several PSR fixes in libxl

[PATCH for Xen 4.6 0/5] Several PSR fixes in libxl

[Chao Peng]

The patch basically contains several PSR fixes in libxl.
patch1-3: fix the socket display error in certain hotplug case.
patch4:   fix a minor range check.
patch5:   improve the PSR document.

Detailed problem and fix please see commit message.

Chao Peng (5):
  tools/libxl: introduce libxl_socket_bitmap_fill
  tools/libxl: fix socket display error for CMT
  tools/libxl: return socket id from libxl_psr_cat_get_l3_info
  tools/libxl: fix range check in main_psr_cat_cbm_set
  docs: make xl-psr.markdown more precise

 docs/misc/xl-psr.markdown   |  8 ++---
 tools/libxl/libxl.h         |  7 ++--
 tools/libxl/libxl_psr.c     | 21 +++++++++---
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/libxl_utils.c   | 21 ++++++++++++
 tools/libxl/libxl_utils.h   |  2 ++
 tools/libxl/xl_cmdimpl.c    | 81 +++++++++++++++++++++++----------------------
 7 files changed, 90 insertions(+), 51 deletions(-)

-- 
1.9.1

[PATCH for Xen 4.6 1/5] tools/libxl: introduce libxl_socket_bitmap_fill

[Chao Peng]

It sets the bit on the given bitmap if the corresponding socket is
available and clears the bit when the corresponding socket is not
available.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
 tools/libxl/libxl.h       |  7 ++++---
 tools/libxl/libxl_utils.c | 21 +++++++++++++++++++++
 tools/libxl/libxl_utils.h |  2 ++
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 5f9047c..5a91687 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -807,11 +807,12 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
 #define LIBXL_HAVE_PCITOPOLOGY 1
 
 /*
- * LIBXL_HAVE_SOCKET_BITMAP_ALLOC
+ * LIBXL_HAVE_SOCKET_BITMAP
  *
- * If this is defined, then libxl_socket_bitmap_alloc exists.
+ * If this is defined, then libxl_socket_bitmap_alloc and
+ * libxl_socket_bitmap_Fill exist.
  */
-#define LIBXL_HAVE_SOCKET_BITMAP_ALLOC 1
+#define LIBXL_HAVE_SOCKET_BITMAP 1
 
 /*
  * LIBXL_HAVE_SRM_V2
diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
index bfc9699..557f279 100644
--- a/tools/libxl/libxl_utils.c
+++ b/tools/libxl/libxl_utils.c
@@ -886,6 +886,27 @@ int libxl_socket_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *socketmap,
 
 }
 
+int libxl_socket_bitmap_fill(libxl_ctx *ctx, libxl_bitmap *socketmap)
+{
+    libxl_cputopology *tinfo = NULL;
+    int nr_cpus = 0, i, rc = 0;
+
+    tinfo = libxl_get_cpu_topology(ctx, &nr_cpus);
+    if (tinfo == NULL) {
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    libxl_bitmap_set_none(socketmap);
+    for (i = 0; i < nr_cpus; i++)
+        if (tinfo[i].socket != XEN_INVALID_SOCKET_ID
+            && !libxl_bitmap_test(socketmap, tinfo[i].socket))
+            libxl_bitmap_set(socketmap, tinfo[i].socket);
+ out:
+    libxl_cputopology_list_free(tinfo, nr_cpus);
+    return rc;
+
+}
 int libxl_nodemap_to_cpumap(libxl_ctx *ctx,
                             const libxl_bitmap *nodemap,
                             libxl_bitmap *cpumap)
diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
index 1e5ca8a..b55099a 100644
--- a/tools/libxl/libxl_utils.h
+++ b/tools/libxl/libxl_utils.h
@@ -143,6 +143,8 @@ int libxl_node_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *nodemap,
                             int max_nodes);
 int libxl_socket_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *socketmap,
                               int max_sockets);
+/* Fill socketmap with the CPU topology information on the system. */
+int libxl_socket_bitmap_fill(libxl_ctx *ctx, libxl_bitmap *socketmap);
 
 /* Populate cpumap with the cpus spanned by the nodes in nodemap */
 int libxl_nodemap_to_cpumap(libxl_ctx *ctx,
-- 
1.9.1

[PATCH for Xen 4.6 2/5] tools/libxl: fix socket display error for CMT

[Chao Peng]

When displaying the CMT information for all the sockets, we assume socket
number is continuous. This is not true in the hotplug case. For instance,
when the 3rd socket is plugged out on a 4-socket system, the available
sockets numbers are 1,2,4 but current we will display the CMT
information for socket 1,2,3.

The fix is getting the socket bitmap for all the sockets on the system
first and then displaying CMT information for_each_set_bit in that bitmap.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
 tools/libxl/xl_cmdimpl.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 2706759..c72d3df 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -8192,7 +8192,7 @@ static int psr_cmt_get_mem_bandwidth(uint32_t domid,
 
 static void psr_cmt_print_domain_info(libxl_dominfo *dominfo,
                                       libxl_psr_cmt_type type,
-                                      uint32_t nr_sockets)
+                                      libxl_bitmap *socketmap)
 {
     char *domain_name;
     uint32_t socketid;
@@ -8205,7 +8205,7 @@ static void psr_cmt_print_domain_info(libxl_dominfo *dominfo,
     printf("%-40s %5d", domain_name, dominfo->domid);
     free(domain_name);
 
-    for (socketid = 0; socketid < nr_sockets; socketid++) {
+    libxl_for_each_set_bit(socketid, *socketmap) {
         switch (type) {
         case LIBXL_PSR_CMT_TYPE_CACHE_OCCUPANCY:
             if (!libxl_psr_cmt_get_sample(ctx, dominfo->domid, type, socketid,
@@ -8228,9 +8228,9 @@ static void psr_cmt_print_domain_info(libxl_dominfo *dominfo,
 
 static int psr_cmt_show(libxl_psr_cmt_type type, uint32_t domid)
 {
-    uint32_t i, socketid, nr_sockets, total_rmid;
+    uint32_t i, socketid, total_rmid;
     uint32_t l3_cache_size;
-    libxl_physinfo info;
+    libxl_bitmap socketmap;
     int rc, nr_domains;
 
     if (!libxl_psr_cmt_enabled(ctx)) {
@@ -8244,41 +8244,38 @@ static int psr_cmt_show(libxl_psr_cmt_type type, uint32_t domid)
         return -1;
     }
 
-    libxl_physinfo_init(&info);
-    rc = libxl_get_physinfo(ctx, &info);
+    libxl_socket_bitmap_alloc(ctx, &socketmap, 0);
+    rc = libxl_socket_bitmap_fill(ctx, &socketmap);
     if (rc < 0) {
-        fprintf(stderr, "Failed getting physinfo, rc: %d\n", rc);
-        libxl_physinfo_dispose(&info);
-        return -1;
+        fprintf(stderr, "Failed getting available sockets, rc: %d\n", rc);
+        goto out;
     }
-    nr_sockets = info.nr_cpus / info.threads_per_core / info.cores_per_socket;
-    libxl_physinfo_dispose(&info);
 
     rc = libxl_psr_cmt_get_total_rmid(ctx, &total_rmid);
     if (rc < 0) {
         fprintf(stderr, "Failed to get max RMID value\n");
-        return -1;
+        goto out;
     }
 
     printf("Total RMID: %d\n", total_rmid);
 
     /* Header */
     printf("%-40s %5s", "Name", "ID");
-    for (socketid = 0; socketid < nr_sockets; socketid++)
+    libxl_for_each_set_bit(socketid, socketmap)
         printf("%14s %d", "Socket", socketid);
     printf("\n");
 
     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++) {
+            libxl_for_each_set_bit(socketid, socketmap) {
                 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;
+                    goto out;
                 }
                 printf("%13u KB", l3_cache_size);
             }
@@ -8292,9 +8289,10 @@ static int psr_cmt_show(libxl_psr_cmt_type type, uint32_t domid)
         libxl_dominfo_init(&dominfo);
         if (libxl_domain_info(ctx, &dominfo, domid)) {
             fprintf(stderr, "Failed to get domain info for %d\n", domid);
-            return -1;
+            rc = -1;
+            goto out;
         }
-        psr_cmt_print_domain_info(&dominfo, type, nr_sockets);
+        psr_cmt_print_domain_info(&dominfo, type, &socketmap);
         libxl_dominfo_dispose(&dominfo);
     }
     else
@@ -8302,13 +8300,17 @@ static int psr_cmt_show(libxl_psr_cmt_type type, uint32_t domid)
         libxl_dominfo *list;
         if (!(list = libxl_list_domain(ctx, &nr_domains))) {
             fprintf(stderr, "Failed to get domain info for domain list.\n");
-            return -1;
+            rc = -1;
+            goto out;
         }
         for (i = 0; i < nr_domains; i++)
-            psr_cmt_print_domain_info(list + i, type, nr_sockets);
+            psr_cmt_print_domain_info(list + i, type, &socketmap);
         libxl_dominfo_list_free(list, nr_domains);
     }
-    return 0;
+
+out:
+    libxl_bitmap_dispose(&socketmap);
+    return rc;
 }
 
 int main_psr_cmt_attach(int argc, char **argv)
-- 
1.9.1

[PATCH for Xen 4.6 3/5] tools/libxl: return socket id from libxl_psr_cat_get_l3_info

[Chao Peng]

The entries returned from libxl_psr_cat_get_l3_info are assumed
to be socket-continuous. But this is not true in the hotplug case.

This patch gets the socket bitmap for all the sockets on the system
first and stores the socket id in the structure libxl_psr_cat_info in
libxl_psr_cat_get_l3_info. The xl or similar consumers then can display
socket information correctly. For the sake of future extention, the
field added to libxl_psr_cat_info is named as target_id.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
 tools/libxl/libxl_psr.c     | 21 ++++++++++++++++-----
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/xl_cmdimpl.c    | 37 +++++++++++++++++++------------------
 3 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
index 3378239..10e1113 100644
--- a/tools/libxl/libxl_psr.c
+++ b/tools/libxl/libxl_psr.c
@@ -339,7 +339,8 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, libxl_psr_cat_info **info,
 {
     GC_INIT(ctx);
     int rc;
-    int i, nr_sockets;
+    int i = 0, socket, nr_sockets;
+    libxl_bitmap socketmap;
     libxl_psr_cat_info *ptr;
 
     rc = libxl__count_physical_sockets(gc, &nr_sockets);
@@ -348,21 +349,31 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, libxl_psr_cat_info **info,
         goto out;
     }
 
+    libxl_socket_bitmap_alloc(ctx, &socketmap, nr_sockets);
+    rc = libxl_socket_bitmap_fill(ctx, &socketmap);
+    if (rc < 0) {
+        LOGE(ERROR, "failed to get available sockets");
+        goto out;
+    }
+
     ptr = libxl__malloc(NOGC, nr_sockets * sizeof(libxl_psr_cat_info));
 
-    for (i = 0; i < nr_sockets; i++) {
-        if (xc_psr_cat_get_l3_info(ctx->xch, i, &ptr[i].cos_max,
-                                                &ptr[i].cbm_len)) {
+    libxl_for_each_set_bit(socket, socketmap) {
+        ptr[i].target_id = socket;
+        if (xc_psr_cat_get_l3_info(ctx->xch, socket, &ptr[i].cos_max,
+                                                     &ptr[i].cbm_len)) {
             libxl__psr_cat_log_err_msg(gc, errno);
             rc = ERROR_FAIL;
             free(ptr);
             goto out;
         }
+        i++;
     }
 
     *info = ptr;
-    *nr = nr_sockets;
+    *nr = i;
 out:
+    libxl_bitmap_dispose(&socketmap);
     GC_FREE;
     return rc;
 }
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 9f6ec00..c7bd425 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -792,6 +792,7 @@ libxl_psr_cbm_type = Enumeration("psr_cbm_type", [
     ])
 
 libxl_psr_cat_info = Struct("psr_cat_info", [
+    ("target_id", uint32),
     ("cos_max", uint32),
     ("cbm_len", uint32),
     ])
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c72d3df..cec0d2c 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -8383,35 +8383,37 @@ int main_psr_cmt_show(int argc, char **argv)
 static int psr_cat_hwinfo(void)
 {
     int rc;
-    int socketid, nr_sockets;
+    int i, nr;
     uint32_t l3_cache_size;
     libxl_psr_cat_info *info;
 
     printf("Cache Allocation Technology (CAT):\n");
 
-    rc = libxl_psr_cat_get_l3_info(ctx, &info, &nr_sockets);
+    rc = libxl_psr_cat_get_l3_info(ctx, &info, &nr);
     if (rc) {
         fprintf(stderr, "Failed to get cat info\n");
         return rc;
     }
 
-    for (socketid = 0; socketid < nr_sockets; socketid++) {
-        rc = libxl_psr_cmt_get_l3_cache_size(ctx, socketid, &l3_cache_size);
+    for (i = 0; i < nr; i++) {
+        rc = libxl_psr_cmt_get_l3_cache_size(ctx, info->target_id,
+                                             &l3_cache_size);
         if (rc) {
             fprintf(stderr, "Failed to get l3 cache size for socket:%d\n",
-                    socketid);
+                    info->target_id);
             goto out;
         }
-        printf("%-16s: %u\n", "Socket ID", socketid);
+        printf("%-16s: %u\n", "Socket ID", info->target_id);
         printf("%-16s: %uKB\n", "L3 Cache", l3_cache_size);
         printf("%-16s: %u\n", "Maximum COS", info->cos_max);
         printf("%-16s: %u\n", "CBM length", info->cbm_len);
         printf("%-16s: %#llx\n", "Default CBM",
                (1ull << info->cbm_len) - 1);
+        info++;
     }
 
 out:
-    libxl_psr_cat_info_list_free(info, nr_sockets);
+    libxl_psr_cat_info_list_free(info, nr);
     return rc;
 }
 
@@ -8453,47 +8455,46 @@ static int psr_cat_print_domain_cbm(uint32_t domid, uint32_t socketid)
     return 0;
 }
 
-static int psr_cat_print_socket(uint32_t domid, uint32_t socketid,
-                                libxl_psr_cat_info *info)
+static int psr_cat_print_socket(uint32_t domid, libxl_psr_cat_info *info)
 {
     int rc;
     uint32_t l3_cache_size;
 
-    rc = libxl_psr_cmt_get_l3_cache_size(ctx, socketid, &l3_cache_size);
+    rc = libxl_psr_cmt_get_l3_cache_size(ctx, info->target_id, &l3_cache_size);
     if (rc) {
         fprintf(stderr, "Failed to get l3 cache size for socket:%d\n",
-                socketid);
+                info->target_id);
         return -1;
     }
 
-    printf("%-16s: %u\n", "Socket ID", socketid);
+    printf("%-16s: %u\n", "Socket ID", info->target_id);
     printf("%-16s: %uKB\n", "L3 Cache", l3_cache_size);
     printf("%-16s: %#llx\n", "Default CBM", (1ull << info->cbm_len) - 1);
     printf("%5s%25s%16s\n", "ID", "NAME", "CBM");
 
-    return psr_cat_print_domain_cbm(domid, socketid);
+    return psr_cat_print_domain_cbm(domid, info->target_id);
 }
 
 static int psr_cat_show(uint32_t domid)
 {
-    int socketid, nr_sockets;
+    int i, nr;
     int rc;
     libxl_psr_cat_info *info;
 
-    rc = libxl_psr_cat_get_l3_info(ctx, &info, &nr_sockets);
+    rc = libxl_psr_cat_get_l3_info(ctx, &info, &nr);
     if (rc) {
         fprintf(stderr, "Failed to get cat info\n");
         return rc;
     }
 
-    for (socketid = 0; socketid < nr_sockets; socketid++) {
-        rc = psr_cat_print_socket(domid, socketid, info + socketid);
+    for (i = 0; i < nr; i++) {
+        rc = psr_cat_print_socket(domid, info + i);
         if (rc)
             goto out;
     }
 
 out:
-    libxl_psr_cat_info_list_free(info, nr_sockets);
+    libxl_psr_cat_info_list_free(info, nr);
     return rc;
 }
 
-- 
1.9.1

[PATCH for Xen 4.6 4/5] tools/libxl: fix range check in main_psr_cat_cbm_set

[Chao Peng]

The 'end' should be inclusive.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
 tools/libxl/xl_cmdimpl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index cec0d2c..045b70e 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -8525,7 +8525,7 @@ int main_psr_cat_cbm_set(int argc, char **argv)
         len = libxl_string_list_length(&socket_list);
         for (i = 0; i < len; i++) {
             parse_range(socket_list[i], &start, &end);
-            for (j = start; j < end; j++)
+            for (j = start; j <= end; j++)
                 libxl_bitmap_set(&target_map, j);
         }
 
-- 
1.9.1

[PATCH for Xen 4.6 5/5] docs: make xl-psr.markdown more precise

[Chao Peng]

Make the chapter name and reference url more precise. The chapter number
is dropped as it can be confusing when it gets changed in the referred
document.

Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
---
 docs/misc/xl-psr.markdown | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/docs/misc/xl-psr.markdown b/docs/misc/xl-psr.markdown
index 3545912..737f0f7 100644
--- a/docs/misc/xl-psr.markdown
+++ b/docs/misc/xl-psr.markdown
@@ -14,7 +14,7 @@ tracks cache utilization of memory accesses according to the RMID and reports
 monitored data via a counter register.
 
 For more detailed information please refer to Intel SDM chapter
-"17.14 - Platform Shared Resource Monitoring: Cache Monitoring Technology".
+"Platform Shared Resource Monitoring: Cache Monitoring Technology".
 
 In Xen's implementation, each domain in the system can be assigned a RMID
 independently, while RMID=0 is reserved for monitoring domains that don't
@@ -52,7 +52,7 @@ event type to monitor system total/local memory bandwidth. The same RMID can
 be used to monitor both cache usage and memory bandwidth at the same time.
 
 For more detailed information please refer to Intel SDM chapter
-"17.14 - Platform Shared Resource Monitoring: Cache Monitoring Technology".
+"Overview of Cache Monitoring Technology and Memory Bandwidth Monitoring".
 
 In Xen's implementation, MBM shares the same set of underlying monitoring
 service with CMT and can be used to monitor memory bandwidth on a per domain
@@ -92,7 +92,7 @@ For example, assuming a system with 8 portions and 3 domains:
    access to one quarter each.
 
 For more detailed information please refer to Intel SDM chapter
-"17.15 - Platform Shared Resource Control: Cache Allocation Technology".
+"Platform Shared Resource Control: Cache Allocation Technology".
 
 In Xen's implementation, CBM can be configured with libxl/xl interfaces but
 COS is maintained in hypervisor only. The cache partition granularity is per
@@ -130,4 +130,4 @@ Per domain CBM settings can be shown by:
 ## Reference
 
 [1] Intel SDM
-(http://www.intel.com/content/www/us/en/processors/architectures-software-developer-manuals.html).
+(http://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-system-programming-manual-325384.pdf).
-- 
1.9.1

Re: [PATCH for Xen 4.6 1/5] tools/libxl: introduce libxl_socket_bitmap_fill

[Wei Liu]

On Mon, Sep 28, 2015 at 07:54:49PM +0800, Chao Peng wrote:
> It sets the bit on the given bitmap if the corresponding socket is
> available and clears the bit when the corresponding socket is not
> available.
> 
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> ---
>  tools/libxl/libxl.h       |  7 ++++---
>  tools/libxl/libxl_utils.c | 21 +++++++++++++++++++++
>  tools/libxl/libxl_utils.h |  2 ++
>  3 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 5f9047c..5a91687 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -807,11 +807,12 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
>  #define LIBXL_HAVE_PCITOPOLOGY 1
>  
>  /*
> - * LIBXL_HAVE_SOCKET_BITMAP_ALLOC
> + * LIBXL_HAVE_SOCKET_BITMAP
>   *
> - * If this is defined, then libxl_socket_bitmap_alloc exists.
> + * If this is defined, then libxl_socket_bitmap_alloc and
> + * libxl_socket_bitmap_Fill exist.

_Fill -> _fill.

>   */
> -#define LIBXL_HAVE_SOCKET_BITMAP_ALLOC 1
> +#define LIBXL_HAVE_SOCKET_BITMAP 1
>  

Normally we don't / can't delete existing macros. However this macro was
introduced in this cycle, so we have the liberty to change it before
releasing.

If for any reason this series doesn't make it in 4.6, you would need to
come up with a different macro name.

>  /*
>   * LIBXL_HAVE_SRM_V2
> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index bfc9699..557f279 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -886,6 +886,27 @@ int libxl_socket_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *socketmap,
>  
>  }
>  
> +int libxl_socket_bitmap_fill(libxl_ctx *ctx, libxl_bitmap *socketmap)
> +{
> +    libxl_cputopology *tinfo = NULL;
> +    int nr_cpus = 0, i, rc = 0;
> +
> +    tinfo = libxl_get_cpu_topology(ctx, &nr_cpus);
> +    if (tinfo == NULL) {
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    libxl_bitmap_set_none(socketmap);
> +    for (i = 0; i < nr_cpus; i++)
> +        if (tinfo[i].socket != XEN_INVALID_SOCKET_ID
> +            && !libxl_bitmap_test(socketmap, tinfo[i].socket))
> +            libxl_bitmap_set(socketmap, tinfo[i].socket);
> + out:
> +    libxl_cputopology_list_free(tinfo, nr_cpus);
> +    return rc;
> +
> +}

Need extra blank line.

>  int libxl_nodemap_to_cpumap(libxl_ctx *ctx,
>                              const libxl_bitmap *nodemap,
>                              libxl_bitmap *cpumap)
> diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h
> index 1e5ca8a..b55099a 100644
> --- a/tools/libxl/libxl_utils.h
> +++ b/tools/libxl/libxl_utils.h
> @@ -143,6 +143,8 @@ int libxl_node_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *nodemap,
>                              int max_nodes);
>  int libxl_socket_bitmap_alloc(libxl_ctx *ctx, libxl_bitmap *socketmap,
>                                int max_sockets);
> +/* Fill socketmap with the CPU topology information on the system. */
> +int libxl_socket_bitmap_fill(libxl_ctx *ctx, libxl_bitmap *socketmap);
>  
>  /* Populate cpumap with the cpus spanned by the nodes in nodemap */
>  int libxl_nodemap_to_cpumap(libxl_ctx *ctx,
> -- 
> 1.9.1

Re: [PATCH for Xen 4.6 2/5] tools/libxl: fix socket display error for CMT

[Wei Liu]

On Mon, Sep 28, 2015 at 07:54:50PM +0800, Chao Peng wrote:
> When displaying the CMT information for all the sockets, we assume socket
> number is continuous. This is not true in the hotplug case. For instance,
> when the 3rd socket is plugged out on a 4-socket system, the available
> sockets numbers are 1,2,4 but current we will display the CMT
> information for socket 1,2,3.
> 
> The fix is getting the socket bitmap for all the sockets on the system
> first and then displaying CMT information for_each_set_bit in that bitmap.
> 
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>

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

Re: [PATCH for Xen 4.6 3/5] tools/libxl: return socket id from libxl_psr_cat_get_l3_info

[Wei Liu]

On Mon, Sep 28, 2015 at 07:54:51PM +0800, Chao Peng wrote:
> The entries returned from libxl_psr_cat_get_l3_info are assumed
> to be socket-continuous. But this is not true in the hotplug case.
> 
> This patch gets the socket bitmap for all the sockets on the system
> first and stores the socket id in the structure libxl_psr_cat_info in
> libxl_psr_cat_get_l3_info. The xl or similar consumers then can display
> socket information correctly. For the sake of future extention, the
> field added to libxl_psr_cat_info is named as target_id.
> 
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> ---
>  tools/libxl/libxl_psr.c     | 21 ++++++++++++++++-----
>  tools/libxl/libxl_types.idl |  1 +
>  tools/libxl/xl_cmdimpl.c    | 37 +++++++++++++++++++------------------
>  3 files changed, 36 insertions(+), 23 deletions(-)
> 
> diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> index 3378239..10e1113 100644
> --- a/tools/libxl/libxl_psr.c
> +++ b/tools/libxl/libxl_psr.c
> @@ -339,7 +339,8 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, libxl_psr_cat_info **info,
>  {
>      GC_INIT(ctx);
>      int rc;
> -    int i, nr_sockets;
> +    int i = 0, socket, nr_sockets;
> +    libxl_bitmap socketmap;
>      libxl_psr_cat_info *ptr;
>  
>      rc = libxl__count_physical_sockets(gc, &nr_sockets);
> @@ -348,21 +349,31 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx, libxl_psr_cat_info **info,
>          goto out;
>      }

This is a path that you call libxl_bitmap_dispose on an uninitialised
socketmap.

>  
> +    libxl_socket_bitmap_alloc(ctx, &socketmap, nr_sockets);
> +    rc = libxl_socket_bitmap_fill(ctx, &socketmap);
> +    if (rc < 0) {
> +        LOGE(ERROR, "failed to get available sockets");
> +        goto out;
> +    }
> +
>      ptr = libxl__malloc(NOGC, nr_sockets * sizeof(libxl_psr_cat_info));
>  
> -    for (i = 0; i < nr_sockets; i++) {
> -        if (xc_psr_cat_get_l3_info(ctx->xch, i, &ptr[i].cos_max,
> -                                                &ptr[i].cbm_len)) {
> +    libxl_for_each_set_bit(socket, socketmap) {
> +        ptr[i].target_id = socket;
> +        if (xc_psr_cat_get_l3_info(ctx->xch, socket, &ptr[i].cos_max,
> +                                                     &ptr[i].cbm_len)) {
>              libxl__psr_cat_log_err_msg(gc, errno);
>              rc = ERROR_FAIL;
>              free(ptr);
>              goto out;
>          }
> +        i++;
>      }
>  
>      *info = ptr;
> -    *nr = nr_sockets;
> +    *nr = i;
>  out:
> +    libxl_bitmap_dispose(&socketmap);
>      GC_FREE;
>      return rc;
>  }

Again, we get away with changing behaviour of API because this is not
yet released.

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 9f6ec00..c7bd425 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -792,6 +792,7 @@ libxl_psr_cbm_type = Enumeration("psr_cbm_type", [
>      ])
>  
>  libxl_psr_cat_info = Struct("psr_cat_info", [
> +    ("target_id", uint32),

Or just call it "socket_id"? Or even just "id" because you know this
structure is for socket?

Wei.

Re: [PATCH for Xen 4.6 4/5] tools/libxl: fix range check in main_psr_cat_cbm_set

[Wei Liu]

On Mon, Sep 28, 2015 at 07:54:52PM +0800, Chao Peng wrote:
> The 'end' should be inclusive.
> 
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>

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

> ---
>  tools/libxl/xl_cmdimpl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index cec0d2c..045b70e 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -8525,7 +8525,7 @@ int main_psr_cat_cbm_set(int argc, char **argv)
>          len = libxl_string_list_length(&socket_list);
>          for (i = 0; i < len; i++) {
>              parse_range(socket_list[i], &start, &end);
> -            for (j = start; j < end; j++)
> +            for (j = start; j <= end; j++)
>                  libxl_bitmap_set(&target_map, j);
>          }
>  
> -- 
> 1.9.1

Re: [PATCH for Xen 4.6 5/5] docs: make xl-psr.markdown more precise

[Wei Liu]

On Mon, Sep 28, 2015 at 07:54:53PM +0800, Chao Peng wrote:
> Make the chapter name and reference url more precise. The chapter number
> is dropped as it can be confusing when it gets changed in the referred
> document.
> 
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>

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

Re: [PATCH for Xen 4.6 0/5] Several PSR fixes in libxl

[Wei Liu]

On Mon, Sep 28, 2015 at 07:54:48PM +0800, Chao Peng wrote:
> The patch basically contains several PSR fixes in libxl.
> patch1-3: fix the socket display error in certain hotplug case.
> patch4:   fix a minor range check.
> patch5:   improve the PSR document.
> 
> Detailed problem and fix please see commit message.
> 

I've gone through all patches. This series looks sensible to me. And I
see the point of getting in it for 4.6 to avoid releasing APIs that are
deemed buggy.

I would still like some more proof reading from someone who have played
with this feature. I've CCed Dario and Meng.

Wei.

Re: [PATCH for Xen 4.6 1/5] tools/libxl: introduce libxl_socket_bitmap_fill

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2978 bytes --]

On Mon, 2015-09-28 at 15:12 +0100, Wei Liu wrote:
> On Mon, Sep 28, 2015 at 07:54:49PM +0800, Chao Peng wrote:

> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 5f9047c..5a91687 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> >  
> >  /*
> > - * LIBXL_HAVE_SOCKET_BITMAP_ALLOC
> > + * LIBXL_HAVE_SOCKET_BITMAP
> >   *
> > - * If this is defined, then libxl_socket_bitmap_alloc exists.
> > + * If this is defined, then libxl_socket_bitmap_alloc and
> > + * libxl_socket_bitmap_Fill exist.
> 
> _Fill -> _fill.
> 
Right.

However, it seems to me that the function would be better named
libxl_get_online_sockets() or something like that.

I see that you want the actual map. For CPUs and NUMA nodes, we do have
libxl_get_online_{cpus,nodes}(), but they return the number of online
CPUs and nodes, so for consistency, libxl_get_online_sockets() would
better (if necessary at some point) behave similarly.

What about libxl_get_online_socketmap() ?  This is still a nice (IMO)
name for what you're after here, and it leaves us room to implement
libxl_get_online_cpumap() and libxl_get_online_nodemap(), if we'll ever
need those.

Thoughs?

> >   */
> > -#define LIBXL_HAVE_SOCKET_BITMAP_ALLOC 1
> > +#define LIBXL_HAVE_SOCKET_BITMAP 1
> >  
> 
> Normally we don't / can't delete existing macros. However this macro
> was
> introduced in this cycle, so we have the liberty to change it before
> releasing.
> 
I was about to say exactly the same thing.

> > diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> > index bfc9699..557f279 100644
> > --- a/tools/libxl/libxl_utils.c
> > +++ b/tools/libxl/libxl_utils.c

> > +int libxl_socket_bitmap_fill(libxl_ctx *ctx, libxl_bitmap
> > *socketmap)
> > +{
> > +    libxl_cputopology *tinfo = NULL;
> > +    int nr_cpus = 0, i, rc = 0;
> > +
> > +    tinfo = libxl_get_cpu_topology(ctx, &nr_cpus);
> > +    if (tinfo == NULL) {
> > +        rc = ERROR_FAIL;
> > +        goto out;
> > +    }
> > +
> > +    libxl_bitmap_set_none(socketmap);
> > +    for (i = 0; i < nr_cpus; i++)
> > +        if (tinfo[i].socket != XEN_INVALID_SOCKET_ID
> > +            && !libxl_bitmap_test(socketmap, tinfo[i].socket))
> > +            libxl_bitmap_set(socketmap, tinfo[i].socket);
> > + out:
> > +    libxl_cputopology_list_free(tinfo, nr_cpus);
> > +    return rc;
> > +
> > +}
> 
> Need extra blank line.
> 
And, at the same time, the blank line between 'return rc;' and the '{'
is not needed. IOW, we want this:

   ...
   libxl_cputopology_list_free(tinfo, nr_cpus);
   return rc;
 }

 int libxl_nodemap_to_cpumap(...
 ...

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH for Xen 4.6 1/5] tools/libxl: introduce libxl_socket_bitmap_fill

[Wei Liu]

On Mon, Sep 28, 2015 at 04:53:58PM +0200, Dario Faggioli wrote:
> On Mon, 2015-09-28 at 15:12 +0100, Wei Liu wrote:
> > On Mon, Sep 28, 2015 at 07:54:49PM +0800, Chao Peng wrote:
> 
> > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > > index 5f9047c..5a91687 100644
> > > --- a/tools/libxl/libxl.h
> > > +++ b/tools/libxl/libxl.h
> > >  
> > >  /*
> > > - * LIBXL_HAVE_SOCKET_BITMAP_ALLOC
> > > + * LIBXL_HAVE_SOCKET_BITMAP
> > >   *
> > > - * If this is defined, then libxl_socket_bitmap_alloc exists.
> > > + * If this is defined, then libxl_socket_bitmap_alloc and
> > > + * libxl_socket_bitmap_Fill exist.
> > 
> > _Fill -> _fill.
> > 
> Right.
> 
> However, it seems to me that the function would be better named
> libxl_get_online_sockets() or something like that.
> 
> I see that you want the actual map. For CPUs and NUMA nodes, we do have
> libxl_get_online_{cpus,nodes}(), but they return the number of online
> CPUs and nodes, so for consistency, libxl_get_online_sockets() would
> better (if necessary at some point) behave similarly.
> 
> What about libxl_get_online_socketmap() ?  This is still a nice (IMO)
> name for what you're after here, and it leaves us room to implement
> libxl_get_online_cpumap() and libxl_get_online_nodemap(), if we'll ever
> need those.
> 
> Thoughs?
> 

I think this is sensible suggestion. But then I'm bad at naming things
so I will leave it up to you guys to decide.

Wei.

Re: [PATCH for Xen 4.6 2/5] tools/libxl: fix socket display error for CMT

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1412 bytes --]

On Mon, 2015-09-28 at 19:54 +0800, Chao Peng wrote:

> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 2706759..c72d3df 100644

> @@ -8228,9 +8228,9 @@ static void
> psr_cmt_print_domain_info(libxl_dominfo *dominfo,
>  
>  static int psr_cmt_show(libxl_psr_cmt_type type, uint32_t domid)
>  {
> -    uint32_t i, socketid, nr_sockets, total_rmid;
> +    uint32_t i, socketid, total_rmid;
>      uint32_t l3_cache_size;
> -    libxl_physinfo info;
> +    libxl_bitmap socketmap;
>      int rc, nr_domains;
>  
>      if (!libxl_psr_cmt_enabled(ctx)) {
> @@ -8244,41 +8244,38 @@ static int psr_cmt_show(libxl_psr_cmt_type
> type, uint32_t domid)
>          return -1;
>      }
>  
> -    libxl_physinfo_init(&info);
> -    rc = libxl_get_physinfo(ctx, &info);
> +    libxl_socket_bitmap_alloc(ctx, &socketmap, 0);
> +    rc = libxl_socket_bitmap_fill(ctx, &socketmap);
>
Shouldn't socketmap be initied with libxl_bitmap_init() before being
allocated and used?

Not doing so is certainly not an issue, in this case, but AFAIR, we
always require that for libxl types, don't we?

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH for Xen 4.6 5/5] docs: make xl-psr.markdown more precise

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 892 bytes --]

On Mon, 2015-09-28 at 19:54 +0800, Chao Peng wrote:
> Make the chapter name and reference url more precise. 
>
This is pretty much the title of the changeset already, so I don't
think it's worth repeating it here. I think you can well kill it, and
just start with "Drop the chapter number as it can..." or "by dropping
the chapter number as it can..."

> The chapter number
> is dropped as it can be confusing when it gets changed in the
> referred
> document.
> 
> Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
>
In any case:

Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH for Xen 4.6 4/5] tools/libxl: fix range check in main_psr_cat_cbm_set

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 604 bytes --]

On Mon, 2015-09-28 at 15:14 +0100, Wei Liu wrote:
> On Mon, Sep 28, 2015 at 07:54:52PM +0800, Chao Peng wrote:
> > The 'end' should be inclusive.
> > 
> > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>
> 
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH for Xen 4.6 3/5] tools/libxl: return socket id from libxl_psr_cat_get_l3_info

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 2144 bytes --]

On Mon, 2015-09-28 at 15:13 +0100, Wei Liu wrote:
> On Mon, Sep 28, 2015 at 07:54:51PM +0800, Chao Peng wrote:

> > diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> > index 3378239..10e1113 100644

> > @@ -339,7 +339,8 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx,
> > libxl_psr_cat_info **info,
> >  {
> >      GC_INIT(ctx);
> >      int rc;
> > -    int i, nr_sockets;
> > +    int i = 0, socket, nr_sockets;
> > +    libxl_bitmap socketmap;
> >      libxl_psr_cat_info *ptr;
> >  
> >      rc = libxl__count_physical_sockets(gc, &nr_sockets);
> > @@ -348,21 +349,31 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx,
> > libxl_psr_cat_info **info,
> >          goto out;
> >      }
> 
> This is a path that you call libxl_bitmap_dispose on an uninitialised
> socketmap.
> 
Yep.

However, do we still need to go through libxl__count_physical_sockets()
explicitly? AFAICS, you need it for allocating ptr, and for returning
it back.

But since now you're building the full bitmap, we can use
libxl_bitmap_count_set(), for that.

This may not be a bit deal, but if I'm not wrong, it saves us an
hypercall (the PHYSINFO that libxl__count_physical_socket() issues).

> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -792,6 +792,7 @@ libxl_psr_cbm_type =
> > Enumeration("psr_cbm_type", [
> >      ])
> >  
> >  libxl_psr_cat_info = Struct("psr_cat_info", [
> > +    ("target_id", uint32),
> 
> Or just call it "socket_id"? Or even just "id" because you know this
> structure is for socket?
> 
Yeah, we discussed this already, AFAICR. I think the point is that, at
least in theory, these features may be extended to become more fine
-grained than per-socket, hence the point of not binding the interface
to sockets.

That being said, FWIW, I'm fine either way.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH for Xen 4.6 2/5] tools/libxl: fix socket display error for CMT

[Wei Liu]

On Mon, Sep 28, 2015 at 05:06:31PM +0200, Dario Faggioli wrote:
> On Mon, 2015-09-28 at 19:54 +0800, Chao Peng wrote:
> 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 2706759..c72d3df 100644
> 
> > @@ -8228,9 +8228,9 @@ static void
> > psr_cmt_print_domain_info(libxl_dominfo *dominfo,
> >  
> >  static int psr_cmt_show(libxl_psr_cmt_type type, uint32_t domid)
> >  {
> > -    uint32_t i, socketid, nr_sockets, total_rmid;
> > +    uint32_t i, socketid, total_rmid;
> >      uint32_t l3_cache_size;
> > -    libxl_physinfo info;
> > +    libxl_bitmap socketmap;
> >      int rc, nr_domains;
> >  
> >      if (!libxl_psr_cmt_enabled(ctx)) {
> > @@ -8244,41 +8244,38 @@ static int psr_cmt_show(libxl_psr_cmt_type
> > type, uint32_t domid)
> >          return -1;
> >      }
> >  
> > -    libxl_physinfo_init(&info);
> > -    rc = libxl_get_physinfo(ctx, &info);
> > +    libxl_socket_bitmap_alloc(ctx, &socketmap, 0);
> > +    rc = libxl_socket_bitmap_fill(ctx, &socketmap);
> >
> Shouldn't socketmap be initied with libxl_bitmap_init() before being
> allocated and used?
> 

Yes.

> Not doing so is certainly not an issue, in this case, but AFAIR, we

That's my thought when I first saw this patch.

> always require that for libxl types, don't we?
> 

But let's stick with convention.

Thanks for your careful review.

Wei.

> Regards,
> Dario
> -- 
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
> 

Re: [PATCH for Xen 4.6 0/5] Several PSR fixes in libxl

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1525 bytes --]

On Mon, 2015-09-28 at 15:16 +0100, Wei Liu wrote:
> On Mon, Sep 28, 2015 at 07:54:48PM +0800, Chao Peng wrote:
> > The patch basically contains several PSR fixes in libxl.
> > patch1-3: fix the socket display error in certain hotplug case.
> > patch4:   fix a minor range check.
> > patch5:   improve the PSR document.
> > 
> > Detailed problem and fix please see commit message.
> > 
> 
> I've gone through all patches. This series looks sensible to me. And
> I
> see the point of getting in it for 4.6 to avoid releasing APIs that
> are
> deemed buggy.
> 
FWIW, I totally agree.

The patch looks sensible to me too, I've only found a couple of minor
issues.

> I would still like some more proof reading from someone who have
> played
> with this feature. I've CCed Dario and Meng.
> 
Thanks for pinging... I've done my best. :-)

About the suggestions I provided, the more important thing, IMO, is the
name of the new function for retrieving the available sockets bitmap,
which I really think it should be called libxl_get_online_socketmap(). 

The others are indeed minor things, which don't affect public APIs and
hence --although I'd prefer to see them addressed-- we can fix/change
them post 4.6 too.

Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: [PATCH for Xen 4.6 3/5] tools/libxl: return socket id from libxl_psr_cat_get_l3_info

[Wei Liu]

On Mon, Sep 28, 2015 at 05:35:56PM +0200, Dario Faggioli wrote:
> On Mon, 2015-09-28 at 15:13 +0100, Wei Liu wrote:
> > On Mon, Sep 28, 2015 at 07:54:51PM +0800, Chao Peng wrote:
> 
> > > diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> > > index 3378239..10e1113 100644
> 
> > > @@ -339,7 +339,8 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx,
> > > libxl_psr_cat_info **info,
> > >  {
> > >      GC_INIT(ctx);
> > >      int rc;
> > > -    int i, nr_sockets;
> > > +    int i = 0, socket, nr_sockets;
> > > +    libxl_bitmap socketmap;
> > >      libxl_psr_cat_info *ptr;
> > >  
> > >      rc = libxl__count_physical_sockets(gc, &nr_sockets);
> > > @@ -348,21 +349,31 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx,
> > > libxl_psr_cat_info **info,
> > >          goto out;
> > >      }
> > 
> > This is a path that you call libxl_bitmap_dispose on an uninitialised
> > socketmap.
> > 
> Yep.
> 
> However, do we still need to go through libxl__count_physical_sockets()
> explicitly? AFAICS, you need it for allocating ptr, and for returning
> it back.
> 
> But since now you're building the full bitmap, we can use
> libxl_bitmap_count_set(), for that.
> 
> This may not be a bit deal, but if I'm not wrong, it saves us an
> hypercall (the PHYSINFO that libxl__count_physical_socket() issues).
> 

Right, this is optimisation. I'm not too fussed either way as long as
this function is functionally correct. :-)

> > > --- a/tools/libxl/libxl_types.idl
> > > +++ b/tools/libxl/libxl_types.idl
> > > @@ -792,6 +792,7 @@ libxl_psr_cbm_type =
> > > Enumeration("psr_cbm_type", [
> > >      ])
> > >  
> > >  libxl_psr_cat_info = Struct("psr_cat_info", [
> > > +    ("target_id", uint32),
> > 
> > Or just call it "socket_id"? Or even just "id" because you know this
> > structure is for socket?
> > 
> Yeah, we discussed this already, AFAICR. I think the point is that, at
> least in theory, these features may be extended to become more fine
> -grained than per-socket, hence the point of not binding the interface
> to sockets.
> 
> That being said, FWIW, I'm fine either way.
> 

The thing that bugs me a little is that "target_id" doesn't seem very
meaningful. I would just go for "id" if it is not bound to socket.

Chao, what do you think?

Wei.

> Regards,
> Dario
> -- 
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
> 

Re: [PATCH for Xen 4.6 1/5] tools/libxl: introduce libxl_socket_bitmap_fill

[Chao Peng]

On Mon, Sep 28, 2015 at 04:53:58PM +0200, Dario Faggioli wrote:
> On Mon, 2015-09-28 at 15:12 +0100, Wei Liu wrote:
> > On Mon, Sep 28, 2015 at 07:54:49PM +0800, Chao Peng wrote:
> 
> > > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > > index 5f9047c..5a91687 100644
> > > --- a/tools/libxl/libxl.h
> > > +++ b/tools/libxl/libxl.h
> > >  
> > >  /*
> > > - * LIBXL_HAVE_SOCKET_BITMAP_ALLOC
> > > + * LIBXL_HAVE_SOCKET_BITMAP
> > >   *
> > > - * If this is defined, then libxl_socket_bitmap_alloc exists.
> > > + * If this is defined, then libxl_socket_bitmap_alloc and
> > > + * libxl_socket_bitmap_Fill exist.
> > 
> > _Fill -> _fill.
> > 
> Right.
> 
> However, it seems to me that the function would be better named
> libxl_get_online_sockets() or something like that.
> 
> I see that you want the actual map. For CPUs and NUMA nodes, we do have
> libxl_get_online_{cpus,nodes}(), but they return the number of online
> CPUs and nodes, so for consistency, libxl_get_online_sockets() would
> better (if necessary at some point) behave similarly.

Actually for the similar purpose of libxl_get_online_{cpus,nodes}, we have
libxl__count_physical_sockets() already, which would ideally be called
libxl_get_online_sockets(). Since it's not public so we have liberty to
change it in the future, if everyone agrees.

> 
> What about libxl_get_online_socketmap() ?  This is still a nice (IMO)
> name for what you're after here, and it leaves us room to implement
> libxl_get_online_cpumap() and libxl_get_online_nodemap(), if we'll ever
> need those.
> 
> Thoughs?

I have been strugging with the naming, but libxl_get_online_socketmap() looks
right for me. Thanks for suggestion.

Chao

Re: [PATCH for Xen 4.6 1/5] tools/libxl: introduce libxl_socket_bitmap_fill

[Chao Peng]

On Mon, Sep 28, 2015 at 03:12:27PM +0100, Wei Liu wrote:
> On Mon, Sep 28, 2015 at 07:54:49PM +0800, Chao Peng wrote:
> > It sets the bit on the given bitmap if the corresponding socket is
> > available and clears the bit when the corresponding socket is not
> > available.
> > 
> > Signed-off-by: Chao Peng <chao.p.peng@linux.intel.com>
> > ---
> >  tools/libxl/libxl.h       |  7 ++++---
> >  tools/libxl/libxl_utils.c | 21 +++++++++++++++++++++
> >  tools/libxl/libxl_utils.h |  2 ++
> >  3 files changed, 27 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 5f9047c..5a91687 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -807,11 +807,12 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
> >  #define LIBXL_HAVE_PCITOPOLOGY 1
> >  
> >  /*
> > - * LIBXL_HAVE_SOCKET_BITMAP_ALLOC
> > + * LIBXL_HAVE_SOCKET_BITMAP
> >   *
> > - * If this is defined, then libxl_socket_bitmap_alloc exists.
> > + * If this is defined, then libxl_socket_bitmap_alloc and
> > + * libxl_socket_bitmap_Fill exist.
> 
> _Fill -> _fill.
> 
> >   */
> > -#define LIBXL_HAVE_SOCKET_BITMAP_ALLOC 1
> > +#define LIBXL_HAVE_SOCKET_BITMAP 1
> >  
> 
> Normally we don't / can't delete existing macros. However this macro was
> introduced in this cycle, so we have the liberty to change it before
> releasing.
> 
> If for any reason this series doesn't make it in 4.6, you would need to
> come up with a different macro name.

Agreed, if it misses 4.6, I will use another name. Thanks :)

Chao

Re: [PATCH for Xen 4.6 2/5] tools/libxl: fix socket display error for CMT

[Chao Peng]

On Mon, Sep 28, 2015 at 04:36:54PM +0100, Wei Liu wrote:
> On Mon, Sep 28, 2015 at 05:06:31PM +0200, Dario Faggioli wrote:
> > On Mon, 2015-09-28 at 19:54 +0800, Chao Peng wrote:
> > 
> > > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > > index 2706759..c72d3df 100644
> > 
> > > @@ -8228,9 +8228,9 @@ static void
> > > psr_cmt_print_domain_info(libxl_dominfo *dominfo,
> > >  
> > >  static int psr_cmt_show(libxl_psr_cmt_type type, uint32_t domid)
> > >  {
> > > -    uint32_t i, socketid, nr_sockets, total_rmid;
> > > +    uint32_t i, socketid, total_rmid;
> > >      uint32_t l3_cache_size;
> > > -    libxl_physinfo info;
> > > +    libxl_bitmap socketmap;
> > >      int rc, nr_domains;
> > >  
> > >      if (!libxl_psr_cmt_enabled(ctx)) {
> > > @@ -8244,41 +8244,38 @@ static int psr_cmt_show(libxl_psr_cmt_type
> > > type, uint32_t domid)
> > >          return -1;
> > >      }
> > >  
> > > -    libxl_physinfo_init(&info);
> > > -    rc = libxl_get_physinfo(ctx, &info);
> > > +    libxl_socket_bitmap_alloc(ctx, &socketmap, 0);
> > > +    rc = libxl_socket_bitmap_fill(ctx, &socketmap);
> > >
> > Shouldn't socketmap be initied with libxl_bitmap_init() before being
> > allocated and used?
> > 
> 
> Yes.
> 
> > Not doing so is certainly not an issue, in this case, but AFAIR, we
> 
> That's my thought when I first saw this patch.
> 
> > always require that for libxl types, don't we?
> > 
> 
> But let's stick with convention.

Sure, let's stick with it.

Chao

Re: [PATCH for Xen 4.6 3/5] tools/libxl: return socket id from libxl_psr_cat_get_l3_info

[Chao Peng]

On Mon, Sep 28, 2015 at 04:46:17PM +0100, Wei Liu wrote:
> On Mon, Sep 28, 2015 at 05:35:56PM +0200, Dario Faggioli wrote:
> > On Mon, 2015-09-28 at 15:13 +0100, Wei Liu wrote:
> > > On Mon, Sep 28, 2015 at 07:54:51PM +0800, Chao Peng wrote:
> > 
> > > > diff --git a/tools/libxl/libxl_psr.c b/tools/libxl/libxl_psr.c
> > > > index 3378239..10e1113 100644
> > 
> > > > @@ -339,7 +339,8 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx,
> > > > libxl_psr_cat_info **info,
> > > >  {
> > > >      GC_INIT(ctx);
> > > >      int rc;
> > > > -    int i, nr_sockets;
> > > > +    int i = 0, socket, nr_sockets;
> > > > +    libxl_bitmap socketmap;
> > > >      libxl_psr_cat_info *ptr;
> > > >  
> > > >      rc = libxl__count_physical_sockets(gc, &nr_sockets);
> > > > @@ -348,21 +349,31 @@ int libxl_psr_cat_get_l3_info(libxl_ctx *ctx,
> > > > libxl_psr_cat_info **info,
> > > >          goto out;
> > > >      }
> > > 
> > > This is a path that you call libxl_bitmap_dispose on an uninitialised
> > > socketmap.
> > > 
> > Yep.
> > 
> > However, do we still need to go through libxl__count_physical_sockets()
> > explicitly? AFAICS, you need it for allocating ptr, and for returning
> > it back.

Not just for allocating ptr only, but also for allocating socketmap.

> > 
> > But since now you're building the full bitmap, we can use
> > libxl_bitmap_count_set(), for that.
> > 
> > This may not be a bit deal, but if I'm not wrong, it saves us an
> > hypercall (the PHYSINFO that libxl__count_physical_socket() issues).
> >

I can pass 0 to libxl_socket_bitmap_alloc() but it will call hypercall
internally. We need the size for the socketmap anyway before we
allocating it.

> 
> Right, this is optimisation. I'm not too fussed either way as long as
> this function is functionally correct. :-)
> 
> > > > --- a/tools/libxl/libxl_types.idl
> > > > +++ b/tools/libxl/libxl_types.idl
> > > > @@ -792,6 +792,7 @@ libxl_psr_cbm_type =
> > > > Enumeration("psr_cbm_type", [
> > > >      ])
> > > >  
> > > >  libxl_psr_cat_info = Struct("psr_cat_info", [
> > > > +    ("target_id", uint32),
> > > 
> > > Or just call it "socket_id"? Or even just "id" because you know this
> > > structure is for socket?
> > > 
> > Yeah, we discussed this already, AFAICR. I think the point is that, at
> > least in theory, these features may be extended to become more fine
> > -grained than per-socket, hence the point of not binding the interface
> > to sockets.
> > 
> > That being said, FWIW, I'm fine either way.
> > 
> 
> The thing that bugs me a little is that "target_id" doesn't seem very
> meaningful. I would just go for "id" if it is not bound to socket.
> 
> Chao, what do you think?

"id" looks right for me. I don't like "target_id" as well but just can't
find out a better name :)

Chao

Re: [PATCH for Xen 4.6 0/5] Several PSR fixes in libxl

[Chao Peng]

On Mon, Sep 28, 2015 at 05:42:25PM +0200, Dario Faggioli wrote:
> On Mon, 2015-09-28 at 15:16 +0100, Wei Liu wrote:
> > On Mon, Sep 28, 2015 at 07:54:48PM +0800, Chao Peng wrote:
> > > The patch basically contains several PSR fixes in libxl.
> > > patch1-3: fix the socket display error in certain hotplug case.
> > > patch4:   fix a minor range check.
> > > patch5:   improve the PSR document.
> > > 
> > > Detailed problem and fix please see commit message.
> > > 
> > 
> > I've gone through all patches. This series looks sensible to me. And
> > I
> > see the point of getting in it for 4.6 to avoid releasing APIs that
> > are
> > deemed buggy.
> > 
> FWIW, I totally agree.
> 
> The patch looks sensible to me too, I've only found a couple of minor
> issues.
> 
> > I would still like some more proof reading from someone who have
> > played
> > with this feature. I've CCed Dario and Meng.
> > 
> Thanks for pinging... I've done my best. :-)
> 
> About the suggestions I provided, the more important thing, IMO, is the
> name of the new function for retrieving the available sockets bitmap,
> which I really think it should be called libxl_get_online_socketmap(). 
> 
> The others are indeed minor things, which don't affect public APIs and
> hence --although I'd prefer to see them addressed-- we can fix/change
> them post 4.6 too.

Thanks for the quick followup from you guys. I will send another version
to address these comments.

Chao

Re: [PATCH for Xen 4.6 3/5] tools/libxl: return socket id from libxl_psr_cat_get_l3_info

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1144 bytes --]

On Tue, 2015-09-29 at 11:05 +0800, Chao Peng wrote:
> On Mon, Sep 28, 2015 at 04:46:17PM +0100, Wei Liu wrote:
> > On Mon, Sep 28, 2015 at 05:35:56PM +0200, Dario Faggioli wrote:

> > > But since now you're building the full bitmap, we can use
> > > libxl_bitmap_count_set(), for that.
> > > 
> > > This may not be a bit deal, but if I'm not wrong, it saves us an
> > > hypercall (the PHYSINFO that libxl__count_physical_socket()
> > > issues).
> > > 
> 
> I can pass 0 to libxl_socket_bitmap_alloc() but it will call
> hypercall
> internally. We need the size for the socketmap anyway before we
> allocating it.
> 
Yes, looking better, the number of hypercall we need seems the same in
both cases. Furthermore, as Wei said, this is an internal detail /
optimization that we can always look into in future, so nevermind, do
as you like best.

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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