[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87sfnvmgql.fsf@linux.ibm.com>
Date: Thu, 23 Jun 2022 12:28:34 -0500
From: Nathan Lynch <nathanl@...ux.ibm.com>
To: Laurent Dufour <ldufour@...ux.ibm.com>
Cc: linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
mpe@...erman.id.au, benh@...nel.crashing.org, paulus@...ba.org,
haren@...ux.vnet.ibm.com, npiggin@...il.com
Subject: Re: [PATCH v2 4/4] pseries/mobility: Set NMI watchdog factor during
LPM
Laurent Dufour <ldufour@...ux.ibm.com> writes:
> 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.
> + .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().
* Add no-op versions of watchdog_nmi_set_lpm_factor for
!CONFIG_PPC_WATCHDOG so we can minimize the #ifdef here.
Otherwise this looks fine to me.
Powered by blists - more mailing lists