| Message ID | 1520829790-14029-2-git-send-email-kernelfans@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | enable nr_cpus for powerpc | expand |
On Mon, 2018-03-12 at 12:43 +0800, Pingfan Liu wrote: > For kexec -p, the boot cpu can be not the cpu0, this causes the problem > to alloc paca[]. In theory, there is no requirement to assign cpu's logical > id as its present seq by device tree. But we have something like > cpu_first_thread_sibling(), which makes assumption on the mapping inside > a core. Hence partially changing the mapping, i.e. unbind the mapping of > core while keep the mapping inside a core. After this patch, boot-cpu > will always be mapped into the range [0,threads_per_core). I'm ok with the idea but not fan of the implementation: > Signed-off-by: Pingfan Liu <kernelfans@gmail.com> > --- > arch/powerpc/include/asm/smp.h | 1 + > arch/powerpc/kernel/prom.c | 25 ++++++++++++++----------- > arch/powerpc/kernel/setup-common.c | 21 +++++++++++++++++++++ > 3 files changed, 36 insertions(+), 11 deletions(-) > > diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h > index fac963e..1299100 100644 > --- a/arch/powerpc/include/asm/smp.h > +++ b/arch/powerpc/include/asm/smp.h > @@ -30,6 +30,7 @@ > #include <asm/percpu.h> > > extern int boot_cpuid; > +extern int boot_cpuhwid; > extern int spinning_secondaries; > > extern void cpu_die(void); > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c > index da67606..d0ebb25 100644 > --- a/arch/powerpc/kernel/prom.c > +++ b/arch/powerpc/kernel/prom.c > @@ -315,8 +315,7 @@ static int __init early_init_dt_scan_cpus(unsigned long node, > const __be32 *intserv; > int i, nthreads; > int len; > - int found = -1; > - int found_thread = 0; > + bool found = false; > > /* We are scanning "cpu" nodes only */ > if (type == NULL || strcmp(type, "cpu") != 0) > @@ -341,8 +340,11 @@ static int __init early_init_dt_scan_cpus(unsigned long node, > if (fdt_version(initial_boot_params) >= 2) { > if (be32_to_cpu(intserv[i]) == > fdt_boot_cpuid_phys(initial_boot_params)) { > - found = boot_cpu_count; > - found_thread = i; > + /* always map the boot-cpu logical id into the > + * the range of [0, thread_per_core) > + */ > + boot_cpuid = i; > + found = true; > } Call it boot_thread_id > } else { > /* > @@ -351,8 +353,10 @@ static int __init early_init_dt_scan_cpus(unsigned long node, > * off secondary threads. > */ > if (of_get_flat_dt_prop(node, > - "linux,boot-cpu", NULL) != NULL) > - found = boot_cpu_count; > + "linux,boot-cpu", NULL) != NULL) { > + boot_cpuid = i; > + found = true; > + } > } > #ifdef CONFIG_SMP > /* logical cpu id is always 0 on UP kernels */ > @@ -361,13 +365,12 @@ static int __init early_init_dt_scan_cpus(unsigned long node, > } > > /* Not the boot CPU */ > - if (found < 0) > + if (!found) > return 0; > > - DBG("boot cpu: logical %d physical %d\n", found, > - be32_to_cpu(intserv[found_thread])); > - boot_cpuid = found; > - set_hard_smp_processor_id(found, be32_to_cpu(intserv[found_thread])); > + boot_cpuhwid = be32_to_cpu(intserv[boot_cpuid]); > + DBG("boot cpu: logical %d physical %d\n", boot_cpuid, boot_cpuhwid); > + set_hard_smp_processor_id(boot_cpuid, boot_cpuhwid); > > /* > * PAPR defines "logical" PVR values for cpus that > diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c > index 66f7cc6..1a67344 100644 > --- a/arch/powerpc/kernel/setup-common.c > +++ b/arch/powerpc/kernel/setup-common.c > @@ -86,6 +86,7 @@ struct machdep_calls *machine_id; > EXPORT_SYMBOL(machine_id); > > int boot_cpuid = -1; > +int boot_cpuhwid = -1; > EXPORT_SYMBOL_GPL(boot_cpuid); > > /* > @@ -459,11 +460,17 @@ static void __init cpu_init_thread_core_maps(int tpc) > void __init smp_setup_cpu_maps(void) > { > struct device_node *dn; > + struct device_node *boot_dn = NULL; > + bool handling_bootdn = true; > int cpu = 0; > int nthreads = 1; > > DBG("smp_setup_cpu_maps()\n"); > > +again: > + /* E.g. kexec will not boot from the 1st core. So firstly loop to find out > + * the dn of boot-cpu, and map them onto [0, nthreads) > + */ > for_each_node_by_type(dn, "cpu") { > const __be32 *intserv; > __be32 cpu_be; > @@ -488,6 +495,16 @@ void __init smp_setup_cpu_maps(void) > > nthreads = len / sizeof(int); > > + if (handling_bootdn) { > + if (boot_cpuid < nthreads && > + be32_to_cpu(intserv[boot_cpuid]) == boot_cpuhwid) { > + boot_dn = dn; > + } > + if (boot_dn == NULL) > + continue; > + } else if (dn == boot_dn) > + continue; > + > for (j = 0; j < nthreads && cpu < nr_cpu_ids; j++) { > bool avail; > > @@ -509,6 +526,10 @@ void __init smp_setup_cpu_maps(void) > of_node_put(dn); > break; > } > + if (handling_bootdn) { > + handling_bootdn = false; > + goto again; > + } > } You don't need that "again" loop and "handling_bootdn" weird boolean. Instead, start with cpu = 1 instead of cpu = 0, and rename it to "next_cpu". Then, before the thread loop, check if we are on the same core as boot_cpuhwid: if (same_core_as_boot_cpu(intserv)) { cpu = 0; } else if (next_cpu < nr_cpus_ids) { cpu = next_cpu++; } else { of_node_put(dn); break; } > > /* If no SMT supported, nthreads is forced to 1 */
Hi Pingfan,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on v4.16-rc4]
[also build test ERROR on next-20180309]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Pingfan-Liu/powerpc-cpu-partially-unbind-the-mapping-between-cpu-logical-id-and-its-seq-in-dt/20180313-034420
config: powerpc-pq2fads_defconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc
All errors (new ones prefixed by >>):
>> arch/powerpc/kernel/prom.c:79:23: error: 'boot_cpu_count' defined but not used [-Werror=unused-variable]
static int __initdata boot_cpu_count;
^~~~~~~~~~~~~~
cc1: all warnings being treated as errors
vim +/boot_cpu_count +79 arch/powerpc/kernel/prom.c
9b6b563c Paul Mackerras 2005-10-06 71
9b6b563c Paul Mackerras 2005-10-06 72 #ifdef CONFIG_PPC64
28897731 Olof Johansson 2006-04-12 73 int __initdata iommu_is_off;
9b6b563c Paul Mackerras 2005-10-06 74 int __initdata iommu_force_on;
cf00a8d1 Paul Mackerras 2005-10-31 75 unsigned long tce_alloc_start, tce_alloc_end;
cd3db0c4 Benjamin Herrenschmidt 2010-07-06 76 u64 ppc64_rma_size;
9b6b563c Paul Mackerras 2005-10-06 77 #endif
03bf469a Benjamin Herrenschmidt 2011-05-11 78 static phys_addr_t first_memblock_size;
7ac87abb Matt Evans 2011-05-25 @79 static int __initdata boot_cpu_count;
9b6b563c Paul Mackerras 2005-10-06 80
:::::: The code at line 79 was first introduced by commit
:::::: 7ac87abb8166b99584149fcfb2efef5773a078e9 powerpc: Fix early boot accounting of CPUs
:::::: TO: Matt Evans <matt@ozlabs.org>
:::::: CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tue, Mar 13, 2018 at 10:58 AM, Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Mon, 2018-03-12 at 12:43 +0800, Pingfan Liu wrote: >> For kexec -p, the boot cpu can be not the cpu0, this causes the problem >> to alloc paca[]. In theory, there is no requirement to assign cpu's logical >> id as its present seq by device tree. But we have something like >> cpu_first_thread_sibling(), which makes assumption on the mapping inside >> a core. Hence partially changing the mapping, i.e. unbind the mapping of >> core while keep the mapping inside a core. After this patch, boot-cpu >> will always be mapped into the range [0,threads_per_core). > > I'm ok with the idea but not fan of the implementation: > >> Signed-off-by: Pingfan Liu <kernelfans@gmail.com> >> --- >> arch/powerpc/include/asm/smp.h | 1 + >> arch/powerpc/kernel/prom.c | 25 ++++++++++++++----------- >> arch/powerpc/kernel/setup-common.c | 21 +++++++++++++++++++++ >> 3 files changed, 36 insertions(+), 11 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h >> index fac963e..1299100 100644 >> --- a/arch/powerpc/include/asm/smp.h >> +++ b/arch/powerpc/include/asm/smp.h >> @@ -30,6 +30,7 @@ >> #include <asm/percpu.h> >> >> extern int boot_cpuid; >> +extern int boot_cpuhwid; >> extern int spinning_secondaries; >> >> extern void cpu_die(void); >> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c >> index da67606..d0ebb25 100644 >> --- a/arch/powerpc/kernel/prom.c >> +++ b/arch/powerpc/kernel/prom.c >> @@ -315,8 +315,7 @@ static int __init early_init_dt_scan_cpus(unsigned long node, >> const __be32 *intserv; >> int i, nthreads; >> int len; >> - int found = -1; >> - int found_thread = 0; >> + bool found = false; >> >> /* We are scanning "cpu" nodes only */ >> if (type == NULL || strcmp(type, "cpu") != 0) >> @@ -341,8 +340,11 @@ static int __init early_init_dt_scan_cpus(unsigned long node, >> if (fdt_version(initial_boot_params) >= 2) { >> if (be32_to_cpu(intserv[i]) == >> fdt_boot_cpuid_phys(initial_boot_params)) { >> - found = boot_cpu_count; >> - found_thread = i; >> + /* always map the boot-cpu logical id into the >> + * the range of [0, thread_per_core) >> + */ >> + boot_cpuid = i; >> + found = true; >> } > > Call it boot_thread_id > But I think boot_cpuid has the meaning of global index, while the thread_id has the meaning of index in a core. >> } else { >> /* >> @@ -351,8 +353,10 @@ static int __init early_init_dt_scan_cpus(unsigned long node, >> * off secondary threads. >> */ >> if (of_get_flat_dt_prop(node, >> - "linux,boot-cpu", NULL) != NULL) >> - found = boot_cpu_count; >> + "linux,boot-cpu", NULL) != NULL) { >> + boot_cpuid = i; >> + found = true; >> + } >> } >> #ifdef CONFIG_SMP >> /* logical cpu id is always 0 on UP kernels */ >> @@ -361,13 +365,12 @@ static int __init early_init_dt_scan_cpus(unsigned long node, >> } >> >> /* Not the boot CPU */ >> - if (found < 0) >> + if (!found) >> return 0; >> >> - DBG("boot cpu: logical %d physical %d\n", found, >> - be32_to_cpu(intserv[found_thread])); >> - boot_cpuid = found; >> - set_hard_smp_processor_id(found, be32_to_cpu(intserv[found_thread])); >> + boot_cpuhwid = be32_to_cpu(intserv[boot_cpuid]); >> + DBG("boot cpu: logical %d physical %d\n", boot_cpuid, boot_cpuhwid); >> + set_hard_smp_processor_id(boot_cpuid, boot_cpuhwid); >> >> /* >> * PAPR defines "logical" PVR values for cpus that >> diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c >> index 66f7cc6..1a67344 100644 >> --- a/arch/powerpc/kernel/setup-common.c >> +++ b/arch/powerpc/kernel/setup-common.c >> @@ -86,6 +86,7 @@ struct machdep_calls *machine_id; >> EXPORT_SYMBOL(machine_id); >> >> int boot_cpuid = -1; >> +int boot_cpuhwid = -1; >> EXPORT_SYMBOL_GPL(boot_cpuid); >> >> /* >> @@ -459,11 +460,17 @@ static void __init cpu_init_thread_core_maps(int tpc) >> void __init smp_setup_cpu_maps(void) >> { >> struct device_node *dn; >> + struct device_node *boot_dn = NULL; >> + bool handling_bootdn = true; >> int cpu = 0; >> int nthreads = 1; >> >> DBG("smp_setup_cpu_maps()\n"); >> >> +again: >> + /* E.g. kexec will not boot from the 1st core. So firstly loop to find out >> + * the dn of boot-cpu, and map them onto [0, nthreads) >> + */ >> for_each_node_by_type(dn, "cpu") { >> const __be32 *intserv; >> __be32 cpu_be; >> @@ -488,6 +495,16 @@ void __init smp_setup_cpu_maps(void) >> >> nthreads = len / sizeof(int); >> >> + if (handling_bootdn) { >> + if (boot_cpuid < nthreads && >> + be32_to_cpu(intserv[boot_cpuid]) == boot_cpuhwid) { >> + boot_dn = dn; >> + } >> + if (boot_dn == NULL) >> + continue; >> + } else if (dn == boot_dn) >> + continue; >> + >> for (j = 0; j < nthreads && cpu < nr_cpu_ids; j++) { >> bool avail; >> >> @@ -509,6 +526,10 @@ void __init smp_setup_cpu_maps(void) >> of_node_put(dn); >> break; >> } >> + if (handling_bootdn) { >> + handling_bootdn = false; >> + goto again; >> + } >> } > > You don't need that "again" loop and "handling_bootdn" weird boolean. > > Instead, start with cpu = 1 instead of cpu = 0, and rename it to > "next_cpu". > > Then, before the thread loop, check if we are on the same core > as boot_cpuhwid: > > if (same_core_as_boot_cpu(intserv)) { > cpu = 0; > } else if (next_cpu < nr_cpus_ids) { > cpu = next_cpu++; > } else { > of_node_put(dn); > break; > } > OK. Thanks for your review. Regards, Pingfan
diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h index fac963e..1299100 100644 --- a/arch/powerpc/include/asm/smp.h +++ b/arch/powerpc/include/asm/smp.h @@ -30,6 +30,7 @@ #include <asm/percpu.h> extern int boot_cpuid; +extern int boot_cpuhwid; extern int spinning_secondaries; extern void cpu_die(void); diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c index da67606..d0ebb25 100644 --- a/arch/powerpc/kernel/prom.c +++ b/arch/powerpc/kernel/prom.c @@ -315,8 +315,7 @@ static int __init early_init_dt_scan_cpus(unsigned long node, const __be32 *intserv; int i, nthreads; int len; - int found = -1; - int found_thread = 0; + bool found = false; /* We are scanning "cpu" nodes only */ if (type == NULL || strcmp(type, "cpu") != 0) @@ -341,8 +340,11 @@ static int __init early_init_dt_scan_cpus(unsigned long node, if (fdt_version(initial_boot_params) >= 2) { if (be32_to_cpu(intserv[i]) == fdt_boot_cpuid_phys(initial_boot_params)) { - found = boot_cpu_count; - found_thread = i; + /* always map the boot-cpu logical id into the + * the range of [0, thread_per_core) + */ + boot_cpuid = i; + found = true; } } else { /* @@ -351,8 +353,10 @@ static int __init early_init_dt_scan_cpus(unsigned long node, * off secondary threads. */ if (of_get_flat_dt_prop(node, - "linux,boot-cpu", NULL) != NULL) - found = boot_cpu_count; + "linux,boot-cpu", NULL) != NULL) { + boot_cpuid = i; + found = true; + } } #ifdef CONFIG_SMP /* logical cpu id is always 0 on UP kernels */ @@ -361,13 +365,12 @@ static int __init early_init_dt_scan_cpus(unsigned long node, } /* Not the boot CPU */ - if (found < 0) + if (!found) return 0; - DBG("boot cpu: logical %d physical %d\n", found, - be32_to_cpu(intserv[found_thread])); - boot_cpuid = found; - set_hard_smp_processor_id(found, be32_to_cpu(intserv[found_thread])); + boot_cpuhwid = be32_to_cpu(intserv[boot_cpuid]); + DBG("boot cpu: logical %d physical %d\n", boot_cpuid, boot_cpuhwid); + set_hard_smp_processor_id(boot_cpuid, boot_cpuhwid); /* * PAPR defines "logical" PVR values for cpus that diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c index 66f7cc6..1a67344 100644 --- a/arch/powerpc/kernel/setup-common.c +++ b/arch/powerpc/kernel/setup-common.c @@ -86,6 +86,7 @@ struct machdep_calls *machine_id; EXPORT_SYMBOL(machine_id); int boot_cpuid = -1; +int boot_cpuhwid = -1; EXPORT_SYMBOL_GPL(boot_cpuid); /* @@ -459,11 +460,17 @@ static void __init cpu_init_thread_core_maps(int tpc) void __init smp_setup_cpu_maps(void) { struct device_node *dn; + struct device_node *boot_dn = NULL; + bool handling_bootdn = true; int cpu = 0; int nthreads = 1; DBG("smp_setup_cpu_maps()\n"); +again: + /* E.g. kexec will not boot from the 1st core. So firstly loop to find out + * the dn of boot-cpu, and map them onto [0, nthreads) + */ for_each_node_by_type(dn, "cpu") { const __be32 *intserv; __be32 cpu_be; @@ -488,6 +495,16 @@ void __init smp_setup_cpu_maps(void) nthreads = len / sizeof(int); + if (handling_bootdn) { + if (boot_cpuid < nthreads && + be32_to_cpu(intserv[boot_cpuid]) == boot_cpuhwid) { + boot_dn = dn; + } + if (boot_dn == NULL) + continue; + } else if (dn == boot_dn) + continue; + for (j = 0; j < nthreads && cpu < nr_cpu_ids; j++) { bool avail; @@ -509,6 +526,10 @@ void __init smp_setup_cpu_maps(void) of_node_put(dn); break; } + if (handling_bootdn) { + handling_bootdn = false; + goto again; + } } /* If no SMT supported, nthreads is forced to 1 */
For kexec -p, the boot cpu can be not the cpu0, this causes the problem to alloc paca[]. In theory, there is no requirement to assign cpu's logical id as its present seq by device tree. But we have something like cpu_first_thread_sibling(), which makes assumption on the mapping inside a core. Hence partially changing the mapping, i.e. unbind the mapping of core while keep the mapping inside a core. After this patch, boot-cpu will always be mapped into the range [0,threads_per_core). Signed-off-by: Pingfan Liu <kernelfans@gmail.com> --- arch/powerpc/include/asm/smp.h | 1 + arch/powerpc/kernel/prom.c | 25 ++++++++++++++----------- arch/powerpc/kernel/setup-common.c | 21 +++++++++++++++++++++ 3 files changed, 36 insertions(+), 11 deletions(-)