|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 ]libxl: allow to set more than 31 vcpus
On Fri, 2012-06-01 at 12:44 +0100, Ian Campbell wrote:
> On Fri, 2012-06-01 at 03:48 +0100, Zhang, Yang Z wrote:
> > Change from v2:
> > Add function libxl_cpumap_to_hex_string to covert cpumap to hex string.
> > According to Ian's comments, modified some codes to make the logic more
> > reasonable.
> >
> > In current implementation, it uses integer to record current avail cpus and
> > this only allows user to specify 31 vcpus.
> > In following patch, it uses cpumap instead integer which make more sense
> > than before. Also there is no limit to the max vcpus.
> >
> > Signed-off-by: Yang Zhang <yang.z.zhang@xxxxxxxxx>
Hi Yang,
Have I missed a follow up posting of this patch somewhere? Or is it
still pending?
Thanks,
Ian.
> >
> > diff -r 3b0eed731020 tools/libxl/libxl_create.c
> > --- a/tools/libxl/libxl_create.c Fri Jun 01 09:27:17 2012 +0800
> > +++ b/tools/libxl/libxl_create.c Fri Jun 01 10:34:13 2012 +0800
> > @@ -146,8 +146,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(CTX, &b_info->avail_vcpus, 1))
> > + return ERROR_FAIL;
> > + libxl_cpumap_set(&b_info->avail_vcpus, 0);
> > + }
> >
> > if (!b_info->cpumap.size) {
> > if (libxl_cpumap_alloc(CTX, &b_info->cpumap, 0))
> > diff -r 3b0eed731020 tools/libxl/libxl_dm.c
> > --- a/tools/libxl/libxl_dm.c Fri Jun 01 09:27:17 2012 +0800
> > +++ b/tools/libxl/libxl_dm.c Fri Jun 01 10:34:13 2012 +0800
> > @@ -160,6 +160,8 @@ static char ** libxl__build_device_model
> > }
> > if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
> > int ioemu_vifs = 0;
> > + int nr_set_cpus = 0;
> > + char *s;
> >
> > if (b_info->u.hvm.serial) {
> > flexarray_vappend(dm_args, "-serial", b_info->u.hvm.serial,
> > NULL);
> > @@ -200,11 +202,14 @@ static char ** libxl__build_device_model
> > libxl__sprintf(gc, "%d", b_info->max_vcpus),
> > NULL);
> > }
> > - if (b_info->cur_vcpus) {
> > - flexarray_vappend(dm_args, "-vcpu_avail",
> > - libxl__sprintf(gc, "0x%x",
> > b_info->cur_vcpus),
> > - NULL);
> > - }
> > +
> > + nr_set_cpus = libxl_cpumap_count_set(&b_info->avail_vcpus);
> > + s = libxl_cpumap_to_hex_string(&b_info->avail_vcpus);
> > + flexarray_vappend(dm_args, "-vcpu_avail",
> > + libxl__sprintf(gc, "0x%s", s),
>
> You might as well make to_hex_string include the 0x?
>
> > + NULL);
> > + free(s);
> > +
> > for (i = 0; i < num_vifs; i++) {
> > if (vifs[i].nictype == LIBXL_NIC_TYPE_IOEMU) {
> > char *smac = libxl__sprintf(gc,
> > diff -r 3b0eed731020 tools/libxl/libxl_dom.c
> > --- a/tools/libxl/libxl_dom.c Fri Jun 01 09:27:17 2012 +0800
> > +++ b/tools/libxl/libxl_dom.c Fri Jun 01 10:34:13 2012 +0800
> > @@ -195,7 +195,7 @@ 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 && !libxl_cpumap_test(&info->avail_vcpus, i))
> > ? "offline" : "online";
>
> Weren't you going to drop the "i &&" and invert the ternary?
>
> > }
> >
> > @@ -350,7 +350,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);
>
> This needs some sort of limit check, probably in terms of HVM_MAX_VCPUS.
> otherwise a particularly large vcpus= in the config will cause mayhem...
>
> I guess this should probably be done in
> libxl__domain_build_info_setdefaults.
>
> > diff -r 3b0eed731020 tools/libxl/libxl_utils.c
> > --- a/tools/libxl/libxl_utils.c Fri Jun 01 09:27:17 2012 +0800
> > +++ b/tools/libxl/libxl_utils.c Fri Jun 01 10:34:13 2012 +0800
> > @@ -533,6 +533,28 @@ void libxl_cpumap_reset(libxl_cpumap *cp
> > cpumap->map[cpu / 8] &= ~(1 << (cpu & 7));
> > }
> >
> > +int libxl_cpumap_count_set(const libxl_cpumap *cpumap)
> > +{
> > + int i, nr_set_cpus = 0;
> > + libxl_for_each_set_cpu(i, *((libxl_cpumap *)cpumap))
>
> Please stop this habit of yours of casting away the const to make the
> warning go away, it is almost always wrong and on the odd occasion that
> it is right it should have a comment explaining why...
>
> In this case please make libxl_cpumap_test const correct instead.
>
> > + nr_set_cpus++;
> > +
> > + return nr_set_cpus;
> > +}
> > +
> > +char *libxl_cpumap_to_hex_string(const libxl_cpumap *cpumap)
> > +{
> > + int i = cpumap->size;
> > + char *p = libxl__zalloc(NULL, cpumap->size * 2 + 1);
> > + char *q = p;
> > + while(--i >= 0) {
> > + sprintf((char *)p, "%02x", cpumap->map[i]);
>
> Why this cast?
>
> > + p += 2;
> > + }
> > + *p = '\0';
> > + return q;
> > +}
> > +
> > int libxl_get_max_cpus(libxl_ctx *ctx)
> > {
> > return xc_get_max_cpus(ctx->xch);
> > diff -r 3b0eed731020 tools/libxl/libxl_utils.h
> > --- a/tools/libxl/libxl_utils.h Fri Jun 01 09:27:17 2012 +0800
> > +++ b/tools/libxl/libxl_utils.h Fri Jun 01 10:34:13 2012 +0800
> > @@ -67,6 +67,8 @@ int libxl_cpumap_alloc(libxl_ctx *ctx, l
> > 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(const libxl_cpumap *cpumap);
>
> Please add a comment describing this function, it should remind the
> caller that they are responsible for freeing the result.
>
> > +char *libxl_cpumap_to_hex_string(const libxl_cpumap *cpumap);
> > static inline void libxl_cpumap_set_any(libxl_cpumap *cpumap)
> > {
> > memset(cpumap->map, -1, cpumap->size);
> > diff -r 3b0eed731020 tools/libxl/xl_cmdimpl.c
> > --- a/tools/libxl/xl_cmdimpl.c Fri Jun 01 09:27:17 2012 +0800
> > +++ b/tools/libxl/xl_cmdimpl.c Fri Jun 01 10:34:13 2012 +0800
> > @@ -650,7 +650,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(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);
>
> This while loop is == libxl_cpumap_set_any.
>
> > }
> >
> > 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 |