[PATCH] riscv: don't stop itself in smp_send_stop

[PATCH] riscv: don't stop itself in smp_send_stop

[Andreas Schwab]

Only run ipi_stop on other cpus, not itself.  Don't wait for ipi_stop to
return, since it never does.  Also, mark each cpu offline.

Signed-off-by: Andreas Schwab <schwab@suse.de>
---
 arch/riscv/kernel/smp.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
index 57b1383e5e..5c6bb5fe5a 100644
--- a/arch/riscv/kernel/smp.c
+++ b/arch/riscv/kernel/smp.c
@@ -23,6 +23,7 @@
 #include <linux/smp.h>
 #include <linux/sched.h>
 #include <linux/seq_file.h>
+#include <linux/delay.h>
 
 #include <asm/sbi.h>
 #include <asm/tlbflush.h>
@@ -148,13 +149,27 @@ void arch_send_call_function_single_ipi(int cpu)
 
 static void ipi_stop(void *unused)
 {
+	set_cpu_online (smp_processor_id(), false);
 	while (1)
 		wait_for_interrupt();
 }
 
 void smp_send_stop(void)
 {
-	on_each_cpu(ipi_stop, NULL, 1);
+	unsigned long timeout;
+	struct cpumask mask;
+
+	cpumask_copy (&mask, cpu_online_mask);
+	cpumask_clear_cpu (smp_processor_id (), &mask);
+	on_each_cpu_mask(&mask, ipi_stop, NULL, 0);
+
+	/* Wait up to one second for other CPUs to stop */
+	timeout = USEC_PER_SEC;
+	while (num_online_cpus() > 1 && timeout--)
+		udelay(1);
+
+	if (num_online_cpus() > 1)
+		pr_warn("SMP: failed to stop secondary CPUs\n");
 }
 
 void smp_send_reschedule(int cpu)
-- 
2.19.2


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: don't stop itself in smp_send_stop

[Atish Patra]

On 11/29/18 8:32 AM, Andreas Schwab wrote:
> Only run ipi_stop on other cpus, not itself.  Don't wait for ipi_stop to
> return, since it never does.  Also, mark each cpu offline.
> 
> Signed-off-by: Andreas Schwab <schwab@suse.de>
> ---
>   arch/riscv/kernel/smp.c | 17 ++++++++++++++++-
>   1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
> index 57b1383e5e..5c6bb5fe5a 100644
> --- a/arch/riscv/kernel/smp.c
> +++ b/arch/riscv/kernel/smp.c
> @@ -23,6 +23,7 @@
>   #include <linux/smp.h>
>   #include <linux/sched.h>
>   #include <linux/seq_file.h>
> +#include <linux/delay.h>
>   
>   #include <asm/sbi.h>
>   #include <asm/tlbflush.h>
> @@ -148,13 +149,27 @@ void arch_send_call_function_single_ipi(int cpu)
>   
>   static void ipi_stop(void *unused)
>   {
> +	set_cpu_online (smp_processor_id(), false);
>   	while (1)
>   		wait_for_interrupt();
>   }
>   
>   void smp_send_stop(void)
>   {
> -	on_each_cpu(ipi_stop, NULL, 1);
> +	unsigned long timeout;
> +	struct cpumask mask;
> +

No need to do mask copy this, if num_online_cpus() == 1.

> +	cpumask_copy (&mask, cpu_online_mask);
> +	cpumask_clear_cpu (smp_processor_id (), &mask);
> +	on_each_cpu_mask(&mask, ipi_stop, NULL, 0);

The function header of on_each_cpu_mask says func should be 
non-blocking. Not sure if that's a hard requirement or not.

If it is, we can introduce an extra IPI_STOP in ipi_names and just send 
ipi. The receiving CPU can handle the IPI_STOP.

> +
> +	/* Wait up to one second for other CPUs to stop */
> +	timeout = USEC_PER_SEC;
> +	while (num_online_cpus() > 1 && timeout--)
> +		udelay(1);
> +
> +	if (num_online_cpus() > 1)
> +		pr_warn("SMP: failed to stop secondary CPUs\n");

May be print the remaining online cpus so that we know which one failed ?

Regards,
Atish
>   }
>   
>   void smp_send_reschedule(int cpu)
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: don't stop itself in smp_send_stop

[Andreas Schwab]

On Nov 29 2018, Atish Patra <atish.patra@wdc.com> wrote:

>> +	cpumask_copy (&mask, cpu_online_mask);
>> +	cpumask_clear_cpu (smp_processor_id (), &mask);
>> +	on_each_cpu_mask(&mask, ipi_stop, NULL, 0);
>
> The function header of on_each_cpu_mask says func should be
> non-blocking. Not sure if that's a hard requirement or not.

What does that mean?

>> +
>> +	/* Wait up to one second for other CPUs to stop */
>> +	timeout = USEC_PER_SEC;
>> +	while (num_online_cpus() > 1 && timeout--)
>> +		udelay(1);
>> +
>> +	if (num_online_cpus() > 1)
>> +		pr_warn("SMP: failed to stop secondary CPUs\n");
>
> May be print the remaining online cpus so that we know which one failed ?

I have copied that from arm.  The only user of smp_send_stop is panic,
which can cope with that, and the probability of that happening is very
low anyway.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: don't stop itself in smp_send_stop

[Atish Patra]

On 12/10/18 2:58 AM, Andreas Schwab wrote:
> On Nov 29 2018, Atish Patra <atish.patra@wdc.com> wrote:
> 
>>> +	cpumask_copy (&mask, cpu_online_mask);
>>> +	cpumask_clear_cpu (smp_processor_id (), &mask);
>>> +	on_each_cpu_mask(&mask, ipi_stop, NULL, 0);
>>
>> The function header of on_each_cpu_mask says func should be
>> non-blocking. Not sure if that's a hard requirement or not.
> 
> What does that mean?
> 

https://elixir.bootlin.com/linux/v4.20-rc6/source/kernel/smp.c#L617

>>> +
>>> +	/* Wait up to one second for other CPUs to stop */
>>> +	timeout = USEC_PER_SEC;
>>> +	while (num_online_cpus() > 1 && timeout--)
>>> +		udelay(1);
>>> +
>>> +	if (num_online_cpus() > 1)
>>> +		pr_warn("SMP: failed to stop secondary CPUs\n");
>>
>> May be print the remaining online cpus so that we know which one failed ?
> 
> I have copied that from arm.  The only user of smp_send_stop is panic,
> which can cope with that, and the probability of that happening is very
> low anyway.
> 

My suggestion was based on arm64 code. IMHO, it makes more sense to 
follow arm64 code than arm.

https://elixir.bootlin.com/linux/v4.20-rc6/source/arch/arm64/kernel/smp.c#L955


Regards,
Atish
> Andreas.
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v2] riscv: don't stop itself in smp_send_stop

[Andreas Schwab]

Add IPI_CPU_STOP message and use it in smp_send_stop to stop other cpus,
but not itself.  Mark cpu offline on reception of IPI_CPU_STOP.

Signed-off-by: Andreas Schwab <schwab@suse.de>
---
 arch/riscv/kernel/smp.c | 43 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
index 57b1383e5e..b813264416 100644
--- a/arch/riscv/kernel/smp.c
+++ b/arch/riscv/kernel/smp.c
@@ -23,6 +23,7 @@
 #include <linux/smp.h>
 #include <linux/sched.h>
 #include <linux/seq_file.h>
+#include <linux/delay.h>
 
 #include <asm/sbi.h>
 #include <asm/tlbflush.h>
@@ -31,6 +32,7 @@
 enum ipi_message_type {
 	IPI_RESCHEDULE,
 	IPI_CALL_FUNC,
+	IPI_CPU_STOP,
 	IPI_MAX
 };
 
@@ -66,6 +68,13 @@ int setup_profiling_timer(unsigned int multiplier)
 	return -EINVAL;
 }
 
+static void ipi_stop(void)
+{
+	set_cpu_online (smp_processor_id(), false);
+	while (1)
+		wait_for_interrupt();
+}
+
 void riscv_software_interrupt(void)
 {
 	unsigned long *pending_ipis = &ipi_data[smp_processor_id()].bits;
@@ -94,6 +103,11 @@ void riscv_software_interrupt(void)
 			generic_smp_call_function_interrupt();
 		}
 
+		if (ops & (1 << IPI_CPU_STOP)) {
+			stats[IPI_CPU_STOP]++;
+			ipi_stop();
+		}
+
 		BUG_ON((ops >> IPI_MAX) != 0);
 
 		/* Order data access and bit testing. */
@@ -121,6 +135,7 @@ send_ipi_message(const struct cpumask *to_whom, enum ipi_message_type operation)
 static const char * const ipi_names[] = {
 	[IPI_RESCHEDULE]	= "Rescheduling interrupts",
 	[IPI_CALL_FUNC]		= "Function call interrupts",
+	[IPI_CPU_STOP]		= "CPU stop interrupts",
 };
 
 void show_ipi_stats(struct seq_file *p, int prec)
@@ -146,15 +161,29 @@ void arch_send_call_function_single_ipi(int cpu)
 	send_ipi_message(cpumask_of(cpu), IPI_CALL_FUNC);
 }
 
-static void ipi_stop(void *unused)
-{
-	while (1)
-		wait_for_interrupt();
-}
-
 void smp_send_stop(void)
 {
-	on_each_cpu(ipi_stop, NULL, 1);
+	unsigned long timeout;
+
+	if (num_online_cpus() > 1) {
+		cpumask_t mask;
+
+		cpumask_copy(&mask, cpu_online_mask);
+		cpumask_clear_cpu(smp_processor_id(), &mask);
+
+		if (system_state <= SYSTEM_RUNNING)
+			pr_crit("SMP: stopping secondary CPUs\n");
+		send_ipi_message(&mask, IPI_CPU_STOP);
+	}
+
+	/* Wait up to one second for other CPUs to stop */
+	timeout = USEC_PER_SEC;
+	while (num_online_cpus() > 1 && timeout--)
+		udelay(1);
+
+	if (num_online_cpus() > 1)
+		pr_warning("SMP: failed to stop secondary CPUs %*pbl\n",
+			   cpumask_pr_args(cpu_online_mask));
 }
 
 void smp_send_reschedule(int cpu)
-- 
2.20.0


-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] riscv: don't stop itself in smp_send_stop

[Atish Patra]

On 12/11/18 2:20 AM, Andreas Schwab wrote:
> Add IPI_CPU_STOP message and use it in smp_send_stop to stop other cpus,
> but not itself.  Mark cpu offline on reception of IPI_CPU_STOP.
> 
> Signed-off-by: Andreas Schwab <schwab@suse.de>
> ---
>   arch/riscv/kernel/smp.c | 43 ++++++++++++++++++++++++++++++++++-------
>   1 file changed, 36 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
> index 57b1383e5e..b813264416 100644
> --- a/arch/riscv/kernel/smp.c
> +++ b/arch/riscv/kernel/smp.c
> @@ -23,6 +23,7 @@
>   #include <linux/smp.h>
>   #include <linux/sched.h>
>   #include <linux/seq_file.h>
> +#include <linux/delay.h>
>   
>   #include <asm/sbi.h>
>   #include <asm/tlbflush.h>
> @@ -31,6 +32,7 @@
>   enum ipi_message_type {
>   	IPI_RESCHEDULE,
>   	IPI_CALL_FUNC,
> +	IPI_CPU_STOP,
>   	IPI_MAX
>   };
>   
> @@ -66,6 +68,13 @@ int setup_profiling_timer(unsigned int multiplier)
>   	return -EINVAL;
>   }
>   
> +static void ipi_stop(void)
> +{
> +	set_cpu_online (smp_processor_id(), false);
> +	while (1)
> +		wait_for_interrupt();
> +}
> +
>   void riscv_software_interrupt(void)
>   {
>   	unsigned long *pending_ipis = &ipi_data[smp_processor_id()].bits;
> @@ -94,6 +103,11 @@ void riscv_software_interrupt(void)
>   			generic_smp_call_function_interrupt();
>   		}
>   
> +		if (ops & (1 << IPI_CPU_STOP)) {
> +			stats[IPI_CPU_STOP]++;
> +			ipi_stop();
> +		}
> +
>   		BUG_ON((ops >> IPI_MAX) != 0);
>   
>   		/* Order data access and bit testing. */
> @@ -121,6 +135,7 @@ send_ipi_message(const struct cpumask *to_whom, enum ipi_message_type operation)
>   static const char * const ipi_names[] = {
>   	[IPI_RESCHEDULE]	= "Rescheduling interrupts",
>   	[IPI_CALL_FUNC]		= "Function call interrupts",
> +	[IPI_CPU_STOP]		= "CPU stop interrupts",
>   };
>   
>   void show_ipi_stats(struct seq_file *p, int prec)
> @@ -146,15 +161,29 @@ void arch_send_call_function_single_ipi(int cpu)
>   	send_ipi_message(cpumask_of(cpu), IPI_CALL_FUNC);
>   }
>   
> -static void ipi_stop(void *unused)
> -{
> -	while (1)
> -		wait_for_interrupt();
> -}
> -
>   void smp_send_stop(void)
>   {
> -	on_each_cpu(ipi_stop, NULL, 1);
> +	unsigned long timeout;
> +
> +	if (num_online_cpus() > 1) {
> +		cpumask_t mask;
> +
> +		cpumask_copy(&mask, cpu_online_mask);
> +		cpumask_clear_cpu(smp_processor_id(), &mask);
> +
> +		if (system_state <= SYSTEM_RUNNING)
> +			pr_crit("SMP: stopping secondary CPUs\n");
> +		send_ipi_message(&mask, IPI_CPU_STOP);
> +	}
> +
> +	/* Wait up to one second for other CPUs to stop */
> +	timeout = USEC_PER_SEC;
> +	while (num_online_cpus() > 1 && timeout--)
> +		udelay(1);
> +
> +	if (num_online_cpus() > 1)
> +		pr_warning("SMP: failed to stop secondary CPUs %*pbl\n",
> +			   cpumask_pr_args(cpu_online_mask));
>   }
>   
>   void smp_send_reschedule(int cpu)
> 

Checkpatch warnings.
WARNING: space prohibited between function name and open parenthesis '('
#40: FILE: arch/riscv/kernel/smp.c:73:
+       set_cpu_online (smp_processor_id(), false);

WARNING: Prefer pr_warn(... to pr_warning(...
#100: FILE: arch/riscv/kernel/smp.c:185:
+               pr_warning("SMP: failed to stop secondary CPUs %*pbl\n",


Otherwise, Looks good to me.
Reviewed-by: Atish Patra <atish.patra@wdc.com>

Regards,
Atish

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] riscv: don't stop itself in smp_send_stop

[Palmer Dabbelt]

On Thu, 13 Dec 2018 15:27:36 PST (-0800), atish.patra@wdc.com wrote:
> On 12/11/18 2:20 AM, Andreas Schwab wrote:
>> Add IPI_CPU_STOP message and use it in smp_send_stop to stop other cpus,
>> but not itself.  Mark cpu offline on reception of IPI_CPU_STOP.
>>
>> Signed-off-by: Andreas Schwab <schwab@suse.de>
>> ---
>>   arch/riscv/kernel/smp.c | 43 ++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 36 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
>> index 57b1383e5e..b813264416 100644
>> --- a/arch/riscv/kernel/smp.c
>> +++ b/arch/riscv/kernel/smp.c
>> @@ -23,6 +23,7 @@
>>   #include <linux/smp.h>
>>   #include <linux/sched.h>
>>   #include <linux/seq_file.h>
>> +#include <linux/delay.h>
>>
>>   #include <asm/sbi.h>
>>   #include <asm/tlbflush.h>
>> @@ -31,6 +32,7 @@
>>   enum ipi_message_type {
>>   	IPI_RESCHEDULE,
>>   	IPI_CALL_FUNC,
>> +	IPI_CPU_STOP,
>>   	IPI_MAX
>>   };
>>
>> @@ -66,6 +68,13 @@ int setup_profiling_timer(unsigned int multiplier)
>>   	return -EINVAL;
>>   }
>>
>> +static void ipi_stop(void)
>> +{
>> +	set_cpu_online (smp_processor_id(), false);
>> +	while (1)
>> +		wait_for_interrupt();
>> +}
>> +
>>   void riscv_software_interrupt(void)
>>   {
>>   	unsigned long *pending_ipis = &ipi_data[smp_processor_id()].bits;
>> @@ -94,6 +103,11 @@ void riscv_software_interrupt(void)
>>   			generic_smp_call_function_interrupt();
>>   		}
>>
>> +		if (ops & (1 << IPI_CPU_STOP)) {
>> +			stats[IPI_CPU_STOP]++;
>> +			ipi_stop();
>> +		}
>> +
>>   		BUG_ON((ops >> IPI_MAX) != 0);
>>
>>   		/* Order data access and bit testing. */
>> @@ -121,6 +135,7 @@ send_ipi_message(const struct cpumask *to_whom, enum ipi_message_type operation)
>>   static const char * const ipi_names[] = {
>>   	[IPI_RESCHEDULE]	= "Rescheduling interrupts",
>>   	[IPI_CALL_FUNC]		= "Function call interrupts",
>> +	[IPI_CPU_STOP]		= "CPU stop interrupts",
>>   };
>>
>>   void show_ipi_stats(struct seq_file *p, int prec)
>> @@ -146,15 +161,29 @@ void arch_send_call_function_single_ipi(int cpu)
>>   	send_ipi_message(cpumask_of(cpu), IPI_CALL_FUNC);
>>   }
>>
>> -static void ipi_stop(void *unused)
>> -{
>> -	while (1)
>> -		wait_for_interrupt();
>> -}
>> -
>>   void smp_send_stop(void)
>>   {
>> -	on_each_cpu(ipi_stop, NULL, 1);
>> +	unsigned long timeout;
>> +
>> +	if (num_online_cpus() > 1) {
>> +		cpumask_t mask;
>> +
>> +		cpumask_copy(&mask, cpu_online_mask);
>> +		cpumask_clear_cpu(smp_processor_id(), &mask);
>> +
>> +		if (system_state <= SYSTEM_RUNNING)
>> +			pr_crit("SMP: stopping secondary CPUs\n");
>> +		send_ipi_message(&mask, IPI_CPU_STOP);
>> +	}
>> +
>> +	/* Wait up to one second for other CPUs to stop */
>> +	timeout = USEC_PER_SEC;
>> +	while (num_online_cpus() > 1 && timeout--)
>> +		udelay(1);
>> +
>> +	if (num_online_cpus() > 1)
>> +		pr_warning("SMP: failed to stop secondary CPUs %*pbl\n",
>> +			   cpumask_pr_args(cpu_online_mask));
>>   }
>>
>>   void smp_send_reschedule(int cpu)
>>
>
> Checkpatch warnings.
> WARNING: space prohibited between function name and open parenthesis '('
> #40: FILE: arch/riscv/kernel/smp.c:73:
> +       set_cpu_online (smp_processor_id(), false);
>
> WARNING: Prefer pr_warn(... to pr_warning(...
> #100: FILE: arch/riscv/kernel/smp.c:185:
> +               pr_warning("SMP: failed to stop secondary CPUs %*pbl\n",
>
>
> Otherwise, Looks good to me.
> Reviewed-by: Atish Patra <atish.patra@wdc.com>

I've fixed these up and added the patch to my fix queue, it should go up next 
week.

Thanks!

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv