[PATCH 0/3] live partition migration vs cacheinfo

Partition migration often results in the platform telling the OS to
replace all the cache nodes in the device tree. The cacheinfo code has
no knowledge of this, and continues to maintain references to the
deleted/detached nodes, causing subsequent CPU online/offline
operations to get warnings and oopses. This series addresses this
longstanding issue by providing an interface to the cacheinfo layer
that the migration code uses to rebuild the cacheinfo data structures
at a safe time after migration, with appropriate serialization vs CPU
hotplug.


Nathan Lynch (3):
  powerpc/cacheinfo: add cacheinfo_teardown, cacheinfo_rebuild
  powerpc/pseries/mobility: prevent cpu hotplug during DT update
  powerpc/pseries/mobility: rebuild cacheinfo hierarchy post-migration

 arch/powerpc/kernel/cacheinfo.c           | 21 +++++++++++++++++++++
 arch/powerpc/kernel/cacheinfo.h           |  4 ++++
 arch/powerpc/platforms/pseries/mobility.c | 19 +++++++++++++++++++
 3 files changed, 44 insertions(+)

-- 
2.20.1

Allow external callers to force the cacheinfo code to release all its
references to cache nodes, e.g. before processing device tree updates
post-migration, and to rebuild the hierarchy afterward.

CPU online/offline must be blocked by callers; enforce this.

Fixes: 410bccf97881 ("powerpc/pseries: Partition migration in the kernel")
Signed-off-by: Nathan Lynch <redacted>
---
 arch/powerpc/kernel/cacheinfo.c | 21 +++++++++++++++++++++
 arch/powerpc/kernel/cacheinfo.h |  4 ++++
 2 files changed, 25 insertions(+)

diff --git a/arch/powerpc/kernel/cacheinfo.c b/arch/powerpc/kernel/cacheinfo.c
index 862e2890bd3d..42c559efe060 100644
--- a/arch/powerpc/kernel/cacheinfo.c
+++ b/arch/powerpc/kernel/cacheinfo.c

@@ -896,4 +896,25 @@ void cacheinfo_cpu_offline(unsigned int cpu_id)
 	if (cache)
 		cache_cpu_clear(cache, cpu_id);
 }
+
+void cacheinfo_teardown(void)
+{
+	unsigned int cpu;
+
+	lockdep_assert_cpus_held();
+
+	for_each_online_cpu(cpu)
+		cacheinfo_cpu_offline(cpu);
+}
+
+void cacheinfo_rebuild(void)
+{
+	unsigned int cpu;
+
+	lockdep_assert_cpus_held();
+
+	for_each_online_cpu(cpu)
+		cacheinfo_cpu_online(cpu);
+}
+
 #endif /* (CONFIG_PPC_PSERIES && CONFIG_SUSPEND) || CONFIG_HOTPLUG_CPU */

diff --git a/arch/powerpc/kernel/cacheinfo.h b/arch/powerpc/kernel/cacheinfo.h
index 955f5e999f1b..52bd3fc6642d 100644
--- a/arch/powerpc/kernel/cacheinfo.h
+++ b/arch/powerpc/kernel/cacheinfo.h

@@ -6,4 +6,8 @@
 extern void cacheinfo_cpu_online(unsigned int cpu_id);
 extern void cacheinfo_cpu_offline(unsigned int cpu_id);
 
+/* Allow migration/suspend to tear down and rebuild the hierarchy. */
+extern void cacheinfo_teardown(void);
+extern void cacheinfo_rebuild(void);
+
 #endif /* _PPC_CACHEINFO_H */

-- 
2.20.1

On Wed, Jun 12, 2019 at 10:15 AM Nathan Lynch [off-list ref] wrote:

Allow external callers to force the cacheinfo code to release all its
references to cache nodes, e.g. before processing device tree updates
post-migration, and to rebuild the hierarchy afterward.

CPU online/offline must be blocked by callers; enforce this.

Fixes: 410bccf97881 ("powerpc/pseries: Partition migration in the kernel")
Signed-off-by: Nathan Lynch <redacted>

Reviewed-by: Gautham R. Shenoy <redacted>

-- 
Thanks and Regards
gautham.

On Wed, 2019-06-12 at 04:45:04 UTC, Nathan Lynch wrote:

Allow external callers to force the cacheinfo code to release all its
references to cache nodes, e.g. before processing device tree updates
post-migration, and to rebuild the hierarchy afterward.

CPU online/offline must be blocked by callers; enforce this.

Fixes: 410bccf97881 ("powerpc/pseries: Partition migration in the kernel")
Signed-off-by: Nathan Lynch <redacted>
Reviewed-by: Gautham R. Shenoy <redacted>

Series applied to powerpc next, thanks.

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

cheers

It's common for the platform to replace the cache device nodes after a
migration. Since the cacheinfo code is never informed about this, it
never drops its references to the source system's cache nodes, causing
it to wind up in an inconsistent state resulting in warnings and oopses
as soon as CPU online/offline occurs after the migration, e.g.

cache for /cpus/l3-cache@3113(Unified) refers to cache for /cpus/l2-cache@200d(Unified)
WARNING: CPU: 15 PID: 86 at arch/powerpc/kernel/cacheinfo.c:176 release_cache+0x1bc/0x1d0
[...]
NIP [c00000000002d9bc] release_cache+0x1bc/0x1d0
LR [c00000000002d9b8] release_cache+0x1b8/0x1d0
Call Trace:
[c0000001fc99fa70] [c00000000002d9b8] release_cache+0x1b8/0x1d0 (unreliable)
[c0000001fc99fb10] [c00000000002ebf4] cacheinfo_cpu_offline+0x1c4/0x2c0
[c0000001fc99fbe0] [c00000000002ae58] unregister_cpu_online+0x1b8/0x260
[c0000001fc99fc40] [c000000000165a64] cpuhp_invoke_callback+0x114/0xf40
[c0000001fc99fcd0] [c000000000167450] cpuhp_thread_fun+0x270/0x310
[c0000001fc99fd40] [c0000000001a8bb8] smpboot_thread_fn+0x2c8/0x390
[c0000001fc99fdb0] [c0000000001a1cd8] kthread+0x1b8/0x1c0
[c0000001fc99fe20] [c00000000000c2d4] ret_from_kernel_thread+0x5c/0x68

Using device tree notifiers won't work since we want to rebuild the
hierarchy only after all the removals and additions have occurred and
the device tree is in a consistent state. Call cacheinfo_teardown()
before processing device tree updates, and rebuild the hierarchy
afterward.

Fixes: 410bccf97881 ("powerpc/pseries: Partition migration in the kernel")
Signed-off-by: Nathan Lynch <redacted>
---
 arch/powerpc/platforms/pseries/mobility.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index edc1ec408589..b8c8096907d4 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c

@@ -23,6 +23,7 @@
 #include <asm/machdep.h>
 #include <asm/rtas.h>
 #include "pseries.h"
+#include "../../kernel/cacheinfo.h"
 
 static struct kobject *mobility_kobj;
 

@@ -345,11 +346,20 @@ void post_mobility_fixup(void)
 	 */
 	cpus_read_lock();
 
+	/*
+	 * It's common for the destination firmware to replace cache
+	 * nodes.  Release all of the cacheinfo hierarchy's references
+	 * before updating the device tree.
+	 */
+	cacheinfo_teardown();
+
 	rc = pseries_devicetree_update(MIGRATION_SCOPE);
 	if (rc)
 		printk(KERN_ERR "Post-mobility device tree update "
 			"failed: %d\n", rc);
 
+	cacheinfo_rebuild();
+
 	cpus_read_unlock();
 
 	/* Possibly switch to a new RFI flush type */

-- 
2.20.1

On Wed, Jun 12, 2019 at 10:17 AM Nathan Lynch [off-list ref] wrote:

It's common for the platform to replace the cache device nodes after a
migration. Since the cacheinfo code is never informed about this, it
never drops its references to the source system's cache nodes, causing
it to wind up in an inconsistent state resulting in warnings and oopses
as soon as CPU online/offline occurs after the migration, e.g.

cache for /cpus/l3-cache@3113(Unified) refers to cache for /cpus/l2-cache@200d(Unified)
WARNING: CPU: 15 PID: 86 at arch/powerpc/kernel/cacheinfo.c:176 release_cache+0x1bc/0x1d0
[...]
NIP [c00000000002d9bc] release_cache+0x1bc/0x1d0
LR [c00000000002d9b8] release_cache+0x1b8/0x1d0
Call Trace:
[c0000001fc99fa70] [c00000000002d9b8] release_cache+0x1b8/0x1d0 (unreliable)
[c0000001fc99fb10] [c00000000002ebf4] cacheinfo_cpu_offline+0x1c4/0x2c0
[c0000001fc99fbe0] [c00000000002ae58] unregister_cpu_online+0x1b8/0x260
[c0000001fc99fc40] [c000000000165a64] cpuhp_invoke_callback+0x114/0xf40
[c0000001fc99fcd0] [c000000000167450] cpuhp_thread_fun+0x270/0x310
[c0000001fc99fd40] [c0000000001a8bb8] smpboot_thread_fn+0x2c8/0x390
[c0000001fc99fdb0] [c0000000001a1cd8] kthread+0x1b8/0x1c0
[c0000001fc99fe20] [c00000000000c2d4] ret_from_kernel_thread+0x5c/0x68

Using device tree notifiers won't work since we want to rebuild the
hierarchy only after all the removals and additions have occurred and
the device tree is in a consistent state. Call cacheinfo_teardown()
before processing device tree updates, and rebuild the hierarchy
afterward.

Fixes: 410bccf97881 ("powerpc/pseries: Partition migration in the kernel")
Signed-off-by: Nathan Lynch <redacted>

Reviewed-by: Gautham R. Shenoy <redacted>

-- 
Thanks and Regards
gautham.

CPU online/offline code paths are sensitive to parts of the device
tree (various cpu node properties, cache nodes) that can be changed as
a result of a migration.

Prevent CPU hotplug while the device tree potentially is inconsistent.

Fixes: 410bccf97881 ("powerpc/pseries: Partition migration in the kernel")
Signed-off-by: Nathan Lynch <redacted>
---
 arch/powerpc/platforms/pseries/mobility.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index 88925f8ca8a0..edc1ec408589 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c

@@ -9,6 +9,7 @@
  * 2 as published by the Free Software Foundation.
  */
 
+#include <linux/cpu.h>
 #include <linux/kernel.h>
 #include <linux/kobject.h>
 #include <linux/smp.h>

@@ -338,11 +339,19 @@ void post_mobility_fixup(void)
 	if (rc)
 		printk(KERN_ERR "Post-mobility activate-fw failed: %d\n", rc);
 
+	/*
+	 * We don't want CPUs to go online/offline while the device
+	 * tree is being updated.
+	 */
+	cpus_read_lock();
+
 	rc = pseries_devicetree_update(MIGRATION_SCOPE);
 	if (rc)
 		printk(KERN_ERR "Post-mobility device tree update "
 			"failed: %d\n", rc);
 
+	cpus_read_unlock();
+
 	/* Possibly switch to a new RFI flush type */
 	pseries_setup_rfi_flush();
 

-- 
2.20.1

Hello Nathan,

On Wed, Jun 12, 2019 at 10:19 AM Nathan Lynch [off-list ref] wrote:

CPU online/offline code paths are sensitive to parts of the device
tree (various cpu node properties, cache nodes) that can be changed as
a result of a migration.

Prevent CPU hotplug while the device tree potentially is inconsistent.

Fixes: 410bccf97881 ("powerpc/pseries: Partition migration in the kernel")
Signed-off-by: Nathan Lynch <redacted>

Audited the callbacks of of_reconfig_notify(). We are fine with
respect to CPU-Hotplug locking.

Reviewed-by: Gautham R. Shenoy <redacted>

-- 
Thanks and Regards
gautham.