From mboxrd@z Thu Jan 1 00:00:00 1970 From: Juergen Gross Subject: Re: [Patch 2/2] support of cpupools in xl: commands and library changes Date: Tue, 05 Oct 2010 15:49:25 +0200 Message-ID: <4CAB2CE5.1070208@ts.fujitsu.com> References: <4CA58BC1.9030407@ts.fujitsu.com> <1285926115.16095.46889.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1285926115.16095.46889.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Ian Campbell Cc: "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org 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