[V5,01/12] NUMA: add NumaOptions, NumaNodeOptions and NumaMemOptions

Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 qapi-schema.json | 44 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

comments below

On 07/17/13 11:29, Wanlong Gao wrote:
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> ---
>  qapi-schema.json | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 7b9fef1..f753a35 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3679,3 +3679,47 @@
>              '*cpuid-input-ecx': 'int',
>              'cpuid-register': 'X86CPURegister32',
>              'features': 'int' } }
> +
> +##
> +# @NumaOptions
> +#
> +# A discriminated record of NUMA options.
> +#
> +# Since 1.6
> +##
> +{ 'union': 'NumaOptions',
> +  'data': {
> +    'node':	'NumaNodeOptions',
> +    'mem':	'NumaMemOptions' }}
> +
> +##
> +# @NumaNodeOptions
> +#
> +# Create a guest NUMA node.
> +#
> +# @nodeid: #optional NUMA node ID
> +#
> +# @cpus: #optional VCPUs belong to this node
> +#
> +# Since: 1.6
> +##
> +{ 'type': 'NumaNodeOptions',
> +  'data': {
> +   '*nodeid':		'int',
> +   '*cpus':		'str' }}
> +

Should we document the format for "cpus" here too?

> +##
> +# @NumaMemOptions
> +#
> +# Set memory information of guest NUMA node.
> +#
> +# @nodeid: #optional NUMA node ID
> +#
> +# @size: #optional memory size of this node
> +#
> +# Since 1.6
> +##
> +{ 'type': 'NumaMemOptions',
> +  'data': {
> +   '*nodeid':		'int',
> +   '*size':		'size' }}
> 

Looks good in general but I'm not sure if hardware tabs are allowed (or
usual) in this file.

Thanks
Laszlo

Il 17/07/2013 12:35, Laszlo Ersek ha scritto:
> comments below
> 
> On 07/17/13 11:29, Wanlong Gao wrote:
>> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
>> ---
>>  qapi-schema.json | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 44 insertions(+)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 7b9fef1..f753a35 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -3679,3 +3679,47 @@
>>              '*cpuid-input-ecx': 'int',
>>              'cpuid-register': 'X86CPURegister32',
>>              'features': 'int' } }
>> +
>> +##
>> +# @NumaOptions
>> +#
>> +# A discriminated record of NUMA options.
>> +#
>> +# Since 1.6
>> +##
>> +{ 'union': 'NumaOptions',
>> +  'data': {
>> +    'node':	'NumaNodeOptions',
>> +    'mem':	'NumaMemOptions' }}
>> +
>> +##
>> +# @NumaNodeOptions
>> +#
>> +# Create a guest NUMA node.
>> +#
>> +# @nodeid: #optional NUMA node ID
>> +#
>> +# @cpus: #optional VCPUs belong to this node
>> +#
>> +# Since: 1.6
>> +##
>> +{ 'type': 'NumaNodeOptions',
>> +  'data': {
>> +   '*nodeid':		'int',
>> +   '*cpus':		'str' }}
>> +
> 
> Should we document the format for "cpus" here too?

I think so---good catch.

>> +##
>> +# @NumaMemOptions
>> +#
>> +# Set memory information of guest NUMA node.
>> +#
>> +# @nodeid: #optional NUMA node ID
>> +#
>> +# @size: #optional memory size of this node
>> +#
>> +# Since 1.6
>> +##
>> +{ 'type': 'NumaMemOptions',
>> +  'data': {
>> +   '*nodeid':		'int',
>> +   '*size':		'size' }}
>>
> 
> Looks good in general but I'm not sure if hardware tabs are allowed (or
> usual) in this file.

Definitely not usual---in fact not used at all, so they're probably not
allowed too.

Paolo

On 07/17/2013 04:35 AM, Laszlo Ersek wrote:

>> +# @cpus: #optional VCPUs belong to this node
>> +#
>> +# Since: 1.6
>> +##
>> +{ 'type': 'NumaNodeOptions',
>> +  'data': {
>> +   '*nodeid':		'int',
>> +   '*cpus':		'str' }}
>> +
> 
> Should we document the format for "cpus" here too?

Not only that, but is this even the right representation?  The fact that
you are requiring the receiver to further parse this string means you
probably represented it at the wrong level in JSON.  That is, a JSON
string "1,2,4" requires post-processing to turn it into 3 processor ids,
while a JSON array [1, 2, 4] does not, so you should probably consider
'*cpus':['int'] as your preferred datatype.

>> +# Since 1.6
>> +##
>> +{ 'type': 'NumaMemOptions',
>> +  'data': {
>> +   '*nodeid':		'int',
>> +   '*size':		'size' }}
>>
> 
> Looks good in general but I'm not sure if hardware tabs are allowed (or
> usual) in this file.

Drop the tabs.  Also, this missed soft freeze for 1.6, so you will
probably end up using Since 1.7 by the time it actually gets accepted.

On 07/17/2013 07:11 PM, Paolo Bonzini wrote:
> Il 17/07/2013 12:35, Laszlo Ersek ha scritto:
>> comments below
>>
>> On 07/17/13 11:29, Wanlong Gao wrote:
>>> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
>>> ---
>>>  qapi-schema.json | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 44 insertions(+)
>>>
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index 7b9fef1..f753a35 100644
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -3679,3 +3679,47 @@
>>>              '*cpuid-input-ecx': 'int',
>>>              'cpuid-register': 'X86CPURegister32',
>>>              'features': 'int' } }
>>> +
>>> +##
>>> +# @NumaOptions
>>> +#
>>> +# A discriminated record of NUMA options.
>>> +#
>>> +# Since 1.6
>>> +##
>>> +{ 'union': 'NumaOptions',
>>> +  'data': {
>>> +    'node':	'NumaNodeOptions',
>>> +    'mem':	'NumaMemOptions' }}
>>> +
>>> +##
>>> +# @NumaNodeOptions
>>> +#
>>> +# Create a guest NUMA node.
>>> +#
>>> +# @nodeid: #optional NUMA node ID
>>> +#
>>> +# @cpus: #optional VCPUs belong to this node
>>> +#
>>> +# Since: 1.6
>>> +##
>>> +{ 'type': 'NumaNodeOptions',
>>> +  'data': {
>>> +   '*nodeid':		'int',
>>> +   '*cpus':		'str' }}
>>> +
>>
>> Should we document the format for "cpus" here too?
> 
> I think so---good catch.

Got it, thank you.

> 
>>> +##
>>> +# @NumaMemOptions
>>> +#
>>> +# Set memory information of guest NUMA node.
>>> +#
>>> +# @nodeid: #optional NUMA node ID
>>> +#
>>> +# @size: #optional memory size of this node
>>> +#
>>> +# Since 1.6
>>> +##
>>> +{ 'type': 'NumaMemOptions',
>>> +  'data': {
>>> +   '*nodeid':		'int',
>>> +   '*size':		'size' }}
>>>
>>
>> Looks good in general but I'm not sure if hardware tabs are allowed (or
>> usual) in this file.
> 
> Definitely not usual---in fact not used at all, so they're probably not
> allowed too.

Got it, thank you.

Wanlong Gao

> 
> Paolo
>

On 07/17/13 14:24, Eric Blake wrote:
> On 07/17/2013 04:35 AM, Laszlo Ersek wrote:
> 
>>> +# @cpus: #optional VCPUs belong to this node
>>> +#
>>> +# Since: 1.6
>>> +##
>>> +{ 'type': 'NumaNodeOptions',
>>> +  'data': {
>>> +   '*nodeid':		'int',
>>> +   '*cpus':		'str' }}
>>> +
>>
>> Should we document the format for "cpus" here too?
> 
> Not only that, but is this even the right representation?  The fact that
> you are requiring the receiver to further parse this string means you
> probably represented it at the wrong level in JSON.  That is, a JSON
> string "1,2,4" requires post-processing to turn it into 3 processor ids,
> while a JSON array [1, 2, 4] does not, so you should probably consider
> '*cpus':['int'] as your preferred datatype.

opts-visitor can handle lists of simple scalar types. Ie. it can do
-numa node,nodeid=3,cpus=3-4,cpus=9-10. It can't save the parsing of
intervals (eg. 3-4).

This is of course not to say that the interface should be limited by
what opts-visitor can do; just that opts-visitor may not be appropriate
for (or solve completely the needs of) very intricate options.

Laszlo

Il 17/07/2013 15:57, Laszlo Ersek ha scritto:
>> > Not only that, but is this even the right representation?  The fact that
>> > you are requiring the receiver to further parse this string means you
>> > probably represented it at the wrong level in JSON.  That is, a JSON
>> > string "1,2,4" requires post-processing to turn it into 3 processor ids,
>> > while a JSON array [1, 2, 4] does not, so you should probably consider
>> > '*cpus':['int'] as your preferred datatype.
> opts-visitor can handle lists of simple scalar types. Ie. it can do
> -numa node,nodeid=3,cpus=3-4,cpus=9-10. It can't save the parsing of
> intervals (eg. 3-4).

Saving the parsing of intervals is not necessary for this use case.  So
if we can make it '*cpus':['int'], we should.

But is it the opts-visitor "can handle" lists of integers, or does code
have to be written?  If the latter, can you whip up a prototype?

Paolo

> This is of course not to say that the interface should be limited by
> what opts-visitor can do; just that opts-visitor may not be appropriate
> for (or solve completely the needs of) very intricate options.

On 07/17/13 16:20, Paolo Bonzini wrote:
> Il 17/07/2013 15:57, Laszlo Ersek ha scritto:
>>>> Not only that, but is this even the right representation?  The fact that
>>>> you are requiring the receiver to further parse this string means you
>>>> probably represented it at the wrong level in JSON.  That is, a JSON
>>>> string "1,2,4" requires post-processing to turn it into 3 processor ids,
>>>> while a JSON array [1, 2, 4] does not, so you should probably consider
>>>> '*cpus':['int'] as your preferred datatype.
>> opts-visitor can handle lists of simple scalar types. Ie. it can do
>> -numa node,nodeid=3,cpus=3-4,cpus=9-10. It can't save the parsing of
>> intervals (eg. 3-4).
> 
> Saving the parsing of intervals is not necessary for this use case.  So
> if we can make it '*cpus':['int'], we should.
> 
> But is it the opts-visitor "can handle" lists of integers, or does code
> have to be written?  If the latter, can you whip up a prototype?

No extra code needs to be written. The current use case is
NetdevUserOptions.{dnssearch,hostfwd,guestfwd}; see commit 094f15c5, and
(by Klaus Stengel) commit 63d2960b.

Laszlo

Il 17/07/2013 16:33, Laszlo Ersek ha scritto:
>>> >> opts-visitor can handle lists of simple scalar types. Ie. it can do
>>> >> -numa node,nodeid=3,cpus=3-4,cpus=9-10. It can't save the parsing of
>>> >> intervals (eg. 3-4).
>> > 
>> > Saving the parsing of intervals is not necessary for this use case.  So
>> > if we can make it '*cpus':['int'], we should.
>> > 
>> > But is it the opts-visitor "can handle" lists of integers, or does code
>> > have to be written?  If the latter, can you whip up a prototype?
> No extra code needs to be written. The current use case is
> NetdevUserOptions.{dnssearch,hostfwd,guestfwd}; see commit 094f15c5, and
> (by Klaus Stengel) commit 63d2960b.

This is to handle lists, but want about converting

  cpus=3-4,cpus=9-10

to

  'cpus': [3,4,9,10]

?

Paolo

On 07/17/13 16:44, Paolo Bonzini wrote:
> Il 17/07/2013 16:33, Laszlo Ersek ha scritto:
>>>>>> opts-visitor can handle lists of simple scalar types. Ie. it can do
>>>>>> -numa node,nodeid=3,cpus=3-4,cpus=9-10. It can't save the parsing of
>>>>>> intervals (eg. 3-4).
>>>>
>>>> Saving the parsing of intervals is not necessary for this use case.  So
>>>> if we can make it '*cpus':['int'], we should.
>>>>
>>>> But is it the opts-visitor "can handle" lists of integers, or does code
>>>> have to be written?  If the latter, can you whip up a prototype?
>> No extra code needs to be written. The current use case is
>> NetdevUserOptions.{dnssearch,hostfwd,guestfwd}; see commit 094f15c5, and
>> (by Klaus Stengel) commit 63d2960b.
> 
> This is to handle lists, but want about converting
> 
>   cpus=3-4,cpus=9-10
> 
> to
> 
>   'cpus': [3,4,9,10]

Oh, that. :) That does need extra code. Something along the lines of:

(a), in the JSON, reuse the existing String wrapper type, and make
"cpus" an optional list of String[s]:

{ 'type': 'NumaNodeOptions',
  'data': {
   '*nodeid':		'uint16',
   '*cpus':		['String'] }}

(b) in the code, traverse the StringList like net_init_slirp_configs()
or slirp_dnssearch() does. Parse each element as an interval, set bit
ranges / report errors.

    static int numa_node_parse_cpu_range(int nodeid,
                                         const char *cpu_range)
    {
        /* what numa_node_parse_cpus() does in 02/12 */
    }

    static int numa_node_parse(const NumaNodeOptions *opts)
    {
        const StringList *cpu_range;

        /* not sure how to handle the (!opts->has_nodeid) case; let's
         * assume we have a nodeid here */

        if (opts->nodeid >= MAX_NODES) {
            fprintf(stderr,
                    "NUMA nodeid %d reaches / exceeds maximum %d\n",
                    opts->nodeid, MAX_NODES);
            return -1;
        }

        for (cpu_range = opts->cpus;
             cpu_range != NULL;
             cpu_range = cpu_range->next) {
            int ret;

            ret = numa_node_parse_cpu_range(opts->nodeid,
                                            cpu_range->value->str);
            if (ret < 0) {
                return ret;
            }
        }
        return 0;
    }

Did you mean something like this by prototype?

Thanks,
Laszlo

Il 17/07/2013 17:24, Laszlo Ersek ha scritto:
> On 07/17/13 16:44, Paolo Bonzini wrote:
>> Il 17/07/2013 16:33, Laszlo Ersek ha scritto:
>>>>>>> opts-visitor can handle lists of simple scalar types. Ie. it can do
>>>>>>> -numa node,nodeid=3,cpus=3-4,cpus=9-10. It can't save the parsing of
>>>>>>> intervals (eg. 3-4).
>>>>>
>>>>> Saving the parsing of intervals is not necessary for this use case.  So
>>>>> if we can make it '*cpus':['int'], we should.
>>>>>
>>>>> But is it the opts-visitor "can handle" lists of integers, or does code
>>>>> have to be written?  If the latter, can you whip up a prototype?
>>> No extra code needs to be written. The current use case is
>>> NetdevUserOptions.{dnssearch,hostfwd,guestfwd}; see commit 094f15c5, and
>>> (by Klaus Stengel) commit 63d2960b.
>>
>> This is to handle lists, but want about converting
>>
>>   cpus=3-4,cpus=9-10
>>
>> to
>>
>>   'cpus': [3,4,9,10]
> 
> Oh, that. :) That does need extra code. Something along the lines of:
> 
> (a), in the JSON, reuse the existing String wrapper type, and make
> "cpus" an optional list of String[s]:
> 
> { 'type': 'NumaNodeOptions',
>   'data': {
>    '*nodeid':		'uint16',
>    '*cpus':		['String'] }}
> 
> (b) in the code, traverse the StringList like net_init_slirp_configs()
> or slirp_dnssearch() does. Parse each element as an interval, set bit
> ranges / report errors.
> 
>     static int numa_node_parse_cpu_range(int nodeid,
>                                          const char *cpu_range)
>     {
>         /* what numa_node_parse_cpus() does in 02/12 */
>     }
> 
>     static int numa_node_parse(const NumaNodeOptions *opts)
>     {
>         const StringList *cpu_range;
> 
>         /* not sure how to handle the (!opts->has_nodeid) case; let's
>          * assume we have a nodeid here */
> 
>         if (opts->nodeid >= MAX_NODES) {
>             fprintf(stderr,
>                     "NUMA nodeid %d reaches / exceeds maximum %d\n",
>                     opts->nodeid, MAX_NODES);
>             return -1;
>         }
> 
>         for (cpu_range = opts->cpus;
>              cpu_range != NULL;
>              cpu_range = cpu_range->next) {
>             int ret;
> 
>             ret = numa_node_parse_cpu_range(opts->nodeid,
>                                             cpu_range->value->str);
>             if (ret < 0) {
>                 return ret;
>             }
>         }
>         return 0;
>     }
> 
> Did you mean something like this by prototype?

Yes, though I guess Wanlong could do this by himself.  A more
interesting prototype is "how to add code to OptsVisitor that parses
intervals when it sees ['int']", and this where you can help the most.

Paolo

On 07/17/13 17:26, Paolo Bonzini wrote:
> Il 17/07/2013 17:24, Laszlo Ersek ha scritto:
>> On 07/17/13 16:44, Paolo Bonzini wrote:
>>> Il 17/07/2013 16:33, Laszlo Ersek ha scritto:
>>>>>>>> opts-visitor can handle lists of simple scalar types. Ie. it can do
>>>>>>>> -numa node,nodeid=3,cpus=3-4,cpus=9-10. It can't save the parsing of
>>>>>>>> intervals (eg. 3-4).
>>>>>>
>>>>>> Saving the parsing of intervals is not necessary for this use case.  So
>>>>>> if we can make it '*cpus':['int'], we should.
>>>>>>
>>>>>> But is it the opts-visitor "can handle" lists of integers, or does code
>>>>>> have to be written?  If the latter, can you whip up a prototype?
>>>> No extra code needs to be written. The current use case is
>>>> NetdevUserOptions.{dnssearch,hostfwd,guestfwd}; see commit 094f15c5, and
>>>> (by Klaus Stengel) commit 63d2960b.
>>>
>>> This is to handle lists, but want about converting
>>>
>>>   cpus=3-4,cpus=9-10
>>>
>>> to
>>>
>>>   'cpus': [3,4,9,10]
>>
>> Oh, that. :) That does need extra code. Something along the lines of:
>>
>> (a), in the JSON, reuse the existing String wrapper type, and make
>> "cpus" an optional list of String[s]:
>>
>> { 'type': 'NumaNodeOptions',
>>   'data': {
>>    '*nodeid':		'uint16',
>>    '*cpus':		['String'] }}
>>
>> (b) in the code, traverse the StringList like net_init_slirp_configs()
>> or slirp_dnssearch() does. Parse each element as an interval, set bit
>> ranges / report errors.
>>
>>     static int numa_node_parse_cpu_range(int nodeid,
>>                                          const char *cpu_range)
>>     {
>>         /* what numa_node_parse_cpus() does in 02/12 */
>>     }
>>
>>     static int numa_node_parse(const NumaNodeOptions *opts)
>>     {
>>         const StringList *cpu_range;
>>
>>         /* not sure how to handle the (!opts->has_nodeid) case; let's
>>          * assume we have a nodeid here */
>>
>>         if (opts->nodeid >= MAX_NODES) {
>>             fprintf(stderr,
>>                     "NUMA nodeid %d reaches / exceeds maximum %d\n",
>>                     opts->nodeid, MAX_NODES);
>>             return -1;
>>         }
>>
>>         for (cpu_range = opts->cpus;
>>              cpu_range != NULL;
>>              cpu_range = cpu_range->next) {
>>             int ret;
>>
>>             ret = numa_node_parse_cpu_range(opts->nodeid,
>>                                             cpu_range->value->str);
>>             if (ret < 0) {
>>                 return ret;
>>             }
>>         }
>>         return 0;
>>     }
>>
>> Did you mean something like this by prototype?
> 
> Yes, though I guess Wanlong could do this by himself.  A more
> interesting prototype is "how to add code to OptsVisitor that parses
> intervals when it sees ['int']", and this where you can help the most.

Do you want each element of the range present in the flat list, or just
the boundaries? (The above example, ie [3..4]U[9..10] doesn't
distinguish between these two.)

If the list contains the boundaries only, that's more frugal but
requires smarter code. If the list contains all elements, then big
ranges will result in huge lists (which are then easy to handle piecewise).

Do you also want a<=b checking for [a,b]? (That would imply "inclusive"
on both sides and not allow empty sets easily.)

This is going to be a huge hack, but I can already express the condition
"I'm in a list and looking for the next element as int" in the code. So
maybe I could force some more state into OptsVisitor (specifically
opts_type_int()/opts_type_uint64() and lookup_scalar()) and fake extra
elements.

Better: I could pop "a-b" off "ov->repeated_opts", return "a" (after
parsing), and push back "b". Then "b" would not differ from the current
"individual int" case, and I wouldn't have to add extra state to
maintain between calls.

You just torpedoed planned review efforts for today / tomorrow :)

Laszlo

Il 17/07/2013 17:45, Laszlo Ersek ha scritto:
>> Yes, though I guess Wanlong could do this by himself.  A more
>> interesting prototype is "how to add code to OptsVisitor that parses
>> intervals when it sees ['int']", and this where you can help the most.
> 
> Do you want each element of the range present in the flat list, or just
> the boundaries? (The above example, ie [3..4]U[9..10] doesn't
> distinguish between these two.)

It should be a real list.

> If the list contains the boundaries only, that's more frugal but
> requires smarter code. If the list contains all elements, then big
> ranges will result in huge lists (which are then easy to handle piecewise).

Huge lists wouldn't be a problem, I think.

> Do you also want a<=b checking for [a,b]? (That would imply "inclusive"
> on both sides and not allow empty sets easily.)
> 
> This is going to be a huge hack, but I can already express the condition
> "I'm in a list and looking for the next element as int" in the code. So
> maybe I could force some more state into OptsVisitor (specifically
> opts_type_int()/opts_type_uint64() and lookup_scalar()) and fake extra
> elements.

Yeah, I guessed something like that.

> Better: I could pop "a-b" off "ov->repeated_opts", return "a" (after
> parsing), and push back "b". Then "b" would not differ from the current
> "individual int" case, and I wouldn't have to add extra state to
> maintain between calls.
> 
> You just torpedoed planned review efforts for today / tomorrow :)

:)

Paolo