Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 12 Jul 2022 11:46:33 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [PATCH v3 4/4] pseries/mobility: set NMI watchdog factor during
 LPM
To:     benh@kernel.crashing.org, haren@linux.vnet.ibm.com,
        Laurent Dufour <ldufour@linux.ibm.com>, linux@roeck-us.net,
        mpe@ellerman.id.au, nathanl@linux.ibm.com, paulus@samba.org,
        wim@linux-watchdog.org
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
        linux-watchdog@vger.kernel.org
References: <20220627135347.32624-1-ldufour@linux.ibm.com>
        <20220627135347.32624-5-ldufour@linux.ibm.com>
In-Reply-To: <20220627135347.32624-5-ldufour@linux.ibm.com>
MIME-Version: 1.0
Message-Id: <1657590189.b3s2aqv3sj.astroid@bobo.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Excerpts from Laurent Dufour's message of June 27, 2022 11:53 pm:
> During a LPM, while the memory transfer is in progress on the arrival sid=
e,
> some latencies is generated when accessing not yet transferred pages on t=
he
> arrival side. Thus, the NMI watchdog may be triggered too frequently, whi=
ch
> increases the risk to hit a NMI interrupt in a bad place in the kernel,
> leading to a kernel panic.
>=20
> Disabling the Hard Lockup Watchdog until the memory transfer could be a t=
oo
> strong work around, some users would want this timeout to be eventually
> triggered if the system is hanging even during LPM.
>=20
> Introduce a new sysctl variable nmi_watchdog_factor. It allows to apply
> a factor to the NMI watchdog timeout during a LPM. Just before the CPU ar=
e
> stopped for the switchover sequence, the NMI watchdog timer is set to
>  watchdog_tresh + factor%
>=20
> A value of 0 has no effect. The default value is 200, meaning that the NM=
I
> watchdog is set to 30s during LPM (based on a 10s watchdog_tresh value).
> Once the memory transfer is achieved, the factor is reset to 0.
>=20
> Setting this value to a high number is like disabling the NMI watchdog
> during a LPM.
>=20
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
>  Documentation/admin-guide/sysctl/kernel.rst | 12 ++++++
>  arch/powerpc/platforms/pseries/mobility.c   | 43 +++++++++++++++++++++
>  2 files changed, 55 insertions(+)
>=20
> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/=
admin-guide/sysctl/kernel.rst
> index ddccd1077462..0bb0b7f27e96 100644
> --- a/Documentation/admin-guide/sysctl/kernel.rst
> +++ b/Documentation/admin-guide/sysctl/kernel.rst
> @@ -592,6 +592,18 @@ to the guest kernel command line (see
>  Documentation/admin-guide/kernel-parameters.rst).
> =20
> =20
> +nmi_watchdog_factor (PPC only)
> +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> +
> +Factor apply to to the NMI watchdog timeout (only when ``nmi_watchdog`` =
is
> +set to 1). This factor represents the percentage added to
> +``watchdog_thresh`` when calculating the NMI watchdog timeout during a
> +LPM. The soft lockup timeout is not impacted.

Could "LPM" or "mobility" be a bit more prominent in the parameter name
and documentation? Something else might want to add a factor as well,
one day.

Otherwise the code looks okay.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> +
> +A value of 0 means no change. The default value is 200 meaning the NMI
> +watchdog is set to 30s (based on ``watchdog_thresh`` equal to 10).
> +
> +
>  numa_balancing
>  =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> =20
> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/pla=
tforms/pseries/mobility.c
> index 907a779074d6..649155faafc2 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
> =20
> +#ifdef CONFIG_PPC_WATCHDOG
> +static unsigned int nmi_wd_factor =3D 200;
> +
> +#ifdef CONFIG_SYSCTL
> +static struct ctl_table nmi_wd_factor_ctl_table[] =3D {
> +	{
> +		.procname	=3D "nmi_watchdog_factor",
> +		.data		=3D &nmi_wd_factor,
> +		.maxlen		=3D sizeof(int),
> +		.mode		=3D 0644,
> +		.proc_handler	=3D proc_douintvec_minmax,
> +	},
> +	{}
> +};
> +static struct ctl_table nmi_wd_factor_sysctl_root[] =3D {
> +	{
> +		.procname       =3D "kernel",
> +		.mode           =3D 0555,
> +		.child          =3D nmi_wd_factor_ctl_table,
> +	},
> +	{}
> +};
> +
> +static int __init register_nmi_wd_factor_sysctl(void)
> +{
> +	register_sysctl_table(nmi_wd_factor_sysctl_root);
> +
> +	return 0;
> +}
> +device_initcall(register_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,13 +735,20 @@ static int pseries_suspend(u64 handle)
>  static int pseries_migrate_partition(u64 handle)
>  {
>  	int ret;
> +	unsigned int factor =3D 0;
> =20
> +#ifdef CONFIG_PPC_WATCHDOG
> +	factor =3D nmi_wd_factor;
> +#endif
>  	ret =3D wait_for_vasi_session_suspending(handle);
>  	if (ret)
>  		return ret;
> =20
>  	vas_migration_handler(VAS_SUSPEND);
> =20
> +	if (factor)
> +		watchdog_nmi_set_lpm_factor(factor);
> +
>  	ret =3D pseries_suspend(handle);
>  	if (ret =3D=3D 0) {
>  		post_mobility_fixup();
> @@ -716,6 +756,9 @@ static int pseries_migrate_partition(u64 handle)
>  	} else
>  		pseries_cancel_migration(handle, ret);
> =20
> +	if (factor)
> +		watchdog_nmi_set_lpm_factor(0);
> +
>  	vas_migration_handler(VAS_RESUME);
> =20
>  	return ret;
> --=20
> 2.36.1
>=20
>=20