Re: [Xen-devel] [PATCH 1/5] x86: allow specifying the NUMA nodes Dom0 should run on

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.03.15 at 17:11, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 26/02/15 13:52, Jan Beulich wrote:
>> ---
>> I'm uncertain whether range restricting the PXMs permitted for Dom0 is
>> the right approach (matching what other NUMA code did until recently),
>> or whether we would instead want to simply limit the number of PXMs we
>> can handler there (i.e. using a static array instead of a static
>> bitmap).
> 
> I am not quite sure what you mean by this.

Since we somehow need to store the information provided on the
command line, we have basically two options (taking into consideration
that command line parsing happens very early): Either we use a bitmap
(as done here) to track the PXMs provided (which puts an upper bound
on the PXM values) or we store each PXM in a (static) array, which puts
an upper bound on how many PXMs may be specified (and requires
more storage per PXM).

>> +static struct vcpu *__init setup_vcpu(struct domain *d, unsigned int 
>> vcpu_id,
>> +                                      unsigned int cpu)
> 
> I would be tempted to name this setup_dom0_vcpu() to bring it in line
> with other dom0 construction functions, and to make it clear that it is
> not for use on general domains.

Makes sense, will do.

>>  unsigned int __init dom0_max_vcpus(void)
>>  {
>> -    unsigned max_vcpus;
>> +    unsigned int pxm, max_vcpus;
>> +    nodeid_t node;
>> +
>> +    for ( pxm = 0; pxm <= MAX_DOM0_PXM; ++pxm )
>> +        if ( test_bit(pxm, dom0_pxms) &&
>> +             (node = pxm_to_node(pxm)) != NUMA_NO_NODE )
>> +            node_set(node, dom0_nodes);
> 
> for_each_set_bit() ?  It would simply the above loop slightly, and is
> more efficient.

Sure - how did I forget about this?

>> @@ -1230,11 +1284,11 @@ int __init construct_dom0(
>>  
>>      printk("Dom0 has maximum %u VCPUs\n", d->max_vcpus);
>>  
>> -    cpu = cpumask_first(cpupool0->cpu_valid);
>> +    cpu = v->processor;
>>      for ( i = 1; i < d->max_vcpus; i++ )
>>      {
>> -        cpu = cpumask_cycle(cpu, cpupool0->cpu_valid);
>> -        (void)alloc_vcpu(d, i, cpu);
>> +        cpu = cpumask_cycle(cpu, &dom0_cpus);
>> +        setup_vcpu(d, i, cpu);
> 
> I know this is a preexisting bug, but you might want to fix it as you
> are changing the affected codepath.
> 
> Construction of dom0 should fail if any alloc_vcpu() call fails (and now
> setup_vcpu()).   Nothing currently catches a failure to allocate vcpu
> 1..$N, and this patch introduces a way for dom0 be more
> memory-constrained than previously.

I'm not sure - starting Dom0 with (in the worst case) just one vCPU
would seem better to me than not starting it at all.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel