[prev in list] [next in list] [prev in thread] [next in thread]
List: qemu-devel
Subject: [Qemu-devel] Re: [PATCH 1/4] added -numa cmdline parameter parser
From: Andre Przywara <andre.przywara () amd ! com>
Date: 2009-03-31 20:34:40
Message-ID: 49D27E60.8080501 () amd ! com
[Download RAW message or body]
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
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic