Re: [Qemu-devel] [PATCH V5 01/12] NUMA: add NumaOptions, NumaNodeOptions and NumaMemOptions

[Laszlo Ersek <lersek@redhat.com>]

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