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

[Andre Przywara]

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