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

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Sorry, I was on vacation on past two weeks and just backing to work now. :)
I will give you a modified patch ASAP.

best regards
yang


> -----Original Message-----
> From: Ian Campbell [mailto:Ian.Campbell@xxxxxxxxxx]
> Sent: Friday, June 22, 2012 8:11 PM
> To: Zhang, Yang Z
> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx
> Subject: 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