Re: [Qemu-devel] [PATCH 3/7] NUMA: parse guest numa nodes memory policy

[Paolo Bonzini <pbonzini@redhat.com>]

Il 18/06/2013 10:09, Wanlong Gao ha scritto:
> +static unsigned int numa_node_parse_mpol(const char *str, unsigned long *bm)
> +{
> +    unsigned long long value, endvalue;
> +    char *endptr;
> +    unsigned int flags = 0;
> +
> +    if (str[0] == '!') {
> +        flags |= 2;

clear = true;

> +        bitmap_fill(bm, MAX_CPUMASK_BITS);
> +        str++;
> +    }
> +    if (str[0] == '+') {
> +        flags |= 1;

flags = NODE_HOST_RELATIVE

> +        str++;
> +    }
> +
> +    if (!strcmp(str, "all")) {
> +        bitmap_fill(bm, MAX_CPUMASK_BITS);
> +        return flags;
> +    }
> +
> +    if (parse_uint(str, &value, &endptr, 10) < 0)
> +        goto error;
> +    if (*endptr == '-') {
> +        if (parse_uint_full(endptr + 1, &endvalue, 10) < 0) {
> +            goto error;
> +        }
> +    } else if (*endptr == '\0') {
> +        endvalue = value;
> +    } else {
> +        goto error;
> +    }
> +
> +    if (endvalue >= MAX_CPUMASK_BITS) {
> +        endvalue = MAX_CPUMASK_BITS - 1;
> +        fprintf(stderr,
> +            "qemu: NUMA: A max of %d host nodes are supported\n",
> +             MAX_CPUMASK_BITS);
> +    }
> +
> +    if (endvalue < value) {
> +        goto error;
> +    }
> +
> +    if (flags & 2)

if (clear)

> +        bitmap_clear(bm, value, endvalue - value + 1);
> +    else
> +        bitmap_set(bm, value, endvalue - value + 1);
> +
> +    return flags;
> +
> +error:
> +    fprintf(stderr, "qemu: Invalid host NUMA nodes range: %s\n", str);

Please change the functions (numa_add and numa_node_parse_mpol) to
accept an Error *.  This will make it much easier to reuse them for e.g.
memory hotplug in the future.

> +    return 4;

return -EINVAL;

> +}

> +        if (get_param_value(option, 128, "interleave", optarg) != 0)
> +            numa_info[nodenr].flags |= NODE_HOST_INTERLEAVE;
> +        else if (get_param_value(option, 128, "preferred", optarg) != 0)
> +            numa_info[nodenr].flags |= NODE_HOST_PREFERRED;
> +        else if (get_param_value(option, 128, "membind", optarg) != 0)
> +            numa_info[nodenr].flags |= NODE_HOST_BIND;

You're not handling the case where someone specifies more than one option.

What about:

   policy={interleave,preferred,bind},mem-hostnode=0

?

Also, please use QemuOpts instead of yet another homegrown parser.
Eduardo, I think you had the most recent attempt to convert -numa to
QemuOpts?

> +        if (option[0] != 0) {
> +            ret = numa_node_parse_mpol(option, numa_info[nodenr].host_mem);
> +            if (ret == 4) {

if (ret < 0)

> +                exit(1);
> +            } else if (ret & 1) {
> +                numa_info[nodenr].flags |= NODE_HOST_RELATIVE;

else {
    numa_info[nodenr].flags |= ret;
}

> +            }
> +        }
> +

Paolo