Re: stop_this_cpu - redundant code?

[Ralf Baechle <ralf@linux-mips.org>]

On Fri, Oct 17, 2008 at 07:57:12PM -0700, Anirban Sinha wrote:

> This function  (stop_this_cpu) in /arch/mips/kernel/smp.c does a
> local_irq_enable() and the adjacent comment says that it's because it
> may need to service _machine_restart IPI. Unfortunately,
> smp_call_function only sends IPIs to cores that are still online ( it
> uses the cpu_online_map U all_but_myself_map in
> smp_call_function_map()).

Usually a system would be restarted through some hardware mechanism -
probably a reset - anyway.

> So the bottom-line is, should we still keep the local irqs enabled or is
> this code totally redundant? I have seen other similar functions in
> other archs where they actually disable the local irqs.

You're right.  The code is ancient old and once uppon a time it made sense
to do things this way but the MIPS version was never updates.  Stop_this_cpu
also should try to minimize the power consumption by using the WAIT
instruction or whatever else a particular process has to offer.

I didn't try to optimize this for the 34K where a TC could try to halt
itself - there isn't really a point, I think.

A few other architectures are explicitly disabling interrupts but that's
also redundant because smp_call_function() invokes the function on other
processors with interrupts disabled.

Thanks for posting this,

  Ralf

Signed-off-by: Ralf Baechle <ralf@linux-mips.org>

diff --git a/arch/mips/kernel/smp.c b/arch/mips/kernel/smp.c
index 7b59cfb..b79ea70 100644
--- a/arch/mips/kernel/smp.c
+++ b/arch/mips/kernel/smp.c
@@ -163,8 +163,10 @@ static void stop_this_cpu(void *dummy)
 	 * Remove this CPU:
 	 */
 	cpu_clear(smp_processor_id(), cpu_online_map);
-	local_irq_enable();	/* May need to service _machine_restart IPI */
-	for (;;);		/* Wait if available. */
+	for (;;) {
+		if (cpu_wait)
+			(*cpu_wait)();		/* Wait if available. */
+	}
 }
 
 void smp_send_stop(void)