|
[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
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.
> 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |