Ian Campbell
2012-Jan-12 08:43 UTC
Re: [PATCH 2 of 3] libxl: allow for specifying the CPU affinity in the config file.
On Wed, 2012-01-11 at 18:00 +0000, Dario Faggioli wrote:> Enable CPU affinity specification in a VM''s config file with the > exact syntax `xl vcpu-pin'' provides. > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> > > diff -r 9ce68a4036b1 tools/libxl/libxl_create.c > --- a/tools/libxl/libxl_create.c Wed Jan 11 17:38:04 2012 +0000 > +++ b/tools/libxl/libxl_create.c Wed Jan 11 17:40:45 2012 +0000 > @@ -78,6 +78,8 @@ int libxl_init_build_info(libxl_ctx *ctx > memset(b_info, ''\0'', sizeof(*b_info)); > b_info->max_vcpus = 1; > b_info->cur_vcpus = 1; > + if (libxl_cpumap_alloc(ctx, &b_info->cpumap)) > + return ERROR_NOMEM;Should probably set all here since that is the best default?> b_info->max_memkb = 32 * 1024; > b_info->target_memkb = b_info->max_memkb; > b_info->disable_migrate = 0; > diff -r 9ce68a4036b1 tools/libxl/libxl_dom.c > --- a/tools/libxl/libxl_dom.c Wed Jan 11 17:38:04 2012 +0000 > +++ b/tools/libxl/libxl_dom.c Wed Jan 11 17:40:45 2012 +0000 > @@ -72,8 +72,14 @@ int libxl__build_pre(libxl__gc *gc, uint > libxl_domain_build_info *info, libxl__domain_build_state *state) > { > libxl_ctx *ctx = libxl__gc_owner(gc); > - int tsc_mode; > + int i, tsc_mode; > xc_domain_max_vcpus(ctx->xch, domid, info->max_vcpus); > + for (i = 0; i < info->max_vcpus; i++) { > + if (libxl_set_vcpuaffinity(ctx, domid, i, &info->cpumap) == -1) { > + LIBXL__LOG(ctx, LIBXL__LOG_WARNING, "libxl_set_vcpuaffinity failed. " > + "Going ahead without setting affinity for cpu %d.\n", i); > + } > + }4b > xc_domain_setmaxmem(ctx->xch, domid, info->target_memkb + LIBXL_MAXMEM_CONSTANT); > if (info->type == LIBXL_DOMAIN_TYPE_PV) > xc_domain_set_memmap_limit(ctx->xch, domid, > diff -r 9ce68a4036b1 tools/libxl/libxl_types.idl > --- a/tools/libxl/libxl_types.idl Wed Jan 11 17:38:04 2012 +0000 > +++ b/tools/libxl/libxl_types.idl Wed Jan 11 17:40:45 2012 +0000 > @@ -162,6 +162,7 @@ libxl_domain_create_info = Struct("domai > libxl_domain_build_info = Struct("domain_build_info",[ > ("max_vcpus", integer), > ("cur_vcpus", integer), > + ("cpumap", libxl_cpumap), > ("tsc_mode", libxl_tsc_mode), > ("max_memkb", uint32), > ("target_memkb", uint32), > diff -r 9ce68a4036b1 tools/libxl/xl_cmdimpl.c > --- a/tools/libxl/xl_cmdimpl.c Wed Jan 11 17:38:04 2012 +0000 > +++ b/tools/libxl/xl_cmdimpl.c Wed Jan 11 17:40:45 2012 +0000 > @@ -288,16 +288,24 @@ static void dolog(const char *file, int > libxl_write_exactly(NULL, logfile, s, rc, NULL, NULL); > } > > +static void print_bitmap(uint8_t *map, int maplen, FILE *stream);Just move the function up? If we are going to do this sort of forward declaration there should be single block of them near the top somewhere.> + > static void printf_info(int domid, > libxl_domain_config *d_config, > libxl_device_model_info *dm_info) > { > - int i; > + int i, nr_cpus = -1; > libxl_dominfo info; > + libxl_physinfo physinfo; > > libxl_domain_create_info *c_info = &d_config->c_info; > libxl_domain_build_info *b_info = &d_config->b_info; > > + if (libxl_get_physinfo(ctx, &physinfo) == 0) > + nr_cpus = physinfo.nr_cpus; > + else > + fprintf(stderr, "libxl_physinfo failed.\n"); > + > printf("(domain\n\t(domid %d)\n", domid); > printf("\t(create_info)\n"); > printf("\t(hvm %d)\n", c_info->type == LIBXL_DOMAIN_TYPE_HVM); > @@ -328,6 +336,9 @@ static void printf_info(int domid, > > printf("\t(build_info)\n"); > printf("\t(max_vcpus %d)\n", b_info->max_vcpus); > + printf("\t(CPU affinity "); > + print_bitmap(b_info->cpumap.map, nr_cpus, stdout); > + printf(")\n"); > printf("\t(tsc_mode %s)\n", libxl_tsc_mode_to_string(b_info->tsc_mode)); > printf("\t(max_memkb %d)\n", b_info->max_memkb); > printf("\t(target_memkb %d)\n", b_info->target_memkb); > @@ -569,6 +580,8 @@ static void split_string_into_string_lis > free(s); > } > > +static int vcpupin_parse(char *cpu, libxl_cpumap *cpumap);Again, maybe just reorder the functions?> static void parse_config_data(const char *configfile_filename_report, > const char *configfile_data, > int configfile_len, > @@ -661,6 +674,13 @@ static void parse_config_data(const char > if (!xlu_cfg_get_long (config, "maxvcpus", &l, 0)) > b_info->max_vcpus = l; > > + if (!xlu_cfg_get_string (config, "cpus", &buf, 0)) {The syntax supported here is different to that in a cpupool cfg? (see main_cpupoolcreate) Perhaps they should be similar?> + char *buf2 = strdup(buf); > + vcpupin_parse(buf2, &b_info->cpumap); > + } else { > + memset(b_info->cpumap.map, -1, b_info->cpumap.size);This could do with a helper function in libxl_utils.h. Ian.> + } > + > if (!xlu_cfg_get_long (config, "memory", &l, 0)) { > b_info->max_memkb = l * 1024; > b_info->target_memkb = b_info->max_memkb; >
Ian Campbell
2012-Jan-13 08:09 UTC
Re: [PATCH 2 of 3] libxl: allow for specifying the CPU affinity in the config file.
On Thu, 2012-01-12 at 22:56 +0000, Dario Faggioli wrote:> On Thu, 2012-01-12 at 08:43 +0000, Ian Campbell wrote: > > > diff -r 9ce68a4036b1 tools/libxl/libxl_create.c > > > --- a/tools/libxl/libxl_create.c Wed Jan 11 17:38:04 2012 +0000 > > > +++ b/tools/libxl/libxl_create.c Wed Jan 11 17:40:45 2012 +0000 > > > @@ -78,6 +78,8 @@ int libxl_init_build_info(libxl_ctx *ctx > > > memset(b_info, ''\0'', sizeof(*b_info)); > > > b_info->max_vcpus = 1; > > > b_info->cur_vcpus = 1; > > > + if (libxl_cpumap_alloc(ctx, &b_info->cpumap)) > > > + return ERROR_NOMEM; > > > > Should probably set all here since that is the best default? > > > Having this set to "none" here simplifies a bit the code when we came to > config file parsing (if I set it to "any CPU" here, I''ll have to reset > to "none" before parsing). Also, default is exactly what you''re > suggesting already, as I set the map to "any CPU" if no "cpus" config > option is found.xl''s default is "any CPU" but libxl''s default is "no CPU" which will mean every toolstack author needs to do be aware of this even if they don''t care about affinity.> Anyway, I guess I can do as you suggest if you really think it''s > better. :-)Please ;-)> > > static void parse_config_data(const char *configfile_filename_report, > > > const char *configfile_data, > > > int configfile_len, > > > @@ -661,6 +674,13 @@ static void parse_config_data(const char > > > if (!xlu_cfg_get_long (config, "maxvcpus", &l, 0)) > > > b_info->max_vcpus = l; > > > > > > + if (!xlu_cfg_get_string (config, "cpus", &buf, 0)) { > > > > The syntax supported here is different to that in a cpupool cfg? (see > > main_cpupoolcreate) Perhaps they should be similar? > > > It is different, and that was intentional. What I wanted, was the syntax > to be exactly the same of `xl vcpu-pin'', which is the command line > equivalent of this option, much more than cpupool-*. If you want, I > think I can easily enable list-like CPU specification as in cpupool > config file so that _both_ syntax are allowed. What do you think?I think supporting both makes sense, we do something similar in a couple of places already. Ian.