Hello,
here is a patch to solve all (I hope I missed none) possible problems that
could occur on SMP machines running a preemptible kernel when
smp_call_function() calls a function which should be also executed on the
current processor.
This patch is based on the one Dave Jones sent to the LKML last friday and
applies to the linux kernel version 2.5.63.
Thank you for any response...
Thomas Schlichter
Thomas Schlichter <[email protected]> wrote:
>
> Hello,
>
> here is a patch to solve all (I hope I missed none) possible problems that
> could occur on SMP machines running a preemptible kernel when
> smp_call_function() calls a function which should be also executed on the
> current processor.
>
Patch looks pretty good, thanks. Fixes a real bug.
I worry a little about the s390/s390x change. smp_ctl_set_bit() and
smp_ctl_clear_bit(). The functions which are being called on local and
remote are fairly different. I'm sure it's OK, but... I changed that bit
to open-code the preempt_disable/enable.
The arch/x86_64/kernel/bluesmoke.c change looks right. Seems that someone
didn't understand the smp_call_function() API in there.
On Tue, Feb 25, 2003 at 07:08:48PM +0100, Thomas Schlichter wrote:
> here is a patch to solve all (I hope I missed none) possible problems that
> could occur on SMP machines running a preemptible kernel when
> smp_call_function() calls a function which should be also executed on the
> current processor.
>
> This patch is based on the one Dave Jones sent to the LKML last friday and
> applies to the linux kernel version 2.5.63.
Just one comment. You moved quite a few of the preempt_disable/enable
pairs outside of the CONFIG_SMP checks. The issue we're working against
here is to try and prevent preemption and ending up on a different CPU.
As this cannot happen if CONFIG_SMP=n, I don't see why you've done this.
Dave
Dave Jones <[email protected]> wrote:
>
> On Tue, Feb 25, 2003 at 07:08:48PM +0100, Thomas Schlichter wrote:
>
> > here is a patch to solve all (I hope I missed none) possible problems that
> > could occur on SMP machines running a preemptible kernel when
> > smp_call_function() calls a function which should be also executed on the
> > current processor.
> >
> > This patch is based on the one Dave Jones sent to the LKML last friday and
> > applies to the linux kernel version 2.5.63.
>
> Just one comment. You moved quite a few of the preempt_disable/enable
> pairs outside of the CONFIG_SMP checks. The issue we're working against
> here is to try and prevent preemption and ending up on a different CPU.
> As this cannot happen if CONFIG_SMP=n, I don't see why you've done this.
>
Just in two places.
arch/i386/kernel/ldt.c | 6 ++++--
arch/x86_64/kernel/ldt.c | 6 ++++--
2 files changed, 8 insertions(+), 4 deletions(-)
diff -puN arch/i386/kernel/ldt.c~on_each_cpu-ldt-cleanup arch/i386/kernel/ldt.c
--- 25/arch/i386/kernel/ldt.c~on_each_cpu-ldt-cleanup 2003-02-26 01:51:27.000000000 -0800
+++ 25-akpm/arch/i386/kernel/ldt.c 2003-02-26 01:52:21.000000000 -0800
@@ -55,13 +55,15 @@ static int alloc_ldt(mm_context_t *pc, i
wmb();
if (reload) {
+#ifdef CONFIG_SMP
preempt_disable();
load_LDT(pc);
-#ifdef CONFIG_SMP
if (current->mm->cpu_vm_mask != (1 << smp_processor_id()))
smp_call_function(flush_ldt, 0, 1, 1);
-#endif
preempt_enable();
+#else
+ load_LDT(pc);
+#endif
}
if (oldsize) {
if (oldsize*LDT_ENTRY_SIZE > PAGE_SIZE)
diff -puN arch/x86_64/kernel/ldt.c~on_each_cpu-ldt-cleanup arch/x86_64/kernel/ldt.c
--- 25/arch/x86_64/kernel/ldt.c~on_each_cpu-ldt-cleanup 2003-02-26 01:51:36.000000000 -0800
+++ 25-akpm/arch/x86_64/kernel/ldt.c 2003-02-26 01:52:37.000000000 -0800
@@ -60,13 +60,15 @@ static int alloc_ldt(mm_context_t *pc, i
pc->size = mincount;
wmb();
if (reload) {
+#ifdef CONFIG_SMP
preempt_disable();
load_LDT(pc);
-#ifdef CONFIG_SMP
if (current->mm->cpu_vm_mask != (1<<smp_processor_id()))
smp_call_function(flush_ldt, 0, 1, 1);
-#endif
preempt_enable();
+#else
+ load_LDT(pc);
+#endif
}
if (oldsize) {
if (oldsize*LDT_ENTRY_SIZE > PAGE_SIZE)
_
Dave Jones <[email protected]> wrote:
> Just one comment. You moved quite a few of the preempt_disable/enable
> pairs outside of the CONFIG_SMP checks. The issue we're working against
> here is to try and prevent preemption and ending up on a different CPU.
> As this cannot happen if CONFIG_SMP=n, I don't see why you've done this.
Andrew Morton wrote:
> Just in two places.
[snip]
Yes, thanks for delivering this better patch!
My approach wanted just to be the most simple possibility... ;-)
Thomas
On Wed, Feb 26, 2003 at 01:54:09AM -0800, Andrew Morton wrote:
> > Just one comment. You moved quite a few of the preempt_disable/enable
> > pairs outside of the CONFIG_SMP checks. The issue we're working against
> > here is to try and prevent preemption and ending up on a different CPU.
> > As this cannot happen if CONFIG_SMP=n, I don't see why you've done this.
> Just in two places.
Ok, slight exaggeration 8-) Looks good to me.
btw, (unrelated) shouldn't smp_call_function be doing magick checks
with cpu_online() ?
Dave
Dave Jones <[email protected]> wrote:
>
> btw, (unrelated) shouldn't smp_call_function be doing magick checks
> with cpu_online() ?
Looks OK? It sprays the IPI out to all the other CPUs in cpu_online_map,
and waits for num_online_cpus()-1 CPUs to answer.
All very racy in the presence of CPUs going offline, but that's all over
the place. Depends how the offlining will be done I guess.
On Wed, Feb 26, 2003 at 02:28:19AM -0800, Andrew Morton wrote:
> > btw, (unrelated) shouldn't smp_call_function be doing magick checks
> > with cpu_online() ?
> Looks OK? It sprays the IPI out to all the other CPUs in cpu_online_map,
> and waits for num_online_cpus()-1 CPUs to answer.
Doh, of course.
Ugh, mornings.
Dave
Dave Jones <[email protected]> wrote:
> btw, (unrelated) shouldn't smp_call_function be doing magick checks
> with cpu_online() ?
Andrew Morton wrote:
> Looks OK? It sprays the IPI out to all the other CPUs in cpu_online_map,
> and waits for num_online_cpus()-1 CPUs to answer.
>
> All very racy in the presence of CPUs going offline, but that's all over
> the place. Depends how the offlining will be done I guess.
Well, now I see the check for num_online_cpus() != 1 in smp_call_function(),
too, so the check in on_each_cpu() is not needed and possibly better this
patch should apply to the include/linux/smp.h file...
Thomas
--- linux-2.5.63/include/linux/smp.h.orig Mon Feb 24 20:05:33 2003
+++ linux-2.5.63/include/linux/smp.h Wed Feb 26 11:41:45 2003
@@ -10,9 +10,10 @@
#ifdef CONFIG_SMP
+#include <linux/preempt.h>
#include <linux/kernel.h>
#include <linux/compiler.h>
-#include <linux/threads.h>
+#include <linux/thread_info.h>
#include <asm/smp.h>
#include <asm/bug.h>
@@ -54,6 +55,24 @@
int retry, int wait);
/*
+ * Call a function on all processors
+ */
+static inline int on_each_cpu(void (*func) (void *info), void *info,
+ int retry, int wait)
+{
+ int ret;
+
+ preempt_disable();
+
+ ret = smp_call_function(func, info, retry, wait);
+ func(info);
+
+ preempt_enable();
+
+ return ret;
+}
+
+/*
* True once the per process idle is forked
*/
extern int smp_threads_ready;
@@ -96,6 +115,7 @@
#define hard_smp_processor_id() 0
#define smp_threads_ready 1
#define smp_call_function(func,info,retry,wait) ({ 0; })
+#define on_each_cpu(func,info,retry,wait) ({ func(info); 0; })
static inline void smp_send_reschedule(int cpu) { }
static inline void smp_send_reschedule_all(void) { }
#define cpu_online_map 1
On Wed, 2003-02-26 at 10:28, Andrew Morton wrote:
> Dave Jones <[email protected]> wrote:
> >
> > btw, (unrelated) shouldn't smp_call_function be doing magick checks
> > with cpu_online() ?
>
> Looks OK? It sprays the IPI out to all the other CPUs in cpu_online_map,
> and waits for num_online_cpus()-1 CPUs to answer.
You cannot do that by counting without a lot of care. IPI messages do not have
guaranteed "once only" semantics. On an error a resend can and has been observed
to cause a reissue of an IPI on PII/PIII setups
>> btw, (unrelated) shouldn't smp_call_function be doing magick checks
>> with cpu_online() ?
>
> Looks OK? It sprays the IPI out to all the other CPUs in cpu_online_map,
> and waits for num_online_cpus()-1 CPUs to answer.
Incidentally, would be nice if this thing had a timeout and bugged out when
one didn't reply, rather than just wedging the whole CPU for ever. Yes,
it's normally some other bug that makes the other CPU not reply, but would
make diags a damned sight easier.
M.
Alan Cox <[email protected]> wrote:
>
> On Wed, 2003-02-26 at 10:28, Andrew Morton wrote:
> > Dave Jones <[email protected]> wrote:
> > >
> > > btw, (unrelated) shouldn't smp_call_function be doing magick checks
> > > with cpu_online() ?
> >
> > Looks OK? It sprays the IPI out to all the other CPUs in cpu_online_map,
> > and waits for num_online_cpus()-1 CPUs to answer.
>
> You cannot do that by counting without a lot of care. IPI messages do not have
> guaranteed "once only" semantics. On an error a resend can and has been observed
> to cause a reissue of an IPI on PII/PIII setups
If that resend results in delivery of an actual extra interrupt, the
resent-to CPU can start playing with stuff which used to be on the sender's
stack and the box goes splat.
Didn't sct have a fix for that?
On Wed, 2003-02-26 at 18:47, Andrew Morton wrote:
> If that resend results in delivery of an actual extra interrupt, the
> resent-to CPU can start playing with stuff which used to be on the sender's
> stack and the box goes splat.
>
> Didn't sct have a fix for that?
Yes but it was never merged mainstream for some reason. I think it kind of
got away