lkml.org 
[lkml]   [2022]   [Jun]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v2 4/4] pseries/mobility: Set NMI watchdog factor during LPM
From
On 23/06/2022, 19:28:34, Nathan Lynch wrote:
> Laurent Dufour <ldufour@linux.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.

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

>
>> + .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.

\
 
 \ /
  Last update: 2022-06-24 16:11    [W:2.082 / U:6.964 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site