2006-11-30 01:01:04

by Randy Dunlap

[permalink] [raw]
Subject: [PATCH -mm] x86_64 UP needs smp_call_function_single

From: Randy Dunlap <[email protected]>

smp_call_function_single() needs to be visible in non-SMP builds, to fix:

arch/x86_64/kernel/vsyscall.c:283: warning: implicit declaration of function 'smp_call_function_single'

The (other/trivial) fix (instead of this one) is to add:
#include <asm/smp.h>
to linux-2.6.19-rc6-mm2/arch/x86_64/kernel/vsyscall.c

Signed-off-by: Randy Dunlap <[email protected]>
---
include/asm-x86_64/smp.h | 7 -------
include/linux/smp.h | 7 +++++++
2 files changed, 7 insertions(+), 7 deletions(-)

--- linux-2.6.19-rc6-mm2.orig/include/asm-x86_64/smp.h
+++ linux-2.6.19-rc6-mm2/include/asm-x86_64/smp.h
@@ -113,13 +113,6 @@ static __inline int logical_smp_processo
#define cpu_physical_id(cpu) x86_cpu_to_apicid[cpu]
#else
#define cpu_physical_id(cpu) boot_cpu_id
-static inline int smp_call_function_single(int cpuid, void (*func) (void *info),
- void *info, int retry, int wait)
-{
- /* Disable interrupts here? */
- func(info);
- return 0;
-}
#endif /* !CONFIG_SMP */
#endif

--- linux-2.6.19-rc6-mm2.orig/include/linux/smp.h
+++ linux-2.6.19-rc6-mm2/include/linux/smp.h
@@ -99,6 +99,13 @@ 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),
+ void *info, int retry, int wait)
+{
+ /* Disable interrupts here? */
+ func(info);
+ return 0;
+}

#endif /* !SMP */



---


2006-11-30 01:46:41

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH -mm] x86_64 UP needs smp_call_function_single

On Wed, 29 Nov 2006 17:01:11 -0800
Randy Dunlap <[email protected]> wrote:

> From: Randy Dunlap <[email protected]>
>
> smp_call_function_single() needs to be visible in non-SMP builds, to fix:
>
> arch/x86_64/kernel/vsyscall.c:283: warning: implicit declaration of function 'smp_call_function_single'
>
> The (other/trivial) fix (instead of this one) is to add:
> #include <asm/smp.h>
> to linux-2.6.19-rc6-mm2/arch/x86_64/kernel/vsyscall.c
>
> Signed-off-by: Randy Dunlap <[email protected]>
> ---
> include/asm-x86_64/smp.h | 7 -------
> include/linux/smp.h | 7 +++++++
> 2 files changed, 7 insertions(+), 7 deletions(-)
>
> --- linux-2.6.19-rc6-mm2.orig/include/asm-x86_64/smp.h
> +++ linux-2.6.19-rc6-mm2/include/asm-x86_64/smp.h
> @@ -113,13 +113,6 @@ static __inline int logical_smp_processo
> #define cpu_physical_id(cpu) x86_cpu_to_apicid[cpu]
> #else
> #define cpu_physical_id(cpu) boot_cpu_id
> -static inline int smp_call_function_single(int cpuid, void (*func) (void *info),
> - void *info, int retry, int wait)
> -{
> - /* Disable interrupts here? */
> - func(info);
> - return 0;
> -}
> #endif /* !CONFIG_SMP */
> #endif
>
> --- linux-2.6.19-rc6-mm2.orig/include/linux/smp.h
> +++ linux-2.6.19-rc6-mm2/include/linux/smp.h
> @@ -99,6 +99,13 @@ 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),
> + void *info, int retry, int wait)
> +{
> + /* Disable interrupts here? */
> + func(info);
> + return 0;
> +}
>
> #endif /* !SMP */
>

No, I think this patch is right - the declaration of the CONFIG_SMP
smp_call_function_single() is in linux/smp.h so the !CONFIG_SMP declaration
or definition should be there too.

It's still buggy though. It should disable local interrupts around the
call to match the SMP version. I'll fix that separately.

2006-11-30 07:01:54

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH -mm] x86_64 UP needs smp_call_function_single

On Wed, 2006-11-29 at 17:45 -0800, Andrew Morton wrote:
> No, I think this patch is right - the declaration of the CONFIG_SMP
> smp_call_function_single() is in linux/smp.h so the !CONFIG_SMP
> declaration
> or definition should be there too.
>
> It's still buggy though. It should disable local interrupts around
> the
> call to match the SMP version. I'll fix that separately.

hm, didnt i send an updated patch for that already? See the patch below,
from many days ago. I sent it after the tsc-sync-rewrite patch.

Ingo

--------------->
Subject: x86_64: build fixes
From: Ingo Molnar <[email protected]>

x86_64 does not build cleanly on UP:

arch/x86_64/kernel/vsyscall.c: In function 'cpu_vsyscall_notifier':
arch/x86_64/kernel/vsyscall.c:282: warning: implicit declaration of
function 'smp_call_function_single'
arch/x86_64/kernel/vsyscall.c: At top level:
arch/x86_64/kernel/vsyscall.c:279: warning: 'cpu_vsyscall_notifier'
defined but not used

this patch fixes it by making smp_call_function_single() globally
available.

Signed-off-by: Ingo Molnar <[email protected]>
---
include/asm-x86_64/smp.h | 11 ++---------
include/linux/smp.h | 10 +++++++---
kernel/sched.c | 19 +++++++++++++++++++
3 files changed, 28 insertions(+), 12 deletions(-)

Index: linux/include/asm-x86_64/smp.h
===================================================================
--- linux.orig/include/asm-x86_64/smp.h
+++ linux/include/asm-x86_64/smp.h
@@ -115,16 +115,9 @@ static __inline int logical_smp_processo
}

#ifdef CONFIG_SMP
-#define cpu_physical_id(cpu) x86_cpu_to_apicid[cpu]
+# define cpu_physical_id(cpu) x86_cpu_to_apicid[cpu]
#else
-#define cpu_physical_id(cpu) boot_cpu_id
-static inline int smp_call_function_single(int cpuid, void (*func)
(void *info),
- void *info, int retry, int wait)
-{
- /* Disable interrupts here? */
- func(info);
- return 0;
-}
+# define cpu_physical_id(cpu) boot_cpu_id
#endif /* !CONFIG_SMP */
#endif

Index: linux/include/linux/smp.h
===================================================================
--- linux.orig/include/linux/smp.h
+++ linux/include/linux/smp.h
@@ -53,9 +53,6 @@ extern void smp_cpus_done(unsigned int m
*/
int smp_call_function(void(*func)(void *info), void *info, int retry,
int wait);

-int smp_call_function_single(int cpuid, void (*func) (void *info), void
*info,
- int retry, int wait);
-
/*
* Call a function on all processors
*/
@@ -103,6 +100,13 @@ static inline void smp_send_reschedule(i
#endif /* !SMP */

/*
+ * Call a function on a specific CPU (on UP the function gets executed
+ * on the current CPU, immediately):
+ */
+int smp_call_function_single(int cpuid, void (*func) (void *info), void
*info,
+ int retry, int wait);
+
+/*
* smp_processor_id(): get the current CPU ID.
*
* if DEBUG_PREEMPT is enabled the we check whether it is
Index: linux/kernel/sched.c
===================================================================
--- linux.orig/kernel/sched.c
+++ linux/kernel/sched.c
@@ -1110,6 +1110,25 @@ repeat:
task_rq_unlock(rq, &flags);
}

+#ifndef CONFIG_SMP
+/*
+ * Call a function on a specific CPU (on UP the function gets executed
+ * on the current CPU, immediately):
+ */
+int smp_call_function_single(int cpuid, void (*func) (void *info), void
*info,
+ int retry, int wait)
+{
+ unsigned long flags;
+
+ local_irq_save(flags);
+ func(info);
+ local_irq_restore(flags);
+
+ return 0;
+}
+EXPORT_SYMBOL(smp_call_function_single);
+#endif
+
/***
* kick_process - kick a running thread to enter/exit the kernel
* @p: the to-be-kicked thread


2006-11-30 07:54:22

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH -mm] x86_64 UP needs smp_call_function_single

On Thu, 30 Nov 2006 08:00:00 +0100
Ingo Molnar <[email protected]> wrote:

> On Wed, 2006-11-29 at 17:45 -0800, Andrew Morton wrote:
> > No, I think this patch is right - the declaration of the CONFIG_SMP
> > smp_call_function_single() is in linux/smp.h so the !CONFIG_SMP
> > declaration
> > or definition should be there too.
> >
> > It's still buggy though. It should disable local interrupts around
> > the
> > call to match the SMP version. I'll fix that separately.
>
> hm, didnt i send an updated patch for that already? See the patch below,
> from many days ago. I sent it after the tsc-sync-rewrite patch.
>

Might have got lost.

> --------------->
> Subject: x86_64: build fixes
> From: Ingo Molnar <[email protected]>
>
> x86_64 does not build cleanly on UP:
>
> arch/x86_64/kernel/vsyscall.c: In function 'cpu_vsyscall_notifier':
> arch/x86_64/kernel/vsyscall.c:282: warning: implicit declaration of
> function 'smp_call_function_single'
> arch/x86_64/kernel/vsyscall.c: At top level:
> arch/x86_64/kernel/vsyscall.c:279: warning: 'cpu_vsyscall_notifier'
> defined but not used
>
> this patch fixes it by making smp_call_function_single() globally
> available.
>
> Signed-off-by: Ingo Molnar <[email protected]>
> ---
> include/asm-x86_64/smp.h | 11 ++---------
> include/linux/smp.h | 10 +++++++---
> kernel/sched.c | 19 +++++++++++++++++++
> 3 files changed, 28 insertions(+), 12 deletions(-)
>
> Index: linux/include/asm-x86_64/smp.h
> ===================================================================
> --- linux.orig/include/asm-x86_64/smp.h
> +++ linux/include/asm-x86_64/smp.h
> @@ -115,16 +115,9 @@ static __inline int logical_smp_processo
> }
>
> #ifdef CONFIG_SMP
> -#define cpu_physical_id(cpu) x86_cpu_to_apicid[cpu]
> +# define cpu_physical_id(cpu) x86_cpu_to_apicid[cpu]
> #else
> -#define cpu_physical_id(cpu) boot_cpu_id
> -static inline int smp_call_function_single(int cpuid, void (*func)
> (void *info),

congratulations-your-first-wordwrapped-patch ;)

> --- linux.orig/kernel/sched.c
> +++ linux/kernel/sched.c
> @@ -1110,6 +1110,25 @@ repeat:
> task_rq_unlock(rq, &flags);
> }
>
> +#ifndef CONFIG_SMP
> +/*
> + * Call a function on a specific CPU (on UP the function gets executed
> + * on the current CPU, immediately):
> + */
> +int smp_call_function_single(int cpuid, void (*func) (void *info), void
> *info,
> + int retry, int wait)
> +{
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + func(info);
> + local_irq_restore(flags);
> +
> + return 0;
> +}

yes, but a) calling the SMP version with local interrupts disabled is a
bug, so we can use bare local_irq_disable() here and b) only two
archictures call or use this function, so all the others don't want a copy
of it.

So I did:

--- a/include/linux/smp.h~up-smp_call_function_single-should-disable-interrupts
+++ a/include/linux/smp.h
@@ -15,6 +15,7 @@ extern void cpu_idle(void);
#include <linux/kernel.h>
#include <linux/compiler.h>
#include <linux/thread_info.h>
+#include <linux/irqflags.h>
#include <asm/smp.h>

/*
@@ -102,8 +103,9 @@ static inline void smp_send_reschedule(i
static inline int smp_call_function_single(int cpuid, void (*func) (void *info),
void *info, int retry, int wait)
{
- /* Disable interrupts here? */
+ local_irq_disable(); /* Match the SMP call environment */
func(info);
+ local_irq_enable();
return 0;
}

_

which is somewhat unpleasant. I added a WARN_ON(irqs_disabled()) to the
out-of-line SMP version.


btw, does anyone know why the SMP versions of this function use
spin_lock_bh(&call_lock)?

2006-11-30 09:24:18

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH -mm] x86_64 UP needs smp_call_function_single

On Wed, 2006-11-29 at 23:54 -0800, Andrew Morton wrote:
> which is somewhat unpleasant. I added a WARN_ON(irqs_disabled()) to
> the
> out-of-line SMP version.

ok.
>
> btw, does anyone know why the SMP versions of this function use
> spin_lock_bh(&call_lock)?

that makes no sense (neither the get_cpu()/put_cpu() gymnastics) if this
is called with irqs disabled all the time.

Ingo
>

2006-11-30 09:35:47

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH -mm] x86_64 UP needs smp_call_function_single

On Thu, 30 Nov 2006 10:22:20 +0100
Ingo Molnar <[email protected]> wrote:

> >
> > btw, does anyone know why the SMP versions of this function use
> > spin_lock_bh(&call_lock)?
>
> that makes no sense (neither the get_cpu()/put_cpu() gymnastics) if this
> is called with irqs disabled all the time.

smp_call_function_single() must be called with local interrupts ENabled.

But why isn't it just spin_lock()?

<looks>

Eric simply copied that code from ia64, which added the spin_lock_bh()
in 2.4.8. Ho-hum.

2006-11-30 22:11:31

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH -mm] x86_64 UP needs smp_call_function_single

On Thu, 30 Nov 2006 08:00:00 +0100 Ingo Molnar wrote:

> On Wed, 2006-11-29 at 17:45 -0800, Andrew Morton wrote:
> > No, I think this patch is right - the declaration of the CONFIG_SMP
> > smp_call_function_single() is in linux/smp.h so the !CONFIG_SMP
> > declaration
> > or definition should be there too.
> >
> > It's still buggy though. It should disable local interrupts around
> > the
> > call to match the SMP version. I'll fix that separately.
>
> hm, didnt i send an updated patch for that already? See the patch below,
> from many days ago. I sent it after the tsc-sync-rewrite patch.

Hi Ingo,

Has there been a patch for this one? (UP again, not SMP)

drivers/input/ff-memless.c:384: warning: implicit declaration of function 'local_bh_disable'
drivers/input/ff-memless.c:393: warning: implicit declaration of function 'local_bh_enable'

Thanks,
---
~Randy
config: http://oss.oracle.com/~rdunlap/configs/config-input-up-header

2006-11-30 22:28:32

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH -mm] x86_64 UP needs smp_call_function_single

On Thu, 30 Nov 2006 14:11:40 -0800
Randy Dunlap <[email protected]> wrote:

> On Thu, 30 Nov 2006 08:00:00 +0100 Ingo Molnar wrote:
>
> > On Wed, 2006-11-29 at 17:45 -0800, Andrew Morton wrote:
> > > No, I think this patch is right - the declaration of the CONFIG_SMP
> > > smp_call_function_single() is in linux/smp.h so the !CONFIG_SMP
> > > declaration
> > > or definition should be there too.
> > >
> > > It's still buggy though. It should disable local interrupts around
> > > the
> > > call to match the SMP version. I'll fix that separately.
> >
> > hm, didnt i send an updated patch for that already? See the patch below,
> > from many days ago. I sent it after the tsc-sync-rewrite patch.
>
> Hi Ingo,
>
> Has there been a patch for this one? (UP again, not SMP)
>
> drivers/input/ff-memless.c:384: warning: implicit declaration of function 'local_bh_disable'
> drivers/input/ff-memless.c:393: warning: implicit declaration of function 'local_bh_enable'
>
> Thanks,
> ---
> ~Randy
> config: http://oss.oracle.com/~rdunlap/configs/config-input-up-header

eww.. I guess linux/spinlock.h should really include linux/interrupt.h.
But interrupt.h includes stuff like sched.h which will want spinlock.h.

This, maybe?

include/linux/bottom_half.h | 5 +++++
include/linux/interrupt.h | 7 +------
include/linux/spinlock.h | 1 +
3 files changed, 7 insertions(+), 6 deletions(-)

diff -puN /dev/null include/linux/bottom_half.h
--- /dev/null
+++ a/include/linux/bottom_half.h
@@ -0,0 +1,5 @@
+extern void local_bh_disable(void);
+extern void __local_bh_enable(void);
+extern void _local_bh_enable(void);
+extern void local_bh_enable(void);
+extern void local_bh_enable_ip(unsigned long ip);
diff -puN include/linux/interrupt.h~add-bottom_half.h include/linux/interrupt.h
--- a/include/linux/interrupt.h~add-bottom_half.h
+++ a/include/linux/interrupt.h
@@ -11,6 +11,7 @@
#include <linux/hardirq.h>
#include <linux/sched.h>
#include <linux/irqflags.h>
+#include <linux/bottom_half.h>
#include <asm/atomic.h>
#include <asm/ptrace.h>
#include <asm/system.h>
@@ -217,12 +218,6 @@ static inline void __deprecated save_and
#define save_and_cli(x) save_and_cli(&x)
#endif /* CONFIG_SMP */

-extern void local_bh_disable(void);
-extern void __local_bh_enable(void);
-extern void _local_bh_enable(void);
-extern void local_bh_enable(void);
-extern void local_bh_enable_ip(unsigned long ip);
-
/* PLEASE, avoid to allocate new softirqs, if you need not _really_ high
frequency threaded job scheduling. For almost all the purposes
tasklets are more than enough. F.e. all serial device BHs et
diff -puN include/linux/spinlock.h~add-bottom_half.h include/linux/spinlock.h
--- a/include/linux/spinlock.h~add-bottom_half.h
+++ a/include/linux/spinlock.h
@@ -52,6 +52,7 @@
#include <linux/thread_info.h>
#include <linux/kernel.h>
#include <linux/stringify.h>
+#include <linux/bottom_half.h>

#include <asm/system.h>

_

2006-11-30 22:31:10

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH -mm] x86_64 UP needs smp_call_function_single

Andrew Morton wrote:
> On Thu, 30 Nov 2006 14:11:40 -0800
> Randy Dunlap <[email protected]> wrote:
>
>> On Thu, 30 Nov 2006 08:00:00 +0100 Ingo Molnar wrote:
>>
>>> On Wed, 2006-11-29 at 17:45 -0800, Andrew Morton wrote:
>>>> No, I think this patch is right - the declaration of the CONFIG_SMP
>>>> smp_call_function_single() is in linux/smp.h so the !CONFIG_SMP
>>>> declaration
>>>> or definition should be there too.
>>>>
>>>> It's still buggy though. It should disable local interrupts around
>>>> the
>>>> call to match the SMP version. I'll fix that separately.
>>> hm, didnt i send an updated patch for that already? See the patch below,
>>> from many days ago. I sent it after the tsc-sync-rewrite patch.
>> Hi Ingo,
>>
>> Has there been a patch for this one? (UP again, not SMP)
>>
>> drivers/input/ff-memless.c:384: warning: implicit declaration of function 'local_bh_disable'
>> drivers/input/ff-memless.c:393: warning: implicit declaration of function 'local_bh_enable'
>>
>> Thanks,
>> ---
>> ~Randy
>> config: http://oss.oracle.com/~rdunlap/configs/config-input-up-header
>
> eww.. I guess linux/spinlock.h should really include linux/interrupt.h.
> But interrupt.h includes stuff like sched.h which will want spinlock.h.
>
> This, maybe?

Looks good. I had already tried (and failed) adding interrupt.h to spinlock.h --
what a mess.

--
~Randy

2006-11-30 23:13:53

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH -mm] x86_64 UP needs smp_call_function_single

On Thu, 30 Nov 2006 14:27:19 -0800 Andrew Morton wrote:

> On Thu, 30 Nov 2006 14:11:40 -0800
> Randy Dunlap <[email protected]> wrote:
>
> > On Thu, 30 Nov 2006 08:00:00 +0100 Ingo Molnar wrote:
> >
> > > On Wed, 2006-11-29 at 17:45 -0800, Andrew Morton wrote:
> > > > No, I think this patch is right - the declaration of the CONFIG_SMP
> > > > smp_call_function_single() is in linux/smp.h so the !CONFIG_SMP
> > > > declaration
> > > > or definition should be there too.
> > > >
> > > > It's still buggy though. It should disable local interrupts around
> > > > the
> > > > call to match the SMP version. I'll fix that separately.
> > >
> > > hm, didnt i send an updated patch for that already? See the patch below,
> > > from many days ago. I sent it after the tsc-sync-rewrite patch.
> >
> > Hi Ingo,
> >
> > Has there been a patch for this one? (UP again, not SMP)
> >
> > drivers/input/ff-memless.c:384: warning: implicit declaration of function 'local_bh_disable'
> > drivers/input/ff-memless.c:393: warning: implicit declaration of function 'local_bh_enable'
> >
> > Thanks,
> > ---
> > ~Randy
> > config: http://oss.oracle.com/~rdunlap/configs/config-input-up-header
>
> eww.. I guess linux/spinlock.h should really include linux/interrupt.h.
> But interrupt.h includes stuff like sched.h which will want spinlock.h.
>
> This, maybe?

Ack. Tested on UP and SMP x86_64.

> include/linux/bottom_half.h | 5 +++++
> include/linux/interrupt.h | 7 +------
> include/linux/spinlock.h | 1 +
> 3 files changed, 7 insertions(+), 6 deletions(-)
>
> diff -puN /dev/null include/linux/bottom_half.h
> --- /dev/null
> +++ a/include/linux/bottom_half.h
> @@ -0,0 +1,5 @@
> +extern void local_bh_disable(void);
> +extern void __local_bh_enable(void);
> +extern void _local_bh_enable(void);
> +extern void local_bh_enable(void);
> +extern void local_bh_enable_ip(unsigned long ip);
> diff -puN include/linux/interrupt.h~add-bottom_half.h include/linux/interrupt.h
> --- a/include/linux/interrupt.h~add-bottom_half.h
> +++ a/include/linux/interrupt.h
> @@ -11,6 +11,7 @@
> #include <linux/hardirq.h>
> #include <linux/sched.h>
> #include <linux/irqflags.h>
> +#include <linux/bottom_half.h>
> #include <asm/atomic.h>
> #include <asm/ptrace.h>
> #include <asm/system.h>
> @@ -217,12 +218,6 @@ static inline void __deprecated save_and
> #define save_and_cli(x) save_and_cli(&x)
> #endif /* CONFIG_SMP */
>
> -extern void local_bh_disable(void);
> -extern void __local_bh_enable(void);
> -extern void _local_bh_enable(void);
> -extern void local_bh_enable(void);
> -extern void local_bh_enable_ip(unsigned long ip);
> -
> /* PLEASE, avoid to allocate new softirqs, if you need not _really_ high
> frequency threaded job scheduling. For almost all the purposes
> tasklets are more than enough. F.e. all serial device BHs et
> diff -puN include/linux/spinlock.h~add-bottom_half.h include/linux/spinlock.h
> --- a/include/linux/spinlock.h~add-bottom_half.h
> +++ a/include/linux/spinlock.h
> @@ -52,6 +52,7 @@
> #include <linux/thread_info.h>
> #include <linux/kernel.h>
> #include <linux/stringify.h>
> +#include <linux/bottom_half.h>
>
> #include <asm/system.h>

---
~Randy