| Message ID | 1408608842.3003.20.camel@TP420 (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 70ad237515d99595ed03848bd8e549e50e83c4f2 |
| Headers | show |
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) > > 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> > --- > 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! -Nish
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! > > -Nish
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) -Nish
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
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); } }
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! 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> --- arch/powerpc/mm/numa.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)