Re: [PATCH v8 01/13] x86: add socket_cpumask

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

On Fri, May 29, 2015 at 09:01:53AM +0100, Jan Beulich wrote:
> >>> On 29.05.15 at 04:35, <chao.p.peng@linux.intel.com> wrote:
> > On Thu, May 28, 2015 at 01:38:05PM +0100, Jan Beulich wrote:
> >> >>> On 21.05.15 at 10:41, <chao.p.peng@linux.intel.com> wrote:
> >> > --- a/xen/arch/x86/mpparse.c
> >> > +++ b/xen/arch/x86/mpparse.c
> >> > @@ -87,6 +87,18 @@ void __init set_nr_cpu_ids(unsigned int max_cpus)
> >> >  #endif
> >> >  }
> >> >  
> >> > +void __init set_nr_sockets(void)
> >> > +{
> >> > +    unsigned int cpus = bitmap_weight(phys_cpu_present_map.mask,
> >> > +                                      boot_cpu_data.x86_max_cores *
> >> > +                                      boot_cpu_data.x86_num_siblings);
> >> 
> >> How did you come to this expression for the bitmap size? I.e.
> >> why not simply physids_weight(phys_cpu_present_map)?
> > 
> > physids_weight(phys_cpu_present_map) gives me cpus for all sockets.
> > While here the 'cpus' is actually _cpus_per_socket_. I used the max
> > possible cpus indicated in cpuid as the upper bound so bitmap_weight()
> > returns the actual available cpus on socket 0.
> 
> In which case the variable name is badly chosen, or a respective
> comment is missing.
> 
> >> > +
> >> > +    if ( cpus == 0 )
> >> > +        cpus = 1;
> >> > +
> >> > +    nr_sockets = DIV_ROUND_UP(num_processors + disabled_cpus, cpus);
> >> > +}
> >> 
> >> Is there a reason why this can't just be added to the end of the
> >> immediately preceding set_nr_cpu_ids()?
> > 
> > You mean the declaration or invocation? If the former I have no special
> > reason for it (e.g. I can change it).
> 
> Neither - I just don't see the need for a new function.

In which case the invocation of set_nr_cpu_ids() should move to the
place where now set_nr_sockets() is invoked, to make sure
boot_cpu_data.x86_max_cores/x86_num_siblings available, which may not be
your expectation.

Chao