[PATCH 1/2] powerpc/perf/imc: use cpu_to_node instead topology_physical_package_id

Subsystems: linux for powerpc (32-bit and 64-bit), the rest

STALE3182d REVIEWED: 1 (0M)

1 review trailer.

5 messages, 3 authors, 2017-11-24 · open the first message on its own page

[PATCH 1/2] powerpc/perf/imc: use cpu_to_node instead topology_physical_package_id

From: Madhavan Srinivasan <hidden>
Date: 2017-10-15 18:43:56

...
[1069001518.000000] [c000003f95b3f770] [c0000000000b2574] init_imc_pmu+0x1f4/0xc40
[1069005374.000000] [c000003f95b3f850] [c00000000008fec8] opal_imc_counters_probe+0x2e8/0x3e0
[1069009426.000000] [c000003f95b3f950] [c0000000006153a4] platform_drv_probe+0x44/0x90
[1069012818.000000] [c000003f95b3f9c0] [c0000000006124c0] really_probe+0x290/0x370
[1069016302.000000] [c000003f95b3fa50] [c0000000006126c8] __driver_attach+0x128/0x130
[1069019564.000000] [c000003f95b3fa90] [c00000000060f38c] bus_for_each_dev+0x9c/0x110
[1069022838.000000] [c000003f95b3fae0] [c000000000611dfc] driver_attach+0x3c/0x60
[1069026104.000000] [c000003f95b3fb10] [c0000000006118d8] bus_add_driver+0x298/0x320
[1069029428.000000] [c000003f95b3fb90] [c0000000006135b8] driver_register+0xb8/0x1a0
[1069033016.000000] [c000003f95b3fc00] [c00000000061533c] __platform_driver_register+0x8c/0xb0
[1069036362.000000] [c000003f95b3fc30] [c000000000cbb0fc] opal_imc_driver_init+0x24/0x38
[1069039756.000000] [c000003f95b3fc50] [c00000000000cc70] do_one_initcall+0xd0/0x250
[1069043094.000000] [c000003f95b3fd20] [c000000000ca44d0] kernel_init_freeable+0x244/0x324
[1069046490.000000] [c000003f95b3fdc0] [c00000000000d600] kernel_init+0x30/0x1b0
[1069050348.000000] [c000003f95b3fe30] [c00000000000b268] ret_from_kernel_thread+0x5c/0x74

init_imc_pmu() use topology_physical_package_id() to detect the phy_id
of the processor it is on to get local memory. But this cause crashes
when node_id are not same as physicaly id. As a fix use cpu_to_node().

Reported-By: Rob Lippert <redacted>
Tested-By: Madhavan Srinivasan <redacted>
Signed-off-by: Madhavan Srinivasan <redacted>
---
 arch/powerpc/perf/imc-pmu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
index 88126245881b..88d1415e2547 100644
--- a/arch/powerpc/perf/imc-pmu.c
+++ b/arch/powerpc/perf/imc-pmu.c

@@ -467,7 +467,7 @@ static int nest_imc_event_init(struct perf_event *event)
 	 * Nest HW counter memory resides in a per-chip reserve-memory (HOMER).
 	 * Get the base memory addresss for this cpu.
 	 */
-	chip_id = topology_physical_package_id(event->cpu);
+	chip_id = cpu_to_chip_id(event->cpu);
 	pcni = pmu->mem_info;
 	do {
 		if (pcni->id == chip_id) {

@@ -524,19 +524,19 @@ static int nest_imc_event_init(struct perf_event *event)
  */
 static int core_imc_mem_init(int cpu, int size)
 {
-	int phys_id, rc = 0, core_id = (cpu / threads_per_core);
+	int nid, rc = 0, core_id = (cpu / threads_per_core);
 	struct imc_mem_info *mem_info;
 
 	/*
 	 * alloc_pages_node() will allocate memory for core in the
 	 * local node only.
 	 */
-	phys_id = topology_physical_package_id(cpu);
+	nid = cpu_to_node(cpu);
 	mem_info = &core_imc_pmu->mem_info[core_id];
 	mem_info->id = core_id;
 
 	/* We need only vbase for core counters */
-	mem_info->vbase = page_address(alloc_pages_node(phys_id,
+	mem_info->vbase = page_address(alloc_pages_node(nid,
 					  GFP_KERNEL | __GFP_ZERO | __GFP_THISNODE |
 					  __GFP_NOWARN, get_order(size)));
 	if (!mem_info->vbase)

@@ -783,14 +783,14 @@ static int core_imc_event_init(struct perf_event *event)
 static int thread_imc_mem_alloc(int cpu_id, int size)
 {
 	u64 ldbar_value, *local_mem = per_cpu(thread_imc_mem, cpu_id);
-	int phys_id = topology_physical_package_id(cpu_id);
+	int nid = cpu_to_node(cpu_id);
 
 	if (!local_mem) {
 		/*
 		 * This case could happen only once at start, since we dont
 		 * free the memory in cpu offline path.
 		 */
-		local_mem = page_address(alloc_pages_node(phys_id,
+		local_mem = page_address(alloc_pages_node(nid,
 				  GFP_KERNEL | __GFP_ZERO | __GFP_THISNODE |
 				  __GFP_NOWARN, get_order(size)));
 		if (!local_mem)

-- 
2.7.4

[PATCH 2/2] powerpc/perf/imc: use NUMA_NO_NODE for alloc_pages_node

From: Madhavan Srinivasan <hidden>
Date: 2017-10-15 18:43:59

alloc_pages_node() when passed NUMA_NO_NODE for the
node_id, could get memory from closest node. Cleanup
core imc and thread imc memory init functions to use
NUMA_NO_NODE.

Signed-off-by: Madhavan Srinivasan <redacted>
---
 arch/powerpc/perf/imc-pmu.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
index 88d1415e2547..897098fb2a31 100644
--- a/arch/powerpc/perf/imc-pmu.c
+++ b/arch/powerpc/perf/imc-pmu.c

@@ -524,19 +524,18 @@ static int nest_imc_event_init(struct perf_event *event)
  */
 static int core_imc_mem_init(int cpu, int size)
 {
-	int nid, rc = 0, core_id = (cpu / threads_per_core);
+	int rc = 0, core_id = (cpu / threads_per_core);
 	struct imc_mem_info *mem_info;
 
 	/*
 	 * alloc_pages_node() will allocate memory for core in the
 	 * local node only.
 	 */
-	nid = cpu_to_node(cpu);
 	mem_info = &core_imc_pmu->mem_info[core_id];
 	mem_info->id = core_id;
 
 	/* We need only vbase for core counters */
-	mem_info->vbase = page_address(alloc_pages_node(nid,
+	mem_info->vbase = page_address(alloc_pages_node(NUMA_NO_NODE,
 					  GFP_KERNEL | __GFP_ZERO | __GFP_THISNODE |
 					  __GFP_NOWARN, get_order(size)));
 	if (!mem_info->vbase)

@@ -783,14 +782,13 @@ static int core_imc_event_init(struct perf_event *event)
 static int thread_imc_mem_alloc(int cpu_id, int size)
 {
 	u64 ldbar_value, *local_mem = per_cpu(thread_imc_mem, cpu_id);
-	int nid = cpu_to_node(cpu_id);
 
 	if (!local_mem) {
 		/*
 		 * This case could happen only once at start, since we dont
 		 * free the memory in cpu offline path.
 		 */
-		local_mem = page_address(alloc_pages_node(nid,
+		local_mem = page_address(alloc_pages_node(NUMA_NO_NODE,
 				  GFP_KERNEL | __GFP_ZERO | __GFP_THISNODE |
 				  __GFP_NOWARN, get_order(size)));
 		if (!local_mem)

-- 
2.7.4

Re: [PATCH 2/2] powerpc/perf/imc: use NUMA_NO_NODE for alloc_pages_node

From: Balbir Singh <bsingharora@gmail.com>
Date: 2017-10-16 02:19:09

On Mon, 16 Oct 2017 00:13:42 +0530
Madhavan Srinivasan [off-list ref] wrote:

alloc_pages_node() when passed NUMA_NO_NODE for the
node_id, could get memory from closest node. Cleanup
core imc and thread imc memory init functions to use
NUMA_NO_NODE.


The changelog is not clear, alloc_pages_node() takes a
preferred node id and creates a node zonelist from it.
How is NUMA_NO_NODE better? 

Balbir Singh.

Re: [PATCH 2/2] powerpc/perf/imc: use NUMA_NO_NODE for alloc_pages_node

From: Madhavan Srinivasan <hidden>
Date: 2017-10-16 16:09:35


On Monday 16 October 2017 07:48 AM, Balbir Singh wrote:

On Mon, 16 Oct 2017 00:13:42 +0530
Madhavan Srinivasan [off-list ref] wrote:

quoted

alloc_pages_node() when passed NUMA_NO_NODE for the
node_id, could get memory from closest node. Cleanup
core imc and thread imc memory init functions to use
NUMA_NO_NODE.

The changelog is not clear, alloc_pages_node() takes a
preferred node id and creates a node zonelist from it.
How is NUMA_NO_NODE better?

IIUC with NUMA_NO_NODE we could remove the __GFP_NOWARN
for alloc_pages_node(). That said, one must be careful to make
sure we dont end up allocating memory from the boot cpu.
And incase of In Memory Collection (IMC) counters, it is handled
in the cpu online path.

Will send a v2 with __GFP_NOWARN removed which
I missed in this :)  .

Maddy

Balbir Singh.

Re: [1/2] powerpc/perf/imc: use cpu_to_node instead topology_physical_package_id

From: Michael Ellerman <hidden>
Date: 2017-11-24 09:46:35

On Sun, 2017-10-15 at 18:43:41 UTC, Madhavan Srinivasan wrote:

...
[1069001518.000000] [c000003f95b3f770] [c0000000000b2574] init_imc_pmu+0x1f4/0xc40
[1069005374.000000] [c000003f95b3f850] [c00000000008fec8] opal_imc_counters_probe+0x2e8/0x3e0
[1069009426.000000] [c000003f95b3f950] [c0000000006153a4] platform_drv_probe+0x44/0x90
[1069012818.000000] [c000003f95b3f9c0] [c0000000006124c0] really_probe+0x290/0x370
[1069016302.000000] [c000003f95b3fa50] [c0000000006126c8] __driver_attach+0x128/0x130
[1069019564.000000] [c000003f95b3fa90] [c00000000060f38c] bus_for_each_dev+0x9c/0x110
[1069022838.000000] [c000003f95b3fae0] [c000000000611dfc] driver_attach+0x3c/0x60
[1069026104.000000] [c000003f95b3fb10] [c0000000006118d8] bus_add_driver+0x298/0x320
[1069029428.000000] [c000003f95b3fb90] [c0000000006135b8] driver_register+0xb8/0x1a0
[1069033016.000000] [c000003f95b3fc00] [c00000000061533c] __platform_driver_register+0x8c/0xb0
[1069036362.000000] [c000003f95b3fc30] [c000000000cbb0fc] opal_imc_driver_init+0x24/0x38
[1069039756.000000] [c000003f95b3fc50] [c00000000000cc70] do_one_initcall+0xd0/0x250
[1069043094.000000] [c000003f95b3fd20] [c000000000ca44d0] kernel_init_freeable+0x244/0x324
[1069046490.000000] [c000003f95b3fdc0] [c00000000000d600] kernel_init+0x30/0x1b0
[1069050348.000000] [c000003f95b3fe30] [c00000000000b268] ret_from_kernel_thread+0x5c/0x74

init_imc_pmu() use topology_physical_package_id() to detect the phy_id
of the processor it is on to get local memory. But this cause crashes
when node_id are not same as physicaly id. As a fix use cpu_to_node().

Reported-By: Rob Lippert <redacted>
Tested-By: Madhavan Srinivasan <redacted>
Signed-off-by: Madhavan Srinivasan <redacted>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/f3f1dfd600ff82b18b7ea73d80eb27

cheers

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help