From: Laurent Dufour <ldufour@linux.ibm.com> To: Nathan Lynch <nathanl@linux.ibm.com> Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, mpe@ellerman.id.au, benh@kernel.crashing.org, paulus@samba.org, haren@linux.vnet.ibm.com, npiggin@gmail.com Subject: Re: [PATCH v2 4/4] pseries/mobility: Set NMI watchdog factor during LPM Date: Fri, 24 Jun 2022 16:09:34 +0200 [thread overview] Message-ID: <040a8f52-980f-146c-6811-9a0ce9157f08@linux.ibm.com> (raw) In-Reply-To: <87sfnvmgql.fsf@linux.ibm.com> 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.
WARNING: multiple messages have this Message-ID (diff)
From: Laurent Dufour <ldufour@linux.ibm.com> To: Nathan Lynch <nathanl@linux.ibm.com> Cc: linux-kernel@vger.kernel.org, npiggin@gmail.com, paulus@samba.org, linuxppc-dev@lists.ozlabs.org, haren@linux.vnet.ibm.com Subject: Re: [PATCH v2 4/4] pseries/mobility: Set NMI watchdog factor during LPM Date: Fri, 24 Jun 2022 16:09:34 +0200 [thread overview] Message-ID: <040a8f52-980f-146c-6811-9a0ce9157f08@linux.ibm.com> (raw) In-Reply-To: <87sfnvmgql.fsf@linux.ibm.com> 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.
next prev parent reply other threads:[~2022-06-24 14:10 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-06-14 13:54 [PATCH v2 0/4] Extending NMI watchdog during LPM Laurent Dufour 2022-06-14 13:54 ` [PATCH v2 1/4] powerpc/mobility: Wait for memory transfer to complete Laurent Dufour 2022-06-21 16:52 ` Nathan Lynch 2022-06-14 13:54 ` [PATCH v2 2/4] watchdog: export watchdog_mutex and lockup_detector_reconfigure Laurent Dufour 2022-06-24 6:31 ` Michael Ellerman 2022-06-24 8:27 ` Laurent Dufour 2022-06-24 9:37 ` Christoph Hellwig 2022-06-24 9:37 ` Christoph Hellwig 2022-06-24 12:45 ` Laurent Dufour 2022-06-24 12:45 ` Laurent Dufour 2022-06-14 13:54 ` [PATCH v2 3/4] powerpc/watchdog: introduce a LPM factor Laurent Dufour 2022-06-22 9:26 ` kernel test robot 2022-06-22 9:26 ` kernel test robot 2022-06-14 13:54 ` [PATCH v2 4/4] pseries/mobility: Set NMI watchdog factor during LPM Laurent Dufour 2022-06-23 17:28 ` Nathan Lynch 2022-06-23 17:28 ` Nathan Lynch 2022-06-24 14:09 ` Laurent Dufour [this message] 2022-06-24 14:09 ` Laurent Dufour
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=040a8f52-980f-146c-6811-9a0ce9157f08@linux.ibm.com \ --to=ldufour@linux.ibm.com \ --cc=benh@kernel.crashing.org \ --cc=haren@linux.vnet.ibm.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mpe@ellerman.id.au \ --cc=nathanl@linux.ibm.com \ --cc=npiggin@gmail.com \ --cc=paulus@samba.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.