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

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

> -----Original Message-----
> From: Ian Campbell [mailto:Ian.Campbell@xxxxxxxxxx]
> Sent: Thursday, May 17, 2012 5:45 PM
> >
> >      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.

Agree.

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

Correct! Your point is the right way.

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

Agree. 

> > +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?

Sure, I will send a separated patch to do it.
 
> 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_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)

Sorry, this only for debugging. I forget to delete it before sending the patch. 
:)

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