On 23/06/2022, 19:28:34, Nathan Lynch wrote:

Laurent Dufour [off-list ref] writes:

quoted

diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index 179bbd4ae881..4284ceaf9060 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c

@@ -48,6 +48,39 @@ struct update_props_workarea {
 #define MIGRATION_SCOPE	(1)
 #define PRRN_SCOPE -2
 
+#ifdef CONFIG_PPC_WATCHDOG
+static unsigned int lpm_nmi_wd_factor = 200;
+
+#ifdef CONFIG_SYSCTL
+static struct ctl_table lpm_nmi_wd_factor_ctl_table[] = {
+	{
+		.procname	= "lpm_nmi_watchdog_factor",

Assuming the basic idea is acceptable, I suggest making the user-visible
name more generic (e.g. "nmi_watchdog_factor") in case it makes sense to
apply this to other contexts in the future.

Fair enough, indeed, I was wondering if "lpm" is meaningful.

quoted

+		.data		= &lpm_nmi_wd_factor,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_douintvec_minmax,
+	},
+	{}
+};
+static struct ctl_table lpm_nmi_wd_factor_sysctl_root[] = {
+	{
+		.procname       = "kernel",
+		.mode           = 0555,
+		.child          = lpm_nmi_wd_factor_ctl_table,
+	},
+	{}
+};
+
+static int __init register_lpm_nmi_wd_factor_sysctl(void)
+{
+	register_sysctl_table(lpm_nmi_wd_factor_sysctl_root);
+
+	return 0;
+}
+device_initcall(register_lpm_nmi_wd_factor_sysctl);
+#endif /* CONFIG_SYSCTL */
+#endif /* CONFIG_PPC_WATCHDOG */
+
 static int mobility_rtas_call(int token, char *buf, s32 scope)
 {
 	int rc;

@@ -702,6 +735,7 @@ static int pseries_suspend(u64 handle)
 static int pseries_migrate_partition(u64 handle)
 {
 	int ret;
+	unsigned int factor = lpm_nmi_wd_factor;
 
 	ret = wait_for_vasi_session_suspending(handle);
 	if (ret)

@@ -709,6 +743,13 @@ static int pseries_migrate_partition(u64 handle)
 
 	vas_migration_handler(VAS_SUSPEND);
 
+#ifdef CONFIG_PPC_WATCHDOG
+	if (factor) {
+		pr_info("Set the NMI watchdog factor to %u%%\n", factor);
+		watchdog_nmi_set_lpm_factor(factor);
+	}
+#endif /* CONFIG_PPC_WATCHDOG */
+
 	ret = pseries_suspend(handle);
 	if (ret == 0) {
 		post_mobility_fixup();

@@ -716,6 +757,13 @@ static int pseries_migrate_partition(u64 handle)
 	} else
 		pseries_cancel_migration(handle, ret);
 
+#ifdef CONFIG_PPC_WATCHDOG
+	if (factor) {
+		pr_info("Restoring NMI watchdog timer\n");
+		watchdog_nmi_set_lpm_factor(0);
+	}
+#endif /* CONFIG_PPC_WATCHDOG */
+

A couple more suggestions:

* Move the prints into a single statement in watchdog_nmi_set_lpm_factor().

You're right that sounds a better place.

* Add no-op versions of watchdog_nmi_set_lpm_factor for
  !CONFIG_PPC_WATCHDOG so we can minimize the #ifdef here.

Furthermore, this breaks compilation when !CONFIG_PPC_WATCHDOG because
lpm_nmi_wd_factor is not defined. I'll rework that part.

Otherwise this looks fine to me.

Thanks,
Laurent.