From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:58314)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <gaowanlong@cn.fujitsu.com>) id 1UoseB-0006bg-Sq
	for qemu-devel@nongnu.org; Tue, 18 Jun 2013 05:56:22 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <gaowanlong@cn.fujitsu.com>) id 1Uose8-0001O3-JV
	for qemu-devel@nongnu.org; Tue, 18 Jun 2013 05:56:19 -0400
Received: from [222.73.24.84] (port=6964 helo=song.cn.fujitsu.com)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <gaowanlong@cn.fujitsu.com>) id 1Uose7-0001NK-Ut
	for qemu-devel@nongnu.org; Tue, 18 Jun 2013 05:56:16 -0400
Message-ID: <51C02E50.9070200@cn.fujitsu.com>
Date: Tue, 18 Jun 2013 17:54:24 +0800
From: Wanlong Gao <gaowanlong@cn.fujitsu.com>
MIME-Version: 1.0
References: <1371542991-15911-1-git-send-email-gaowanlong@cn.fujitsu.com>
	<1371542991-15911-4-git-send-email-gaowanlong@cn.fujitsu.com>
	<51C02665.1060205@redhat.com>
In-Reply-To: <51C02665.1060205@redhat.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset=ISO-8859-15
Subject: Re: [Qemu-devel] [PATCH 3/7] NUMA: parse guest numa nodes memory
	policy
Reply-To: gaowanlong@cn.fujitsu.com
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: andre.przywara@amd.com, aliguori@us.ibm.com, ehabkost@redhat.com, qemu-devel@nongnu.org, y-goto@jp.fujitsu.com, afaerber@suse.de, Wanlong Gao <gaowanlong@cn.fujitsu.com>

On 06/18/2013 05:20 PM, Paolo Bonzini wrote:
> 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.

Got it, I'll try. Thank you.

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

OK, will follow this, thank you.

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

So, any patches I can based on or change it myself?  Eduardo?


Thanks,
Wanlong Gao

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