Re: [Xen-devel] [PATCH for-4.6 2/3] xl: fix vNUMA vcpus parsing

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Aug 13, 2015 at 11:32:35AM +0200, Dario Faggioli wrote:
> On Wed, 2015-08-12 at 20:36 +0100, Wei Liu wrote:
> > Originally, if user didn't specify maxvcpus= in xl config file, the
> > maximum size of vcpu bitmap was always equal to maximum number of pcpus.
> > This might not be what user wants.
> > 
> So, to understand, the issue here is that the bitmaps may be too bit,

Too small.

> and hence we're wasting memory? Or (since you're mentioning valgrind)
> are (without this patch) also leaking stuff in some way?
> 

No. I mention valgrind because I want to be sure the refactoring doesn't
cause memory leak.

> I'm confused by he "not be what user wants" part, as, IMO, the actual
> user --i.e., the person writing the config file and then issuing the `xl
> ceate' command-- wouldn't notice any difference, would it? Of course,
> that does not mean that we should waste memory... as I said, I'm asking
> in order to understand what this patch is actually trying to fix...
> 

The trick here is that xl is "smart" enough to sum up the number of
vcpus needed in vNUMA configuration and check that against maxvcpus (if
specified).

So if users has not specified maxvcpus, b_info->..max_vcpus would be 0,
and the allocated bitmap would have the size of number of pcpus. That
bitmap may be too small for the number of vcpus specified in vNUMA
configuration.

Consider following configuration:

memory = 186368
vcpus = 36
cpus = "nodes:0"
vnuma = [ [ "pnode=0","size=2048","vcpus=0-35","vdistances=10,20,20,20" ],
          [ "pnode=1","size=61440","vdistances=20,10,20,20" ],
          [ "pnode=2","size=61440","vdistances=20,20,10,20" ],
          [ "pnode=3","size=61440","vdistances=20,20,20,10" ] ]
# no maxvcpus=

If you only have, say, 8 pcpus, the parsed configuration is wrong.

> About the code...
> 
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 078acd1..0fcef98 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> >
> 
> Would it make sense to skip this (the first) step if the user actually
> did specify "maxvcpus="? Or, if not, at least... ... ...
> 

No, see above.

> > +    /* Pass one, get the total number of vcpus */
> > +    PARSE_VNUMA_SPEC({
> > +            if (!strcmp("vcpus", option)) {
> > +                split_string_into_string_list(value, ",", &cpu_spec_list);
> > +                len = libxl_string_list_length(&cpu_spec_list);
> >  
> > -        vnode_spec = xlu_cfg_get_listitem2(vnuma, i);
> > -        assert(vnode_spec);
> > +                for (j = 0; j < len; j++) {
> > +                    parse_range(cpu_spec_list[j], &s, &e);
> > +                    for (; s <= e; s++)
> > +                        max_vcpus++;
> > +                }
> > +                libxl_string_list_dispose(&cpu_spec_list);
> > +            }
> > +        });
> >  
> ... ... ... Move the check for that here, i.e., avoiding doing the
> second pass if there is a mismatch?
> 

Yes, this is doable.

Wei.

> > +    for (i = 0; i < num_vnuma; i++) {
> > +        libxl_bitmap_init(&vcpu_parsed[i]);
> > +        if (libxl_cpu_bitmap_alloc(ctx, &vcpu_parsed[i], max_vcpus)) {
> > +            fprintf(stderr, "libxl_node_bitmap_alloc failed.\n");
> >              exit(1);
> >          }
> > +    }
> 
> > +    /* Pass two, fill in structs */
> > +    PARSE_VNUMA_SPEC({
> > +            if (!strcmp("pnode", option)) {
> > +                val = parse_ulong(value);
> > +                if (val >= nr_nodes) {
> > +                    fprintf(stderr,
> > +                            "xl: invalid pnode number: %lu\n", val);
> 
> >      /* User has specified maxvcpus= */
> >      if (b_info->max_vcpus != 0 &&  b_info->max_vcpus != max_vcpus) {
> 
> Regards,
> Dario
> -- 
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)



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