Juergen Gross
2010-Sep-22 05:42 UTC
[Xen-devel] [Patch 2/2] support of cpupools in xl: commands and library changes
Hi, attached patch adds support of cpu pools in xl/libxl. 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Juergen Gross
2010-Oct-01 07:20 UTC
[Xen-devel] [Patch 2/2] support of cpupools in xl: commands and library changes
Hi, attached patch adds support of cpu pools in xl/libxl. 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Oct-01 09:41 UTC
Re: [Xen-devel] [Patch 2/2] support of cpupools in xl: commands and library changes
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@ts.fujitsu.comShould 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.> 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). 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?> @@ -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).> + 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); ... }> + 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.> + } > + } > + > + 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?> + } > +} > + > +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?> + 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.> +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?> @@ -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?> + > + 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.> +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.> + 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? Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Juergen Gross
2010-Oct-05 13:49 UTC
Re: [Xen-devel] [Patch 2/2] support of cpupools in xl: commands and library changes
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@ts.fujitsu.com > > 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@ts.fujitsu.com Domagkstr. 28 Internet: ts.fujitsu.com D-80807 Muenchen Company details: ts.fujitsu.com/imprint.html _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2010-Oct-06 13:17 UTC
Re: [Xen-devel] [Patch 2/2] support of cpupools in xl: commands and library changes
On Tue, 2010-10-05 at 14:49 +0100, Juergen Gross wrote:> >> 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?I''m not sure, what do others on the list think of this?> > > >> + } > >> + } > >> + > >> + 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.OK, I''m not sure what is considered correct here but if its the norm then fine.> >> + > >> +int main_poolcreate(int argc, char **argv) > >> +{[...]> >> + while (1) {[...]> } > >> + > >> + memset(extra_config, 0, sizeof(extra_config)); > >> + while (optind< argc) {[...]> >> + } > > > > 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...OK, it''s consistent with xl create too so I guess even though it looks weird to me it must be ok ;-)> > 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!Some of us have been actively working towards making xl free its memory correctly since it is a useful to have xl leak free in order to validate libxl which may be used by longer running toolstacks and hence better not leak! I''d certainly like to avoid taking backward steps where possible.> >> + 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.I see, cpupool_qualifier_to_cpupoolid is just a more clever strtoul-like function not an actual cpupoolid lookup function. No problem then. Thanks, Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel