Re: [PATCH RESEND v7] libxl: vnuma topology configuration parser and doc

[Elena Ufimtseva <ufimtseva@gmail.com>]

Konrad

Thank you for reviewing.
I will read your comment for all patches more careful and will
reply/fix tonight.

Elena

On Thu, Aug 21, 2014 at 10:06 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Thu, Aug 21, 2014 at 01:10:29AM -0400, Elena Ufimtseva wrote:
>> Parses vnuma topoplogy number of nodes and memory
>> ranges. If not defined, initializes vnuma with
>> only one node and default topology. This one node covers
>> all domain memory and all vcpus assigned to it.
>
> .. snip..
>> +/*
>> + * init vNUMA to "zero config" with one node and all other
>> + * topology parameters set to default.
>> + */
>> +static int vnuma_default_config(libxl_domain_build_info *b_info)
>> +{
>> +    b_info->vnodes = 1;
>> +    /* all memory goes to this one vnode, as well as vcpus. */
>> +    if (!(b_info->vnuma_mem = (uint64_t *)calloc(b_info->vnodes,
>> +                                sizeof(*b_info->vnuma_mem))))
>> +        goto bad_vnumazerocfg;
>> +
>> +    if (!(b_info->vnuma_vcpumap = (unsigned int *)calloc(b_info->max_vcpus,
>> +                                sizeof(*b_info->vnuma_vcpumap))))
>> +        goto bad_vnumazerocfg;
>> +
>> +    if (!(b_info->vdistance = (unsigned int *)calloc(b_info->vnodes *
>> +                                b_info->vnodes, sizeof(*b_info->vdistance))))
>> +        goto bad_vnumazerocfg;
>> +
>> +    if (!(b_info->vnuma_vnodemap = (unsigned int *)calloc(b_info->vnodes,
>> +                                sizeof(*b_info->vnuma_vnodemap))))
>> +        goto bad_vnumazerocfg;
>> +
>> +    b_info->vnuma_mem[0] = b_info->max_memkb >> 10;
>> +
>> +    /* all vcpus assigned to this vnode. */
>> +    vcputovnode_default(b_info->vnuma_vcpumap, b_info->vnodes,
>> +                        b_info->max_vcpus);
>> +
>> +    /* default vdistance is 10. */
>> +    vdistance_set(b_info->vdistance, b_info->vnodes, 10, 10);
>> +
>> +    /* VNUMA_NO_NODE for vnode_to_pnode. */
>> +    vnuma_vnodemap_default(b_info->vnuma_vnodemap, b_info->vnodes);
>> +
>> +    /*
>> +     * will be placed to some physical nodes defined by automatic
>> +     * numa placement or VNUMA_NO_NODE will not request exact node.
>> +     */
>> +    libxl_defbool_set(&b_info->vnuma_autoplacement, true);
>> +    return 0;
>> +
>> + bad_vnumazerocfg:
>
> I know that the caller ends up calling free_vnuma_info if this fails,
> but my impression is that we should do it here, as in:
>
>         free_vnuma_info(&b_info);
>
> However it is up to the maintainer (Ian) to weigh in which way
> he would like it done.
>
>> +    return -1;
>> +}
>> +
>> +static void free_vnuma_info(libxl_domain_build_info *b_info)
>> +{
>> +    free(b_info->vnuma_mem);
>> +    free(b_info->vdistance);
>> +    free(b_info->vnuma_vcpumap);
>> +    free(b_info->vnuma_vnodemap);
>
> I think you need to set
>         b_info->vnuma_mem = NULL;
>         b_info->vdistance = NULL;
>         ..
>
> and so on. This is more of future proofing the code in case somebody
> in the future adds this inadvertly:
>
>  if (b_info->vnuma_mem) .. blah
>
> without first checking for 'if (b_info->vnodes)'.
>
> It has tripped me in the Linux kernel with the 'unbind' and 'bind' on
> SysFs with drivers forgetting to set their internal states to NULL
> and we crash.
>
>> +    b_info->vnodes = 0;
>> +}
>> +
>> +static int parse_vnuma_mem(XLU_Config *config,
>> +                            libxl_domain_build_info **b_info)
>> +{
>> +    libxl_domain_build_info *dst;
>> +    XLU_ConfigList *vnumamemcfg;
>> +    int nr_vnuma_regions, i;
>> +    unsigned long long vnuma_memparsed = 0;
>> +    unsigned long ul;
>> +    const char *buf;
>> +
>> +    dst = *b_info;
>> +    if (!xlu_cfg_get_list(config, "vnuma_mem",
>> +                          &vnumamemcfg, &nr_vnuma_regions, 0)) {
>> +
>> +        if (nr_vnuma_regions != dst->vnodes) {
>> +            fprintf(stderr, "Number of numa regions (vnumamem = %d) is \
>> +                    incorrect (should be %d).\n", nr_vnuma_regions,
>> +                    dst->vnodes);
>> +            goto bad_vnuma_mem;
>> +        }
>> +
>> +        dst->vnuma_mem = calloc(dst->vnodes,
>> +                                 sizeof(*dst->vnuma_mem));
>> +        if (dst->vnuma_mem == NULL) {
>> +            fprintf(stderr, "Unable to allocate memory for vnuma ranges.\n");
>> +            goto bad_vnuma_mem;
>> +        }
>> +
>> +        char *ep;
>
> I am surprised the compiler didn't throw a fit!
>
> This is usually done at the start of the function or the '{ }' block.
> Please move it there.
>> +        /*
>> +         * Will parse only nr_vnodes times, even if we have more/less regions.
>> +         * Take care of it later if less or discard if too many regions.
>> +         */
>> +        for (i = 0; i < dst->vnodes; i++) {
>> +            buf = xlu_cfg_get_listitem(vnumamemcfg, i);
>> +            if (!buf) {
>> +                fprintf(stderr,
>> +                        "xl: Unable to get element %d in vnuma memory list.\n", i);
>
> You also need to free dst->vnuma_mem before you do the 'goto'.
>
>> +                goto bad_vnuma_mem;
>> +            }
>> +
>> +            ul = strtoul(buf, &ep, 10);
>> +            if (ep == buf) {
>> +                fprintf(stderr, "xl: Invalid argument parsing vnumamem: %s.\n", buf);
>
> Ditto.
>> +                goto bad_vnuma_mem;
>> +            }
>> +
>> +            /* 32Mb is a min size for a node, taken from Linux */
>
> I thought it was 64? Or perhaps I forgot it? Could you give a pointer to
> where this was defined in Linux? Say (taken from Linux - see arch/x86/blahbla/bla.c).
>
>> +            if (ul >= UINT32_MAX || ul < MIN_VNODE_SIZE) {
>> +                fprintf(stderr, "xl: vnuma memory %lu is not within %u - %u range.\n",
>> +                        ul, MIN_VNODE_SIZE, UINT32_MAX);
>
>         free(dst->vnuma_mem);
>
> before you jump. Or you can have in the bad_vnuma_mem:
>         free(dst->vnuma_mem);
>         dst->vnuma_mem = NULL;
>
>> +                goto bad_vnuma_mem;
>> +            }
>> +
>> +            /* memory in MBytes */
>> +            dst->vnuma_mem[i] = ul;
>> +        }
>> +
>> +        /* Total memory for vNUMA parsed to verify */
>> +        for (i = 0; i < nr_vnuma_regions; i++)
>> +            vnuma_memparsed = vnuma_memparsed + (dst->vnuma_mem[i]);
>> +
>> +        /* Amount of memory for vnodes same as total? */
>> +        if ((vnuma_memparsed << 10) != (dst->max_memkb)) {
>> +            fprintf(stderr, "xl: vnuma memory is not the same as domain \
>> +                    memory size.\n");
>> +            goto bad_vnuma_mem;
>
> Again, either free it or the label is responsible for it.
>> +        }
>> +    } else {
>> +        dst->vnuma_mem = calloc(dst->vnodes,
>> +                                      sizeof(*dst->vnuma_mem));
>> +        if (dst->vnuma_mem == NULL) {
>> +            fprintf(stderr, "Unable to allocate memory for vnuma ranges.\n");
>> +            goto bad_vnuma_mem;
>> +        }
>> +
>> +        fprintf(stderr, "WARNING: vNUMA memory ranges were not specified.\n");
>> +        fprintf(stderr, "Using default equal vnode memory size %lu Kbytes \
>> +                to cover %lu Kbytes.\n",
>> +                dst->max_memkb / dst->vnodes, dst->max_memkb);
>> +
>> +        if (split_vnumamem(dst) < 0) {
>> +            fprintf(stderr, "Could not split vnuma memory into equal chunks.\n");
>
> Guess what I am going to say :-)
>
>> +            goto bad_vnuma_mem;
>> +        }
>> +    }
>> +    return 0;
>> +
>> + bad_vnuma_mem:
>> +    return -1;
>> +}
>> +
>> +static int parse_vnuma_distance(XLU_Config *config,
>> +                                libxl_domain_build_info **b_info)
>> +{
>> +    libxl_domain_build_info *dst;
>> +    XLU_ConfigList *vdistancecfg;
>> +    int nr_vdist;
>> +
>> +    dst = *b_info;
>> +    dst->vdistance = calloc(dst->vnodes * dst->vnodes,
>> +                               sizeof(*dst->vdistance));
>> +    if (dst->vdistance == NULL)
>> +        goto bad_distance;
>> +
>> +    if (!xlu_cfg_get_list(config, "vdistance", &vdistancecfg, &nr_vdist, 0)) {
>> +        int d1, d2, i;
>> +        /*
>> +         * First value is the same node distance, the second as the
>> +         * rest of distances. The following is required right now to
>> +         * avoid non-symmetrical distance table as it may break latest kernel.
>> +         * TODO: Better way to analyze extended distance table, possibly
>> +         * OS specific.
>> +         */
>> +
>> +        for (i = 0; i < nr_vdist; i++) {
>> +            d1 = get_list_item_uint(vdistancecfg, i);
>> +        }
>> +
>> +        d1 = get_list_item_uint(vdistancecfg, 0);
>> +        if (dst->vnodes > 1)
>> +           d2 = get_list_item_uint(vdistancecfg, 1);
>> +        else
>> +           d2 = d1;
>> +
>> +        if (d1 >= 0 && d2 >= 0) {
>> +            if (d1 < d2)
>> +                fprintf(stderr, "WARNING: vnuma distance d1 < d2, %u < %u\n", d1, d2);
>> +            vdistance_set(dst->vdistance, dst->vnodes, d1, d2);
>> +        } else {
>> +            fprintf(stderr, "WARNING: vnuma distance values are incorrect.\n");
>> +            goto bad_distance;
>
> free dst->vdistance first.
>
>> +        }
>> +    } else {
>> +        fprintf(stderr, "Could not parse vnuma distances.\n");
>> +        vdistance_set(dst->vdistance, dst->vnodes, 10, 20);
>> +    }
>> +    return 0;
>> +
>> + bad_distance:
>> +    return -1;
>> +}
>> +
>> +static int parse_vnuma_vcpumap(XLU_Config *config,
>> +                                libxl_domain_build_info **b_info)
>> +{
>> +    libxl_domain_build_info *dst;
>> +    XLU_ConfigList *vcpumap;
>> +    int nr_vcpumap, i;
>> +
>> +    dst = *b_info;
>> +    dst->vnuma_vcpumap = (unsigned int *)calloc(dst->max_vcpus,
>> +                                     sizeof(*dst->vnuma_vcpumap));
>> +    if (dst->vnuma_vcpumap == NULL)
>> +        goto bad_vcpumap;
>> +
>> +    if (!xlu_cfg_get_list(config, "vnuma_vcpumap",
>> +                          &vcpumap, &nr_vcpumap, 0)) {
>> +        if (nr_vcpumap == dst->max_vcpus) {
>> +            unsigned int  vnode, vcpumask = 0, vmask;
>
> An newline here please.
>
>> +            vmask = ~(~0 << nr_vcpumap);
>> +            for (i = 0; i < nr_vcpumap; i++) {
>> +                vnode = get_list_item_uint(vcpumap, i);
>> +                if (vnode >= 0 && vnode < dst->vnodes) {
>> +                    vcpumask  |= (1 << i);
>
> There is an extra space there.
>
>> +                    dst->vnuma_vcpumap[i] = vnode;
>> +                }
>> +            }
>> +
>> +            /* Did it covered all vnodes in the vcpu mask? */
>> +            if ( !(((vmask & vcpumask) + 1) == (1 << nr_vcpumap)) ) {
>> +                fprintf(stderr, "WARNING: Not all vnodes were covered \
>> +                        in numa_cpumask.\n");
>
> Either free it here or the label has to clear it.
>
>> +                goto bad_vcpumap;
>> +            }
>> +        } else {
>> +            fprintf(stderr, "WARNING:  Bad vnuma_vcpumap.\n");
>> +            goto bad_vcpumap;
>> +        }
>> +    }
>> +    else
>> +        vcputovnode_default(dst->vnuma_vcpumap,
>> +                            dst->vnodes,
>> +                            dst->max_vcpus);
>> +    return 0;
>> +
>> + bad_vcpumap:
>> +    return -1;
>> +}
>> +
>> +static int parse_vnuma_vnodemap(XLU_Config *config,
>> +                                libxl_domain_build_info **b_info)
>> +{
>> +    libxl_domain_build_info *dst;
>> +    XLU_ConfigList *vnodemap;
>> +    int nr_vnodemap, i;
>> +
>> +    dst = *b_info;
>> +
>> +    /* There is mapping to NUMA physical nodes? */
>> +    dst->vnuma_vnodemap = (unsigned int *)calloc(dst->vnodes,
>> +                                    sizeof(*dst->vnuma_vnodemap));
>> +    if (dst->vnuma_vnodemap == NULL)
>> +        goto bad_vnodemap;
>> +    if (!xlu_cfg_get_list(config, "vnuma_vnodemap",&vnodemap,
>> +                                            &nr_vnodemap, 0)) {
>
> something is off. It doesn't need to be so far off. The &nr_vnodemap
> can be aligned with the first parameter.
>> +        /*
>> +        * If not specified or incorred, will be defined
>
> s/incorred/incorrect/



-- 
Elena