|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 ]libxl: allow to set more than 31 vcpus
On Thu, 2012-05-17 at 06:02 +0100, Zhang, Yang Z wrote:
> In current implementation, it uses integer for record current avail cpus and
> this only allows user to specify 31 vcpus.
> In following patch, it uses cpumap instead interger which make more sense
> than before. Also there is no limit to the max vcpus.
>
> Signed-off-by: Yang Zhang <yang.z.zhang@xxxxxxxxx>
>
> diff -r c8a01e966ec8 tools/libxl/libxl_create.c
> --- a/tools/libxl/libxl_create.c Thu May 17 12:59:49 2012 +0800
> +++ b/tools/libxl/libxl_create.c Thu May 17 13:01:26 2012 +0800
> @@ -142,8 +142,11 @@ int libxl__domain_build_info_setdefault(
>
> if (!b_info->max_vcpus)
> b_info->max_vcpus = 1;
> - if (!b_info->cur_vcpus)
> - b_info->cur_vcpus = 1;
> + if (!b_info->avail_vcpus.size) {
> + if (libxl_cpumap_alloc_size(CTX, &b_info->avail_vcpus, 1))
> + return ERROR_NOMEM;
After your previous patch the only thing cpumap_alloc_size can fail with
is ERROR_FAIL (inability to determine number of processors), converting
that into ERROR_NOMEM seems inappropriate.
This should propagate the actual error code.
It might also be reasonable for libxl to abort/crash if
libxl_get_max_cpus fails in which case cpumap_alloc_size would return
void.
> + libxl_cpumap_set(&b_info->avail_vcpus, 0);
> + }
>
> if (!b_info->cpumap.size) {
> if (libxl_cpumap_alloc(CTX, &b_info->cpumap))
> diff -r c8a01e966ec8 tools/libxl/libxl_dm.c
> --- a/tools/libxl/libxl_dm.c Thu May 17 12:59:49 2012 +0800
> +++ b/tools/libxl/libxl_dm.c Thu May 17 13:01:26 2012 +0800
> @@ -200,10 +200,19 @@ static char ** libxl__build_device_model
> libxl__sprintf(gc, "%d", b_info->max_vcpus),
> NULL);
> }
> - if (b_info->cur_vcpus) {
> + if (b_info->avail_vcpus.size) {
Can never be false, due to allocation in setdefaults?
> + int nr_set_cpus = 0;
> + char *s;
> +
> + nr_set_cpus = libxl_cpumap_count_set(
> + &(((libxl_domain_build_info *)b_info)->avail_vcpus));
> +
> + s = libxl_cpumap_to_hex_string(
> + &(((libxl_domain_build_info *)b_info)->avail_vcpus));
Why the casts to (libxl_domain_build_info *)?
> flexarray_vappend(dm_args, "-vcpu_avail",
> - libxl__sprintf(gc, "0x%x", b_info->cur_vcpus),
> + libxl__sprintf(gc, "0x%s", s),
> NULL);
> + free(s);
> }
> for (i = 0; i < num_vifs; i++) {
> if (vifs[i].nictype == LIBXL_NIC_TYPE_IOEMU) {
> @@ -441,11 +450,15 @@ static char ** libxl__build_device_model
> }
> if (b_info->max_vcpus > 1) {
> flexarray_append(dm_args, "-smp");
> - if (b_info->cur_vcpus)
> + if (b_info->avail_vcpus.size) {
> + int nr_set_cpus = 0;
> + nr_set_cpus = libxl_cpumap_count_set(
> + &(((libxl_domain_build_info *)b_info)->avail_vcpus));
Another strange looking cast.
Are you perhaps casting away the const-ness of b_info?
You most likely want to make libxl_cpumap_count_set const-correct
instead.
> +
> flexarray_append(dm_args, libxl__sprintf(gc, "%d,maxcpus=%d",
> b_info->max_vcpus,
> - b_info->cur_vcpus));
> - else
> + nr_set_cpus));
> + } else
> flexarray_append(dm_args, libxl__sprintf(gc, "%d",
> b_info->max_vcpus));
> }
> diff -r c8a01e966ec8 tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c Thu May 17 12:59:49 2012 +0800
> +++ b/tools/libxl/libxl_dom.c Thu May 17 13:01:26 2012 +0800
> @@ -195,7 +195,8 @@ int libxl__build_post(libxl__gc *gc, uin
> ents[11] = libxl__sprintf(gc, "%lu", state->store_mfn);
> for (i = 0; i < info->max_vcpus; i++) {
> ents[12+(i*2)] = libxl__sprintf(gc, "cpu/%d/availability", i);
> - ents[12+(i*2)+1] = (i && info->cur_vcpus && !(info->cur_vcpus & (1
> << i)))
> + ents[12+(i*2)+1] = (i && info->avail_vcpus.size
"!info->avail_vcpus.size" should always be a bug here since you always
allocate one in setdefaults.
I think you can also stop special casing i == 0, that would be a user
error to supply a cpumap without cpu0 set in it. Might be worth checking
that in the setdefaults fn.
> + && !libxl_cpumap_test(&info->avail_vcpus, i))
> ? "offline" : "online";
> }
>
> @@ -346,7 +347,7 @@ static int hvm_build_set_params(xc_inter
> va_hvm = (struct hvm_info_table *)(va_map + HVM_INFO_OFFSET);
> va_hvm->apic_mode = libxl_defbool_val(info->u.hvm.apic);
> va_hvm->nr_vcpus = info->max_vcpus;
> - memcpy(va_hvm->vcpu_online, &info->cur_vcpus, sizeof(info->cur_vcpus));
> + memcpy(va_hvm->vcpu_online, info->avail_vcpus.map,
> info->avail_vcpus.size);
> for (i = 0, sum = 0; i < va_hvm->length; i++)
> sum += ((uint8_t *) va_hvm)[i];
> va_hvm->checksum -= sum;
> diff -r c8a01e966ec8 tools/libxl/libxl_types.idl
> --- a/tools/libxl/libxl_types.idl Thu May 17 12:59:49 2012 +0800
> +++ b/tools/libxl/libxl_types.idl Thu May 17 13:01:26 2012 +0800
> @@ -242,7 +242,7 @@ libxl_sched_params = Struct("sched_param
> # libxl_file_reference_unmap.
> libxl_domain_build_info = Struct("domain_build_info",[
> ("max_vcpus", integer),
> - ("cur_vcpus", integer),
> + ("avail_vcpus", libxl_cpumap),
> ("cpumap", libxl_cpumap),
> ("tsc_mode", libxl_tsc_mode),
> ("max_memkb", MemKB),
> diff -r c8a01e966ec8 tools/libxl/libxl_utils.c
> --- a/tools/libxl/libxl_utils.c Thu May 17 12:59:49 2012 +0800
> +++ b/tools/libxl/libxl_utils.c Thu May 17 13:01:26 2012 +0800
> @@ -503,6 +503,19 @@ int libxl_cpumap_alloc(libxl_ctx *ctx, l
> return 0;
> }
>
> +int libxl_cpumap_alloc_size(libxl_ctx *ctx, libxl_cpumap *cpumap, int
> max_cpus)
> +{
> + int sz;
> +
> + if (max_cpus == 0)
<= ?
> + return ERROR_FAIL;
> +
> + sz = (max_cpus + 7) / 8;
> + cpumap->map = libxl__zalloc(NULL, sz * sizeof(*cpumap->map));
> + cpumap->size = sz;
> + return 0;
> +}
Can you reimplement libxl_cpumap_alloc in terms of this function?
Alternatively add a new max_cpus param to the existing function and have
it do:
if (max_cpus == 0)
max_cpus = libxl_get_max_cpus(ctx);
(IOW libxl_cpumap_alloc_size(ctx, &map, 0) means allocate the biggest
possibly required map).
> +
> void libxl_cpumap_dispose(libxl_cpumap *map)
> {
> free(map->map);
> @@ -529,6 +542,29 @@ void libxl_cpumap_reset(libxl_cpumap *cp
> cpumap->map[cpu / 8] &= ~(1 << (cpu & 7));
> }
>
> +int libxl_cpumap_count_set(libxl_cpumap *cpumap)
> +{
> + int i, nr_set_cpus = 0;
> + libxl_for_each_set_cpu(i, *cpumap)
> + nr_set_cpus++;
> +
> + return nr_set_cpus;
> +}
> +
> +void *libxl_cpumap_to_hex_string(libxl_cpumap *cpumap)
shouldn't this return char *?
Other libxl types provide a _to_string method -- is the fact that this
one happens to be hex formatted especially relevant? The general form of
those is
(const?) char *libxl_FOO_to_string(libxl_FOO *foo);
> +{
> + int i = cpumap->size;
> + uint8_t *p = libxl__zalloc(NULL, cpumap->size * 2 + 1);
> + uint8_t *q = p;
Why are the uint8_t and not char *?
> + while(--i >= 0) {
> + fprintf(stderr,"i:%d, p :%p, map[%d]: %x\n", i, p, i,
> cpumap->map[i]);
libxl shouldn't be logging to stderr. If this function needs to log then
it should take a ctx and use the logging primitives. I expect this is
just left over debugging which can be removed (it'd be rather verbose
for the final verison)
> + sprintf((char *)p, "%02x", cpumap->map[i]);
> + p += 2;
> + }
> + *p = '\0';
> + return q;
> +}
> +
> int libxl_get_max_cpus(libxl_ctx *ctx)
> {
> return xc_get_max_cpus(ctx->xch);
> diff -r c8a01e966ec8 tools/libxl/libxl_utils.h
> --- a/tools/libxl/libxl_utils.h Thu May 17 12:59:49 2012 +0800
> +++ b/tools/libxl/libxl_utils.h Thu May 17 13:01:26 2012 +0800
> @@ -64,9 +64,12 @@ int libxl_vdev_to_device_disk(libxl_ctx
> libxl_device_disk *disk);
>
> int libxl_cpumap_alloc(libxl_ctx *ctx, libxl_cpumap *cpumap);
> +int libxl_cpumap_alloc_size(libxl_ctx *ctx, libxl_cpumap *cpumap, int
> max_cpus);
> int libxl_cpumap_test(libxl_cpumap *cpumap, int cpu);
> void libxl_cpumap_set(libxl_cpumap *cpumap, int cpu);
> void libxl_cpumap_reset(libxl_cpumap *cpumap, int cpu);
> +int libxl_cpumap_count_set(libxl_cpumap *cpumap);
> +void *libxl_cpumap_to_hex_string(libxl_cpumap *cpumap);
> static inline void libxl_cpumap_set_any(libxl_cpumap *cpumap)
> {
> memset(cpumap->map, -1, cpumap->size);
> diff -r c8a01e966ec8 tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c Thu May 17 12:59:49 2012 +0800
> +++ b/tools/libxl/xl_cmdimpl.c Thu May 17 13:01:26 2012 +0800
> @@ -647,7 +647,14 @@ static void parse_config_data(const char
>
> if (!xlu_cfg_get_long (config, "vcpus", &l, 0)) {
> b_info->max_vcpus = l;
> - b_info->cur_vcpus = (1 << l) - 1;
> +
> + if (libxl_cpumap_alloc_size(ctx, &b_info->avail_vcpus, l)) {
> + fprintf(stderr, "Unable to allocate cpumap\n");
> + exit(1);
> + }
> + libxl_cpumap_set_none(&b_info->avail_vcpus);
> + while (l-- > 0)
> + libxl_cpumap_set((&b_info->avail_vcpus), l);
> }
>
> if (!xlu_cfg_get_long (config, "maxvcpus", &l, 0))
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |