[PATCH 0 of 4] support of NUMA topology in xl

[PATCH 0 of 4] support of NUMA topology in xl

[Juergen Gross]

This patch series adds support of NUMA topology in xl.

Patch 1 adds a libxl function to retireve topology information from hypervisor
Patch 2 adds support of -n option to xl info
Patch 3 adds possibility to specify complete nodes instead of cpus for cpupools
Patch 4 adds a new xl command cpupool-numa-split to create one cpupool per numa node

9 files changed, 430 insertions(+), 19 deletions(-)
tools/libxl/libxl.c               |   58 ++++++
tools/libxl/libxl.h               |    8 
tools/libxl/libxl.idl             |    7 
tools/libxl/libxl_utils.c         |   24 ++
tools/libxl/libxl_utils.h         |    2 
tools/libxl/xl.h                  |    1 
tools/libxl/xl_cmdimpl.c          |  314 +++++++++++++++++++++++++++++++++++--
tools/libxl/xl_cmdtable.c         |   11 -
tools/python/xen/lowlevel/xl/xl.c |   24 ++

[PATCH 1 of 4] Support getting topology info in libxl

[Juergen Gross]

[-- Attachment #1: Type: text/plain, Size: 501 bytes --]

Added new function libxl_get_topologyinfo() to obtain this information from
hypervisor.

Signed-off-by: juergen.gross@ts.fujitsu.com


6 files changed, 123 insertions(+)
tools/libxl/libxl.c               |   58 +++++++++++++++++++++++++++++++++++++
tools/libxl/libxl.h               |    8 +++++
tools/libxl/libxl.idl             |    7 ++++
tools/libxl/libxl_utils.c         |   24 +++++++++++++++
tools/libxl/libxl_utils.h         |    2 +
tools/python/xen/lowlevel/xl/xl.c |   24 +++++++++++++++



[-- Attachment #2: xen-work-4.patch --]
[-- Type: text/x-patch, Size: 7397 bytes --]

# HG changeset patch
# User Juergen Gross <juergen.gross@ts.fujitsu.com>
# Date 1290673783 -3600
# Node ID 37fdfe90e0c26bfac22eff6b30b58a5365a50746
# Parent  79b71c77907b80772ee8cba0c5bbf8e444e61226
Support getting topology info in libxl

Added new function libxl_get_topologyinfo() to obtain this information from
hypervisor.

Signed-off-by: juergen.gross@ts.fujitsu.com

diff -r 79b71c77907b -r 37fdfe90e0c2 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Wed Nov 24 10:20:03 2010 +0000
+++ b/tools/libxl/libxl.c	Thu Nov 25 09:29:43 2010 +0100
@@ -3156,6 +3156,64 @@
     return 0;
 }
 
+libxl_topologyinfo *libxl_get_topologyinfo(libxl_ctx *ctx)
+{
+    libxl_topologyinfo *info;
+    xc_topologyinfo_t tinfo;
+    DECLARE_HYPERCALL_BUFFER(xc_cpu_to_core_t, coremap);
+    DECLARE_HYPERCALL_BUFFER(xc_cpu_to_socket_t, socketmap);
+    DECLARE_HYPERCALL_BUFFER(xc_cpu_to_node_t, nodemap);
+    int i;
+    int rc = 0;
+
+    info = calloc(1, sizeof(libxl_topologyinfo));
+    if (info == NULL)
+        return NULL;
+
+    rc += libxl_cpuarray_alloc(ctx, &info->coremap);
+    rc += libxl_cpuarray_alloc(ctx, &info->socketmap);
+    rc += libxl_cpuarray_alloc(ctx, &info->nodemap);
+    if (rc)
+        goto fail;
+
+    coremap = xc_hypercall_buffer_alloc(ctx->xch, coremap, sizeof(*coremap) * info->coremap.entries);
+    socketmap = xc_hypercall_buffer_alloc(ctx->xch, socketmap, sizeof(*socketmap) * info->socketmap.entries);
+    nodemap = xc_hypercall_buffer_alloc(ctx->xch, nodemap, sizeof(*nodemap) * info->nodemap.entries);
+    if ((coremap == NULL) || (socketmap == NULL) || (nodemap == NULL))
+        goto fail;
+
+    set_xen_guest_handle(tinfo.cpu_to_core, coremap);
+    set_xen_guest_handle(tinfo.cpu_to_socket, socketmap);
+    set_xen_guest_handle(tinfo.cpu_to_node, nodemap);
+    tinfo.max_cpu_index = info->coremap.entries - 1;
+    if (xc_topologyinfo(ctx->xch, &tinfo) != 0)
+        goto fail;
+
+    for (i = 0; i <= tinfo.max_cpu_index; i++) {
+        if (i < info->coremap.entries)
+            info->coremap.array[i] = (coremap[i] == INVALID_TOPOLOGY_ID) ?
+                LIBXL_CPUARRAY_INVENTRY : coremap[i];
+        if (i < info->socketmap.entries)
+            info->socketmap.array[i] = (socketmap[i] == INVALID_TOPOLOGY_ID) ?
+                LIBXL_CPUARRAY_INVENTRY : socketmap[i];
+        if (i < info->nodemap.entries)
+            info->nodemap.array[i] = (nodemap[i] == INVALID_TOPOLOGY_ID) ?
+                LIBXL_CPUARRAY_INVENTRY : nodemap[i];
+    }
+
+    xc_hypercall_buffer_free(ctx->xch, coremap);
+    xc_hypercall_buffer_free(ctx->xch, socketmap);
+    xc_hypercall_buffer_free(ctx->xch, nodemap);
+    return info;
+
+fail:
+    xc_hypercall_buffer_free(ctx->xch, coremap);
+    xc_hypercall_buffer_free(ctx->xch, socketmap);
+    xc_hypercall_buffer_free(ctx->xch, nodemap);
+    libxl_topologyinfo_destroy(info);
+    return NULL;
+}
+
 const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
 {
     union {
diff -r 79b71c77907b -r 37fdfe90e0c2 tools/libxl/libxl.h
--- a/tools/libxl/libxl.h	Wed Nov 24 10:20:03 2010 +0000
+++ b/tools/libxl/libxl.h	Thu Nov 25 09:29:43 2010 +0100
@@ -148,6 +148,13 @@
     uint8_t *map;
 } libxl_cpumap;
 void libxl_cpumap_destroy(libxl_cpumap *map);
+
+typedef struct {
+    uint32_t entries;
+    uint32_t *array;
+} libxl_cpuarray;
+#define LIBXL_CPUARRAY_INVENTRY  ~0
+void libxl_cpuarray_destroy(libxl_cpuarray *array);
 
 typedef enum {
     XENFV = 1,
@@ -464,6 +471,7 @@
 int libxl_button_press(libxl_ctx *ctx, uint32_t domid, libxl_button button);
 
 int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo);
+libxl_topologyinfo *libxl_get_topologyinfo(libxl_ctx *ctx);
 libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
                                        int *nb_vcpu, int *nrcpus);
 int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
diff -r 79b71c77907b -r 37fdfe90e0c2 tools/libxl/libxl.idl
--- a/tools/libxl/libxl.idl	Wed Nov 24 10:20:03 2010 +0000
+++ b/tools/libxl/libxl.idl	Thu Nov 25 09:29:43 2010 +0100
@@ -7,6 +7,7 @@
 libxl_uuid = Builtin("uuid")
 libxl_mac = Builtin("mac")
 libxl_cpumap = Builtin("cpumap", destructor_fn="libxl_cpumap_destroy", passby=PASS_BY_REFERENCE)
+libxl_cpuarray = Builtin("cpuarray", destructor_fn="libxl_cpuarray_destroy", passby=PASS_BY_REFERENCE)
 libxl_qemu_machine_type = Number("qemu_machine_type", namespace="libxl_")
 libxl_console_consback = Number("console_consback", namespace="libxl_")
 libxl_console_constype = Number("console_constype", namespace="libxl_")
@@ -302,6 +303,12 @@
     ("phys_cap", uint32),
     ], destructor_fn=None)
 
+libxl_topologyinfo = Struct("topologyinfo", [
+    ("coremap", libxl_cpuarray,   False, "cpu to core map"),
+    ("socketmap", libxl_cpuarray, False, "cpu to socket map"),
+    ("nodemap", libxl_cpuarray,   False, "cpu to node map"),
+    ])
+
 libxl_sched_credit = Struct("sched_credit", [
     ("weight", integer),
     ("cap", integer),
diff -r 79b71c77907b -r 37fdfe90e0c2 tools/libxl/libxl_utils.c
--- a/tools/libxl/libxl_utils.c	Wed Nov 24 10:20:03 2010 +0000
+++ b/tools/libxl/libxl_utils.c	Thu Nov 25 09:29:43 2010 +0100
@@ -751,6 +751,30 @@
     cpumap->map[cpu / 8] &= ~(1 << (cpu & 7));
 }
 
+int libxl_cpuarray_alloc(libxl_ctx *ctx, libxl_cpuarray *cpuarray)
+{
+    int max_cpus;
+    int i;
+
+    max_cpus = libxl_get_max_cpus(ctx);
+    if (max_cpus == 0)
+        return ERROR_FAIL;
+
+    cpuarray->array = calloc(max_cpus, sizeof(*cpuarray->array));
+    if (!cpuarray->array)
+        return ERROR_NOMEM;
+    cpuarray->entries = max_cpus;
+    for (i = 0; i < max_cpus; i++)
+        cpuarray->array[i] = LIBXL_CPUARRAY_INVENTRY;
+
+    return 0;
+}
+
+void libxl_cpuarray_destroy(libxl_cpuarray *array)
+{
+    free(array->array);
+}
+
 int libxl_get_max_cpus(libxl_ctx *ctx)
 {
     return xc_get_max_cpus(ctx->xch);
diff -r 79b71c77907b -r 37fdfe90e0c2 tools/libxl/libxl_utils.h
--- a/tools/libxl/libxl_utils.h	Wed Nov 24 10:20:03 2010 +0000
+++ b/tools/libxl/libxl_utils.h	Thu Nov 25 09:29:43 2010 +0100
@@ -82,5 +82,7 @@
 void libxl_cpumap_reset(libxl_cpumap *cpumap, int cpu);
 #define libxl_for_each_cpu(var, map) for (var = 0; var < (map).size * 8; var++)
 
+int libxl_cpuarray_alloc(libxl_ctx *ctx, libxl_cpuarray *cpuarray);
+
 #endif
 
diff -r 79b71c77907b -r 37fdfe90e0c2 tools/python/xen/lowlevel/xl/xl.c
--- a/tools/python/xen/lowlevel/xl/xl.c	Wed Nov 24 10:20:03 2010 +0000
+++ b/tools/python/xen/lowlevel/xl/xl.c	Thu Nov 25 09:29:43 2010 +0100
@@ -224,6 +224,11 @@
     return 0;
 }
 
+int attrib__libxl_cpuarray_set(PyObject *v, libxl_cpuarray *pptr)
+{
+    return -1;
+}
+
 int attrib__libxl_domain_build_state_ptr_set(PyObject *v, libxl_domain_build_state **pptr)
 {
     return -1;
@@ -284,6 +289,25 @@
         }
     }
     return cpulist;
+}
+
+PyObject *attrib__libxl_cpuarray_get(libxl_cpuarray *pptr)
+{
+    PyObject *list = NULL;
+    int i;
+
+    list = PyList_New(0);
+    for (i = 0; i < pptr->entries; i++) {
+        if (pptr->array[i] == LIBXL_CPUARRAY_INVENTRY) {
+            PyList_Append(list, Py_None);
+        } else {
+            PyObject* pyint = PyInt_FromLong(pptr->array[i]);
+
+            PyList_Append(list, pyint);
+            Py_DECREF(pyint);
+        }
+    }
+    return list;
 }
 
 PyObject *attrib__libxl_domain_build_state_ptr_get(libxl_domain_build_state **pptr)

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH 1 of 4] Support getting topology info in libxl

[Ian Jackson]

Juergen Gross writes ("[Xen-devel] [PATCH 1 of 4] Support getting topology info in libxl"):
> Added new function libxl_get_topologyinfo() to obtain this information from
> hypervisor.

This looks plausible to me.  I'd like an ack from Ian Campbell on the
IDL changes.  Ian ?  

Ian.

Re: [PATCH 1 of 4] Support getting topology info in libxl

[Ian Campbell]

On Fri, 2010-11-26 at 07:10 +0000, Juergen Gross wrote:

> diff -r 79b71c77907b -r 37fdfe90e0c2 tools/libxl/libxl.h
> --- a/tools/libxl/libxl.h       Wed Nov 24 10:20:03 2010 +0000
> +++ b/tools/libxl/libxl.h       Thu Nov 25 09:29:43 2010 +0100
> @@ -148,6 +148,13 @@
>      uint8_t *map;
>  } libxl_cpumap;
>  void libxl_cpumap_destroy(libxl_cpumap *map);
> +
> +typedef struct {
> +    uint32_t entries;
> +    uint32_t *array;
> +} libxl_cpuarray;
> +#define LIBXL_CPUARRAY_INVENTRY  ~0

This looked at first glance like you had misspelled INVENTORY. Perhaps
LIBXL_CPUARRAY_INVALID_ENTRY?

> +void libxl_cpuarray_destroy(libxl_cpuarray *array);
>  
>  typedef enum {
>      XENFV = 1,
> @@ -464,6 +471,7 @@
>  int libxl_button_press(libxl_ctx *ctx, uint32_t domid, libxl_button button);
>  
>  int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo);
> +libxl_topologyinfo *libxl_get_topologyinfo(libxl_ctx *ctx);

The idiom in libxl is for such functions to take a pointer to the
structure to fill in and return a status code, e.g.
	int libxl_get_topologyinfo(libxl_ctx *ctx, libxl_topologyinfo *info)
e..g libxl_get_physinfo()

This is useful since the caller doesn't always need to manage the
dynamic allocation of the info structure e.g, it can use a stack
variable for temporary stuff or nest inside another structure etc.

Both the error path of libxl_get_topologyinfo and output_topologyinfo()
from patch 2/4 leak the info structure which is an error which goes away
with this change.

The IDL stuff and the python bits look fine to me.

Ian.

Re: [PATCH 1 of 4] Support getting topology info in libxl

[Juergen Gross]

On 12/08/10 11:51, Ian Campbell wrote:
> On Fri, 2010-11-26 at 07:10 +0000, Juergen Gross wrote:
>
>> diff -r 79b71c77907b -r 37fdfe90e0c2 tools/libxl/libxl.h
>> --- a/tools/libxl/libxl.h       Wed Nov 24 10:20:03 2010 +0000
>> +++ b/tools/libxl/libxl.h       Thu Nov 25 09:29:43 2010 +0100
>> @@ -148,6 +148,13 @@
>>       uint8_t *map;
>>   } libxl_cpumap;
>>   void libxl_cpumap_destroy(libxl_cpumap *map);
>> +
>> +typedef struct {
>> +    uint32_t entries;
>> +    uint32_t *array;
>> +} libxl_cpuarray;
>> +#define LIBXL_CPUARRAY_INVENTRY  ~0
>
> This looked at first glance like you had misspelled INVENTORY. Perhaps
> LIBXL_CPUARRAY_INVALID_ENTRY?

Okay, I don't mind.

>
>> +void libxl_cpuarray_destroy(libxl_cpuarray *array);
>>
>>   typedef enum {
>>       XENFV = 1,
>> @@ -464,6 +471,7 @@
>>   int libxl_button_press(libxl_ctx *ctx, uint32_t domid, libxl_button button);
>>
>>   int libxl_get_physinfo(libxl_ctx *ctx, libxl_physinfo *physinfo);
>> +libxl_topologyinfo *libxl_get_topologyinfo(libxl_ctx *ctx);
>
> The idiom in libxl is for such functions to take a pointer to the
> structure to fill in and return a status code, e.g.
> 	int libxl_get_topologyinfo(libxl_ctx *ctx, libxl_topologyinfo *info)
> e..g libxl_get_physinfo()
>
> This is useful since the caller doesn't always need to manage the
> dynamic allocation of the info structure e.g, it can use a stack
> variable for temporary stuff or nest inside another structure etc.

I'll change it.


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
TSP ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH 1 of 4] Support getting topology info in libxl

[Gianni Tedesco]

On Fri, 2010-11-26 at 07:10 +0000, Juergen Gross wrote:
> diff -r 79b71c77907b -r 37fdfe90e0c2 tools/python/xen/lowlevel/xl/xl.c
> --- a/tools/python/xen/lowlevel/xl/xl.c Wed Nov 24 10:20:03 2010 +0000
> +++ b/tools/python/xen/lowlevel/xl/xl.c Thu Nov 25 09:29:43 2010 +0100
> @@ -224,6 +224,11 @@
>      return 0;
>  }
>  
> +int attrib__libxl_cpuarray_set(PyObject *v, libxl_cpuarray *pptr)
> +{
> +    return -1;
> +}
> +
>  int attrib__libxl_domain_build_state_ptr_set(PyObject *v, libxl_domain_build_state **pptr)
>  {
>      return -1;
> @@ -284,6 +289,25 @@
>          }
>      }
>      return cpulist;
> +}
> +
> +PyObject *attrib__libxl_cpuarray_get(libxl_cpuarray *pptr)
> +{
> +    PyObject *list = NULL;
> +    int i;
> +
> +    list = PyList_New(0);
> +    for (i = 0; i < pptr->entries; i++) {
> +        if (pptr->array[i] == LIBXL_CPUARRAY_INVENTRY) {
> +            PyList_Append(list, Py_None);

Needs a Py_INCREF(Py_None); - this is a confusing rule about python
lists.

There are actually a slew of these Py_None refcounting bugs in there
that I have a patch for and will send out at some point.

Thanks for the patch

Gianni

[PATCH 2 of 4] support topolgy info in xl info

[Juergen Gross]

[-- Attachment #1: Type: text/plain, Size: 407 bytes --]

Adds option -n/--numa to xl info command to print topology information.
No numa information up to now, as I've no machine which will give this info
via xm info (could be a bug in xm, however).

Signed-off-by: juergen.gross@ts.fujitsu.com


2 files changed, 50 insertions(+), 11 deletions(-)
tools/libxl/xl_cmdimpl.c  |   59 +++++++++++++++++++++++++++++++++++++--------
tools/libxl/xl_cmdtable.c |    2 -



[-- Attachment #2: xen-work-4.patch --]
[-- Type: text/x-patch, Size: 2919 bytes --]

# HG changeset patch
# User Juergen Gross <juergen.gross@ts.fujitsu.com>
# Date 1290673969 -3600
# Node ID 37dc5cb8c5857d7ffa3e3572d6d4090478b8cebc
# Parent  37fdfe90e0c26bfac22eff6b30b58a5365a50746
support topolgy info in xl info

Adds option -n/--numa to xl info command to print topology information.
No numa information up to now, as I've no machine which will give this info
via xm info (could be a bug in xm, however).

Signed-off-by: juergen.gross@ts.fujitsu.com

diff -r 37fdfe90e0c2 -r 37dc5cb8c585 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Thu Nov 25 09:29:43 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Thu Nov 25 09:32:49 2010 +0100
@@ -3925,12 +3925,41 @@
     return;
 }
 
-static void info(void)
+static void output_topologyinfo(void)
+{
+    libxl_topologyinfo *info;
+    int i;
+
+    info = libxl_get_topologyinfo(&ctx);
+    if (info == NULL) {
+        fprintf(stderr, "libxl_get_topologyinfo failed.\n");
+        return;
+    }
+
+    printf("cpu_topology           :\n");
+    printf("cpu:    core    socket     node\n");
+
+    for (i = 0; i < info->coremap.entries; i++) {
+        if (info->coremap.array[i] != LIBXL_CPUARRAY_INVENTRY)
+            printf("%3d:    %4d     %4d     %4d\n", i, info->coremap.array[i],
+                info->socketmap.array[i], info->nodemap.array[i]);
+    }
+
+    printf("numa_info              : none\n");
+
+    libxl_topologyinfo_destroy(info);
+    return;
+}
+
+static void info(int numa)
 {
     output_nodeinfo();
 
     output_physinfo();
 
+    if (numa)
+        output_topologyinfo();
+
     output_xeninfo();
 
     printf("xend_config_format     : 4\n");
@@ -3941,19 +3970,29 @@
 int main_info(int argc, char **argv)
 {
     int opt;
-
-    while ((opt = getopt(argc, argv, "h")) != -1) {
+    int option_index = 0;
+    static struct option long_options[] = {
+        {"help", 0, 0, 'h'},
+        {"numa", 0, 0, 'n'},
+        {0, 0, 0, 0}
+    };
+    int numa = 0;
+
+    while ((opt = getopt_long(argc, argv, "hn", long_options, &option_index)) != -1) {
         switch (opt) {
         case 'h':
             help("info");
             return 0;
-        default:
-            fprintf(stderr, "option `%c' not supported.\n", opt);
-            break;
-        }
-    }
-
-    info();
+        case 'n':
+            numa = 1;
+            break;
+        default:
+            fprintf(stderr, "option `%c' not supported.\n", opt);
+            break;
+        }
+    }
+
+    info(numa);
     return 0;
 }
 
diff -r 37fdfe90e0c2 -r 37dc5cb8c585 tools/libxl/xl_cmdtable.c
--- a/tools/libxl/xl_cmdtable.c	Thu Nov 25 09:29:43 2010 +0100
+++ b/tools/libxl/xl_cmdtable.c	Thu Nov 25 09:32:49 2010 +0100
@@ -185,7 +185,7 @@
     { "info",
       &main_info,
       "Get information about Xen host",
-      "",
+      "-n, --numa         List host NUMA topology information",
     },
     { "sched-credit",
       &main_sched_credit,

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH 2 of 4] support topolgy info in xl info

[Ian Jackson]

Juergen Gross writes ("[Xen-devel] [PATCH 2 of 4] support topolgy info in xl info"):
> Adds option -n/--numa to xl info command to print topology information.
> No numa information up to now, as I've no machine which will give this info
> via xm info (could be a bug in xm, however).

Just a quick question: the reason for requring the user to specify an
extra option to get this info is that:
 * it may be very voluminous
or
 * compatibility
?

It seems to me that printing additional information in xl info should
be doable in a way that won't break old parsers.  This isn't helped of
course by the fact that we don't have a definition of how that parsing
is supposed to be done.

What do you think ?

Ian.

Re: [PATCH 2 of 4] support topolgy info in xl info

[Juergen Gross]

On 12/07/10 18:23, Ian Jackson wrote:
> Juergen Gross writes ("[Xen-devel] [PATCH 2 of 4] support topolgy info in xl info"):
>> Adds option -n/--numa to xl info command to print topology information.
>> No numa information up to now, as I've no machine which will give this info
>> via xm info (could be a bug in xm, however).
>
> Just a quick question: the reason for requring the user to specify an
> extra option to get this info is that:
>   * it may be very voluminous
> or
>   * compatibility
> ?

compatibility with xm


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
TSP ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

[PATCH 3 of 4] Extend cpupools to support numa

[Juergen Gross]

[-- Attachment #1: Type: text/plain, Size: 727 bytes --]

The user interfaces for cpupools are extended to support numa machines:
- xl cpupool-create supports now specifying a node list instead of a cpu list.
  The new cpupool will be created with all free cpus of the specified numa
  nodes.
- xl cpupool-cpu-remove and xl cpupool-cpu-add can take a node number instead
  of a cpu number. Using 'node:1' for the cpu parameter will, depending on
  the operation, either remove all cpus of node 1 in the specified cpupool,
  or add all free cpus of node 1 to the cpupool.

Signed-off-by: juergen.gross@ts.fujitsu.com


2 files changed, 132 insertions(+), 8 deletions(-)
tools/libxl/xl_cmdimpl.c  |  136 +++++++++++++++++++++++++++++++++++++++++++--
tools/libxl/xl_cmdtable.c |    4 -



[-- Attachment #2: xen-work-4.patch --]
[-- Type: text/x-patch, Size: 7011 bytes --]

# HG changeset patch
# User Juergen Gross <juergen.gross@ts.fujitsu.com>
# Date 1290685192 -3600
# Node ID d3880bd8b4674e0ec88dfa380347de9045a4b533
# Parent  37dc5cb8c5857d7ffa3e3572d6d4090478b8cebc
Extend cpupools to support numa

The user interfaces for cpupools are extended to support numa machines:
- xl cpupool-create supports now specifying a node list instead of a cpu list.
  The new cpupool will be created with all free cpus of the specified numa
  nodes.
- xl cpupool-cpu-remove and xl cpupool-cpu-add can take a node number instead
  of a cpu number. Using 'node:1' for the cpu parameter will, depending on
  the operation, either remove all cpus of node 1 in the specified cpupool,
  or add all free cpus of node 1 to the cpupool.

Signed-off-by: juergen.gross@ts.fujitsu.com

diff -r 37dc5cb8c585 -r d3880bd8b467 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Thu Nov 25 09:32:49 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Thu Nov 25 12:39:52 2010 +0100
@@ -5402,10 +5402,12 @@
     uint32_t poolid;
     int schedid = -1;
     XLU_ConfigList *cpus;
-    int n_cpus, i, n;
+    XLU_ConfigList *nodes;
+    int n_cpus, n_nodes, i, n;
     libxl_cpumap freemap;
     libxl_cpumap cpumap;
     libxl_uuid uuid;
+    libxl_topologyinfo *topology;
 
     while (1) {
         opt = getopt_long(argc, argv, "hnf:", long_options, &option_index);
@@ -5514,7 +5516,31 @@
         fprintf(stderr, "Failed to allocate cpumap\n");
         return -ERROR_FAIL;
     }
-    if (!xlu_cfg_get_list(config, "cpus", &cpus, 0, 0)) {
+    if (!xlu_cfg_get_list(config, "nodes", &nodes, 0, 0)) {
+        n_cpus = 0;
+        n_nodes = 0;
+        topology = libxl_get_topologyinfo(&ctx);
+        if (topology == NULL) {
+            fprintf(stderr, "libxl_get_topologyinfo failed\n");
+            return -ERROR_FAIL;
+        }
+        while ((buf = xlu_cfg_get_listitem(nodes, n_nodes)) != NULL) {
+            n = atoi(buf);
+            for (i = 0; i < topology->nodemap.entries; i++) {
+                if ((topology->nodemap.array[i] == n) &&
+                    libxl_cpumap_test(&freemap, i)) {
+                    libxl_cpumap_set(&cpumap, i);
+                    n_cpus++;
+                }
+            }
+            n_nodes++;
+        }
+        libxl_topologyinfo_destroy(topology);
+        if (n_cpus == 0) {
+            fprintf(stderr, "no free cpu found\n");
+            return -ERROR_FAIL;
+        }
+    } else if (!xlu_cfg_get_list(config, "cpus", &cpus, 0, 0)) {
         n_cpus = 0;
         while ((buf = xlu_cfg_get_listitem(cpus, n_cpus)) != NULL) {
             i = atoi(buf);
@@ -5698,6 +5724,10 @@
     const char *pool;
     uint32_t poolid;
     int cpu;
+    int node;
+    int n;
+    libxl_cpumap freemap;
+    libxl_topologyinfo *topology;
 
     while ((opt = getopt(argc, argv, "h")) != -1) {
         switch (opt) {
@@ -5722,7 +5752,13 @@
         help("cpupool-cpu-add");
         return -ERROR_FAIL;
     }
-    cpu = atoi(argv[optind]);
+    node = -1;
+    cpu = -1;
+    if (strncmp(argv[optind], "node:", 5) == 0) {
+        node = atoi(argv[optind] + 5);
+    } else {
+        cpu = atoi(argv[optind]);
+    }
 
     if (cpupool_qualifier_to_cpupoolid(pool, &poolid, NULL) ||
         !libxl_cpupoolid_to_name(&ctx, poolid)) {
@@ -5730,7 +5766,38 @@
         return -ERROR_FAIL;
     }
 
-    return -libxl_cpupool_cpuadd(&ctx, poolid, cpu);
+    if (cpu >= 0) {
+        return -libxl_cpupool_cpuadd(&ctx, poolid, cpu);
+    }
+
+    if (libxl_get_freecpus(&ctx, &freemap)) {
+        fprintf(stderr, "libxl_get_freecpus failed\n");
+        return -ERROR_FAIL;
+    }
+
+    topology = libxl_get_topologyinfo(&ctx);
+    if (topology == NULL) {
+        fprintf(stderr, "libxl_get_topologyinfo failed\n");
+        return -ERROR_FAIL;
+    }
+
+    n = 0;
+    for (cpu = 0; cpu < topology->nodemap.entries; cpu++) {
+        if (libxl_cpumap_test(&freemap, cpu) &&
+            (topology->nodemap.array[cpu] == node) &&
+            !libxl_cpupool_cpuadd(&ctx, poolid, cpu)) {
+                n++;
+        }
+    }
+
+    libxl_topologyinfo_destroy(topology);
+
+    if (n > 0) {
+        return 0;
+    }
+
+    fprintf(stderr, "no free cpu found\n");
+    return -ERROR_FAIL;
 }
 
 int main_cpupoolcpuremove(int argc, char **argv)
@@ -5739,6 +5806,13 @@
     const char *pool;
     uint32_t poolid;
     int cpu;
+    int node;
+    int n;
+    int ret;
+    int p;
+    int n_pools;
+    libxl_topologyinfo *topology;
+    libxl_cpupoolinfo *poolinfo;
 
     while ((opt = getopt(argc, argv, "h")) != -1) {
         switch (opt) {
@@ -5763,7 +5837,13 @@
         help("cpupool-cpu-remove");
         return -ERROR_FAIL;
     }
-    cpu = atoi(argv[optind]);
+    node = -1;
+    cpu = -1;
+    if (strncmp(argv[optind], "node:", 5) == 0) {
+        node = atoi(argv[optind] + 5);
+    } else {
+        cpu = atoi(argv[optind]);
+    }
 
     if (cpupool_qualifier_to_cpupoolid(pool, &poolid, NULL) ||
         !libxl_cpupoolid_to_name(&ctx, poolid)) {
@@ -5771,7 +5851,51 @@
         return -ERROR_FAIL;
     }
 
-    return -libxl_cpupool_cpuremove(&ctx, poolid, cpu);
+    if (cpu >= 0) {
+        return -libxl_cpupool_cpuremove(&ctx, poolid, cpu);
+    }
+
+    ret = 0;
+
+    poolinfo = libxl_list_cpupool(&ctx, &n_pools);
+    if (!poolinfo) {
+        fprintf(stderr, "error getting cpupool info\n");
+        return -ERROR_NOMEM;
+    }
+
+    topology = libxl_get_topologyinfo(&ctx);
+    if (topology == NULL) {
+        fprintf(stderr, "libxl_get_topologyinfo failed\n");
+        ret = -ERROR_FAIL;
+        goto out;
+    }
+
+    n = 0;
+    for (p = 0; p < n_pools; p++) {
+        if (poolinfo[p].poolid == poolid) {
+            for (cpu = 0; cpu < topology->nodemap.entries; cpu++) {
+                if ((topology->nodemap.array[cpu] == node) &&
+                    libxl_cpumap_test(&poolinfo[p].cpumap, cpu) &&
+                    !libxl_cpupool_cpuremove(&ctx, poolid, cpu)) {
+                        n++;
+                }
+            }
+        }
+    }
+
+    libxl_topologyinfo_destroy(topology);
+
+    if (n == 0) {
+        fprintf(stderr, "no cpu of node found in cpupool\n");
+        ret = -ERROR_FAIL;
+    }
+
+out:
+    for (p = 0; p < n_pools; p++) {
+        libxl_cpupoolinfo_destroy(poolinfo + p);
+    }
+
+    return ret;
 }
 
 int main_cpupoolmigrate(int argc, char **argv)
diff -r 37dc5cb8c585 -r d3880bd8b467 tools/libxl/xl_cmdtable.c
--- a/tools/libxl/xl_cmdtable.c	Thu Nov 25 09:32:49 2010 +0100
+++ b/tools/libxl/xl_cmdtable.c	Thu Nov 25 12:39:52 2010 +0100
@@ -361,12 +361,12 @@
     { "cpupool-cpu-add",
       &main_cpupoolcpuadd,
       "Adds a CPU to a CPU pool",
-      "<CPU Pool> <CPU nr>",
+      "<CPU Pool> <CPU nr>|node:<node nr>",
     },
     { "cpupool-cpu-remove",
       &main_cpupoolcpuremove,
       "Removes a CPU from a CPU pool",
-      "<CPU Pool> <CPU nr>",
+      "<CPU Pool> <CPU nr>|node:<node nr>",
     },
     { "cpupool-migrate",
       &main_cpupoolmigrate,

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH 3 of 4] Extend cpupools to support numa

[Ian Jackson]

Juergen Gross writes ("[Xen-devel] [PATCH 3 of 4] Extend cpupools to support numa"):
> The user interfaces for cpupools are extended to support numa machines:
> - xl cpupool-create supports now specifying a node list instead of a cpu list.
>   The new cpupool will be created with all free cpus of the specified numa
>   nodes.
> - xl cpupool-cpu-remove and xl cpupool-cpu-add can take a node number instead
>   of a cpu number. Using 'node:1' for the cpu parameter will, depending on
>   the operation, either remove all cpus of node 1 in the specified cpupool,
>   or add all free cpus of node 1 to the cpupool.
> 
> Signed-off-by: juergen.gross@ts.fujitsu.com

This looks plausible to me.  I'll apply it when the previous two go in.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

Ian.

Re: [PATCH 3 of 4] Extend cpupools to support numa

[Ian Campbell]

See previous comments about the interface to libxl_get_topoplogy and
leaking the returned pointer value.

On Fri, 2010-11-26 at 07:10 +0000, Juergen Gross wrote:
> +    if (libxl_get_freecpus(&ctx, &freemap)) {
> +        fprintf(stderr, "libxl_get_freecpus failed\n");
> +        return -ERROR_FAIL;
> +    }
> +
> +    topology = libxl_get_topologyinfo(&ctx);
> +    if (topology == NULL) {
> +        fprintf(stderr, "libxl_get_topologyinfo failed\n");
> +        return -ERROR_FAIL;
> +    }
> +
> +    n = 0;
> +    for (cpu = 0; cpu < topology->nodemap.entries; cpu++) {
> +        if (libxl_cpumap_test(&freemap, cpu) &&
> +            (topology->nodemap.array[cpu] == node) &&
> +            !libxl_cpupool_cpuadd(&ctx, poolid, cpu)) {
> +                n++;
> +        }
> +    }

This sequence of actions look like they would make a useful addition to
the libxl interface as a helper function.

Not sure what it would be called, perhaps:
	libxl_cpupool_cpuadd_node(&ctx, poolid, node)
?

> +    for (p = 0; p < n_pools; p++) {
> +        if (poolinfo[p].poolid == poolid) {
> +            for (cpu = 0; cpu < topology->nodemap.entries; cpu++) {
> +                if ((topology->nodemap.array[cpu] == node) &&
> +                    libxl_cpumap_test(&poolinfo[p].cpumap, cpu) &&
> +                    !libxl_cpupool_cpuremove(&ctx, poolid, cpu)) {
> +                        n++;
> +                }
> +            }
> +        }
> +    }

also a helper function?

libxl_cpupool_cpuremove_node(&ctx, poolid, node)

Isn't there an existing function to find a poolinfo from a poolid? (if
not then should there be?)

Ian.

Re: [PATCH 3 of 4] Extend cpupools to support numa

[Juergen Gross]

On 12/08/10 11:58, Ian Campbell wrote:
> See previous comments about the interface to libxl_get_topoplogy and
> leaking the returned pointer value.
>
> On Fri, 2010-11-26 at 07:10 +0000, Juergen Gross wrote:
>> +    if (libxl_get_freecpus(&ctx,&freemap)) {
>> +        fprintf(stderr, "libxl_get_freecpus failed\n");
>> +        return -ERROR_FAIL;
>> +    }
>> +
>> +    topology = libxl_get_topologyinfo(&ctx);
>> +    if (topology == NULL) {
>> +        fprintf(stderr, "libxl_get_topologyinfo failed\n");
>> +        return -ERROR_FAIL;
>> +    }
>> +
>> +    n = 0;
>> +    for (cpu = 0; cpu<  topology->nodemap.entries; cpu++) {
>> +        if (libxl_cpumap_test(&freemap, cpu)&&
>> +            (topology->nodemap.array[cpu] == node)&&
>> +            !libxl_cpupool_cpuadd(&ctx, poolid, cpu)) {
>> +                n++;
>> +        }
>> +    }
>
> This sequence of actions look like they would make a useful addition to
> the libxl interface as a helper function.
>
> Not sure what it would be called, perhaps:
> 	libxl_cpupool_cpuadd_node(&ctx, poolid, node)
> ?

Okay.

>
>> +    for (p = 0; p<  n_pools; p++) {
>> +        if (poolinfo[p].poolid == poolid) {
>> +            for (cpu = 0; cpu<  topology->nodemap.entries; cpu++) {
>> +                if ((topology->nodemap.array[cpu] == node)&&
>> +                    libxl_cpumap_test(&poolinfo[p].cpumap, cpu)&&
>> +                    !libxl_cpupool_cpuremove(&ctx, poolid, cpu)) {
>> +                        n++;
>> +                }
>> +            }
>> +        }
>> +    }
>
> also a helper function?
>
> libxl_cpupool_cpuremove_node(&ctx, poolid, node)

Okay.

>
> Isn't there an existing function to find a poolinfo from a poolid? (if
> not then should there be?)

Currently it is only possible to get infos for ALL cpupools. As the
number of cpupools is expected to be not very large, this should be no
problem.
Perhaps it would make sense to create a helper function to locate the correct
poolinfo member based on the poolid.


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
TSP ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

[PATCH 4 of 4] Support new xl command cpupool-numa-split

[Juergen Gross]

[-- Attachment #1: Type: text/plain, Size: 449 bytes --]

New xl command cpupool-numa-split which will create one cpupool for each
numa node of the machine. Can be called only if no other cpupools than Pool-0
are defined. After creation the cpupools can be managed as usual.

Signed-off-by: juergen.gross@ts.fujitsu.com


3 files changed, 125 insertions(+)
tools/libxl/xl.h          |    1 
tools/libxl/xl_cmdimpl.c  |  119 +++++++++++++++++++++++++++++++++++++++++++++
tools/libxl/xl_cmdtable.c |    5 +



[-- Attachment #2: xen-work-4.patch --]
[-- Type: text/x-patch, Size: 5312 bytes --]

# HG changeset patch
# User Juergen Gross <juergen.gross@ts.fujitsu.com>
# Date 1290692690 -3600
# Node ID f31333fe3b3553c90419428624db0983a58e2c82
# Parent  d3880bd8b4674e0ec88dfa380347de9045a4b533
Support new xl command cpupool-numa-split

New xl command cpupool-numa-split which will create one cpupool for each
numa node of the machine. Can be called only if no other cpupools than Pool-0
are defined. After creation the cpupools can be managed as usual.

Signed-off-by: juergen.gross@ts.fujitsu.com

diff -r d3880bd8b467 -r f31333fe3b35 tools/libxl/xl.h
--- a/tools/libxl/xl.h	Thu Nov 25 12:39:52 2010 +0100
+++ b/tools/libxl/xl.h	Thu Nov 25 14:44:50 2010 +0100
@@ -85,6 +85,7 @@
 int main_cpupoolcpuadd(int argc, char **argv);
 int main_cpupoolcpuremove(int argc, char **argv);
 int main_cpupoolmigrate(int argc, char **argv);
+int main_cpupoolnumasplit(int argc, char **argv);
 
 void help(const char *command);
 
diff -r d3880bd8b467 -r f31333fe3b35 tools/libxl/xl_cmdimpl.c
--- a/tools/libxl/xl_cmdimpl.c	Thu Nov 25 12:39:52 2010 +0100
+++ b/tools/libxl/xl_cmdimpl.c	Thu Nov 25 14:44:50 2010 +0100
@@ -5945,3 +5945,122 @@
 
     return -libxl_cpupool_movedomain(&ctx, poolid, domid);
 }
+
+int main_cpupoolnumasplit(int argc, char **argv)
+{
+    int ret;
+    int opt;
+    int p;
+    int c;
+    uint32_t poolid;
+    int schedid;
+    int n_pools;
+    int node;
+    char name[16];
+    libxl_uuid uuid;
+    libxl_cpumap cpumap;
+    libxl_cpumap freemap;
+    libxl_cpupoolinfo *poolinfo;
+    libxl_topologyinfo *topology;
+
+    while ((opt = getopt(argc, argv, "h")) != -1) {
+        switch (opt) {
+        case 'h':
+            help("cpupool-numa-split");
+            return 0;
+        default:
+            fprintf(stderr, "option `%c' not supported.\n", opt);
+            break;
+        }
+    }
+    ret = 0;
+
+    if (libxl_cpumap_alloc(&ctx, &cpumap)) {
+        fprintf(stderr, "Failed to allocate cpumap\n");
+        return -ERROR_FAIL;
+    }
+
+    poolinfo = libxl_list_cpupool(&ctx, &n_pools);
+    if (!poolinfo) {
+        fprintf(stderr, "error getting cpupool info\n");
+        return -ERROR_NOMEM;
+    }
+    poolid = poolinfo[0].poolid;
+    schedid = poolinfo[0].sched_id;
+    libxl_for_each_cpu(c, poolinfo[0].cpumap)
+        if (libxl_cpumap_test(&poolinfo[0].cpumap, c))
+            libxl_cpumap_set(&cpumap, c);
+    for (p = 0; p < n_pools; p++) {
+        libxl_cpupoolinfo_destroy(poolinfo + p);
+    }
+    if (n_pools > 1) {
+        fprintf(stderr, "splitting not possible, already cpupools in use\n");
+        return -ERROR_FAIL;
+    }
+
+    if (libxl_get_freecpus(&ctx, &freemap)) {
+        fprintf(stderr, "libxl_get_freecpus failed\n");
+        return -ERROR_FAIL;
+    }
+
+    topology = libxl_get_topologyinfo(&ctx);
+    if (topology == NULL) {
+        fprintf(stderr, "libxl_get_topologyinfo failed\n");
+        return -ERROR_FAIL;
+    }
+
+    /* Reset Pool-0 to 1st node */
+    node = topology->nodemap.array[0];
+    libxl_for_each_cpu(c, cpumap) {
+        if (!libxl_cpumap_test(&cpumap, c) && (c < topology->nodemap.entries) &&
+            (topology->nodemap.array[c] == node)) {
+            ret = -libxl_cpupool_cpuadd(&ctx, poolid, c);
+            if (ret) {
+                fprintf(stderr, "error on adding cpu to Pool-0\n");
+                goto out;
+            }
+            libxl_cpumap_reset(&freemap, c);
+        }
+    }
+    libxl_for_each_cpu(c, cpumap) {
+        if (libxl_cpumap_test(&cpumap, c) && (c < topology->nodemap.entries) &&
+            (topology->nodemap.array[c] != node)) {
+            ret = -libxl_cpupool_cpuremove(&ctx, poolid, c);
+            if (ret) {
+                fprintf(stderr, "error on removing cpu from Pool-0\n");
+                goto out;
+            }
+            libxl_cpumap_set(&freemap, c);
+        }
+    }
+
+    for (;;) {
+        node = -1;
+        libxl_for_each_cpu(c, freemap) {
+            if (libxl_cpumap_test(&freemap, c) && (node == -1)) {
+                node = topology->nodemap.array[c];
+            }
+            libxl_cpumap_reset(&cpumap, c);
+            if ((node >= 0) && libxl_cpumap_test(&freemap, c) &&
+                (c < topology->nodemap.entries) && (topology->nodemap.array[c] == node)) {
+                libxl_cpumap_reset(&freemap, c);
+                libxl_cpumap_set(&cpumap, c);
+            }
+        }
+        if (node == -1)
+            break;
+
+        snprintf(name, 15, "Pool-node%d", node);
+        libxl_uuid_generate(&uuid);
+        ret = -libxl_create_cpupool(&ctx, name, schedid, cpumap, &uuid, &poolid);
+        if (ret) {
+            fprintf(stderr, "error on creating cpupool\n");
+            goto out;
+        }
+    }
+
+out:
+    libxl_topologyinfo_destroy(topology);
+
+    return ret;
+}
diff -r d3880bd8b467 -r f31333fe3b35 tools/libxl/xl_cmdtable.c
--- a/tools/libxl/xl_cmdtable.c	Thu Nov 25 12:39:52 2010 +0100
+++ b/tools/libxl/xl_cmdtable.c	Thu Nov 25 14:44:50 2010 +0100
@@ -373,6 +373,11 @@
       "Moves a domain into a CPU pool",
       "<Domain> <CPU Pool>",
     },
+    { "cpupool-numa-split",
+      &main_cpupoolnumasplit,
+      "Splits up the machine into one CPU pool per NUMA node",
+      "",
+    },
 };
 
 int cmdtable_len = sizeof(cmd_table)/sizeof(struct cmd_spec);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH 4 of 4] Support new xl command cpupool-numa-split

[Ian Jackson]

Juergen Gross writes ("[Xen-devel] [PATCH 4 of 4] Support new xl command cpupool-numa-split"):
> New xl command cpupool-numa-split which will create one cpupool for each
> numa node of the machine. Can be called only if no other cpupools than Pool-0
> are defined. After creation the cpupools can be managed as usual.

Interesting.   Is this a usual use case ?  I don't object to the feature.

Ian.

Re: [PATCH 4 of 4] Support new xl command cpupool-numa-split

[George Dunlap]

On Tue, Dec 7, 2010 at 5:26 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Juergen Gross writes ("[Xen-devel] [PATCH 4 of 4] Support new xl command cpupool-numa-split"):
>> New xl command cpupool-numa-split which will create one cpupool for each
>> numa node of the machine. Can be called only if no other cpupools than Pool-0
>> are defined. After creation the cpupools can be managed as usual.
>
> Interesting.   Is this a usual use case ?  I don't object to the feature.

it's the default configuration you get if you don't manually muck
about with cpupools.  Sounds like a pretty useful shortcut, rather
than having to parse node info manually.  (Haven't looked at the code
yet.)

 -George

Re: [PATCH 4 of 4] Support new xl command cpupool-numa-split

[Ian Jackson]

George Dunlap writes ("Re: [Xen-devel] [PATCH 4 of 4] Support new xl command cpupool-numa-split"):
> it's the default configuration you get if you don't manually muck
> about with cpupools.  Sounds like a pretty useful shortcut, rather
> than having to parse node info manually.  (Haven't looked at the code
> yet.)

Ah that makes perfect sense, thanks.

Ian.

Re: [PATCH 4 of 4] Support new xl command cpupool-numa-split

[Ian Campbell]

On Fri, 2010-11-26 at 07:10 +0000, Juergen Gross wrote:
> +
> +    if (libxl_cpumap_alloc(&ctx, &cpumap)) {
> +        fprintf(stderr, "Failed to allocate cpumap\n");
> +        return -ERROR_FAIL;
> +    }
> +
> +    poolinfo = libxl_list_cpupool(&ctx, &n_pools);
> +    if (!poolinfo) {
> +        fprintf(stderr, "error getting cpupool info\n");
> +        return -ERROR_NOMEM;
> +    }
> +    poolid = poolinfo[0].poolid;
> +    schedid = poolinfo[0].sched_id;
> +    libxl_for_each_cpu(c, poolinfo[0].cpumap)
> +        if (libxl_cpumap_test(&poolinfo[0].cpumap, c))
> +            libxl_cpumap_set(&cpumap, c);

Can the libxl_cpumap_alloc and this loop be deferred until after the
check and bail for n_pools > 1?

There seems to be no way to find out the number of pools without also
getting all the info about them, which is a shame.

> +    /* Reset Pool-0 to 1st node */
> +    node = topology->nodemap.array[0];
> +    libxl_for_each_cpu(c, cpumap) {
> +        if (!libxl_cpumap_test(&cpumap, c) && (c < topology->nodemap.entries) &&
> +            (topology->nodemap.array[c] == node)) {
> +            ret = -libxl_cpupool_cpuadd(&ctx, poolid, c);
> +            if (ret) {
> +                fprintf(stderr, "error on adding cpu to Pool-0\n");
> +                goto out;
> +            }
> +            libxl_cpumap_reset(&freemap, c);

(nt really related to this series but I wish this was called
libxl_cpumap_clear, I had to go check it wasn't resetting the whole map
or something...)

> +        }
> +    }
> +    libxl_for_each_cpu(c, cpumap) {
> +        if (libxl_cpumap_test(&cpumap, c) && (c < topology->nodemap.entries) &&
> +            (topology->nodemap.array[c] != node)) {
> +            ret = -libxl_cpupool_cpuremove(&ctx, poolid, c);
> +            if (ret) {
> +                fprintf(stderr, "error on removing cpu from Pool-0\n");
> +                goto out;
> +            }
> +            libxl_cpumap_set(&freemap, c);
> +        }
> +    }

Can this loop be merged with the preceding loop, with the body being the
else case of the if?

> +    for (;;) {
> +        node = -1;
> +        libxl_for_each_cpu(c, freemap) {
> +            if (libxl_cpumap_test(&freemap, c) && (node == -1)) {
> +                node = topology->nodemap.array[c];
> +            }
> +            libxl_cpumap_reset(&cpumap, c);
> +            if ((node >= 0) && libxl_cpumap_test(&freemap, c) &&
> +                (c < topology->nodemap.entries) && (topology->nodemap.array[c] == node)) {
> +                libxl_cpumap_reset(&freemap, c);
> +                libxl_cpumap_set(&cpumap, c);
> +            }
> +        }
> +        if (node == -1)
> +            break;
> +
> +        snprintf(name, 15, "Pool-node%d", node);

Do we want to rename Pool-0 at some point too or do we rely on that name
elsewhere?

Ian.

Re: [PATCH 4 of 4] Support new xl command cpupool-numa-split

[Juergen Gross]

On 12/08/10 12:16, Ian Campbell wrote:
> On Fri, 2010-11-26 at 07:10 +0000, Juergen Gross wrote:
>> +
>> +    if (libxl_cpumap_alloc(&ctx,&cpumap)) {
>> +        fprintf(stderr, "Failed to allocate cpumap\n");
>> +        return -ERROR_FAIL;
>> +    }
>> +
>> +    poolinfo = libxl_list_cpupool(&ctx,&n_pools);
>> +    if (!poolinfo) {
>> +        fprintf(stderr, "error getting cpupool info\n");
>> +        return -ERROR_NOMEM;
>> +    }
>> +    poolid = poolinfo[0].poolid;
>> +    schedid = poolinfo[0].sched_id;
>> +    libxl_for_each_cpu(c, poolinfo[0].cpumap)
>> +        if (libxl_cpumap_test(&poolinfo[0].cpumap, c))
>> +            libxl_cpumap_set(&cpumap, c);
>
> Can the libxl_cpumap_alloc and this loop be deferred until after the
> check and bail for n_pools>  1?

Sure.

>
> There seems to be no way to find out the number of pools without also
> getting all the info about them, which is a shame.

Taking a quick look I couldn't spot any way how to find out the number
of domains without also getting all the info about them, too...

>
>> +    /* Reset Pool-0 to 1st node */
>> +    node = topology->nodemap.array[0];
>> +    libxl_for_each_cpu(c, cpumap) {
>> +        if (!libxl_cpumap_test(&cpumap, c)&&  (c<  topology->nodemap.entries)&&
>> +            (topology->nodemap.array[c] == node)) {
>> +            ret = -libxl_cpupool_cpuadd(&ctx, poolid, c);
>> +            if (ret) {
>> +                fprintf(stderr, "error on adding cpu to Pool-0\n");
>> +                goto out;
>> +            }
>> +            libxl_cpumap_reset(&freemap, c);
>
> (nt really related to this series but I wish this was called
> libxl_cpumap_clear, I had to go check it wasn't resetting the whole map
> or something...)

Hmm, do you really think so?
It would make me to check whether it is clearing the whole map :-)
I think the second parameter is a strong hint :-)

>
>> +        }
>> +    }
>> +    libxl_for_each_cpu(c, cpumap) {
>> +        if (libxl_cpumap_test(&cpumap, c)&&  (c<  topology->nodemap.entries)&&
>> +            (topology->nodemap.array[c] != node)) {
>> +            ret = -libxl_cpupool_cpuremove(&ctx, poolid, c);
>> +            if (ret) {
>> +                fprintf(stderr, "error on removing cpu from Pool-0\n");
>> +                goto out;
>> +            }
>> +            libxl_cpumap_set(&freemap, c);
>> +        }
>> +    }
>
> Can this loop be merged with the preceding loop, with the body being the
> else case of the if?

No. I have to add new cpus first to avoid a cpupool without cpus in between.

>
>> +    for (;;) {
>> +        node = -1;
>> +        libxl_for_each_cpu(c, freemap) {
>> +            if (libxl_cpumap_test(&freemap, c)&&  (node == -1)) {
>> +                node = topology->nodemap.array[c];
>> +            }
>> +            libxl_cpumap_reset(&cpumap, c);
>> +            if ((node>= 0)&&  libxl_cpumap_test(&freemap, c)&&
>> +                (c<  topology->nodemap.entries)&&  (topology->nodemap.array[c] == node)) {
>> +                libxl_cpumap_reset(&freemap, c);
>> +                libxl_cpumap_set(&cpumap, c);
>> +            }
>> +        }
>> +        if (node == -1)
>> +            break;
>> +
>> +        snprintf(name, 15, "Pool-node%d", node);
>
> Do we want to rename Pool-0 at some point too or do we rely on that name
> elsewhere?

Good question. There is a hard coded "Pool-0" reference in libxl, but this
could easily be changed.
I'm not sure about implications in xm/xend. I'll check this.


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
TSP ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH 4 of 4] Support new xl command cpupool-numa-split

[Ian Campbell]

On Wed, 2010-12-08 at 12:20 +0000, Juergen Gross wrote:
> On 12/08/10 12:16, Ian Campbell wrote:
> >
> > There seems to be no way to find out the number of pools without also
> > getting all the info about them, which is a shame.
> 
> Taking a quick look I couldn't spot any way how to find out the number
> of domains without also getting all the info about them, too...

Yeah. It's not important, just an observation.
> 
> >
> >> +    /* Reset Pool-0 to 1st node */
> >> +    node = topology->nodemap.array[0];
> >> +    libxl_for_each_cpu(c, cpumap) {
> >> +        if (!libxl_cpumap_test(&cpumap, c)&&  (c<  topology->nodemap.entries)&&
> >> +            (topology->nodemap.array[c] == node)) {
> >> +            ret = -libxl_cpupool_cpuadd(&ctx, poolid, c);
> >> +            if (ret) {
> >> +                fprintf(stderr, "error on adding cpu to Pool-0\n");
> >> +                goto out;
> >> +            }
> >> +            libxl_cpumap_reset(&freemap, c);
> >
> > (nt really related to this series but I wish this was called
> > libxl_cpumap_clear, I had to go check it wasn't resetting the whole map
> > or something...)
> 
> Hmm, do you really think so?
> It would make me to check whether it is clearing the whole map :-)

;-) I think I'm just used to the Linux clear_bit type naming scheme.

> I think the second parameter is a strong hint :-)

True.


> > Can this loop be merged with the preceding loop, with the body being the
> > else case of the if?
> 
> No. I have to add new cpus first to avoid a cpupool without cpus in between.

ok.

I was thinking that because this function only gets here if there is a
single pool that all CPUs must be in that pool -- but that's not
actually true is it? Even if that were the common case there's nothing
to enforce that.

> > Do we want to rename Pool-0 at some point too or do we rely on that name
> > elsewhere?
> 
> Good question. There is a hard coded "Pool-0" reference in libxl, but this
> could easily be changed.
> I'm not sure about implications in xm/xend. I'll check this.

I don't think there is a particularly strong requirement to allow xend
and xl to coexist. I'd recommend just leaving xend doing what it does
today and fix xl/libxl only.

Ian.

Re: [PATCH 4 of 4] Support new xl command cpupool-numa-split

[Juergen Gross]

On 12/08/10 14:12, Ian Campbell wrote:
> On Wed, 2010-12-08 at 12:20 +0000, Juergen Gross wrote:
>> On 12/08/10 12:16, Ian Campbell wrote:
>>> Can this loop be merged with the preceding loop, with the body being the
>>> else case of the if?
>>
>> No. I have to add new cpus first to avoid a cpupool without cpus in between.
>
> ok.
>
> I was thinking that because this function only gets here if there is a
> single pool that all CPUs must be in that pool -- but that's not
> actually true is it? Even if that were the common case there's nothing
> to enforce that.

Perhaps I should add a comment to avoid a problem later...

>
>>> Do we want to rename Pool-0 at some point too or do we rely on that name
>>> elsewhere?
>>
>> Good question. There is a hard coded "Pool-0" reference in libxl, but this
>> could easily be changed.
>> I'm not sure about implications in xm/xend. I'll check this.
>
> I don't think there is a particularly strong requirement to allow xend
> and xl to coexist. I'd recommend just leaving xend doing what it does
> today and fix xl/libxl only.

Okay, I'll modify xl to rename Pool-0 to Pool-node[n].


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
TSP ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH 4 of 4] Support new xl command cpupool-numa-split

[Ian Campbell]

On Wed, 2010-12-08 at 13:17 +0000, Juergen Gross wrote:
> On 12/08/10 14:12, Ian Campbell wrote:
> > On Wed, 2010-12-08 at 12:20 +0000, Juergen Gross wrote:
> >> On 12/08/10 12:16, Ian Campbell wrote:
> >>> Can this loop be merged with the preceding loop, with the body being the
> >>> else case of the if?
> >>
> >> No. I have to add new cpus first to avoid a cpupool without cpus in between.
> >
> > ok.
> >
> > I was thinking that because this function only gets here if there is a
> > single pool that all CPUs must be in that pool -- but that's not
> > actually true is it? Even if that were the common case there's nothing
> > to enforce that.
> 
> Perhaps I should add a comment to avoid a problem later...

That would certainly help.

The alternative would be to bail out if all cpus are not associated with
Pool-0, not just when there are > 1 pools. That would be consistent with
the function only acting on the default configuration.

Ian.

Re: [PATCH 4 of 4] Support new xl command cpupool-numa-split

[Juergen Gross]

On 12/08/10 14:38, Ian Campbell wrote:
> On Wed, 2010-12-08 at 13:17 +0000, Juergen Gross wrote:
>> On 12/08/10 14:12, Ian Campbell wrote:
>>> On Wed, 2010-12-08 at 12:20 +0000, Juergen Gross wrote:
>>>> On 12/08/10 12:16, Ian Campbell wrote:
>>>>> Can this loop be merged with the preceding loop, with the body being the
>>>>> else case of the if?
>>>>
>>>> No. I have to add new cpus first to avoid a cpupool without cpus in between.
>>>
>>> ok.
>>>
>>> I was thinking that because this function only gets here if there is a
>>> single pool that all CPUs must be in that pool -- but that's not
>>> actually true is it? Even if that were the common case there's nothing
>>> to enforce that.
>>
>> Perhaps I should add a comment to avoid a problem later...
>
> That would certainly help.
>
> The alternative would be to bail out if all cpus are not associated with
> Pool-0, not just when there are>  1 pools. That would be consistent with
> the function only acting on the default configuration.

I suspect NUMA systems are subject to cpu hot plug...

I'd prefer the way I've done it before. It isn't too complex and more
flexible.


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
TSP ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html

Re: [PATCH 4 of 4] Support new xl command cpupool-numa-split

[Ian Campbell]

On Wed, 2010-12-08 at 13:41 +0000, Juergen Gross wrote:
> On 12/08/10 14:38, Ian Campbell wrote:
> > On Wed, 2010-12-08 at 13:17 +0000, Juergen Gross wrote:
> >> On 12/08/10 14:12, Ian Campbell wrote:
> >>> On Wed, 2010-12-08 at 12:20 +0000, Juergen Gross wrote:
> >>>> On 12/08/10 12:16, Ian Campbell wrote:
> >>>>> Can this loop be merged with the preceding loop, with the body being the
> >>>>> else case of the if?
> >>>>
> >>>> No. I have to add new cpus first to avoid a cpupool without cpus in between.
> >>>
> >>> ok.
> >>>
> >>> I was thinking that because this function only gets here if there is a
> >>> single pool that all CPUs must be in that pool -- but that's not
> >>> actually true is it? Even if that were the common case there's nothing
> >>> to enforce that.
> >>
> >> Perhaps I should add a comment to avoid a problem later...
> >
> > That would certainly help.
> >
> > The alternative would be to bail out if all cpus are not associated with
> > Pool-0, not just when there are>  1 pools. That would be consistent with
> > the function only acting on the default configuration.
> 
> I suspect NUMA systems are subject to cpu hot plug...

And hotplugged CPUs don't automatically go into Pool-0?

> I'd prefer the way I've done it before. It isn't too complex and more
> flexible.

Given the above constraint that seems reasonable.

Ian.

Re: [PATCH 4 of 4] Support new xl command cpupool-numa-split

[Juergen Gross]

On 12/08/10 15:12, Ian Campbell wrote:
> On Wed, 2010-12-08 at 13:41 +0000, Juergen Gross wrote:
>> On 12/08/10 14:38, Ian Campbell wrote:
>>> On Wed, 2010-12-08 at 13:17 +0000, Juergen Gross wrote:
>>>> On 12/08/10 14:12, Ian Campbell wrote:
>>>>> On Wed, 2010-12-08 at 12:20 +0000, Juergen Gross wrote:
>>>>>> On 12/08/10 12:16, Ian Campbell wrote:
>>>>>>> Can this loop be merged with the preceding loop, with the body being the
>>>>>>> else case of the if?
>>>>>>
>>>>>> No. I have to add new cpus first to avoid a cpupool without cpus in between.
>>>>>
>>>>> ok.
>>>>>
>>>>> I was thinking that because this function only gets here if there is a
>>>>> single pool that all CPUs must be in that pool -- but that's not
>>>>> actually true is it? Even if that were the common case there's nothing
>>>>> to enforce that.
>>>>
>>>> Perhaps I should add a comment to avoid a problem later...
>>>
>>> That would certainly help.
>>>
>>> The alternative would be to bail out if all cpus are not associated with
>>> Pool-0, not just when there are>   1 pools. That would be consistent with
>>> the function only acting on the default configuration.
>>
>> I suspect NUMA systems are subject to cpu hot plug...
>
> And hotplugged CPUs don't automatically go into Pool-0?

I just checked it: they ARE added to Pool-0.

This rises another problem: it would be nice to adjust the NUMA
splitting after a hot-plug of a cpu...
An "update" option to xl cpupool-numa-split would be appropriate,
I think. It should move cpus from Pool 0 to the correct node pools
and create new node pools if necessary.

I'll prepare another patch later...


Juergen

-- 
Juergen Gross                 Principal Developer Operating Systems
TSP ES&S SWE OS6                       Telephone: +49 (0) 89 3222 2967
Fujitsu Technology Solutions              e-mail: juergen.gross@ts.fujitsu.com
Domagkstr. 28                           Internet: ts.fujitsu.com
D-80807 Muenchen                 Company details: ts.fujitsu.com/imprint.html