* [Patch 1/2] support of cpupools in xl: update cpumask handling for cpu pools in libxc and python
@ 2010-09-22 5:42 Juergen Gross
0 siblings, 0 replies; 5+ messages in thread
From: Juergen Gross @ 2010-09-22 5:42 UTC (permalink / raw)
To: xen-devel@lists.xensource.com
[-- Attachment #1: Type: text/plain, Size: 496 bytes --]
Hi,
attached patch updates the cpumask handling for cpu pools in libxc and python
to support arbitrary numbers of cpus.
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
[-- Attachment #2: pools1.patch --]
[-- Type: text/x-patch, Size: 13750 bytes --]
Signed-off-by: juergen.gross@ts.fujitsu.com
To be able to support arbitrary numbers of physical cpus it was necessary to
include the size of cpumaps in the xc-interfaces for cpu pools.
These were:
definition of xc_cpupoolinfo_t
xc_cpupool_getinfo()
xc_cpupool_freeinfo()
diff -r a2b46be9958e tools/libxc/xc_cpupool.c
--- a/tools/libxc/xc_cpupool.c Tue Sep 21 17:54:43 2010 +0100
+++ b/tools/libxc/xc_cpupool.c Wed Sep 22 07:20:51 2010 +0200
@@ -34,6 +34,20 @@ static int do_sysctl_save(xc_interface *
return ret;
}
+static int get_cpumap_size(xc_interface *xch)
+{
+ static int cpumap_size = 0;
+ xc_physinfo_t physinfo;
+
+ if ( cpumap_size )
+ return cpumap_size;
+
+ if ( !xc_physinfo(xch, &physinfo) )
+ cpumap_size = ((physinfo.max_cpu_id + 1) + 7) / 8; /* round to bytes */
+
+ return cpumap_size;
+}
+
int xc_cpupool_create(xc_interface *xch,
uint32_t *ppoolid,
uint32_t sched_id)
@@ -64,50 +78,61 @@ int xc_cpupool_destroy(xc_interface *xch
return do_sysctl_save(xch, &sysctl);
}
-int xc_cpupool_getinfo(xc_interface *xch,
- uint32_t first_poolid,
- uint32_t n_max,
- xc_cpupoolinfo_t *info)
+xc_cpupoolinfo_t *xc_cpupool_getinfo(xc_interface *xch,
+ uint32_t poolid)
{
int err = 0;
- int p;
- uint32_t poolid = first_poolid;
- uint8_t local[sizeof (info->cpumap)];
+ xc_cpupoolinfo_t *info;
+ uint8_t *local;
+ int local_size;
+ int cpumap_size;
+ int size;
DECLARE_SYSCTL;
- memset(info, 0, n_max * sizeof(xc_cpupoolinfo_t));
+ local_size = get_cpumap_size(xch);
+ local = alloca(local_size);
+ if (!local_size)
+ {
+ PERROR("Could not get number of cpus");
+ return NULL;
+ }
+ cpumap_size = (local_size + sizeof(*info->cpumap) - 1) / sizeof(*info->cpumap);
+ size = sizeof(xc_cpupoolinfo_t) + cpumap_size * sizeof(*info->cpumap);
+ info = malloc(size);
+ if ( !info )
+ return NULL;
- for (p = 0; p < n_max; p++)
+ memset(info, 0, size);
+ info->cpumap_size = local_size * 8;
+ info->cpumap = (uint64_t *)(info + 1);
+
+ sysctl.cmd = XEN_SYSCTL_cpupool_op;
+ sysctl.u.cpupool_op.op = XEN_SYSCTL_CPUPOOL_OP_INFO;
+ sysctl.u.cpupool_op.cpupool_id = poolid;
+ set_xen_guest_handle(sysctl.u.cpupool_op.cpumap.bitmap, local);
+ sysctl.u.cpupool_op.cpumap.nr_cpus = local_size * 8;
+
+ if ( (err = lock_pages(local, local_size)) != 0 )
{
- sysctl.cmd = XEN_SYSCTL_cpupool_op;
- sysctl.u.cpupool_op.op = XEN_SYSCTL_CPUPOOL_OP_INFO;
- sysctl.u.cpupool_op.cpupool_id = poolid;
- set_xen_guest_handle(sysctl.u.cpupool_op.cpumap.bitmap, local);
- sysctl.u.cpupool_op.cpumap.nr_cpus = sizeof(info->cpumap) * 8;
+ PERROR("Could not lock memory for Xen hypercall");
+ free(info);
+ return NULL;
+ }
+ err = do_sysctl_save(xch, &sysctl);
+ unlock_pages(local, local_size);
- if ( (err = lock_pages(local, sizeof(local))) != 0 )
- {
- PERROR("Could not lock memory for Xen hypercall");
- break;
- }
- err = do_sysctl_save(xch, &sysctl);
- unlock_pages(local, sizeof (local));
-
- if ( err < 0 )
- break;
-
- info->cpupool_id = sysctl.u.cpupool_op.cpupool_id;
- info->sched_id = sysctl.u.cpupool_op.sched_id;
- info->n_dom = sysctl.u.cpupool_op.n_dom;
- bitmap_byte_to_64(&(info->cpumap), local, sizeof(local) * 8);
- poolid = sysctl.u.cpupool_op.cpupool_id + 1;
- info++;
+ if ( err < 0 )
+ {
+ free(info);
+ return NULL;
}
- if ( p == 0 )
- return err;
+ info->cpupool_id = sysctl.u.cpupool_op.cpupool_id;
+ info->sched_id = sysctl.u.cpupool_op.sched_id;
+ info->n_dom = sysctl.u.cpupool_op.n_dom;
+ bitmap_byte_to_64(info->cpumap, local, local_size * 8);
- return p;
+ return info;
}
int xc_cpupool_addcpu(xc_interface *xch,
@@ -150,30 +175,37 @@ int xc_cpupool_movedomain(xc_interface *
}
int xc_cpupool_freeinfo(xc_interface *xch,
- uint64_t *cpumap)
+ uint64_t *cpumap,
+ int cpusize)
{
int err;
- uint8_t local[sizeof (*cpumap)];
+ uint8_t *local;
DECLARE_SYSCTL;
+
+ local = malloc(cpusize);
+ if (local == NULL)
+ return -ENOMEM;
sysctl.cmd = XEN_SYSCTL_cpupool_op;
sysctl.u.cpupool_op.op = XEN_SYSCTL_CPUPOOL_OP_FREEINFO;
set_xen_guest_handle(sysctl.u.cpupool_op.cpumap.bitmap, local);
- sysctl.u.cpupool_op.cpumap.nr_cpus = sizeof(*cpumap) * 8;
+ sysctl.u.cpupool_op.cpumap.nr_cpus = cpusize * 8;
- if ( (err = lock_pages(local, sizeof(local))) != 0 )
+ if ( (err = lock_pages(local, cpusize)) != 0 )
{
PERROR("Could not lock memory for Xen hypercall");
+ free(local);
return err;
}
err = do_sysctl_save(xch, &sysctl);
- unlock_pages(local, sizeof (local));
+ unlock_pages(local, cpusize);
+ bitmap_byte_to_64(cpumap, local, cpusize * 8);
+
+ free(local);
if (err < 0)
return err;
- bitmap_byte_to_64(cpumap, local, sizeof(local) * 8);
-
return 0;
}
diff -r a2b46be9958e tools/libxc/xenctrl.h
--- a/tools/libxc/xenctrl.h Tue Sep 21 17:54:43 2010 +0100
+++ b/tools/libxc/xenctrl.h Wed Sep 22 07:20:51 2010 +0200
@@ -535,7 +535,8 @@ typedef struct xc_cpupoolinfo {
uint32_t cpupool_id;
uint32_t sched_id;
uint32_t n_dom;
- uint64_t cpumap;
+ uint32_t cpumap_size; /* max number of cpus in map */
+ uint64_t *cpumap;
} xc_cpupoolinfo_t;
/**
@@ -564,15 +565,11 @@ int xc_cpupool_destroy(xc_interface *xch
* Get cpupool info. Returns info for up to the specified number of cpupools
* starting at the given id.
* @parm xc_handle a handle to an open hypervisor interface
- * @parm first_poolid lowest id for which info is returned
- * @parm n_max maximum number of cpupools to return info
- * @parm info pointer to xc_cpupoolinfo_t array
- * return number of cpupool infos
+ * @parm poolid lowest id for which info is returned
+ * return cpupool info ptr (obtained by malloc)
*/
-int xc_cpupool_getinfo(xc_interface *xch,
- uint32_t first_poolid,
- uint32_t n_max,
- xc_cpupoolinfo_t *info);
+xc_cpupoolinfo_t *xc_cpupool_getinfo(xc_interface *xch,
+ uint32_t poolid);
/**
* Add cpu to a cpupool. cpu may be -1 indicating the first unassigned.
@@ -615,10 +612,12 @@ int xc_cpupool_movedomain(xc_interface *
*
* @parm xc_handle a handle to an open hypervisor interface
* @parm cpumap pointer where to store the cpumap
+ * @parm cpusize size of cpumap array in bytes
* return 0 on success, -1 on failure
*/
int xc_cpupool_freeinfo(xc_interface *xch,
- uint64_t *cpumap);
+ uint64_t *cpumap,
+ int cpusize);
/*
diff -r a2b46be9958e tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Tue Sep 21 17:54:43 2010 +0100
+++ b/tools/libxl/libxl.c Wed Sep 22 07:20:51 2010 +0200
@@ -609,9 +609,17 @@ libxl_poolinfo * libxl_list_pool(libxl_c
libxl_poolinfo * libxl_list_pool(libxl_ctx *ctx, int *nb_pool)
{
libxl_poolinfo *ptr;
- int i, ret;
- xc_cpupoolinfo_t info[256];
- int size = 256;
+ int i;
+ xc_cpupoolinfo_t *info;
+ int size;
+ uint32_t poolid;
+ libxl_physinfo physinfo;
+
+ if (libxl_get_physinfo(ctx, &physinfo) != 0) {
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting phys info");
+ return NULL;
+ }
+ size = physinfo.max_cpu_id + LIBXL__CPUPOOL_INFO_SPARE; /* allow some inactive cpu pools */
ptr = calloc(size, sizeof(libxl_poolinfo));
if (!ptr) {
@@ -619,16 +627,17 @@ libxl_poolinfo * libxl_list_pool(libxl_c
return NULL;
}
- ret = xc_cpupool_getinfo(ctx->xch, 0, 256, info);
- if (ret<0) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting cpupool info");
- return NULL;
+ poolid = 0;
+ for (i = 0; i < size; i++) {
+ info = xc_cpupool_getinfo(ctx->xch, poolid);
+ if (info == NULL)
+ break;
+ ptr[i].poolid = info->cpupool_id;
+ poolid = info->cpupool_id + 1;
+ free(info);
}
- for (i = 0; i < ret; i++) {
- ptr[i].poolid = info[i].cpupool_id;
- }
- *nb_pool = ret;
+ *nb_pool = i;
return ptr;
}
diff -r a2b46be9958e tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h Tue Sep 21 17:54:43 2010 +0100
+++ b/tools/libxl/libxl_internal.h Wed Sep 22 07:20:51 2010 +0200
@@ -236,6 +236,7 @@ _hidden char *libxl__domid_to_name(libxl
_hidden char *libxl__domid_to_name(libxl__gc *gc, uint32_t domid);
_hidden char *libxl__poolid_to_name(libxl__gc *gc, uint32_t poolid);
+#define LIBXL__CPUPOOL_INFO_SPARE 32
/* holds the CPUID response for a single CPUID leaf
* input contains the value of the EAX and ECX register,
diff -r a2b46be9958e tools/python/xen/lowlevel/xc/xc.c
--- a/tools/python/xen/lowlevel/xc/xc.c Tue Sep 21 17:54:43 2010 +0100
+++ b/tools/python/xen/lowlevel/xc/xc.c Wed Sep 22 07:20:51 2010 +0200
@@ -241,7 +241,7 @@ static PyObject *pyxc_vcpu_setaffinity(X
if ( xc_physinfo(self->xc_handle, &info) != 0 )
return pyxc_error_to_exception(self->xc_handle);
- nr_cpus = info.nr_cpus;
+ nr_cpus = info.max_cpu_id + 1;
size = (nr_cpus + cpumap_size * 8 - 1)/ (cpumap_size * 8);
cpumap = malloc(cpumap_size * size);
@@ -400,7 +400,7 @@ static PyObject *pyxc_vcpu_getinfo(XcObj
if ( xc_physinfo(self->xc_handle, &pinfo) != 0 )
return pyxc_error_to_exception(self->xc_handle);
- nr_cpus = pinfo.nr_cpus;
+ nr_cpus = pinfo.max_cpu_id + 1;
rc = xc_vcpu_getinfo(self->xc_handle, dom, vcpu, &info);
if ( rc < 0 )
@@ -1906,22 +1906,23 @@ static PyObject *pyxc_dom_set_memshr(XcO
return zero;
}
-static PyObject *cpumap_to_cpulist(uint64_t cpumap)
+static PyObject *cpumap_to_cpulist(uint64_t *cpumap, int cpusize)
{
PyObject *cpulist = NULL;
- uint32_t i;
+ int i;
cpulist = PyList_New(0);
- for ( i = 0; cpumap != 0; i++ )
+ for ( i = 0; i < cpusize; i++ )
{
- if ( cpumap & 1 )
+ if ( *cpumap & (1L << (i % 64)) )
{
PyObject* pyint = PyInt_FromLong(i);
PyList_Append(cpulist, pyint);
Py_DECREF(pyint);
}
- cpumap >>= 1;
+ if ( (i % 64) == 63 )
+ cpumap++;
}
return cpulist;
}
@@ -1966,7 +1967,7 @@ static PyObject *pyxc_cpupool_getinfo(Xc
PyObject *list, *info_dict;
uint32_t first_pool = 0;
- int max_pools = 1024, nr_pools, i;
+ int max_pools = 1024, i;
xc_cpupoolinfo_t *info;
static char *kwd_list[] = { "first_pool", "max_pools", NULL };
@@ -1975,38 +1976,31 @@ static PyObject *pyxc_cpupool_getinfo(Xc
&first_pool, &max_pools) )
return NULL;
- info = calloc(max_pools, sizeof(xc_cpupoolinfo_t));
- if (info == NULL)
- return PyErr_NoMemory();
-
- nr_pools = xc_cpupool_getinfo(self->xc_handle, first_pool, max_pools, info);
-
- if (nr_pools < 0)
+ list = PyList_New(0);
+ for (i = 0; i < max_pools; i++)
{
- free(info);
- return pyxc_error_to_exception(self->xc_handle);
- }
-
- list = PyList_New(nr_pools);
- for ( i = 0 ; i < nr_pools; i++ )
- {
+ info = xc_cpupool_getinfo(self->xc_handle, first_pool);
+ if (info == NULL)
+ break;
info_dict = Py_BuildValue(
"{s:i,s:i,s:i,s:N}",
- "cpupool", (int)info[i].cpupool_id,
- "sched", info[i].sched_id,
- "n_dom", info[i].n_dom,
- "cpulist", cpumap_to_cpulist(info[i].cpumap));
+ "cpupool", (int)info->cpupool_id,
+ "sched", info->sched_id,
+ "n_dom", info->n_dom,
+ "cpulist", cpumap_to_cpulist(info->cpumap,
+ info->cpumap_size));
+ first_pool = info->cpupool_id + 1;
+ free(info);
+
if ( info_dict == NULL )
{
Py_DECREF(list);
- if ( info_dict != NULL ) { Py_DECREF(info_dict); }
- free(info);
return NULL;
}
- PyList_SetItem(list, i, info_dict);
+
+ PyList_Append(list, info_dict);
+ Py_DECREF(info_dict);
}
-
- free(info);
return list;
}
@@ -2072,12 +2066,28 @@ static PyObject *pyxc_cpupool_movedomain
static PyObject *pyxc_cpupool_freeinfo(XcObject *self)
{
- uint64_t cpumap;
+ uint64_t *cpumap;
+ xc_physinfo_t physinfo;
+ int ret;
+ PyObject *info = NULL;
- if (xc_cpupool_freeinfo(self->xc_handle, &cpumap) != 0)
+ if (xc_physinfo(self->xc_handle, &physinfo))
return pyxc_error_to_exception(self->xc_handle);
- return cpumap_to_cpulist(cpumap);
+ cpumap = calloc((physinfo.max_cpu_id + 64) / 64, sizeof(uint64_t));
+ if (!cpumap) {
+ errno = -ENOMEM;
+ return PyErr_SetFromErrno(xc_error_obj);
+ }
+
+ ret = xc_cpupool_freeinfo(self->xc_handle, cpumap,
+ (physinfo.max_cpu_id + 8) / 8);
+ if (!ret)
+ info = cpumap_to_cpulist(cpumap, physinfo.max_cpu_id + 1);
+
+ free(cpumap);
+
+ return ret ? pyxc_error_to_exception(self->xc_handle) : info;
}
static PyObject *pyflask_context_to_sid(PyObject *self, PyObject *args,
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Patch 1/2] support of cpupools in xl: update cpumask handling for cpu pools in libxc and python
@ 2010-10-01 7:20 Juergen Gross
2010-10-01 9:35 ` Ian Campbell
0 siblings, 1 reply; 5+ messages in thread
From: Juergen Gross @ 2010-10-01 7:20 UTC (permalink / raw)
To: xen-devel@lists.xensource.com
[-- Attachment #1: Type: text/plain, Size: 497 bytes --]
Hi,
attached patch updates the cpumask handling for cpu pools in libxc and python
to support arbitrary numbers of cpus.
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
[-- Attachment #2: pools1.patch --]
[-- Type: text/x-patch, Size: 13849 bytes --]
Signed-off-by: juergen.gross@ts.fujitsu.com
To be able to support arbitrary numbers of physical cpus it was necessary to
include the size of cpumaps in the xc-interfaces for cpu pools.
These were:
definition of xc_cpupoolinfo_t
xc_cpupool_getinfo()
xc_cpupool_freeinfo()
diff -r 71f836615ea2 tools/libxc/xc_cpupool.c
--- a/tools/libxc/xc_cpupool.c Fri Sep 24 15:54:39 2010 +0100
+++ b/tools/libxc/xc_cpupool.c Fri Oct 01 09:03:17 2010 +0200
@@ -34,6 +34,20 @@ static int do_sysctl_save(xc_interface *
return ret;
}
+static int get_cpumap_size(xc_interface *xch)
+{
+ static int cpumap_size = 0;
+ xc_physinfo_t physinfo;
+
+ if ( cpumap_size )
+ return cpumap_size;
+
+ if ( !xc_physinfo(xch, &physinfo) )
+ cpumap_size = ((physinfo.max_cpu_id + 1) + 7) / 8; /* round to bytes */
+
+ return cpumap_size;
+}
+
int xc_cpupool_create(xc_interface *xch,
uint32_t *ppoolid,
uint32_t sched_id)
@@ -64,50 +78,61 @@ int xc_cpupool_destroy(xc_interface *xch
return do_sysctl_save(xch, &sysctl);
}
-int xc_cpupool_getinfo(xc_interface *xch,
- uint32_t first_poolid,
- uint32_t n_max,
- xc_cpupoolinfo_t *info)
+xc_cpupoolinfo_t *xc_cpupool_getinfo(xc_interface *xch,
+ uint32_t poolid)
{
int err = 0;
- int p;
- uint32_t poolid = first_poolid;
- uint8_t local[sizeof (info->cpumap)];
+ xc_cpupoolinfo_t *info;
+ uint8_t *local;
+ int local_size;
+ int cpumap_size;
+ int size;
DECLARE_SYSCTL;
- memset(info, 0, n_max * sizeof(xc_cpupoolinfo_t));
+ local_size = get_cpumap_size(xch);
+ local = alloca(local_size);
+ if (!local_size)
+ {
+ PERROR("Could not get number of cpus");
+ return NULL;
+ }
+ cpumap_size = (local_size + sizeof(*info->cpumap) - 1) / sizeof(*info->cpumap);
+ size = sizeof(xc_cpupoolinfo_t) + cpumap_size * sizeof(*info->cpumap);
+ info = malloc(size);
+ if ( !info )
+ return NULL;
- for (p = 0; p < n_max; p++)
+ memset(info, 0, size);
+ info->cpumap_size = local_size * 8;
+ info->cpumap = (uint64_t *)(info + 1);
+
+ sysctl.cmd = XEN_SYSCTL_cpupool_op;
+ sysctl.u.cpupool_op.op = XEN_SYSCTL_CPUPOOL_OP_INFO;
+ sysctl.u.cpupool_op.cpupool_id = poolid;
+ set_xen_guest_handle(sysctl.u.cpupool_op.cpumap.bitmap, local);
+ sysctl.u.cpupool_op.cpumap.nr_cpus = local_size * 8;
+
+ if ( (err = lock_pages(local, local_size)) != 0 )
{
- sysctl.cmd = XEN_SYSCTL_cpupool_op;
- sysctl.u.cpupool_op.op = XEN_SYSCTL_CPUPOOL_OP_INFO;
- sysctl.u.cpupool_op.cpupool_id = poolid;
- set_xen_guest_handle(sysctl.u.cpupool_op.cpumap.bitmap, local);
- sysctl.u.cpupool_op.cpumap.nr_cpus = sizeof(info->cpumap) * 8;
+ PERROR("Could not lock memory for Xen hypercall");
+ free(info);
+ return NULL;
+ }
+ err = do_sysctl_save(xch, &sysctl);
+ unlock_pages(local, local_size);
- if ( (err = lock_pages(local, sizeof(local))) != 0 )
- {
- PERROR("Could not lock memory for Xen hypercall");
- break;
- }
- err = do_sysctl_save(xch, &sysctl);
- unlock_pages(local, sizeof (local));
-
- if ( err < 0 )
- break;
-
- info->cpupool_id = sysctl.u.cpupool_op.cpupool_id;
- info->sched_id = sysctl.u.cpupool_op.sched_id;
- info->n_dom = sysctl.u.cpupool_op.n_dom;
- bitmap_byte_to_64(&(info->cpumap), local, sizeof(local) * 8);
- poolid = sysctl.u.cpupool_op.cpupool_id + 1;
- info++;
+ if ( err < 0 )
+ {
+ free(info);
+ return NULL;
}
- if ( p == 0 )
- return err;
+ info->cpupool_id = sysctl.u.cpupool_op.cpupool_id;
+ info->sched_id = sysctl.u.cpupool_op.sched_id;
+ info->n_dom = sysctl.u.cpupool_op.n_dom;
+ bitmap_byte_to_64(info->cpumap, local, local_size * 8);
- return p;
+ return info;
}
int xc_cpupool_addcpu(xc_interface *xch,
@@ -150,30 +175,37 @@ int xc_cpupool_movedomain(xc_interface *
}
int xc_cpupool_freeinfo(xc_interface *xch,
- uint64_t *cpumap)
+ uint64_t *cpumap,
+ int cpusize)
{
int err;
- uint8_t local[sizeof (*cpumap)];
+ uint8_t *local;
DECLARE_SYSCTL;
+
+ local = malloc(cpusize);
+ if (local == NULL)
+ return -ENOMEM;
sysctl.cmd = XEN_SYSCTL_cpupool_op;
sysctl.u.cpupool_op.op = XEN_SYSCTL_CPUPOOL_OP_FREEINFO;
set_xen_guest_handle(sysctl.u.cpupool_op.cpumap.bitmap, local);
- sysctl.u.cpupool_op.cpumap.nr_cpus = sizeof(*cpumap) * 8;
+ sysctl.u.cpupool_op.cpumap.nr_cpus = cpusize * 8;
- if ( (err = lock_pages(local, sizeof(local))) != 0 )
+ if ( (err = lock_pages(local, cpusize)) != 0 )
{
PERROR("Could not lock memory for Xen hypercall");
+ free(local);
return err;
}
err = do_sysctl_save(xch, &sysctl);
- unlock_pages(local, sizeof (local));
+ unlock_pages(local, cpusize);
+ bitmap_byte_to_64(cpumap, local, cpusize * 8);
+
+ free(local);
if (err < 0)
return err;
- bitmap_byte_to_64(cpumap, local, sizeof(local) * 8);
-
return 0;
}
diff -r 71f836615ea2 tools/libxc/xenctrl.h
--- a/tools/libxc/xenctrl.h Fri Sep 24 15:54:39 2010 +0100
+++ b/tools/libxc/xenctrl.h Fri Oct 01 09:03:17 2010 +0200
@@ -535,7 +535,8 @@ typedef struct xc_cpupoolinfo {
uint32_t cpupool_id;
uint32_t sched_id;
uint32_t n_dom;
- uint64_t cpumap;
+ uint32_t cpumap_size; /* max number of cpus in map */
+ uint64_t *cpumap;
} xc_cpupoolinfo_t;
/**
@@ -564,15 +565,11 @@ int xc_cpupool_destroy(xc_interface *xch
* Get cpupool info. Returns info for up to the specified number of cpupools
* starting at the given id.
* @parm xc_handle a handle to an open hypervisor interface
- * @parm first_poolid lowest id for which info is returned
- * @parm n_max maximum number of cpupools to return info
- * @parm info pointer to xc_cpupoolinfo_t array
- * return number of cpupool infos
+ * @parm poolid lowest id for which info is returned
+ * return cpupool info ptr (obtained by malloc)
*/
-int xc_cpupool_getinfo(xc_interface *xch,
- uint32_t first_poolid,
- uint32_t n_max,
- xc_cpupoolinfo_t *info);
+xc_cpupoolinfo_t *xc_cpupool_getinfo(xc_interface *xch,
+ uint32_t poolid);
/**
* Add cpu to a cpupool. cpu may be -1 indicating the first unassigned.
@@ -615,10 +612,12 @@ int xc_cpupool_movedomain(xc_interface *
*
* @parm xc_handle a handle to an open hypervisor interface
* @parm cpumap pointer where to store the cpumap
+ * @parm cpusize size of cpumap array in bytes
* return 0 on success, -1 on failure
*/
int xc_cpupool_freeinfo(xc_interface *xch,
- uint64_t *cpumap);
+ uint64_t *cpumap,
+ int cpusize);
/*
diff -r 71f836615ea2 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Fri Sep 24 15:54:39 2010 +0100
+++ b/tools/libxl/libxl.c Fri Oct 01 09:03:17 2010 +0200
@@ -610,26 +610,34 @@ libxl_poolinfo * libxl_list_pool(libxl_c
libxl_poolinfo * libxl_list_pool(libxl_ctx *ctx, int *nb_pool)
{
libxl_poolinfo *ptr;
- int i, ret;
- xc_cpupoolinfo_t info[256];
- int size = 256;
+ int i;
+ xc_cpupoolinfo_t *info;
+ uint32_t poolid;
+ libxl_physinfo physinfo;
- ptr = calloc(size, sizeof(libxl_poolinfo));
- if (!ptr) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpupool info");
+ if (libxl_get_physinfo(ctx, &physinfo) != 0) {
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting phys info");
return NULL;
}
- ret = xc_cpupool_getinfo(ctx->xch, 0, 256, info);
- if (ret<0) {
- LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting cpupool info");
- return NULL;
+ ptr = NULL;
+
+ poolid = 0;
+ for (i = 0;; i++) {
+ info = xc_cpupool_getinfo(ctx->xch, poolid);
+ if (info == NULL)
+ break;
+ ptr = realloc(ptr, (i + 1) * sizeof(libxl_poolinfo));
+ if (!ptr) {
+ LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpupool info");
+ return NULL;
+ }
+ ptr[i].poolid = info->cpupool_id;
+ poolid = info->cpupool_id + 1;
+ free(info);
}
- for (i = 0; i < ret; i++) {
- ptr[i].poolid = info[i].cpupool_id;
- }
- *nb_pool = ret;
+ *nb_pool = i;
return ptr;
}
diff -r 71f836615ea2 tools/libxl/libxl_internal.h
--- a/tools/libxl/libxl_internal.h Fri Sep 24 15:54:39 2010 +0100
+++ b/tools/libxl/libxl_internal.h Fri Oct 01 09:03:17 2010 +0200
@@ -239,7 +239,6 @@ _hidden char *libxl__domid_to_name(libxl
_hidden char *libxl__domid_to_name(libxl__gc *gc, uint32_t domid);
_hidden char *libxl__poolid_to_name(libxl__gc *gc, uint32_t poolid);
-
/* holds the CPUID response for a single CPUID leaf
* input contains the value of the EAX and ECX register,
* and each policy string contains a filter to apply to
diff -r 71f836615ea2 tools/python/xen/lowlevel/xc/xc.c
--- a/tools/python/xen/lowlevel/xc/xc.c Fri Sep 24 15:54:39 2010 +0100
+++ b/tools/python/xen/lowlevel/xc/xc.c Fri Oct 01 09:03:17 2010 +0200
@@ -241,7 +241,7 @@ static PyObject *pyxc_vcpu_setaffinity(X
if ( xc_physinfo(self->xc_handle, &info) != 0 )
return pyxc_error_to_exception(self->xc_handle);
- nr_cpus = info.nr_cpus;
+ nr_cpus = info.max_cpu_id + 1;
size = (nr_cpus + cpumap_size * 8 - 1)/ (cpumap_size * 8);
cpumap = malloc(cpumap_size * size);
@@ -400,7 +400,7 @@ static PyObject *pyxc_vcpu_getinfo(XcObj
if ( xc_physinfo(self->xc_handle, &pinfo) != 0 )
return pyxc_error_to_exception(self->xc_handle);
- nr_cpus = pinfo.nr_cpus;
+ nr_cpus = pinfo.max_cpu_id + 1;
rc = xc_vcpu_getinfo(self->xc_handle, dom, vcpu, &info);
if ( rc < 0 )
@@ -1906,22 +1906,23 @@ static PyObject *pyxc_dom_set_memshr(XcO
return zero;
}
-static PyObject *cpumap_to_cpulist(uint64_t cpumap)
+static PyObject *cpumap_to_cpulist(uint64_t *cpumap, int cpusize)
{
PyObject *cpulist = NULL;
- uint32_t i;
+ int i;
cpulist = PyList_New(0);
- for ( i = 0; cpumap != 0; i++ )
+ for ( i = 0; i < cpusize; i++ )
{
- if ( cpumap & 1 )
+ if ( *cpumap & (1L << (i % 64)) )
{
PyObject* pyint = PyInt_FromLong(i);
PyList_Append(cpulist, pyint);
Py_DECREF(pyint);
}
- cpumap >>= 1;
+ if ( (i % 64) == 63 )
+ cpumap++;
}
return cpulist;
}
@@ -1966,7 +1967,7 @@ static PyObject *pyxc_cpupool_getinfo(Xc
PyObject *list, *info_dict;
uint32_t first_pool = 0;
- int max_pools = 1024, nr_pools, i;
+ int max_pools = 1024, i;
xc_cpupoolinfo_t *info;
static char *kwd_list[] = { "first_pool", "max_pools", NULL };
@@ -1975,38 +1976,31 @@ static PyObject *pyxc_cpupool_getinfo(Xc
&first_pool, &max_pools) )
return NULL;
- info = calloc(max_pools, sizeof(xc_cpupoolinfo_t));
- if (info == NULL)
- return PyErr_NoMemory();
-
- nr_pools = xc_cpupool_getinfo(self->xc_handle, first_pool, max_pools, info);
-
- if (nr_pools < 0)
+ list = PyList_New(0);
+ for (i = 0; i < max_pools; i++)
{
- free(info);
- return pyxc_error_to_exception(self->xc_handle);
- }
-
- list = PyList_New(nr_pools);
- for ( i = 0 ; i < nr_pools; i++ )
- {
+ info = xc_cpupool_getinfo(self->xc_handle, first_pool);
+ if (info == NULL)
+ break;
info_dict = Py_BuildValue(
"{s:i,s:i,s:i,s:N}",
- "cpupool", (int)info[i].cpupool_id,
- "sched", info[i].sched_id,
- "n_dom", info[i].n_dom,
- "cpulist", cpumap_to_cpulist(info[i].cpumap));
+ "cpupool", (int)info->cpupool_id,
+ "sched", info->sched_id,
+ "n_dom", info->n_dom,
+ "cpulist", cpumap_to_cpulist(info->cpumap,
+ info->cpumap_size));
+ first_pool = info->cpupool_id + 1;
+ free(info);
+
if ( info_dict == NULL )
{
Py_DECREF(list);
- if ( info_dict != NULL ) { Py_DECREF(info_dict); }
- free(info);
return NULL;
}
- PyList_SetItem(list, i, info_dict);
+
+ PyList_Append(list, info_dict);
+ Py_DECREF(info_dict);
}
-
- free(info);
return list;
}
@@ -2072,12 +2066,28 @@ static PyObject *pyxc_cpupool_movedomain
static PyObject *pyxc_cpupool_freeinfo(XcObject *self)
{
- uint64_t cpumap;
+ uint64_t *cpumap;
+ xc_physinfo_t physinfo;
+ int ret;
+ PyObject *info = NULL;
- if (xc_cpupool_freeinfo(self->xc_handle, &cpumap) != 0)
+ if (xc_physinfo(self->xc_handle, &physinfo))
return pyxc_error_to_exception(self->xc_handle);
- return cpumap_to_cpulist(cpumap);
+ cpumap = calloc((physinfo.max_cpu_id + 64) / 64, sizeof(uint64_t));
+ if (!cpumap) {
+ errno = -ENOMEM;
+ return PyErr_SetFromErrno(xc_error_obj);
+ }
+
+ ret = xc_cpupool_freeinfo(self->xc_handle, cpumap,
+ (physinfo.max_cpu_id + 8) / 8);
+ if (!ret)
+ info = cpumap_to_cpulist(cpumap, physinfo.max_cpu_id + 1);
+
+ free(cpumap);
+
+ return ret ? pyxc_error_to_exception(self->xc_handle) : info;
}
static PyObject *pyflask_context_to_sid(PyObject *self, PyObject *args,
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch 1/2] support of cpupools in xl: update cpumask handling for cpu pools in libxc and python
2010-10-01 7:20 Juergen Gross
@ 2010-10-01 9:35 ` Ian Campbell
2010-10-05 13:50 ` Juergen Gross
0 siblings, 1 reply; 5+ messages in thread
From: Ian Campbell @ 2010-10-01 9:35 UTC (permalink / raw)
To: Juergen Gross; +Cc: xen-devel@lists.xensource.com
(I highly recommend the patchbomb extension ("hg email"), it makes
sending series much simpler and threads the mails together in a
convenient way)
On Fri, 2010-10-01 at 08:20 +0100, Juergen Gross wrote:
> Signed-off-by: juergen.gross@ts.fujitsu.com
This should come after the description.
> To be able to support arbitrary numbers of physical cpus it was necessary to
> include the size of cpumaps in the xc-interfaces for cpu pools.
> These were:
> definition of xc_cpupoolinfo_t
> xc_cpupool_getinfo()
> xc_cpupool_freeinfo()
Please also mention the change in xc_cpupool_getinfo semantics from
caller allocated buffer to callee allocated+returned.
> @@ -64,50 +78,61 @@ int xc_cpupool_destroy(xc_interface *xch
> return do_sysctl_save(xch, &sysctl);
> }
>
> -int xc_cpupool_getinfo(xc_interface *xch,
> - uint32_t first_poolid,
> - uint32_t n_max,
> - xc_cpupoolinfo_t *info)
> +xc_cpupoolinfo_t *xc_cpupool_getinfo(xc_interface *xch,
> + uint32_t poolid)
[...]
> - memset(info, 0, n_max * sizeof(xc_cpupoolinfo_t));
> + local_size = get_cpumap_size(xch);
> + local = alloca(local_size);
> + if (!local_size)
> + {
> + PERROR("Could not get number of cpus");
> + return NULL;
> + }
I imagine alloca(0) is most likely safe so long as you don't actually
use the result, but the man page doesn't specifically say. Probably the
check of !local_size should be before the alloca(local_size) to be on
the safe side.
> + cpumap_size = (local_size + sizeof(*info->cpumap) - 1) / sizeof(*info->cpumap);
xg_private.h defines a macro ROUNDUP, I wonder if that should be moved
somewhere more generic and used to clarify code like this?
> diff -r 8b7d253f0e17 tools/libxc/xenctrl.h
> --- a/tools/libxc/xenctrl.h Fri Oct 01 08:39:49 2010 +0100
> +++ b/tools/libxc/xenctrl.h Fri Oct 01 09:13:36 2010 +0100
> @@ -564,15 +565,11 @@ int xc_cpupool_destroy(xc_interface *xch
> [...]
> + * @parm poolid lowest id for which info is returned
> + * return cpupool info ptr (obtained by malloc)
> */
> -int xc_cpupool_getinfo(xc_interface *xch,
> - uint32_t first_poolid,
> - uint32_t n_max,
> - xc_cpupoolinfo_t *info);
> +xc_cpupoolinfo_t *xc_cpupool_getinfo(xc_interface *xch,
> + uint32_t poolid);
>
> /**
> * Add cpu to a cpupool. cpu may be -1 indicating the first unassigned.
> @@ -615,10 +612,12 @@ int xc_cpupool_movedomain(xc_interface *
> *
> * @parm xc_handle a handle to an open hypervisor interface
> * @parm cpumap pointer where to store the cpumap
> + * @parm cpusize size of cpumap array in bytes
> * return 0 on success, -1 on failure
> */
> int xc_cpupool_freeinfo(xc_interface *xch,
> - uint64_t *cpumap);
> + uint64_t *cpumap,
> + int cpusize);
xc_cpupool_getinfo returns a callee allocated buffer and
xc_cpupool_freeinfo expects to be given a caller allocated buffer? I
think we should make this consistent one way of the other.
> diff -r 71f836615ea2 tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c Fri Sep 24 15:54:39 2010 +0100
> +++ b/tools/libxl/libxl.c Fri Oct 01 09:03:17 2010 +0200
> @@ -610,26 +610,34 @@ libxl_poolinfo * libxl_list_pool(libxl_c
> libxl_poolinfo * libxl_list_pool(libxl_ctx *ctx, int *nb_pool)
> {
> libxl_poolinfo *ptr;
> - int i, ret;
> - xc_cpupoolinfo_t info[256];
> - int size = 256;
> + int i;
> + xc_cpupoolinfo_t *info;
> + uint32_t poolid;
> + libxl_physinfo physinfo;
>
> - ptr = calloc(size, sizeof(libxl_poolinfo));
> - if (!ptr) {
> - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpupool info");
> + if (libxl_get_physinfo(ctx, &physinfo) != 0) {
> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting phys info");
> return NULL;
> }
Am I missing where the contents of physinfo is subsequently used in this
function?
>
> - ret = xc_cpupool_getinfo(ctx->xch, 0, 256, info);
> - if (ret<0) {
> - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting cpupool info");
> - return NULL;
> + ptr = NULL;
> +
> + poolid = 0;
> + for (i = 0;; i++) {
> + info = xc_cpupool_getinfo(ctx->xch, poolid);
> + if (info == NULL)
> + break;
> + ptr = realloc(ptr, (i + 1) * sizeof(libxl_poolinfo));
> + if (!ptr) {
> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpupool info");
> + return NULL;
> + }
This will leak the previous value of ptr if realloc() fails. You need to
do:
tmp = realloc(ptr, ....)
if (!tmp) {
free(ptr);
LIBXL__LOG_ERRNO(...);
return NULL;
}
ptr = tmp;
> diff -r 71f836615ea2 tools/python/xen/lowlevel/xc/xc.c
> --- a/tools/python/xen/lowlevel/xc/xc.c Fri Sep 24 15:54:39 2010 +0100
> +++ b/tools/python/xen/lowlevel/xc/xc.c Fri Oct 01 09:03:17 2010 +0200
> @@ -241,7 +241,7 @@ static PyObject *pyxc_vcpu_setaffinity(X
> if ( xc_physinfo(self->xc_handle, &info) != 0 )
> return pyxc_error_to_exception(self->xc_handle);
>
> - nr_cpus = info.nr_cpus;
> + nr_cpus = info.max_cpu_id + 1;
>
> size = (nr_cpus + cpumap_size * 8 - 1)/ (cpumap_size * 8);
> cpumap = malloc(cpumap_size * size);
Is this (and the equivalent in getinfo) an independent bug fix for a
pre-existing issue or does it somehow relate to the rest of the changes?
I don't see any corresponding change to xc_vcpu_setaffinity is all.
Given the repeated uses of physinfo.max_cpu_id (here, in get_cpumap_size
etc) might a xc_get_nr_cpus() function be worthwhile?
Presumably when you change these interfaces to use uint8_t instead of
uint64_t this code becomes the same as the private get_cpumap_size you
defined earlier so it might be worth exporting that functionality from
libxc?
> @@ -1966,7 +1967,7 @@ static PyObject *pyxc_cpupool_getinfo(Xc
> PyObject *list, *info_dict;
>
> uint32_t first_pool = 0;
> - int max_pools = 1024, nr_pools, i;
> + int max_pools = 1024, i;
[...]
> + for (i = 0; i < max_pools; i++)
I don't think there is any 1024 pool limit inherent in the new libxc
code, is there? You've removed the limit from libxl and I think the
right thing to do is remove it here as well.
> {
> - free(info);
> - return pyxc_error_to_exception(self->xc_handle);
> - }
> -
> - list = PyList_New(nr_pools);
> - for ( i = 0 ; i < nr_pools; i++ )
> - {
> + info = xc_cpupool_getinfo(self->xc_handle, first_pool);
> + if (info == NULL)
> + break;
> info_dict = Py_BuildValue(
> "{s:i,s:i,s:i,s:N}",
> - "cpupool", (int)info[i].cpupool_id,
> - "sched", info[i].sched_id,
> - "n_dom", info[i].n_dom,
> - "cpulist", cpumap_to_cpulist(info[i].cpumap));
> + "cpupool", (int)info->cpupool_id,
> + "sched", info->sched_id,
> + "n_dom", info->n_dom,
> + "cpulist", cpumap_to_cpulist(info->cpumap,
> + info->cpumap_size));
> + first_pool = info->cpupool_id + 1;
> + free(info);
> +
> if ( info_dict == NULL )
> {
> Py_DECREF(list);
> - if ( info_dict != NULL ) { Py_DECREF(info_dict); }
> - free(info);
> return NULL;
> }
> - PyList_SetItem(list, i, info_dict);
> +
> + PyList_Append(list, info_dict);
> + Py_DECREF(info_dict);
> }
> -
> - free(info);
>
> return list;
> }
> @@ -2072,12 +2066,28 @@ static PyObject *pyxc_cpupool_movedomain
>
> static PyObject *pyxc_cpupool_freeinfo(XcObject *self)
> {
> - uint64_t cpumap;
> + uint64_t *cpumap;
> + xc_physinfo_t physinfo;
> + int ret;
> + PyObject *info = NULL;
>
> - if (xc_cpupool_freeinfo(self->xc_handle, &cpumap) != 0)
> + if (xc_physinfo(self->xc_handle, &physinfo))
> return pyxc_error_to_exception(self->xc_handle);
>
> - return cpumap_to_cpulist(cpumap);
> + cpumap = calloc((physinfo.max_cpu_id + 64) / 64, sizeof(uint64_t));
Making xc_cpupool_freeinfo allocate the buffer, like xc_cpupool_getinfo
does would remove the need for this sort of arithmetic in the users of
libxc.
Ian.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch 1/2] support of cpupools in xl: update cpumask handling for cpu pools in libxc and python
2010-10-01 9:35 ` Ian Campbell
@ 2010-10-05 13:50 ` Juergen Gross
2010-10-06 13:17 ` Ian Campbell
0 siblings, 1 reply; 5+ messages in thread
From: Juergen Gross @ 2010-10-05 13:50 UTC (permalink / raw)
To: Ian Campbell; +Cc: xen-devel@lists.xensource.com
On 10/01/10 11:35, Ian Campbell wrote:
> (I highly recommend the patchbomb extension ("hg email"), it makes
> sending series much simpler and threads the mails together in a
> convenient way)
Okay, I'll try hg email for the next round...
>
> On Fri, 2010-10-01 at 08:20 +0100, Juergen Gross wrote:
>> Signed-off-by: juergen.gross@ts.fujitsu.com
>
> This should come after the description.
Okay.
>
>> To be able to support arbitrary numbers of physical cpus it was necessary to
>> include the size of cpumaps in the xc-interfaces for cpu pools.
>> These were:
>> definition of xc_cpupoolinfo_t
>> xc_cpupool_getinfo()
>> xc_cpupool_freeinfo()
>
> Please also mention the change in xc_cpupool_getinfo semantics from
> caller allocated buffer to callee allocated+returned.
Okay.
>
>> @@ -64,50 +78,61 @@ int xc_cpupool_destroy(xc_interface *xch
>> return do_sysctl_save(xch,&sysctl);
>> }
>>
>> -int xc_cpupool_getinfo(xc_interface *xch,
>> - uint32_t first_poolid,
>> - uint32_t n_max,
>> - xc_cpupoolinfo_t *info)
>> +xc_cpupoolinfo_t *xc_cpupool_getinfo(xc_interface *xch,
>> + uint32_t poolid)
> [...]
>> - memset(info, 0, n_max * sizeof(xc_cpupoolinfo_t));
>> + local_size = get_cpumap_size(xch);
>> + local = alloca(local_size);
>> + if (!local_size)
>> + {
>> + PERROR("Could not get number of cpus");
>> + return NULL;
>> + }
>
> I imagine alloca(0) is most likely safe so long as you don't actually
> use the result, but the man page doesn't specifically say. Probably the
> check of !local_size should be before the alloca(local_size) to be on
> the safe side.
Yeah, you are right.
>
>> + cpumap_size = (local_size + sizeof(*info->cpumap) - 1) / sizeof(*info->cpumap);
>
> xg_private.h defines a macro ROUNDUP, I wonder if that should be moved
> somewhere more generic and used to clarify code like this?
Doesn't fit really here (needs the number of bits, e.g. 6 in this case).
As this code will vanish with cpumask rework to be byte-based, I don't
change this now.
>
>> diff -r 8b7d253f0e17 tools/libxc/xenctrl.h
>> --- a/tools/libxc/xenctrl.h Fri Oct 01 08:39:49 2010 +0100
>> +++ b/tools/libxc/xenctrl.h Fri Oct 01 09:13:36 2010 +0100
>> @@ -564,15 +565,11 @@ int xc_cpupool_destroy(xc_interface *xch
>> [...]
>> + * @parm poolid lowest id for which info is returned
>> + * return cpupool info ptr (obtained by malloc)
>> */
>> -int xc_cpupool_getinfo(xc_interface *xch,
>> - uint32_t first_poolid,
>> - uint32_t n_max,
>> - xc_cpupoolinfo_t *info);
>> +xc_cpupoolinfo_t *xc_cpupool_getinfo(xc_interface *xch,
>> + uint32_t poolid);
>>
>> /**
>> * Add cpu to a cpupool. cpu may be -1 indicating the first unassigned.
>> @@ -615,10 +612,12 @@ int xc_cpupool_movedomain(xc_interface *
>> *
>> * @parm xc_handle a handle to an open hypervisor interface
>> * @parm cpumap pointer where to store the cpumap
>> + * @parm cpusize size of cpumap array in bytes
>> * return 0 on success, -1 on failure
>> */
>> int xc_cpupool_freeinfo(xc_interface *xch,
>> - uint64_t *cpumap);
>> + uint64_t *cpumap,
>> + int cpusize);
>
> xc_cpupool_getinfo returns a callee allocated buffer and
> xc_cpupool_freeinfo expects to be given a caller allocated buffer? I
> think we should make this consistent one way of the other.
Agreed.
>
>> diff -r 71f836615ea2 tools/libxl/libxl.c
>> --- a/tools/libxl/libxl.c Fri Sep 24 15:54:39 2010 +0100
>> +++ b/tools/libxl/libxl.c Fri Oct 01 09:03:17 2010 +0200
>> @@ -610,26 +610,34 @@ libxl_poolinfo * libxl_list_pool(libxl_c
>> libxl_poolinfo * libxl_list_pool(libxl_ctx *ctx, int *nb_pool)
>> {
>> libxl_poolinfo *ptr;
>> - int i, ret;
>> - xc_cpupoolinfo_t info[256];
>> - int size = 256;
>> + int i;
>> + xc_cpupoolinfo_t *info;
>> + uint32_t poolid;
>> + libxl_physinfo physinfo;
>>
>> - ptr = calloc(size, sizeof(libxl_poolinfo));
>> - if (!ptr) {
>> - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpupool info");
>> + if (libxl_get_physinfo(ctx,&physinfo) != 0) {
>> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting phys info");
>> return NULL;
>> }
>
> Am I missing where the contents of physinfo is subsequently used in this
> function?
Me too :-)
Should be in the second patch.
>
>>
>> - ret = xc_cpupool_getinfo(ctx->xch, 0, 256, info);
>> - if (ret<0) {
>> - LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "getting cpupool info");
>> - return NULL;
>> + ptr = NULL;
>> +
>> + poolid = 0;
>> + for (i = 0;; i++) {
>> + info = xc_cpupool_getinfo(ctx->xch, poolid);
>> + if (info == NULL)
>> + break;
>> + ptr = realloc(ptr, (i + 1) * sizeof(libxl_poolinfo));
>> + if (!ptr) {
>> + LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpupool info");
>> + return NULL;
>> + }
>
> This will leak the previous value of ptr if realloc() fails. You need to
> do:
> tmp = realloc(ptr, ....)
> if (!tmp) {
> free(ptr);
> LIBXL__LOG_ERRNO(...);
> return NULL;
> }
> ptr = tmp;
>
>
Should be changed in other places, too:
libxc/xc_tmem.c
libxl/libxl.c (sometimes not even checked for error)
>> diff -r 71f836615ea2 tools/python/xen/lowlevel/xc/xc.c
>> --- a/tools/python/xen/lowlevel/xc/xc.c Fri Sep 24 15:54:39 2010 +0100
>> +++ b/tools/python/xen/lowlevel/xc/xc.c Fri Oct 01 09:03:17 2010 +0200
>> @@ -241,7 +241,7 @@ static PyObject *pyxc_vcpu_setaffinity(X
>> if ( xc_physinfo(self->xc_handle,&info) != 0 )
>> return pyxc_error_to_exception(self->xc_handle);
>>
>> - nr_cpus = info.nr_cpus;
>> + nr_cpus = info.max_cpu_id + 1;
>>
>> size = (nr_cpus + cpumap_size * 8 - 1)/ (cpumap_size * 8);
>> cpumap = malloc(cpumap_size * size);
>
> Is this (and the equivalent in getinfo) an independent bug fix for a
> pre-existing issue or does it somehow relate to the rest of the changes?
> I don't see any corresponding change to xc_vcpu_setaffinity is all.
It's an independent fix. OTOH it's cpumask related, so I put it in...
xc_vcpu_setaffinity is not touched as it takes the cpumask size as
parameter.
>
> Given the repeated uses of physinfo.max_cpu_id (here, in get_cpumap_size
> etc) might a xc_get_nr_cpus() function be worthwhile?
Okay. I'll call it xc_get_max_cpus().
>
> Presumably when you change these interfaces to use uint8_t instead of
> uint64_t this code becomes the same as the private get_cpumap_size you
> defined earlier so it might be worth exporting that functionality from
> libxc?
I'll merge these functions later.
>
>> @@ -1966,7 +1967,7 @@ static PyObject *pyxc_cpupool_getinfo(Xc
>> PyObject *list, *info_dict;
>>
>> uint32_t first_pool = 0;
>> - int max_pools = 1024, nr_pools, i;
>> + int max_pools = 1024, i;
> [...]
>> + for (i = 0; i< max_pools; i++)
>
> I don't think there is any 1024 pool limit inherent in the new libxc
> code, is there? You've removed the limit from libxl and I think the
> right thing to do is remove it here as well.
Correct.
>
>> {
>> - free(info);
>> - return pyxc_error_to_exception(self->xc_handle);
>> - }
>> -
>> - list = PyList_New(nr_pools);
>> - for ( i = 0 ; i< nr_pools; i++ )
>> - {
>> + info = xc_cpupool_getinfo(self->xc_handle, first_pool);
>> + if (info == NULL)
>> + break;
>> info_dict = Py_BuildValue(
>> "{s:i,s:i,s:i,s:N}",
>> - "cpupool", (int)info[i].cpupool_id,
>> - "sched", info[i].sched_id,
>> - "n_dom", info[i].n_dom,
>> - "cpulist", cpumap_to_cpulist(info[i].cpumap));
>> + "cpupool", (int)info->cpupool_id,
>> + "sched", info->sched_id,
>> + "n_dom", info->n_dom,
>> + "cpulist", cpumap_to_cpulist(info->cpumap,
>> + info->cpumap_size));
>> + first_pool = info->cpupool_id + 1;
>> + free(info);
>> +
>> if ( info_dict == NULL )
>> {
>> Py_DECREF(list);
>> - if ( info_dict != NULL ) { Py_DECREF(info_dict); }
>> - free(info);
>> return NULL;
>> }
>> - PyList_SetItem(list, i, info_dict);
>> +
>> + PyList_Append(list, info_dict);
>> + Py_DECREF(info_dict);
>> }
>> -
>> - free(info);
>>
>> return list;
>> }
>> @@ -2072,12 +2066,28 @@ static PyObject *pyxc_cpupool_movedomain
>>
>> static PyObject *pyxc_cpupool_freeinfo(XcObject *self)
>> {
>> - uint64_t cpumap;
>> + uint64_t *cpumap;
>> + xc_physinfo_t physinfo;
>> + int ret;
>> + PyObject *info = NULL;
>>
>> - if (xc_cpupool_freeinfo(self->xc_handle,&cpumap) != 0)
>> + if (xc_physinfo(self->xc_handle,&physinfo))
>> return pyxc_error_to_exception(self->xc_handle);
>>
>> - return cpumap_to_cpulist(cpumap);
>> + cpumap = calloc((physinfo.max_cpu_id + 64) / 64, sizeof(uint64_t));
>
> Making xc_cpupool_freeinfo allocate the buffer, like xc_cpupool_getinfo
> does would remove the need for this sort of arithmetic in the users of
> libxc.
Yes.
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Patch 1/2] support of cpupools in xl: update cpumask handling for cpu pools in libxc and python
2010-10-05 13:50 ` Juergen Gross
@ 2010-10-06 13:17 ` Ian Campbell
0 siblings, 0 replies; 5+ messages in thread
From: Ian Campbell @ 2010-10-06 13:17 UTC (permalink / raw)
To: Juergen Gross; +Cc: xen-devel@lists.xensource.com
On Tue, 2010-10-05 at 14:50 +0100, Juergen Gross wrote:
> >
> > This will leak the previous value of ptr if realloc() fails. You need to
> > do:
> > tmp = realloc(ptr, ....)
> > if (!tmp) {
> > free(ptr);
> > LIBXL__LOG_ERRNO(...);
> > return NULL;
> > }
> > ptr = tmp;
> >
> >
>
> Should be changed in other places, too:
> libxc/xc_tmem.c
> libxl/libxl.c (sometimes not even checked for error)
Undoubtedly, but lets not make things worse ;-)
I've added this to my todo list...
> >> diff -r 71f836615ea2 tools/python/xen/lowlevel/xc/xc.c
> >> --- a/tools/python/xen/lowlevel/xc/xc.c Fri Sep 24 15:54:39 2010 +0100
> >> +++ b/tools/python/xen/lowlevel/xc/xc.c Fri Oct 01 09:03:17 2010 +0200
> >> @@ -241,7 +241,7 @@ static PyObject *pyxc_vcpu_setaffinity(X
> >> if ( xc_physinfo(self->xc_handle,&info) != 0 )
> >> return pyxc_error_to_exception(self->xc_handle);
> >>
> >> - nr_cpus = info.nr_cpus;
> >> + nr_cpus = info.max_cpu_id + 1;
> >>
> >> size = (nr_cpus + cpumap_size * 8 - 1)/ (cpumap_size * 8);
> >> cpumap = malloc(cpumap_size * size);
> >
> > Is this (and the equivalent in getinfo) an independent bug fix for a
> > pre-existing issue or does it somehow relate to the rest of the changes?
> > I don't see any corresponding change to xc_vcpu_setaffinity is all.
>
> It's an independent fix. OTOH it's cpumask related, so I put it in...
> xc_vcpu_setaffinity is not touched as it takes the cpumask size as
> parameter.
Please separate unrelated fixes into their own patches. Not least
because it allows you to accurately changelog them.
Thanks,
Ian.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-10-06 13:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-22 5:42 [Patch 1/2] support of cpupools in xl: update cpumask handling for cpu pools in libxc and python Juergen Gross
-- strict thread matches above, loose matches on Subject: below --
2010-10-01 7:20 Juergen Gross
2010-10-01 9:35 ` Ian Campbell
2010-10-05 13:50 ` Juergen Gross
2010-10-06 13:17 ` Ian Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.