Re: [PATCH v6 01/14] x86: add socket_to_cpumask

[Chao Peng <chao.p.peng@linux.intel.com>]

On Wed, May 06, 2015 at 08:16:40AM +0100, Jan Beulich wrote:
> >>> On 06.05.15 at 09:05, <chao.p.peng@linux.intel.com> wrote:
> > On Tue, May 05, 2015 at 11:43:57AM +0100, Jan Beulich wrote:
> >> >>> On 05.05.15 at 12:25, <chao.p.peng@linux.intel.com> wrote:
> >> > My concern is the original APIC IDs do not need to be consecutive. Then
> >> > in such case even we do
> >> > 
> >> > DIV_ROUND_UP(cpu_num_from_madt, boot_cpu_data.x86_max_cores *
> >> >                                 boot_cpu_data.x86_num_siblings);
> >> > 
> >> > would not be correct.
> >> > 
> >> > E.g. If we have a machine like this (Each package has two cores and each
> >> > core has two threads but only 3 processors enumerated in MADT):
> >> > 
> >> > APIC_ID  Package_ID Core_ID SMT_ID
> >> >  1(001)  0          0       1
> >> >  2(010)  0          1       0
> >> >  4(100)  1          0       0
> >> > 
> >> > Then nr_sockets = ROUND_UP( 3 / (2 * 2) ) = 1 but we do have two sockets.
> >> 
> >> But that's the case regardless of how many CPUs we actually boot.
> >> Or what am I overlooking here?
> >> 
> > So now we have two CPU numbers. One is the original processor count we
> > got from MADT, the other one is nr_cpu_ids we actually use.
> > 
> > To calculate nr_socket correctly, the former is what we need. While what
> > I concern here is even for the original processors we got from MADT,
> > can we trust they are always continuous?
> 
> Right, there's no such guarantee. We may be making assumptions on
> them being not too sparse elsewhere, but ...

The assumption should be true for most cases, I think. So I'd still use
it so that in most cases Xen will take care of this.

> 
> > I'm not sure for it. But if they are not, then the above calculation is
> > bogus.
> 
> ... for the case here this indeed may need to be done in a more
> robust way.

While still making it robust, I don't have any idea now. Perhaps what we
can do is cautiously doing it when nr_sockets is potentially <= socket_id.
E.g.

cpumask_set_cpu(cpu, &socket_to_cpumask[cpu_to_socket(cpu)]);

will become:

socket = cpu_to_socket(cpu);
if ( socket < nr_sockets )
    cpumask_set_cpu(cpu, &socket_to_cpumask[socket]);

Chao