linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv4 0/3] enable nr_cpus for powerpc
@ 2018-03-12  4:43 Pingfan Liu
  2018-03-12  4:43 ` [PATCHv4 1/3] powerpc, cpu: partially unbind the mapping between cpu logical id and its seq in dt Pingfan Liu
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Pingfan Liu @ 2018-03-12  4:43 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: kexec, mahesh, cascardo, gpiccoli, paulus, benh, mpe

This topic has a very long history. It comes from Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
For v3: https://patchwork.ozlabs.org/patch/834860/
In this series, I separate and change the mapping between cpu logical id and hwid.
I hope we can acquire it for "kexec -p"

Mahesh Salgaonkar (1):
  ppc64 boot: Wait for boot cpu to show up if nr_cpus limit is about to
    hit.

Pingfan Liu (2):
  powerpc, cpu: partially unbind the mapping between cpu logical id and 
       its seq in dt
  powerpc, cpu: handling the special case when boot_cpuid greater than
    nr_cpus

 arch/powerpc/include/asm/paca.h    |  3 +++
 arch/powerpc/include/asm/smp.h     |  1 +
 arch/powerpc/kernel/paca.c         | 19 ++++++++++++++-----
 arch/powerpc/kernel/prom.c         | 25 ++++++++++++++-----------
 arch/powerpc/kernel/setup-common.c | 35 +++++++++++++++++++++++++++++++++--
 5 files changed, 65 insertions(+), 18 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCHv4 1/3] powerpc, cpu: partially unbind the mapping between cpu logical id and its seq in dt
  2018-03-12  4:43 [PATCHv4 0/3] enable nr_cpus for powerpc Pingfan Liu
@ 2018-03-12  4:43 ` Pingfan Liu
  2018-03-13  2:58   ` Benjamin Herrenschmidt
  2018-03-13  5:06   ` kbuild test robot
  2018-03-12  4:43 ` [PATCHv4 2/3] powerpc, cpu: handling the special case when boot_cpuid greater than nr_cpus Pingfan Liu
  2018-03-12  4:43 ` [PATCHv4 3/3] ppc64 boot: Wait for boot cpu to show up if nr_cpus limit is about to hit Pingfan Liu
  2 siblings, 2 replies; 7+ messages in thread
From: Pingfan Liu @ 2018-03-12  4:43 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: kexec, mahesh, cascardo, gpiccoli, paulus, benh, mpe

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(-)

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 */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCHv4 2/3] powerpc, cpu: handling the special case when boot_cpuid greater than nr_cpus
  2018-03-12  4:43 [PATCHv4 0/3] enable nr_cpus for powerpc Pingfan Liu
  2018-03-12  4:43 ` [PATCHv4 1/3] powerpc, cpu: partially unbind the mapping between cpu logical id and its seq in dt Pingfan Liu
@ 2018-03-12  4:43 ` Pingfan Liu
  2018-03-12  4:43 ` [PATCHv4 3/3] ppc64 boot: Wait for boot cpu to show up if nr_cpus limit is about to hit Pingfan Liu
  2 siblings, 0 replies; 7+ messages in thread
From: Pingfan Liu @ 2018-03-12  4:43 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: kexec, mahesh, cascardo, gpiccoli, paulus, benh, mpe

For kexec -p, after boot_cpuid is mapping into the range of [0,
threads_per_core), then if nr_cpus is small, we will have the bitmap
[0,..., nr_cpus, ..., boot_cpuid, ...). This patch chooses cpus inside
the range of [boot_cpuid - nr_cpus +1, ..., boot_cpuid] to be online.

With this patch and the next, on a P9 machine with thread_per_core=4,
and set nr_cpus=2 for the crash kernel.
After
    taskset -c 11 sh -c "echo c > /proc/sysrq-trigger"
Then
    kdump:/sys/devices/system/cpu# cat possible
    2-3
    kdump:/sys/devices/system/cpu# cat present
    2-3
    kdump:/sys/devices/system/cpu# cat online
    2-3

Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
---
 arch/powerpc/kernel/setup-common.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 1a67344..6920b5e 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -462,6 +462,8 @@ void __init smp_setup_cpu_maps(void)
 	struct device_node *dn;
 	struct device_node *boot_dn = NULL;
 	bool handling_bootdn = true;
+	int head_thread = 0;
+	int online_cnt = 0;
 	int cpu = 0;
 	int nthreads = 1;
 
@@ -499,13 +501,19 @@ void __init smp_setup_cpu_maps(void)
 			if (boot_cpuid < nthreads &&
 				be32_to_cpu(intserv[boot_cpuid]) == boot_cpuhwid) {
 				boot_dn = dn;
+				/* choose a bunch of continous threads */
+				if (boot_cpuid > nr_cpu_ids - 1) {
+					head_thread = boot_cpuid - nr_cpu_ids + 1;
+					/* keep the mapping of logical and thread */
+					cpu = head_thread;
+				}
 			}
 			if (boot_dn == NULL)
 				continue;
 		} else if (dn == boot_dn)
 			continue;
 
-		for (j = 0; j < nthreads && cpu < nr_cpu_ids; j++) {
+		for (j = head_thread; j < nthreads && online_cnt < nr_cpu_ids; j++) {
 			bool avail;
 
 			DBG("    thread %d -> cpu %d (hard id %d)\n",
@@ -520,13 +528,15 @@ void __init smp_setup_cpu_maps(void)
 			set_hard_smp_processor_id(cpu, be32_to_cpu(intserv[j]));
 			set_cpu_possible(cpu, true);
 			cpu++;
+			online_cnt++;
 		}
 
-		if (cpu >= nr_cpu_ids) {
+		if (online_cnt >= nr_cpu_ids) {
 			of_node_put(dn);
 			break;
 		}
 		if (handling_bootdn) {
+			head_thread = 0;
 			handling_bootdn = false;
 			goto again;
 		}
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCHv4 3/3] ppc64 boot: Wait for boot cpu to show up if nr_cpus limit is about to hit.
  2018-03-12  4:43 [PATCHv4 0/3] enable nr_cpus for powerpc Pingfan Liu
  2018-03-12  4:43 ` [PATCHv4 1/3] powerpc, cpu: partially unbind the mapping between cpu logical id and its seq in dt Pingfan Liu
  2018-03-12  4:43 ` [PATCHv4 2/3] powerpc, cpu: handling the special case when boot_cpuid greater than nr_cpus Pingfan Liu
@ 2018-03-12  4:43 ` Pingfan Liu
  2 siblings, 0 replies; 7+ messages in thread
From: Pingfan Liu @ 2018-03-12  4:43 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: kexec, mahesh, cascardo, gpiccoli, paulus, benh, mpe

From: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>

The kernel boot parameter 'nr_cpus=' allows one to specify number of
possible cpus in the system. In the normal scenario the first cpu (cpu0)
that shows up is the boot cpu and hence it gets covered under nr_cpus
limit.

But this assumption will be broken in kdump scenario where kdump kenrel
after a crash can boot up on an non-zero boot cpu. The paca structure
allocation depends on value of nr_cpus and is indexed using logical cpu
ids. This definetly will be an issue if boot cpu id > nr_cpus

This patch modifies allocate_pacas() and smp_setup_cpu_maps() to
accommodate boot cpu for the case where boot_cpuid > nr_cpu_ids.

This change would help to reduce the memory reservation requirement for
kdump on ppc64.

Signed-off-by: Mahesh Salgaonkar <mahesh@linux.vnet.ibm.com>
Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com>
Tested-by: Guilherme G. Piccoli <gpiccoli@linux.vnet.ibm.com>
Signed-off-by: Pingfan Liu <kernelfans@gmail.com> (separate the logical for cpu id mapping)
Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
---
 arch/powerpc/include/asm/paca.h |  3 +++
 arch/powerpc/kernel/paca.c      | 19 ++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index b62c310..49ab29d 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -49,6 +49,9 @@ extern unsigned int debug_smp_processor_id(void); /* from linux/smp.h */
 #define get_lppaca()	(get_paca()->lppaca_ptr)
 #define get_slb_shadow()	(get_paca()->slb_shadow_ptr)
 
+/* Maximum number of threads per core. */
+#define	MAX_SMT		8
+
 struct task_struct;
 
 /*
diff --git a/arch/powerpc/kernel/paca.c b/arch/powerpc/kernel/paca.c
index 95ffedf..13be6ab 100644
--- a/arch/powerpc/kernel/paca.c
+++ b/arch/powerpc/kernel/paca.c
@@ -209,6 +209,7 @@ void __init allocate_pacas(void)
 {
 	u64 limit;
 	int cpu;
+	unsigned int nr_cpus_aligned;
 
 #ifdef CONFIG_PPC_BOOK3S_64
 	/*
@@ -220,20 +221,28 @@ void __init allocate_pacas(void)
 	limit = ppc64_rma_size;
 #endif
 
-	paca_size = PAGE_ALIGN(sizeof(struct paca_struct) * nr_cpu_ids);
+	/*
+	 * Alloc the paca[] align up to SMT threads.
+	 * This will help us to prepare for a situation where
+	 * boot cpu id > nr_cpus.
+	 * We keep the schema of nr_cpus in kernel cmdline, but
+	 * waste a bit memory
+	 */
+	nr_cpus_aligned = _ALIGN_UP(nr_cpu_ids, MAX_SMT);
+	paca_size = PAGE_ALIGN(sizeof(struct paca_struct) * nr_cpus_aligned);
 
 	paca = __va(memblock_alloc_base(paca_size, PAGE_SIZE, limit));
 	memset(paca, 0, paca_size);
 
 	printk(KERN_DEBUG "Allocated %u bytes for %u pacas at %p\n",
-		paca_size, nr_cpu_ids, paca);
+		paca_size, nr_cpus_aligned, paca);
 
-	allocate_lppacas(nr_cpu_ids, limit);
+	allocate_lppacas(nr_cpus_aligned, limit);
 
-	allocate_slb_shadows(nr_cpu_ids, limit);
+	allocate_slb_shadows(nr_cpus_aligned, limit);
 
 	/* Can't use for_each_*_cpu, as they aren't functional yet */
-	for (cpu = 0; cpu < nr_cpu_ids; cpu++)
+	for (cpu = 0; cpu < nr_cpus_aligned; cpu++)
 		initialise_paca(&paca[cpu], cpu);
 }
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCHv4 1/3] powerpc, cpu: partially unbind the mapping between cpu logical id and its seq in dt
  2018-03-12  4:43 ` [PATCHv4 1/3] powerpc, cpu: partially unbind the mapping between cpu logical id and its seq in dt Pingfan Liu
@ 2018-03-13  2:58   ` Benjamin Herrenschmidt
  2018-03-14  2:02     ` Pingfan Liu
  2018-03-13  5:06   ` kbuild test robot
  1 sibling, 1 reply; 7+ messages in thread
From: Benjamin Herrenschmidt @ 2018-03-13  2:58 UTC (permalink / raw)
  To: Pingfan Liu, linuxppc-dev; +Cc: kexec, mahesh, cascardo, gpiccoli, paulus, mpe

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 */

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCHv4 1/3] powerpc, cpu: partially unbind the mapping between cpu logical id and its seq in dt
  2018-03-12  4:43 ` [PATCHv4 1/3] powerpc, cpu: partially unbind the mapping between cpu logical id and its seq in dt Pingfan Liu
  2018-03-13  2:58   ` Benjamin Herrenschmidt
@ 2018-03-13  5:06   ` kbuild test robot
  1 sibling, 0 replies; 7+ messages in thread
From: kbuild test robot @ 2018-03-13  5:06 UTC (permalink / raw)
  To: Pingfan Liu; +Cc: kbuild-all, linuxppc-dev, cascardo, gpiccoli, mahesh, kexec

[-- Attachment #1: Type: text/plain, Size: 2210 bytes --]

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 14183 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCHv4 1/3] powerpc, cpu: partially unbind the mapping between cpu logical id and its seq in dt
  2018-03-13  2:58   ` Benjamin Herrenschmidt
@ 2018-03-14  2:02     ` Pingfan Liu
  0 siblings, 0 replies; 7+ messages in thread
From: Pingfan Liu @ 2018-03-14  2:02 UTC (permalink / raw)
  To: Benjamin Herrenschmidt
  Cc: linuxppc-dev, kexec, mahesh, cascardo, gpiccoli, paulus,
	Michael Ellerman

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2018-03-14  2:02 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-12  4:43 [PATCHv4 0/3] enable nr_cpus for powerpc Pingfan Liu
2018-03-12  4:43 ` [PATCHv4 1/3] powerpc, cpu: partially unbind the mapping between cpu logical id and its seq in dt Pingfan Liu
2018-03-13  2:58   ` Benjamin Herrenschmidt
2018-03-14  2:02     ` Pingfan Liu
2018-03-13  5:06   ` kbuild test robot
2018-03-12  4:43 ` [PATCHv4 2/3] powerpc, cpu: handling the special case when boot_cpuid greater than nr_cpus Pingfan Liu
2018-03-12  4:43 ` [PATCHv4 3/3] ppc64 boot: Wait for boot cpu to show up if nr_cpus limit is about to hit Pingfan Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).