[Qemu-devel] Re: [PATCH 1/4] added -numa cmdline parameter parser

[Andre Przywara <andre.przywara@amd.com>]

Anthony Liguori wrote:
> Andre Przywara wrote:
>> diff --git a/sysemu.h b/sysemu.h
>> index 3eab34b..b83a66c 100644
>> --- a/sysemu.h
>> +++ b/sysemu.h
>> @@ -108,6 +108,11 @@ extern const char *bootp_filename;
> 
>> +extern uint64_t node_cpumask[MAX_NODES];
>>   
> 
> This is going to cause some pain because it won't be long before someone 
> wants to support more than 64 cpus.  I think there are two 
> possibilities.  We could go the cpuset route and introduce a type with 
> special accessors to store a CPU bitmap.
Right, I was thinking about that one, too. I couldn't find an already 
defined type for this, so I went the easy way for the first version of 
the patch to make a review easier. Please note that the interface to the 
BIOS is not limited in any way (beside a max of 2**64 nodes), so I could 
sent a patch to overcome this limitation later (I suppose more than 64 
vCPUs break something in other parts of code before that).
> 
> Or, we could rely on the property that each CPU can only be part of one 
> node and make the node association part of the CPUState.  If for some 
> reason it's necessary to enumerate all of the CPUs for a given node, we 
> would have to walk the CPU list to get at that information.  I don't 
> think that'll be a common thing though.
Sounds reasonable, I will take a look at it.
> 
>> +static void numa_add(const char* optarg)
>> +{
>> +char option[128];
>> +char *endptr;
>> +unsigned long long value, endvalue;
>> +int nodenr;
>>   
> 
> That doesn't seem right indent-wise.
I knew I missed something....
> 
>> +        /* assigning the VCPUs round-robin is easier to implement, 
>> guest OSes
>> +         * must cope with this anyway, because there are BIOSes out 
>> there in
>> +         * real machines which also use this scheme.
>> +         */
>> +        if (i == nb_numa_nodes) {
>> +            for (i = 0; i < smp_cpus; i++) {
>> +                node_cpumask[i % nb_numa_nodes] |= 1<<i;
>> +            }
>> +        }
>>   
> 
> The only thing that I don't like about this is that I don't think the 
> current -numa syntax can be used to describe a round-robin allocation.  
> IIUC, you can say -numa cpus=3 or -numa cpus=3-4 but there's no way to 
> say -numa cpus=3:5.
> 
> That means that if we ever change the default behavior, there's no way 
> that a management app could recreate the guest with that particular 
> topology (think live migration).
Good point, I was also not happy with the missing possibility to just 
specify a list of vCPUs (since we already used the comma). If you think 
that the colon could be valid delimiter here, I can introduce that (like 
-numa cpus=0:4:8). That doesn't look very neat, so shall we use the 
colon to separate the various numa sub-parameters (exchange comma and 
colon)?


Thanks for the review and the comments!
Andre.

-- 
Andre Przywara
AMD-Operating System Research Center (OSRC), Dresden, Germany
Tel: +49 351 488-3567-12
----to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Jochen Polster; Thomas M. McCoy; Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632