* [PATCH] powerpc: Fix smp_send_stop NMI IPI handling
@ 2018-04-25 2:17 Nicholas Piggin
2018-04-25 3:15 ` Michael Ellerman
0 siblings, 1 reply; 4+ messages in thread
From: Nicholas Piggin @ 2018-04-25 2:17 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Nicholas Piggin, Abdul Haleem
The NMI IPI handler for a receiving CPU increments nmi_ipi_busy_count
over the handler function call, which causes later smp_send_nmi_ipi()
callers to spin until the call is finished.
The smp_send_stop function never returns, so the busy count is never
decremeted, which can cause the system to hang in some cases. For
example panic() will call smp_send_stop early on, then later in the
reboot path, pnv_restart will call smp_send_stop again, which hangs.
Fix this by adding a special case to the smp_send_stop handler to
decrement the busy count, because it will never return.
Fixes: 6bed3237624e3 ("powerpc: use NMI IPI for smp_send_stop")
Reported-by: Abdul Haleem <abdhalee@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/smp.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index e16ec7b3b427..250fccf04c6e 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -567,10 +567,19 @@ void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
#ifdef CONFIG_NMI_IPI
static void stop_this_cpu(struct pt_regs *regs)
+{
+ /*
+ * This is a special case because it never returns, so the NMI IPI
+ * handling would never mark it as done, which makes any later
+ * smp_send_nmi_ipi() call spin forever. Mark it done now.
+ */
+ nmi_ipi_lock();
+ nmi_ipi_busy_count--;
+ nmi_ipi_unlock();
#else
static void stop_this_cpu(void *dummy)
-#endif
{
+#endif
/* Remove this CPU */
set_cpu_online(smp_processor_id(), false);
--
2.17.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc: Fix smp_send_stop NMI IPI handling
2018-04-25 2:17 [PATCH] powerpc: Fix smp_send_stop NMI IPI handling Nicholas Piggin
@ 2018-04-25 3:15 ` Michael Ellerman
2018-04-25 3:51 ` Nicholas Piggin
0 siblings, 1 reply; 4+ messages in thread
From: Michael Ellerman @ 2018-04-25 3:15 UTC (permalink / raw)
To: Nicholas Piggin, linuxppc-dev; +Cc: Abdul Haleem, Nicholas Piggin
Nicholas Piggin <npiggin@gmail.com> writes:
> The NMI IPI handler for a receiving CPU increments nmi_ipi_busy_count
> over the handler function call, which causes later smp_send_nmi_ipi()
> callers to spin until the call is finished.
>
> The smp_send_stop function never returns, so the busy count is never
> decremeted, which can cause the system to hang in some cases. For
> example panic() will call smp_send_stop early on, then later in the
> reboot path, pnv_restart will call smp_send_stop again, which hangs.
>
> Fix this by adding a special case to the smp_send_stop handler to
> decrement the busy count, because it will never return.
>
> Fixes: 6bed3237624e3 ("powerpc: use NMI IPI for smp_send_stop")
> Reported-by: Abdul Haleem <abdhalee@linux.vnet.ibm.com>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> arch/powerpc/kernel/smp.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index e16ec7b3b427..250fccf04c6e 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -567,10 +567,19 @@ void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
>
> #ifdef CONFIG_NMI_IPI
> static void stop_this_cpu(struct pt_regs *regs)
> +{
> + /*
> + * This is a special case because it never returns, so the NMI IPI
> + * handling would never mark it as done, which makes any later
> + * smp_send_nmi_ipi() call spin forever. Mark it done now.
> + */
> + nmi_ipi_lock();
> + nmi_ipi_busy_count--;
> + nmi_ipi_unlock();
> #else
> static void stop_this_cpu(void *dummy)
> -#endif
> {
> +#endif
I don't love this ifdef/endif business.
Can we do it this way instead?
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index e16ec7b3b427..3582f30b60b7 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -565,11 +565,7 @@ void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
}
#endif
-#ifdef CONFIG_NMI_IPI
-static void stop_this_cpu(struct pt_regs *regs)
-#else
static void stop_this_cpu(void *dummy)
-#endif
{
/* Remove this CPU */
set_cpu_online(smp_processor_id(), false);
@@ -580,10 +576,26 @@ static void stop_this_cpu(void *dummy)
spin_cpu_relax();
}
+#ifdef CONFIG_NMI_IPI
+static void nmi_stop_this_cpu(struct pt_regs *regs)
+{
+ /*
+ * This is a special case because it never returns, so the NMI IPI
+ * handling would never mark it as done, which makes any later
+ * smp_send_nmi_ipi() call spin forever. Mark it done now.
+ */
+ nmi_ipi_lock();
+ nmi_ipi_busy_count--;
+ nmi_ipi_unlock();
+
+ stop_this_cpu(NULL);
+}
+#endif
+
void smp_send_stop(void)
{
#ifdef CONFIG_NMI_IPI
- smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, stop_this_cpu, 1000000);
+ smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, nmi_stop_this_cpu, 1000000);
#else
smp_call_function(stop_this_cpu, NULL, 0);
#endif
cheers
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc: Fix smp_send_stop NMI IPI handling
2018-04-25 3:15 ` Michael Ellerman
@ 2018-04-25 3:51 ` Nicholas Piggin
2018-04-26 9:47 ` Michael Ellerman
0 siblings, 1 reply; 4+ messages in thread
From: Nicholas Piggin @ 2018-04-25 3:51 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, Abdul Haleem
On Wed, 25 Apr 2018 13:15:34 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
>
> > The NMI IPI handler for a receiving CPU increments nmi_ipi_busy_count
> > over the handler function call, which causes later smp_send_nmi_ipi()
> > callers to spin until the call is finished.
> >
> > The smp_send_stop function never returns, so the busy count is never
> > decremeted, which can cause the system to hang in some cases. For
> > example panic() will call smp_send_stop early on, then later in the
> > reboot path, pnv_restart will call smp_send_stop again, which hangs.
> >
> > Fix this by adding a special case to the smp_send_stop handler to
> > decrement the busy count, because it will never return.
> >
> > Fixes: 6bed3237624e3 ("powerpc: use NMI IPI for smp_send_stop")
> > Reported-by: Abdul Haleem <abdhalee@linux.vnet.ibm.com>
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> > arch/powerpc/kernel/smp.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> > index e16ec7b3b427..250fccf04c6e 100644
> > --- a/arch/powerpc/kernel/smp.c
> > +++ b/arch/powerpc/kernel/smp.c
> > @@ -567,10 +567,19 @@ void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
> >
> > #ifdef CONFIG_NMI_IPI
> > static void stop_this_cpu(struct pt_regs *regs)
> > +{
> > + /*
> > + * This is a special case because it never returns, so the NMI IPI
> > + * handling would never mark it as done, which makes any later
> > + * smp_send_nmi_ipi() call spin forever. Mark it done now.
> > + */
> > + nmi_ipi_lock();
> > + nmi_ipi_busy_count--;
> > + nmi_ipi_unlock();
> > #else
> > static void stop_this_cpu(void *dummy)
> > -#endif
> > {
> > +#endif
>
> I don't love this ifdef/endif business.
>
> Can we do it this way instead?
Yeah that's better. Does stop_this_cpu give you an unused function warning
if you compile with NMI though? I think we need an #if/#else
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index e16ec7b3b427..3582f30b60b7 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -565,11 +565,7 @@ void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
> }
> #endif
>
> -#ifdef CONFIG_NMI_IPI
> -static void stop_this_cpu(struct pt_regs *regs)
> -#else
> static void stop_this_cpu(void *dummy)
> -#endif
> {
> /* Remove this CPU */
> set_cpu_online(smp_processor_id(), false);
> @@ -580,10 +576,26 @@ static void stop_this_cpu(void *dummy)
> spin_cpu_relax();
> }
>
> +#ifdef CONFIG_NMI_IPI
> +static void nmi_stop_this_cpu(struct pt_regs *regs)
> +{
> + /*
> + * This is a special case because it never returns, so the NMI IPI
> + * handling would never mark it as done, which makes any later
> + * smp_send_nmi_ipi() call spin forever. Mark it done now.
> + */
> + nmi_ipi_lock();
> + nmi_ipi_busy_count--;
> + nmi_ipi_unlock();
> +
> + stop_this_cpu(NULL);
> +}
> +#endif
> +
> void smp_send_stop(void)
> {
> #ifdef CONFIG_NMI_IPI
> - smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, stop_this_cpu, 1000000);
> + smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, nmi_stop_this_cpu, 1000000);
> #else
> smp_call_function(stop_this_cpu, NULL, 0);
> #endif
>
>
> cheers
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] powerpc: Fix smp_send_stop NMI IPI handling
2018-04-25 3:51 ` Nicholas Piggin
@ 2018-04-26 9:47 ` Michael Ellerman
0 siblings, 0 replies; 4+ messages in thread
From: Michael Ellerman @ 2018-04-26 9:47 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev, Abdul Haleem
Nicholas Piggin <npiggin@gmail.com> writes:
> On Wed, 25 Apr 2018 13:15:34 +1000
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>>
>> > The NMI IPI handler for a receiving CPU increments nmi_ipi_busy_count
>> > over the handler function call, which causes later smp_send_nmi_ipi()
>> > callers to spin until the call is finished.
>> >
>> > The smp_send_stop function never returns, so the busy count is never
>> > decremeted, which can cause the system to hang in some cases. For
>> > example panic() will call smp_send_stop early on, then later in the
>> > reboot path, pnv_restart will call smp_send_stop again, which hangs.
>> >
>> > Fix this by adding a special case to the smp_send_stop handler to
>> > decrement the busy count, because it will never return.
>> >
>> > Fixes: 6bed3237624e3 ("powerpc: use NMI IPI for smp_send_stop")
>> > Reported-by: Abdul Haleem <abdhalee@linux.vnet.ibm.com>
>> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> > ---
>> > arch/powerpc/kernel/smp.c | 11 ++++++++++-
>> > 1 file changed, 10 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
>> > index e16ec7b3b427..250fccf04c6e 100644
>> > --- a/arch/powerpc/kernel/smp.c
>> > +++ b/arch/powerpc/kernel/smp.c
>> > @@ -567,10 +567,19 @@ void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
>> >
>> > #ifdef CONFIG_NMI_IPI
>> > static void stop_this_cpu(struct pt_regs *regs)
>> > +{
>> > + /*
>> > + * This is a special case because it never returns, so the NMI IPI
>> > + * handling would never mark it as done, which makes any later
>> > + * smp_send_nmi_ipi() call spin forever. Mark it done now.
>> > + */
>> > + nmi_ipi_lock();
>> > + nmi_ipi_busy_count--;
>> > + nmi_ipi_unlock();
>> > #else
>> > static void stop_this_cpu(void *dummy)
>> > -#endif
>> > {
>> > +#endif
>>
>> I don't love this ifdef/endif business.
>>
>> Can we do it this way instead?
>
> Yeah that's better. Does stop_this_cpu give you an unused function warning
> if you compile with NMI though? I think we need an #if/#else
No because it's called from nmi_stop_this_cpu():
>> +#ifdef CONFIG_NMI_IPI
>> +static void nmi_stop_this_cpu(struct pt_regs *regs)
>> +{
>> + /*
>> + * This is a special case because it never returns, so the NMI IPI
>> + * handling would never mark it as done, which makes any later
>> + * smp_send_nmi_ipi() call spin forever. Mark it done now.
>> + */
>> + nmi_ipi_lock();
>> + nmi_ipi_busy_count--;
>> + nmi_ipi_unlock();
>> +
>> + stop_this_cpu(NULL);
>> +}
>> +#endif
cheers
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-04-26 9:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-04-25 2:17 [PATCH] powerpc: Fix smp_send_stop NMI IPI handling Nicholas Piggin
2018-04-25 3:15 ` Michael Ellerman
2018-04-25 3:51 ` Nicholas Piggin
2018-04-26 9:47 ` Michael Ellerman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).