Re: [Xen-devel] [PATCH]libxl: allow to set more than 31 vcpus

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Zhang, Yang Z writes ("[Xen-devel] [PATCH]libxl: allow to set more than 31 
vcpus"):
> libxl: allow to set more than 31 vcpus
> 
> 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.
...
> -    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;
> +        libxl_cpumap_set(&b_info->avail_vcpus, 0);
> +    }

This error handling should really go away.  Would you be able to
provide a patch to make libxl_cpumap_alloc use libxl__zalloc(NULL, ) ?
That never fails, so that would also mean libxl_cpumap_alloc can't
fail.

> diff -r c6bde42c8845 -r b1229c220984 tools/libxl/libxl_dm.c
> --- a/tools/libxl/libxl_dm.c    Thu Apr 12 14:01:27 2012 +0100
> +++ b/tools/libxl/libxl_dm.c    Fri May 04 10:47:00 2012 +0800
> @@ -200,10 +200,35 @@ 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) {
> +            int i, nr_set_cpus = 0;
> +            char *s;
> +
> +            libxl_for_each_set_cpu(i,
> +                            ((libxl_domain_build_info *)b_info)->avail_vcpus)
> +                nr_set_cpus++;
> +
> +            s = (char *)malloc((nr_set_cpus + 3) / 4 + 1);
> +
> +            memset(s + ((nr_set_cpus % 4) ? 1 : 0), 'f', nr_set_cpus / 4);
> +            switch (nr_set_cpus % 4) {
> +            case 1:
> +                s[0] = '1';
> +                break;
> +            case 2:
> +                s[0] = '3';
> +                break;
> +            case 3:
> +                s[0] = '7';
> +                break;
> +            }
> +
> +            s[(nr_set_cpus + 3) / 4] = '\0';

This is an arbitrary precision conversion to hex.  Wouldn't it be
better to provide libxl_cpumap_set_all, and libxl_cpumap_hexmask or
something ?

> @@ -437,11 +462,16 @@ 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 i, nr_set_cpus = 0;
> +                libxl_for_each_set_cpu(i,
> +                            ((libxl_domain_build_info *)b_info)->avail_vcpus)
> +                    nr_set_cpus++;

If we had libxl_cpumap_count_set this would be clearer.

> diff -r c6bde42c8845 -r b1229c220984 tools/libxl/libxl_dom.c
> --- a/tools/libxl/libxl_dom.c   Thu Apr 12 14:01:27 2012 +0100
> +++ b/tools/libxl/libxl_dom.c   Fri May 04 10:47:00 2012 +0800
> @@ -146,7 +146,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
> +                            && !libxl_cpumap_test(&info->avail_vcpus, i))
>                              ? "offline" : "online";

If libxl_cpumap_test returned 0 if cpumap==NULL then this would be
simpler.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel