[PATCH] Make smp_call_function{_single} go WARNING and return -EINVAL on !SMP (was Re: [PATCH] i386/x86_64: NMI watchdog: Protect smp_call_function() within CONFIG_SMP)

[PATCH] Make smp_call_function{_single} go WARNING and return -EINVAL on !SMP (was Re: [PATCH] i386/x86_64: NMI watchdog: Protect smp_call_function() within CONFIG_SMP)

[Satyam Sharma]

On 6/7/07, Satyam Sharma <satyam.sharma@gmail.com> wrote:
> [...]
> BTW: smp_call_function() simply returns 0 and
> smp_call_function_single() simply returns -EBUSY when !SMP.
> These appear to be just some ad hoc values. IMHO, we should
> be going BUG() in both these cases because "other" CPUs for
> !SMP are undefined / meaningless.

79974a0e4c6be6e9a3717b4c5a5d5c44c36b1653 from a couple
weeks back (discussed on http://lkml.org/lkml/2007/5/14/68 i.e.
[patch] Let smp_call_function_single return -EBUSY.) introduced
this behaviour. [ Adding Heiko Carstens, Andrew and David Miller
to Cc: list. ]

I realized a warning would be more appropriate for this case than
a BUG at the last moment ... this doesn't quite meet Linus' "You
killed my father; prepare to die!" criterion :-)

---

The smp_call_function{_single} functions are used to run
given function on all {or speicified} *other* CPUs. For
UP systems, "other" CPUs simply don't exist, so we flag
such incorrect usage of these functions using a WARNING.

Also, -EBUSY is generally returned by arch implementations
when they find that target_cpu == current_cpu, which is not
a comparable case to the !SMP case. Use -EINVAL instead,
similar to what powerpc does for !cpu_online(target), which
is somewhat more analogous.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Alan Cox <alan@redhat.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Miller <davem@davemloft.net>

---

 include/linux/smp.h |   15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

---

diff -ruNp a/include/linux/smp.h b/include/linux/smp.h
--- a/include/linux/smp.h	2007-06-07 12:46:50.000000000 +0530
+++ b/include/linux/smp.h	2007-06-07 21:13:29.000000000 +0530
@@ -6,6 +6,7 @@
  *		Alan Cox. <alan@redhat.com>
  */

+#include <linux/bug.h>
 #include <linux/errno.h>

 extern void cpu_idle(void);
@@ -84,11 +85,6 @@ void smp_prepare_boot_cpu(void);
  *	These macros fold the SMP functionality into a single CPU system
  */
 #define raw_smp_processor_id()			0
-static inline int up_smp_call_function(void)
-{
-	return 0;
-}
-#define smp_call_function(func,info,retry,wait)	(up_smp_call_function())
 #define on_each_cpu(func,info,retry,wait)	\
 	({					\
 		local_irq_disable();		\
@@ -99,10 +95,17 @@ static inline int up_smp_call_function(v
 static inline void smp_send_reschedule(int cpu) { }
 #define num_booting_cpus()			1
 #define smp_prepare_boot_cpu()			do {} while (0)
+static inline int smp_call_function(void (*func)(void *info),
+				    void *info, int retry, int wait)
+{
+	WARN_ON(1);
+	return -EINVAL;
+}
 static inline int smp_call_function_single(int cpuid, void (*func)
(void *info),
 					   void *info, int retry, int wait)
 {
-	return -EBUSY;
+	WARN_ON(1);
+	return -EINVAL;
 }

 #endif /* !SMP */

Re: [PATCH] Make smp_call_function{_single} go WARNING and return -EINVAL on !SMP (was Re: [PATCH] i386/x86_64: NMI watchdog: Protect smp_call_function() within CONFIG_SMP)

[Satyam Sharma]

[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]

> On 6/7/07, Satyam Sharma <satyam.sharma@gmail.com> wrote:
> > [...]
> > BTW: smp_call_function() simply returns 0 and
> > smp_call_function_single() simply returns -EBUSY when !SMP.
> > These appear to be just some ad hoc values. IMHO, we should
> > be going BUG() in both these cases because "other" CPUs for
> > !SMP are undefined / meaningless.
>
> 79974a0e4c6be6e9a3717b4c5a5d5c44c36b1653 from a couple
> weeks back (discussed on http://lkml.org/lkml/2007/5/14/68 i.e.
> [patch] Let smp_call_function_single return -EBUSY.) introduced
> this behaviour. [ Adding Heiko Carstens, Andrew and David Miller
> to Cc: list. ]
>
> I realized a warning would be more appropriate for this case than
> a BUG at the last moment ... this doesn't quite meet Linus' "You
> killed my father; prepare to die!" criterion :-)

Ugh, Gmail murdered this patch:

>  static inline int smp_call_function_single(int cpuid, void (*func)
> (void *info),
>                                            void *info, int retry, int wait)

Resending as attachment.

[-- Attachment #2: make-smp_call_function-go-warning-on-up.patch --]
[-- Type: text/x-patch, Size: 2287 bytes --]

[PATCH] Make smp_call_function{_single} go WARNING and return -EINVAL on !SMP

The smp_call_function{_single} functions are used to run
given function on all {or speicified} *other* CPUs. For
UP systems, "other" CPUs simply don't exist, so we flag
such incorrect usage of these functions using a WARNING.

Also, -EBUSY is generally returned by arch implementations
when they find that target_cpu == current_cpu, which is not
a comparable case to the !SMP case. Use -EINVAL instead,
similar to what powerpc does for !cpu_online(target), which
is somewhat more analogous.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Andi Kleen <andi@firstfloor.org>
Cc: Alan Cox <alan@redhat.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Miller <davem@davemloft.net>

---

 include/linux/smp.h |   17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

---

diff -ruNp a/include/linux/smp.h b/include/linux/smp.h
--- a/include/linux/smp.h	2007-06-07 12:46:50.000000000 +0530
+++ b/include/linux/smp.h	2007-06-07 22:02:34.000000000 +0530
@@ -6,6 +6,7 @@
  *		Alan Cox. <alan@redhat.com>
  */
 
+#include <linux/bug.h>
 #include <linux/errno.h>
 
 extern void cpu_idle(void);
@@ -84,11 +85,6 @@ void smp_prepare_boot_cpu(void);
  *	These macros fold the SMP functionality into a single CPU system
  */
 #define raw_smp_processor_id()			0
-static inline int up_smp_call_function(void)
-{
-	return 0;
-}
-#define smp_call_function(func,info,retry,wait)	(up_smp_call_function())
 #define on_each_cpu(func,info,retry,wait)	\
 	({					\
 		local_irq_disable();		\
@@ -99,10 +95,17 @@ static inline int up_smp_call_function(v
 static inline void smp_send_reschedule(int cpu) { }
 #define num_booting_cpus()			1
 #define smp_prepare_boot_cpu()			do {} while (0)
-static inline int smp_call_function_single(int cpuid, void (*func) (void *info),
+static inline int smp_call_function(void (*func)(void *info),
+				    void *info, int retry, int wait)
+{
+	WARN_ON(1);
+	return -EINVAL;
+}
+static inline int smp_call_function_single(int cpuid, void (*func)(void *info),
 					   void *info, int retry, int wait)
 {
-	return -EBUSY;
+	WARN_ON(1);
+	return -EINVAL;
 }
 
 #endif /* !SMP */

Re: [PATCH] Make smp_call_function{_single} go WARNING and return -EINVAL on !SMP (was Re: [PATCH] i386/x86_64: NMI watchdog: Protect smp_call_function() within CONFIG_SMP)

[Heiko Carstens]

> The smp_call_function{_single} functions are used to run
> given function on all {or speicified} *other* CPUs. For
> UP systems, "other" CPUs simply don't exist, so we flag
> such incorrect usage of these functions using a WARNING.

If other cpus don't exist then smp_call_function() should just do
*nothing* (there is no other cpu right?). We don't want to sprinkle
a ton of #ifdef CONFIG_SMP around each smp_call_function().

> Also, -EBUSY is generally returned by arch implementations
> when they find that target_cpu == current_cpu, which is not
> a comparable case to the !SMP case. Use -EINVAL instead,
> similar to what powerpc does for !cpu_online(target), which
> is somewhat more analogous.

No. Current semantics of smp_call_function_single() are that it
returns -EBUSY if called on the *current* cpu. Since on !CONFIG_SMP the
only possible cpu it can be called on is the current one, the only
sane return value is -EBUSY.

Re: [PATCH] Make smp_call_function{_single} go WARNING and return -EINVAL on !SMP (was Re: [PATCH] i386/x86_64: NMI watchdog: Protect smp_call_function() within CONFIG_SMP)

[Satyam Sharma]

On 6/7/07, Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> > The smp_call_function{_single} functions are used to run
> > given function on all {or speicified} *other* CPUs. For
> > UP systems, "other" CPUs simply don't exist, so we flag
> > such incorrect usage of these functions using a WARNING.
>
> If other cpus don't exist then smp_call_function() should just do
> *nothing* (there is no other cpu right?). We don't want to sprinkle
> a ton of #ifdef CONFIG_SMP around each smp_call_function().

Yes, I suspected that, as mentioned on the other thread (ugh).

> > Also, -EBUSY is generally returned by arch implementations
> > when they find that target_cpu == current_cpu, which is not
> > a comparable case to the !SMP case. Use -EINVAL instead,
> > similar to what powerpc does for !cpu_online(target), which
> > is somewhat more analogous.
>
> No. Current semantics of smp_call_function_single() are that it
> returns -EBUSY if called on the *current* cpu. Since on !CONFIG_SMP the
> only possible cpu it can be called on is the current one, the only
> sane return value is -EBUSY.

The inherent assumption that on !SMP the only possible CPU it
can be called on is current (== 0) is precisely what I would want
to be asserted formally in the code over here. If so, then return
-EBUSY, else -EINVAL?