[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [Patch 2/2] support of cpupools in xl: commands and library changes
- To: Ian Campbell <Ian.Campbell@xxxxxxxxxx>
- From: Juergen Gross <juergen.gross@xxxxxxxxxxxxxx>
- Date: Tue, 05 Oct 2010 15:49:25 +0200
- Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
- Delivery-date: Tue, 05 Oct 2010 06:54:16 -0700
- Domainkey-signature: s=s1536a; d=ts.fujitsu.com; c=nofws; q=dns; h=X-SBRSScore:X-IronPort-AV:Received:X-IronPort-AV: Received:Received:Message-ID:Date:From:Organization: User-Agent:MIME-Version:To:CC:Subject:References: In-Reply-To:Content-Type:Content-Transfer-Encoding; b=iTcciLQLOKtTwUp9e6KgMFh5rdf7TOKWZivZ+1qt3jgkDvOAspUDi4cF BzK6KR4XV5rIUn42QmtagB+XYRYtB6Hq1f5/qyLZrf3rpnzZvnlkM1EbJ fKct9/iEsBEj7KvS4RZs09shtnh0/LvroY6zHXfGmWg0HlokWLOKgWYUi f08USOBlLJf6JHZqAE0YRWIFakidqOF94PGnrQ+0nFHyi20567wcQLGqo yrcCtiVgjWbQYFQd8dTAXxVH+F2eZ;
- List-id: Xen developer discussion <xen-devel.lists.xensource.com>
On 10/01/10 11:41, Ian Campbell wrote:
Not a new issue with this series but is there documentation for the pool
configuration file format or an example somewhere? If not could you
maybe add an example at some point (e.g. under tools/examples)?
On Fri, 2010-10-01 at 08:20 +0100, Juergen Gross wrote:
Signed-off-by: juergen.gross@xxxxxxxxxxxxxx
Should go after the comment...
Support of cpu pools in xl:
library functions
xl pool-create
xl pool-list
xl pool-destroy
xl pool-cpu-add
xl pool-cpu-remove
xl pool-migrate
Renamed all cpu pool related names to *cpupool*
But not the actual xl command names -- I presume this is for xm
compatibility, which is shame but unavoidable I suppose.
Hmm. What about adding aliases for xm (xm cpupool-*) and using only
the cpupool-* commands for xl?
diff -r f501dd7581e2 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c Fri Oct 01 09:03:51 2010 +0200
+++ b/tools/libxl/libxl.c Fri Oct 01 09:12:27 2010 +0200
@@ -607,10 +607,10 @@ int libxl_domain_info(libxl_ctx *ctx, li
return 0;
}
-libxl_poolinfo * libxl_list_pool(libxl_ctx *ctx, int *nb_pool)
+libxl_cpupoolinfo * libxl_list_cpupool(libxl_ctx *ctx, int *nb_pool)
{
- libxl_poolinfo *ptr;
- int i;
+ libxl_cpupoolinfo *ptr;
+ int i, m;
xc_cpupoolinfo_t *info;
uint32_t poolid;
libxl_physinfo physinfo;
@@ -627,12 +627,19 @@ libxl_poolinfo * libxl_list_pool(libxl_c
info = xc_cpupool_getinfo(ctx->xch, poolid);
if (info == NULL)
break;
- ptr = realloc(ptr, (i + 1) * sizeof(libxl_poolinfo));
+ ptr = realloc(ptr, (i + 1) * sizeof(libxl_cpupoolinfo));
if (!ptr) {
LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "allocating cpupool
info");
return NULL;
}
ptr[i].poolid = info->cpupool_id;
+ ptr[i].sched_id = info->sched_id;
+ ptr[i].n_dom = info->n_dom;
+ if (libxl_cpumap_alloc(&ptr[i].cpumap, physinfo.max_cpu_id + 1))
+ break;
Ah, here's the user of physinfo I couldn't find in the previous patch!
Why isn't this just using "info->cpumap_size * sizeof(*info->cpumap)"
though? (I have a feeling I asked this before but I can't find the
question or answer in the thread anywhere so maybe I just thought about
it).
Changing to xc_get_max_cpus().
I have a feeling that most users of these interfaces are more interested
in the number of CPUs represented by the cpumap rather than the number
of bytes which happen to be needed to represent them. Perhaps this is
something you could look at while changing the map to uint8_t?
I think this would make sense.
@@ -3659,3 +3664,180 @@ void libxl_file_reference_destroy(libxl_
libxl__file_reference_unmap(f);
free(f->path);
}
+
+int libxl_get_freecpus(libxl_ctx *ctx, libxl_cpumap *cpumap)
+{
+ libxl_physinfo info;
+ int ret;
+
+ if (libxl_get_physinfo(ctx,&info) != 0)
+ return ERROR_FAIL;
+
+ ret = libxl_cpumap_alloc(cpumap, info.max_cpu_id + 1);
+ if (ret)
+ return ret;
+
+ if (xc_cpupool_freeinfo(ctx->xch, cpumap->map, cpumap->size)) {
+ free(cpumap->map);
This should be libxl_cpumap_destroy(&cpumap).
With the change of xc_cpupool_freeinfo() no need for free() any more.
+ cpumap->map = NULL;
+ return ERROR_FAIL;
+ }
+
+ return 0;
+}
+
+int libxl_create_cpupool(libxl_ctx *ctx, char *name, int schedid,
+ libxl_cpumap cpumap, libxl_uuid *uuid,
+ uint32_t *poolid)
+{
+ libxl__gc gc = LIBXL_INIT_GC(ctx);
+ int rc;
+ int i;
+ xs_transaction_t t;
+ char *uuid_string;
+
+ uuid_string = libxl__uuid2string(&gc, *uuid);
+ if (!uuid_string)
+ return ERROR_NOMEM;
+
+ rc = xc_cpupool_create(ctx->xch, poolid, schedid);
+ if (rc) {
+ LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
+ "Could not create cpupool");
+ return ERROR_FAIL;
+ }
+
+ for (i = 0; i< cpumap.size * 8; i++)
+ if (cpumap.map[i / 64]& (1L<< (i % 64))) {
I see a lot of this sort of thing in various places, perhaps it is worth
(later, not now) having an iterator in the style of the Linux foreach_*
macros? e.g.
xl_cpumap_foreach_present(cpumap, i) {
rc = xc_cpupool_addcpu(ctx->xch, *poolid, i);
...
}
I'll look into this when I change the cpumap type.
+ rc = xc_cpupool_addcpu(ctx->xch, *poolid, i);
+ if (rc) {
+ LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
+ "Error moving cpu to cpupool");
+ return ERROR_FAIL;
I guess it's a toss up whether this should destroy the partially created
pool or not.
A call of libxl_destroy_cpupool() should do the job.
+ }
+ }
+
+ for (;;) {
+ t = xs_transaction_start(ctx->xsh);
+
+ xs_mkdir(ctx->xsh, t, libxl__sprintf(&gc, "/local/pool/%d", *poolid));
+ libxl__xs_write(&gc, t, libxl__sprintf(&gc, "/local/pool/%d/uuid",
*poolid),
+ uuid_string);
+ libxl__xs_write(&gc, t, libxl__sprintf(&gc, "/local/pool/%d/name",
*poolid),
+ name);
+
+ if (xs_transaction_end(ctx->xsh, t, 0) || (errno != EAGAIN))
+ return 0;
Does this return success on failure with errno != EAGAIN?
Like lots of other libxl functions...
Missing xenstore entries seem not to be regarded as failures.
+ }
+}
+
+int libxl_destroy_cpupool(libxl_ctx *ctx, uint32_t poolid)
+{
+ libxl__gc gc = LIBXL_INIT_GC(ctx);
+ int rc, i;
+ xc_cpupoolinfo_t *info;
+ xs_transaction_t t;
+
+ info = xc_cpupool_getinfo(ctx->xch, poolid);
+ if (info == NULL)
+ return ERROR_NOMEM;
+
+ rc = ERROR_INVAL;
+ if ((info->cpupool_id != poolid) || (info->n_dom))
+ goto out;
+
+ for (i = 0; i< info->cpumap_size; i++)
+ if (info->cpumap[i / 64]& (1L<< (i % 64))) {
+ rc = xc_cpupool_removecpu(ctx->xch, poolid, i);
I take it this is necessary and that destroying a pool doesn't
implicitly remove all CPUs from the pool?
Not in the hypervisor. This is subject to the tools.
+ if (rc) {
+ LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
+ "Error removing cpu from cpupool");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ }
+
+ rc = xc_cpupool_destroy(ctx->xch, poolid);
+ if (rc) {
+ LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc, "Could not destroy
cpupool");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+
+ for (;;) {
+ t = xs_transaction_start(ctx->xsh);
+
+ xs_rm(ctx->xsh, XBT_NULL, libxl__sprintf(&gc, "/local/pool/%d",
poolid));
+
+ if (xs_transaction_end(ctx->xsh, t, 0) || (errno != EAGAIN))
+ break;
Same comment wrt failure with errno != EAGAIN.
Same answer as above :-)
+int libxl_cpupool_movedomain(libxl_ctx *ctx, uint32_t poolid, uint32_t domid)
+{
+ libxl__gc gc = LIBXL_INIT_GC(ctx);
+ int rc;
+ char *dom_path;
+ char *vm_path;
+ char *poolname;
+ xs_transaction_t t;
+
+ dom_path = libxl__xs_get_dompath(&gc, domid);
+ if (!dom_path) {
+ return ERROR_FAIL;
+ }
+
+ rc = xc_cpupool_movedomain(ctx->xch, poolid, domid);
+ if (rc) {
+ LIBXL__LOG_ERRNOVAL(ctx, LIBXL__LOG_ERROR, rc,
+ "Error moving domain to cpupool");
+ return ERROR_FAIL;
+ }
+
+ poolname = libxl__cpupoolid_to_name(&gc, poolid);
+ vm_path = libxl__xs_read(&gc, XBT_NULL, libxl__sprintf(&gc, "%s/vm",
dom_path));
+ for (; vm_path;) {
+ t = xs_transaction_start(ctx->xsh);
+
+ libxl__xs_write(&gc, t, libxl__sprintf(&gc, "%s/pool_name", vm_path),
poolname);
+
+ if (xs_transaction_end(ctx->xsh, t, 0) || (errno != EAGAIN))
errno != EGAIN handling again.
Have you thought about possible races here? Is it possible that the
reads of vm_path and poolname need to be under the transaction to avoid
races with other operations renaming the pool or migrating the guest
etc?
Theoretically possible, yes.
@@ -5168,3 +5174,444 @@ int main_tmem_freeable(int argc, char **
printf("%d\n", mb);
return 0;
}
+
+int main_poolcreate(int argc, char **argv)
+{
+ char *filename = NULL;
+ char *p, extra_config[1024];
+ int dryrun = 0;
+ int opt;
+ int option_index = 0;
+ static struct option long_options[] = {
+ {"help", 0, 0, 'h'},
+ {"defconfig", 1, 0, 'f'},
+ {"dryrun", 0, 0, 'n'},
+ {0, 0, 0, 0}
+ };
+ int ret;
+ void *config_data = 0;
+ int config_len = 0;
+ XLU_Config *config;
+ const char *buf;
+ char *name, *sched;
+ uint32_t poolid;
+ int schedid = -1;
+ XLU_ConfigList *cpus;
+ int n_cpus, i, n;
+ libxl_cpumap freemap;
+ libxl_cpumap cpumap;
+ libxl_uuid uuid;
+
+ while (1) {
+ opt = getopt_long(argc, argv, "hnf:", long_options,&option_index);
+ if (opt == -1)
+ break;
+
+ switch (opt) {
+ case 'f':
+ filename = optarg;
+ break;
+ case 'h':
+ help("pool-create");
+ return 0;
+ case 'n':
+ dryrun = 1;
+ break;
+ default:
+ fprintf(stderr, "option not supported\n");
+ break;
+ }
+ }
+
+ memset(extra_config, 0, sizeof(extra_config));
+ while (optind< argc) {
+ if ((p = strchr(argv[optind], '='))) {
+ if (strlen(extra_config) + 1< sizeof(extra_config)) {
+ if (strlen(extra_config))
+ strcat(extra_config, "\n");
+ strcat(extra_config, argv[optind]);
+ }
+ } else if (!filename) {
+ filename = argv[optind];
+ } else {
+ help("pool-create");
+ return -ERROR_FAIL;
+ }
+ optind++;
+ }
Move this loop until after libxl_read_file_contents so you can add the
items directly to the end of the data along with the reallocs and
therefore avoid the 1024 character limitation?
The filename may be a result of this loop...
+
+ if (!filename) {
+ help("pool-create");
+ return -ERROR_FAIL;
+ }
+
+ if (libxl_read_file_contents(&ctx, filename,&config_data,&config_len)) {
+ fprintf(stderr, "Failed to read config file: %s: %s\n",
+ filename, strerror(errno));
+ return -ERROR_FAIL;
+ }
+ if (strlen(extra_config)) {
+ if (config_len> INT_MAX - (strlen(extra_config) + 2)) {
+ fprintf(stderr, "Failed to attach extra configration\n");
+ return -ERROR_FAIL;
+ }
+ config_data = realloc(config_data,
+ config_len + strlen(extra_config) + 2);
+ if (!config_data) {
Leaks previous config_data on failure, need to use a temporary variable.
Hmm. I had the impression xl isn't always free-ing all it's allocated
memory...
Switching to xrealloc.
+int main_poollist(int argc, char **argv)
+{
+ int opt;
+ int option_index = 0;
+ static struct option long_options[] = {
+ {"help", 0, 0, 'h'},
+ {"long", 0, 0, 'l'},
+ {"cpus", 0, 0, 'c'},
+ {0, 0, 0, 0}
+ };
+ int opt_long = 0;
+ int opt_cpus = 0;
+ char *pool = NULL;
+ libxl_cpupoolinfo *poolinfo;
+ int n_pools, p, c, n;
+ uint32_t poolid;
+ char *name;
+ int ret = 0;
+
+ while (1) {
+ opt = getopt_long(argc, argv, "hlc", long_options,&option_index);
+ if (opt == -1)
+ break;
+
+ switch (opt) {
+ case 'h':
+ help("pool-list");
+ return 0;
+ case 'l':
+ opt_long = 1;
+ break;
+ case 'c':
+ opt_cpus = 1;
+ break;
+ default:
+ fprintf(stderr, "option not supported\n");
+ break;
+ }
+ }
+
+ if ((optind + 1)< argc) {
+ help("pool-list");
+ return -ERROR_FAIL;
+ }
+ if (optind< argc) {
+ pool = argv[optind];
+ if (libxl_name_to_cpupoolid(&ctx, pool,&poolid)) {
+ fprintf(stderr, "Pool \'%s\' does not exist\n", pool);
+ return -ERROR_FAIL;
+ }
+ }
+
+ poolinfo = libxl_list_cpupool(&ctx,&n_pools);
+ if (!poolinfo) {
+ fprintf(stderr, "error getting cpupool info\n");
+ return -ERROR_NOMEM;
+ }
+
+ if (!opt_long) {
+ printf("%-19s", "Name");
+ if (opt_cpus)
+ printf("CPU list\n");
+ else
+ printf("CPUs Sched Active Domain count\n");
+ }
+
+ for (p = 0; p< n_pools; p++) {
+ if (!ret&& (!pool || (poolinfo[p].poolid != poolid))) {
+ name = libxl_cpupoolid_to_name(&ctx, poolinfo[p].poolid);
Need to free name somewhere.
I can do it, but I think there is much more work ahead for freeing all
allocated memory in xl!
+ if (!name) {
+ fprintf(stderr, "error getting cpupool info\n");
+ ret = -ERROR_NOMEM;
+ }
+ else if (opt_long) {
+ ret = -ERROR_NI;
+ } else {
+ printf("%-19s", name);
+ n = 0;
+ for (c = 0; c< poolinfo[p].cpumap.size * 8; c++)
+ if (poolinfo[p].cpumap.map[c / 64]& (1L<< (c % 64))) {
+ if (n&& opt_cpus) printf(",");
+ if (opt_cpus) printf("%d", c);
+ n++;
+ }
+ if (!opt_cpus) {
+ printf("%3d %9s y %4d", n,
+ libxl_schedid_to_name(&ctx, poolinfo[p].sched_id),
+ poolinfo[p].n_dom);
+ }
+ printf("\n");
+ }
+ }
+ libxl_cpupoolinfo_destroy(poolinfo + p);
+ }
+
+ return ret;
+}
+
+int main_pooldestroy(int argc, char **argv)
+{
+ int opt;
+ char *pool;
+ uint32_t poolid;
+
+ while ((opt = getopt(argc, argv, "h")) != -1) {
+ switch (opt) {
+ case 'h':
+ help("pool-destroy");
+ return 0;
+ default:
+ fprintf(stderr, "option `%c' not supported.\n", opt);
+ break;
+ }
+ }
+
+ pool = argv[optind];
+ if (!pool) {
+ fprintf(stderr, "no cpupool specified\n");
+ help("pool-destroy");
+ return -ERROR_FAIL;
+ }
+
+ if (cpupool_qualifier_to_cpupoolid(pool,&poolid, NULL) ||
+ !libxl_cpupoolid_to_name(&ctx, poolid)) {
Given that cpupool_qualifier_to_cpupoolid basically ==
libxl_name_to_cpupoolid is there really any need to check the inverse
before attempting to destroy the pool?
Yes.
cpupool_qualifier_to_cpupoolid() will always return 0 for any positive
integer given to it (like domain_qualifier_to_domid()). This may be
still no valid cpupoolid.
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@xxxxxxxxxxxxxx
Domagkstr. 28 Internet: ts.fujitsu.com
D-80807 Muenchen Company details: ts.fujitsu.com/imprint.html
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
