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

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

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,
and hence we're wasting memory? Or (since you're mentioning valgrind)
are (without this patch) also leaking stuff in some way?

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

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

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

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

Attachment: signature.asc
Description: This is a digitally signed message part

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