All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] powerpc/smp: Misc fixes
@ 2021-08-21  9:24 Srikar Dronamraju
  2021-08-21  9:24 ` [PATCH 1/3] powerpc/smp: Fix a crash while booting kvm guest with nr_cpus=2 Srikar Dronamraju
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Srikar Dronamraju @ 2021-08-21  9:24 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Nathan Lynch, Gautham R Shenoy, Vincent Guittot,
	Srikar Dronamraju, Peter Zijlstra, Ingo Molnar, linuxppc-dev,
	Valentin Schneider

The 1st patch fixes a regression which causes a crash when booted with
nr_cpus=2.

The 2nd patch fixes a regression where lscpu on PowerVM reports more
number of sockets that that are available.

The 3rd patch updates the fallback when L2-cache properties are not
explicitly exposed to be the cpu_sibling_mask.

Cc: linuxppc-dev@lists.ozlabs.org
Cc: Nathan Lynch <nathanl@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Gautham R Shenoy <ego@linux.vnet.ibm.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Srikar Dronamraju (3):
  powerpc/smp: Fix a crash while booting kvm guest with nr_cpus=2
  powerpc/smp: Update cpu_core_map on PowerVM lpars.
  powerpc/smp: Enable CACHE domain for shared processor

 arch/powerpc/kernel/smp.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

--
2.18.2


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

* [PATCH 1/3] powerpc/smp: Fix a crash while booting kvm guest with nr_cpus=2
  2021-08-21  9:24 [PATCH 0/3] powerpc/smp: Misc fixes Srikar Dronamraju
@ 2021-08-21  9:24 ` Srikar Dronamraju
  2021-08-23  6:11   ` Gautham R Shenoy
  2021-08-21  9:24 ` [PATCH 2/3] powerpc/smp: Update cpu_core_map on PowerVM lpars Srikar Dronamraju
  2021-08-21  9:24 ` [PATCH 3/3] powerpc/smp: Enable CACHE domain for shared processor Srikar Dronamraju
  2 siblings, 1 reply; 7+ messages in thread
From: Srikar Dronamraju @ 2021-08-21  9:24 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Nathan Lynch, Gautham R Shenoy, Vincent Guittot,
	Srikar Dronamraju, Peter Zijlstra, Aneesh Kumar K . V,
	Valentin Schneider, linuxppc-dev, Ingo Molnar

Aneesh reported a crash with a fairly recent upstream kernel when
booting kernel whose commandline was appended with nr_cpus=2

1:mon> e
cpu 0x1: Vector: 300 (Data Access) at [c000000008a67bd0]
    pc: c00000000002557c: cpu_to_chip_id+0x3c/0x100
    lr: c000000000058380: start_secondary+0x460/0xb00
    sp: c000000008a67e70
   msr: 8000000000001033
   dar: 10
 dsisr: 80000
  current = 0xc00000000891bb00
  paca    = 0xc0000018ff981f80   irqmask: 0x03   irq_happened: 0x01
    pid   = 0, comm = swapper/1
Linux version 5.13.0-rc3-15704-ga050a6d2b7e8 (kvaneesh@ltc-boston8) (gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0, GNU ld (GNU Binutils for Ubuntu) 2.34) #433 SMP Tue May 25 02:38:49 CDT 2021
1:mon> t
[link register   ] c000000000058380 start_secondary+0x460/0xb00
[c000000008a67e70] c000000008a67eb0 (unreliable)
[c000000008a67eb0] c0000000000589d4 start_secondary+0xab4/0xb00
[c000000008a67f90] c00000000000c654 start_secondary_prolog+0x10/0x14

Current code assumes that num_possible_cpus() is always greater than
threads_per_core. However this may not be true when using nr_cpus=2 or
similar options. Handle the case where num_possible_cpus is smaller than
threads_per_core.

Cc: linuxppc-dev@lists.ozlabs.org
Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Cc: Nathan Lynch <nathanl@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Gautham R Shenoy <ego@linux.vnet.ibm.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Fixes: c1e53367dab1 ("powerpc/smp: Cache CPU to chip lookup")
Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
Debugged-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/smp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 6c6e4d934d86..3d6874fe1937 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -1074,7 +1074,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
 	}
 
 	if (cpu_to_chip_id(boot_cpuid) != -1) {
-		int idx = num_possible_cpus() / threads_per_core;
+		int idx = max((int)num_possible_cpus() / threads_per_core, 1);
 
 		/*
 		 * All threads of a core will all belong to the same core,
-- 
2.18.2


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

* [PATCH 2/3] powerpc/smp: Update cpu_core_map on PowerVM lpars.
  2021-08-21  9:24 [PATCH 0/3] powerpc/smp: Misc fixes Srikar Dronamraju
  2021-08-21  9:24 ` [PATCH 1/3] powerpc/smp: Fix a crash while booting kvm guest with nr_cpus=2 Srikar Dronamraju
@ 2021-08-21  9:24 ` Srikar Dronamraju
  2021-08-21  9:24 ` [PATCH 3/3] powerpc/smp: Enable CACHE domain for shared processor Srikar Dronamraju
  2 siblings, 0 replies; 7+ messages in thread
From: Srikar Dronamraju @ 2021-08-21  9:24 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Nathan Lynch, Gautham R Shenoy, Vincent Guittot,
	Srikar Dronamraju, Peter Zijlstra, Ingo Molnar, linuxppc-dev,
	Valentin Schneider

lscpu() uses core_siblings to list the number of sockets in the
system. core_siblings is set using topology_core_cpumask.

While optimizing the powerpc bootup path, Commit 4ca234a9cbd7
("powerpc/smp: Stop updating cpu_core_mask").  it was found that
updating cpu_core_mask() ended up taking a lot of time. It was thought
that on Powerpc, cpu_core_mask() would always be same as
cpu_cpu_mask() i.e number of sockets will always be equal to number of
nodes. As an optimization, cpu_core_mask() was made a snapshot of
cpu_cpu_mask().

However that was found to be false with PowerPc KVM guests, where each
node could have more than one socket. So with Commit c47f892d7aa6
("powerpc/smp: Reintroduce cpu_core_mask"), cpu_core_mask was updated
based on chip_id but in an optimized way using some mask manipulations
and chip_id caching.

However PowerVM based lpars (and other not implementing
cpu_to_chip_id(), continued to use a copy of cpu_cpu_mask().

There are two issues that were noticed on PowerVM lpars.
1. lscpu would report one extra socket.
On a IBM,9009-42A (aka zz system) which has only 2 chips/ sockets/
nodes, lscpu would report
Architecture:        ppc64le
Byte Order:          Little Endian
CPU(s):              160
On-line CPU(s) list: 0-159
Thread(s) per core:  8
Core(s) per socket:  6
Socket(s):           3                <--------------
NUMA node(s):        2
Model:               2.2 (pvr 004e 0202)
Model name:          POWER9 (architected), altivec supported
Hypervisor vendor:   pHyp
Virtualization type: para
L1d cache:           32K
L1i cache:           32K
L2 cache:            512K
L3 cache:            10240K
NUMA node0 CPU(s):   0-79
NUMA node1 CPU(s):   80-159

2. Currently cpu_cpu_mask is updated when a core is
added/removed. However its not updated when smt mode switching or on
CPUs are explicitly offlined. However all other percpu masks are
updated to ensure only active/online CPUs are in the masks.
This results in build_sched_domain traces since there will be CPUs in
cpu_cpu_mask() but those CPUs are not present in SMT / CACHE / MC /
NUMA domains. A loop of threads running smt mode switching and core
add/remove will soon show this trace.
Hence cpu_cpu_mask has to be update at smt mode switch.

This will have impact on cpu_core_mask(). cpu_core_mask() is a
snapshot of cpu_cpu_mask. Different CPUs within the same socket will
end up having different cpu_core_masks since they are snapshots at
different points of time. This means when lscpu will start reporting
many more sockets than the actual number of sockets/ nodes / chips.

Different ways to handle this problem:
A. Update the snapshot aka cpu_core_mask for all CPUs whenever
   cpu_cpu_mask is updated. This would a non-optimal solution.
B. Instead of a cpumask_var_t, make cpu_core_map a cpumask pointer
   pointing to cpu_cpu_mask. However percpu cpumask pointer is frowned
   upon and we need a clean way to handle PowerPc KVM guest which is
   not a snapshot.
C. Update cpu_core_masks in PowerVM like in PowerPc KVM guests using
   mask manipulations. This approach is relatively simple and unifies
   with the existing code.
D. On top of 3, we could also resurrect get_physical_package_id which
   could return a nid for the said CPU. However this is not needed at this
   time.

Option C is the preferred approach for now.

While this is somewhat a revert of Commit 4ca234a9cbd7 ("powerpc/smp:
Stop updating cpu_core_mask").

1. Plain revert has some conflicts
2. For chip_id == -1, the cpu_core_mask is made identical to
cpu_cpu_mask, unlike previously where cpu_core_mask was set to a core
if chip_id doesn't exist.

This goes by the principle that if chip_id is not exposed, then
sockets / chip / node share the same set of CPUs.

With the fix, lscpu o/p would be
Architecture:        ppc64le
Byte Order:          Little Endian
CPU(s):              160
On-line CPU(s) list: 0-159
Thread(s) per core:  8
Core(s) per socket:  6
Socket(s):           2                     <--------------
NUMA node(s):        2
Model:               2.2 (pvr 004e 0202)
Model name:          POWER9 (architected), altivec supported
Hypervisor vendor:   pHyp
Virtualization type: para
L1d cache:           32K
L1i cache:           32K
L2 cache:            512K
L3 cache:            10240K
NUMA node0 CPU(s):   0-79
NUMA node1 CPU(s):   80-159

Cc: linuxppc-dev@lists.ozlabs.org
Cc: Nathan Lynch <nathanl@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Gautham R Shenoy <ego@linux.vnet.ibm.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Fixes: 4ca234a9cbd7 ("powerpc/smp: Stop updating cpu_core_mask")
Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/smp.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 3d6874fe1937..3d26d3c61e94 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -1492,6 +1492,7 @@ static void add_cpu_to_masks(int cpu)
 	 * add it to it's own thread sibling mask.
 	 */
 	cpumask_set_cpu(cpu, cpu_sibling_mask(cpu));
+	cpumask_set_cpu(cpu, cpu_core_mask(cpu));
 
 	for (i = first_thread; i < first_thread + threads_per_core; i++)
 		if (cpu_online(i))
@@ -1509,11 +1510,6 @@ static void add_cpu_to_masks(int cpu)
 	if (chip_id_lookup_table && ret)
 		chip_id = cpu_to_chip_id(cpu);
 
-	if (chip_id == -1) {
-		cpumask_copy(per_cpu(cpu_core_map, cpu), cpu_cpu_mask(cpu));
-		goto out;
-	}
-
 	if (shared_caches)
 		submask_fn = cpu_l2_cache_mask;
 
@@ -1523,6 +1519,10 @@ static void add_cpu_to_masks(int cpu)
 	/* Skip all CPUs already part of current CPU core mask */
 	cpumask_andnot(mask, cpu_online_mask, cpu_core_mask(cpu));
 
+	/* If chip_id is -1; limit the cpu_core_mask to within DIE*/
+	if (chip_id == -1)
+		cpumask_and(mask, mask, cpu_cpu_mask(cpu));
+
 	for_each_cpu(i, mask) {
 		if (chip_id == cpu_to_chip_id(i)) {
 			or_cpumasks_related(cpu, i, submask_fn, cpu_core_mask);
@@ -1532,7 +1532,6 @@ static void add_cpu_to_masks(int cpu)
 		}
 	}
 
-out:
 	free_cpumask_var(mask);
 }
 
-- 
2.18.2


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

* [PATCH 3/3] powerpc/smp: Enable CACHE domain for shared processor
  2021-08-21  9:24 [PATCH 0/3] powerpc/smp: Misc fixes Srikar Dronamraju
  2021-08-21  9:24 ` [PATCH 1/3] powerpc/smp: Fix a crash while booting kvm guest with nr_cpus=2 Srikar Dronamraju
  2021-08-21  9:24 ` [PATCH 2/3] powerpc/smp: Update cpu_core_map on PowerVM lpars Srikar Dronamraju
@ 2021-08-21  9:24 ` Srikar Dronamraju
  2021-08-24  8:55   ` Gautham R Shenoy
  2 siblings, 1 reply; 7+ messages in thread
From: Srikar Dronamraju @ 2021-08-21  9:24 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Nathan Lynch, Gautham R Shenoy, Vincent Guittot,
	Srikar Dronamraju, Peter Zijlstra, Ingo Molnar, linuxppc-dev,
	Valentin Schneider

Currently CACHE domain is not enabled on shared processor mode PowerVM
LPARS. On PowerVM systems, 'ibm,thread-group' device-tree property 2
under cpu-device-node indicates which all CPUs share L2-cache. However
'ibm,thread-group' device-tree property 2 is a relatively new property.
In absence of 'ibm,thread-group' property 2, 'l2-cache' device property
under cpu-device-node could help system to identify CPUs sharing L2-cache.
However this property is not exposed by PhyP in shared processor mode
configurations.

In absence of properties that inform OS about which CPUs share L2-cache,
fallback on core boundary.

Here are some stats from Power9 shared LPAR with the changes.

$ lscpu
Architecture:        ppc64le
Byte Order:          Little Endian
CPU(s):              32
On-line CPU(s) list: 0-31
Thread(s) per core:  8
Core(s) per socket:  1
Socket(s):           3
NUMA node(s):        2
Model:               2.2 (pvr 004e 0202)
Model name:          POWER9 (architected), altivec supported
Hypervisor vendor:   pHyp
Virtualization type: para
L1d cache:           32K
L1i cache:           32K
NUMA node0 CPU(s):   16-23
NUMA node1 CPU(s):   0-15,24-31
Physical sockets:    2
Physical chips:      1
Physical cores/chip: 10

Before patch
$ grep -r . /sys/kernel/debug/sched/domains/cpu0/domain*/name
Before
/sys/kernel/debug/sched/domains/cpu0/domain0/name:SMT
/sys/kernel/debug/sched/domains/cpu0/domain1/name:DIE
/sys/kernel/debug/sched/domains/cpu0/domain2/name:NUMA

After
/sys/kernel/debug/sched/domains/cpu0/domain0/name:SMT
/sys/kernel/debug/sched/domains/cpu0/domain1/name:CACHE
/sys/kernel/debug/sched/domains/cpu0/domain2/name:DIE
/sys/kernel/debug/sched/domains/cpu0/domain3/name:NUMA

$  awk '/domain/{print $1, $2}' /proc/schedstat | sort -u | sed -e 's/00000000,//g'
Before
domain0 00000055
domain0 000000aa
domain0 00005500
domain0 0000aa00
domain0 00550000
domain0 00aa0000
domain0 55000000
domain0 aa000000
domain1 00ff0000
domain1 ff00ffff
domain2 ffffffff

After
domain0 00000055
domain0 000000aa
domain0 00005500
domain0 0000aa00
domain0 00550000
domain0 00aa0000
domain0 55000000
domain0 aa000000
domain1 000000ff
domain1 0000ff00
domain1 00ff0000
domain1 ff000000
domain2 ff00ffff
domain2 ffffffff
domain3 ffffffff

(Lower is better)
perf stat -a -r 5 -n perf bench sched pipe  | tail -n 2
Before
           153.798 +- 0.142 seconds time elapsed  ( +-  0.09% )

After
           111.545 +- 0.652 seconds time elapsed  ( +-  0.58% )

which is an improvement of 27.47%

Cc: linuxppc-dev@lists.ozlabs.org
Cc: Nathan Lynch <nathanl@linux.ibm.com>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Gautham R Shenoy <ego@linux.vnet.ibm.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/smp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 3d26d3c61e94..47b15f31cc29 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -1365,7 +1365,7 @@ static bool update_mask_by_l2(int cpu, cpumask_var_t *mask)
 	l2_cache = cpu_to_l2cache(cpu);
 	if (!l2_cache || !*mask) {
 		/* Assume only core siblings share cache with this CPU */
-		for_each_cpu(i, submask_fn(cpu))
+		for_each_cpu(i, cpu_sibling_mask(cpu))
 			set_cpus_related(cpu, i, cpu_l2_cache_mask);
 
 		return false;
-- 
2.18.2


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

* Re: [PATCH 1/3] powerpc/smp: Fix a crash while booting kvm guest with nr_cpus=2
  2021-08-21  9:24 ` [PATCH 1/3] powerpc/smp: Fix a crash while booting kvm guest with nr_cpus=2 Srikar Dronamraju
@ 2021-08-23  6:11   ` Gautham R Shenoy
  2021-08-23 10:04     ` Srikar Dronamraju
  0 siblings, 1 reply; 7+ messages in thread
From: Gautham R Shenoy @ 2021-08-23  6:11 UTC (permalink / raw)
  To: Srikar Dronamraju
  Cc: Nathan Lynch, Gautham R Shenoy, Vincent Guittot, Peter Zijlstra,
	Valentin Schneider, Aneesh Kumar K . V, linuxppc-dev,
	Ingo Molnar

On Sat, Aug 21, 2021 at 02:54:17PM +0530, Srikar Dronamraju wrote:
> Aneesh reported a crash with a fairly recent upstream kernel when
> booting kernel whose commandline was appended with nr_cpus=2
> 
> 1:mon> e
> cpu 0x1: Vector: 300 (Data Access) at [c000000008a67bd0]
>     pc: c00000000002557c: cpu_to_chip_id+0x3c/0x100
>     lr: c000000000058380: start_secondary+0x460/0xb00
>     sp: c000000008a67e70
>    msr: 8000000000001033
>    dar: 10
>  dsisr: 80000
>   current = 0xc00000000891bb00
>   paca    = 0xc0000018ff981f80   irqmask: 0x03   irq_happened: 0x01
>     pid   = 0, comm = swapper/1
> Linux version 5.13.0-rc3-15704-ga050a6d2b7e8 (kvaneesh@ltc-boston8) (gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0, GNU ld (GNU Binutils for Ubuntu) 2.34) #433 SMP Tue May 25 02:38:49 CDT 2021
> 1:mon> t
> [link register   ] c000000000058380 start_secondary+0x460/0xb00
> [c000000008a67e70] c000000008a67eb0 (unreliable)
> [c000000008a67eb0] c0000000000589d4 start_secondary+0xab4/0xb00
> [c000000008a67f90] c00000000000c654 start_secondary_prolog+0x10/0x14
> 
> Current code assumes that num_possible_cpus() is always greater than
> threads_per_core. However this may not be true when using nr_cpus=2 or
> similar options. Handle the case where num_possible_cpus is smaller than
> threads_per_core.
>
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Cc: Nathan Lynch <nathanl@linux.ibm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Gautham R Shenoy <ego@linux.vnet.ibm.com>
> Cc: Vincent Guittot <vincent.guittot@linaro.org>
> Fixes: c1e53367dab1 ("powerpc/smp: Cache CPU to chip lookup")
> Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Debugged-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/smp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 6c6e4d934d86..3d6874fe1937 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -1074,7 +1074,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
>  	}
> 
>  	if (cpu_to_chip_id(boot_cpuid) != -1) {
> -		int idx = num_possible_cpus() / threads_per_core;
> +		int idx = max((int)num_possible_cpus() / threads_per_core, 1);

I think this code was assuming that num_possible_cpus() is a multiple
of threads_per_core.

So, on a system with threads_per_core=8, if we pass nr_cpus=10, we
will still get idx=1. Thus, we will allocate only one entry in
chip_id_lookup_table[] even though there are two cores and
chip_id_lookup_table[] is expected to have one entry per core.

Is this a valid scenario ? If yes, should we use

   idx = DIV_ROUND_UP(num_possible_cpus, threads_per_core);

?

> 
>  		/*
>  		 * All threads of a core will all belong to the same core,
> -- 
> 2.18.2
> 

--
Thanks and Regards
gautham.

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

* Re: [PATCH 1/3] powerpc/smp: Fix a crash while booting kvm guest with nr_cpus=2
  2021-08-23  6:11   ` Gautham R Shenoy
@ 2021-08-23 10:04     ` Srikar Dronamraju
  0 siblings, 0 replies; 7+ messages in thread
From: Srikar Dronamraju @ 2021-08-23 10:04 UTC (permalink / raw)
  To: Gautham R Shenoy
  Cc: Nathan Lynch, Vincent Guittot, Peter Zijlstra,
	Valentin Schneider, Aneesh Kumar K . V, linuxppc-dev,
	Ingo Molnar

* Gautham R Shenoy <ego@linux.vnet.ibm.com> [2021-08-23 11:41:22]:

> On Sat, Aug 21, 2021 at 02:54:17PM +0530, Srikar Dronamraju wrote:
> > Aneesh reported a crash with a fairly recent upstream kernel when
> > booting kernel whose commandline was appended with nr_cpus=2
> > 
> > 1:mon> e
> > cpu 0x1: Vector: 300 (Data Access) at [c000000008a67bd0]
> >     pc: c00000000002557c: cpu_to_chip_id+0x3c/0x100
> >     lr: c000000000058380: start_secondary+0x460/0xb00
> >     sp: c000000008a67e70
> >    msr: 8000000000001033
> >    dar: 10
> >  dsisr: 80000
> >   current = 0xc00000000891bb00
> >   paca    = 0xc0000018ff981f80   irqmask: 0x03   irq_happened: 0x01
> >     pid   = 0, comm = swapper/1
> > Linux version 5.13.0-rc3-15704-ga050a6d2b7e8 (kvaneesh@ltc-boston8) (gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0, GNU ld (GNU Binutils for Ubuntu) 2.34) #433 SMP Tue May 25 02:38:49 CDT 2021
> > 1:mon> t
> > [link register   ] c000000000058380 start_secondary+0x460/0xb00
> > [c000000008a67e70] c000000008a67eb0 (unreliable)
> > [c000000008a67eb0] c0000000000589d4 start_secondary+0xab4/0xb00
> > [c000000008a67f90] c00000000000c654 start_secondary_prolog+0x10/0x14
> > 
> > Current code assumes that num_possible_cpus() is always greater than
> > threads_per_core. However this may not be true when using nr_cpus=2 or
> > similar options. Handle the case where num_possible_cpus is smaller than
> > threads_per_core.
> >
> > Cc: linuxppc-dev@lists.ozlabs.org
> > Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> > Cc: Nathan Lynch <nathanl@linux.ibm.com>
> > Cc: Michael Ellerman <mpe@ellerman.id.au>
> > Cc: Ingo Molnar <mingo@kernel.org>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Valentin Schneider <valentin.schneider@arm.com>
> > Cc: Gautham R Shenoy <ego@linux.vnet.ibm.com>
> > Cc: Vincent Guittot <vincent.guittot@linaro.org>
> > Fixes: c1e53367dab1 ("powerpc/smp: Cache CPU to chip lookup")
> > Reported-by: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> > Debugged-by: Michael Ellerman <mpe@ellerman.id.au>
> > Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> > ---
> >  arch/powerpc/kernel/smp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> > index 6c6e4d934d86..3d6874fe1937 100644
> > --- a/arch/powerpc/kernel/smp.c
> > +++ b/arch/powerpc/kernel/smp.c
> > @@ -1074,7 +1074,7 @@ void __init smp_prepare_cpus(unsigned int max_cpus)
> >  	}
> > 
> >  	if (cpu_to_chip_id(boot_cpuid) != -1) {
> > -		int idx = num_possible_cpus() / threads_per_core;
> > +		int idx = max((int)num_possible_cpus() / threads_per_core, 1);
> 
> I think this code was assuming that num_possible_cpus() is a multiple
> of threads_per_core.
> 
> So, on a system with threads_per_core=8, if we pass nr_cpus=10, we
> will still get idx=1. Thus, we will allocate only one entry in
> chip_id_lookup_table[] even though there are two cores and
> chip_id_lookup_table[] is expected to have one entry per core.
> 
> Is this a valid scenario ? If yes, should we use
> 
>    idx = DIV_ROUND_UP(num_possible_cpus, threads_per_core);
> 

Yes, this can be done.
will resend this patch with this change.

> 
> > 
> >  		/*
> >  		 * All threads of a core will all belong to the same core,
> > -- 
> > 2.18.2
> > 
> 
> --
> Thanks and Regards
> gautham.

-- 
Thanks and Regards
Srikar Dronamraju

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

* Re: [PATCH 3/3] powerpc/smp: Enable CACHE domain for shared processor
  2021-08-21  9:24 ` [PATCH 3/3] powerpc/smp: Enable CACHE domain for shared processor Srikar Dronamraju
@ 2021-08-24  8:55   ` Gautham R Shenoy
  0 siblings, 0 replies; 7+ messages in thread
From: Gautham R Shenoy @ 2021-08-24  8:55 UTC (permalink / raw)
  To: Srikar Dronamraju
  Cc: Nathan Lynch, Gautham R Shenoy, Vincent Guittot, Peter Zijlstra,
	Valentin Schneider, linuxppc-dev, Ingo Molnar

Hello Srikar,

On Sat, Aug 21, 2021 at 02:54:19PM +0530, Srikar Dronamraju wrote:
[..snip..]

The patch looks good to me.

Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>

> ---
>  arch/powerpc/kernel/smp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 3d26d3c61e94..47b15f31cc29 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -1365,7 +1365,7 @@ static bool update_mask_by_l2(int cpu, cpumask_var_t *mask)
>  	l2_cache = cpu_to_l2cache(cpu);
>  	if (!l2_cache || !*mask) {
>  		/* Assume only core siblings share cache with this CPU */
> -		for_each_cpu(i, submask_fn(cpu))
> +		for_each_cpu(i, cpu_sibling_mask(cpu))
>  			set_cpus_related(cpu, i, cpu_l2_cache_mask);
> 
>  		return false;
> -- 
> 2.18.2
> 

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

end of thread, other threads:[~2021-08-24  8:57 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-21  9:24 [PATCH 0/3] powerpc/smp: Misc fixes Srikar Dronamraju
2021-08-21  9:24 ` [PATCH 1/3] powerpc/smp: Fix a crash while booting kvm guest with nr_cpus=2 Srikar Dronamraju
2021-08-23  6:11   ` Gautham R Shenoy
2021-08-23 10:04     ` Srikar Dronamraju
2021-08-21  9:24 ` [PATCH 2/3] powerpc/smp: Update cpu_core_map on PowerVM lpars Srikar Dronamraju
2021-08-21  9:24 ` [PATCH 3/3] powerpc/smp: Enable CACHE domain for shared processor Srikar Dronamraju
2021-08-24  8:55   ` Gautham R Shenoy

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.