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

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

> -----Original Message-----
> From: Ian Jackson [mailto:Ian.Jackson@xxxxxxxxxxxxx]
> Sent: Friday, May 11, 2012 12:53 AM
> Subject: Re: [Xen-devel] [PATCH]libxl: allow to set more than 31 vcpus
> 
> 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.

I don't think so. Libxl_cpumap_alloc() also returned error when max_cpus equal 
zero. So the error handling cannot be avoid even using libxl__zalloc.
 
> 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 ?

Yes, it's better to use a function to do the convert cpumap to hex.

> > @@ -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.

Agree.
 
> > 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.

Sorry. What your mean for this?

Best regard
yang


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