Re: [RFC PATCH powerpc] Fix warning reported by verify_cpu_node_mapping()

[Li Zhong <zhong@linux.vnet.ibm.com>]

On 五, 2014-08-22 at 15:04 -0700, Nishanth Aravamudan wrote:
> On 22.08.2014 [10:12:56 +0800], Li Zhong wrote:
> > On ???, 2014-08-21 at 08:45 -0700, Nishanth Aravamudan wrote:
> > > On 21.08.2014 [16:14:02 +0800], Li Zhong wrote:
> > > > With commit 2fabf084b, during boottime, cpu_numa_callback() is called
> > > > earlier(before their online) for each cpu, and verify_cpu_node_mapping()
> > > > uses cpu_to_node() to check whether siblings are in the same node. 
> > > > 
> > > > It skips the checking for siblings that are not online yet. So the only
> > > > check done here is for the bootcpu, which is online at that time. But
> > > > the per-cpu numa_node cpu_to_node() uses hasn't been set up yet (which
> > > > will be set up in smp_prepare_cpus()).
> > > > 
> > > > So I could see something like following reported:
> > > > [    0.000000] CPU thread siblings 1/2/3 and 0 don't belong to the same
> > > > node!
> > > 
> > > You mean you did see this, right? (as opposed to "could" based upon code
> > > inspection or something)
> > 
> > Yes, I did see the warnings. Seems I didn't express it precisely in
> > English ...
> > 
> > > 
> > > > 
> > > > As we don't actually do the checking during this early stage, so maybe
> > > > we could directly call numa_setup_cpu() in do_init_bootmem()?
> > > > 
> > > > Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
> > > 
> > > Acked-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
> > 
> > Thank you for the review,
> > 
> > Zhong
> > 
> > > 
> > > > ---
> > > >  arch/powerpc/mm/numa.c | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > 
> > > > diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
> > > > index d7737a5..9918c02 100644
> > > > --- a/arch/powerpc/mm/numa.c
> > > > +++ b/arch/powerpc/mm/numa.c
> > > > @@ -1128,8 +1128,7 @@ void __init do_init_bootmem(void)
> > > >  	 * early in boot, cf. smp_prepare_cpus().
> > > >  	 */
> > > >  	for_each_possible_cpu(cpu) {
> > > > -		cpu_numa_callback(&ppc64_numa_nb, CPU_UP_PREPARE,
> > > > -				  (void *)(unsigned long)cpu);
> > > > +		numa_setup_cpu((unsigned long)cpu);
> > > 
> > > This is a good change, thanks for catching it. I must have glossed over
> > > those messages in my testing, my apologies!
> 
> Actually, thinking about this more, do you think it makes more sense to
> do:
> 
> for_each_present_cpu(cpu) in this loop? That is, at boot, ensure all
> present (but possibly offline) CPUs have their NUMA mapping set up. CPUs
> that aren't present (but are possible) might trigger other warnings,
> right? (e.g., the WARN_ON(1) in numa_setup_cpu)

After reading the code that set up the cpu masks, I think you are right.
I will send a new version with this fixed. 

Thanks, Zhong

> 
> -Nish