[PATCH][2.5] flush_tlb_all is not preempt safe.

[PATCH][2.5] flush_tlb_all is not preempt safe.

[Zwane Mwaikambo]

Hi,
	Considering that smp_call_function isn't allowed to hold a lock 
reference and within smp_call_function we lock and unlock call_lock thus 
triggering a preempt point. Therefore we can't guarantee that we'll be on 
the same processor when we hit do_flush_tlb_all_local.

void flush_tlb_all(void)
{
	preempt_disable();
	smp_call_function (flush_tlb_all_ipi,0,1,1);

	do_flush_tlb_all_local();
	preempt_enable();
}

...

smp_call_function()
{
	spin_lock(call_lock);
	...
	spin_unlock(call_lock);
	<preemption point>
}

...

do_flush_tlb_all_local() - possibly not executing on same processor 
anymore.

This case is fixed in my smp_call_function_on_cpu patches by not allowing 
smp_call_function to invoke preemption.

Index: linux-2.5.60-uml/arch/i386/kernel/smp.c
===================================================================
RCS file: /build/cvsroot/linux-2.5.60/arch/i386/kernel/smp.c,v
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 smp.c
--- linux-2.5.60-uml/arch/i386/kernel/smp.c	10 Feb 2003 22:14:16 -0000	1.1.1.1
+++ linux-2.5.60-uml/arch/i386/kernel/smp.c	14 Feb 2003 10:59:19 -0000
@@ -452,9 +452,11 @@
 
 void flush_tlb_all(void)
 {
+	preempt_disable();
 	smp_call_function (flush_tlb_all_ipi,0,1,1);
 
 	do_flush_tlb_all_local();
+	preempt_enable();
 }
 
 /*

-- 
function.linuxpower.ca

Re: [PATCH][2.5] flush_tlb_all is not preempt safe.

[Zwane Mwaikambo]

On Fri, 14 Feb 2003, Zwane Mwaikambo wrote:

> Hi,
> 	Considering that smp_call_function isn't allowed to hold a lock 
> reference and within smp_call_function we lock and unlock call_lock thus 
> triggering a preempt point. Therefore we can't guarantee that we'll be on 
> the same processor when we hit do_flush_tlb_all_local.
> 
> void flush_tlb_all(void)
> {
> 	preempt_disable();
> 	smp_call_function (flush_tlb_all_ipi,0,1,1);
> 
> 	do_flush_tlb_all_local();
> 	preempt_enable();
> }

Of course i had to go and paste the code i was working on. The original 
isn't wrapped in preempt_disable/enable.

	Zwane (who really needs to get to bed now)
-- 
function.linuxpower.ca

Re: [PATCH][2.5] flush_tlb_all is not preempt safe.

[Zwane Mwaikambo]

On Fri, 14 Feb 2003, Zwane Mwaikambo wrote:

> On Fri, 14 Feb 2003, Zwane Mwaikambo wrote:
> 
> > Hi,
> > 	Considering that smp_call_function isn't allowed to hold a lock 
> > reference and within smp_call_function we lock and unlock call_lock thus 
> > triggering a preempt point. Therefore we can't guarantee that we'll be on 
> > the same processor when we hit do_flush_tlb_all_local.
> > 
> > void flush_tlb_all(void)
> > {
> > 	preempt_disable();
> > 	smp_call_function (flush_tlb_all_ipi,0,1,1);
> > 
> > 	do_flush_tlb_all_local();
> > 	preempt_enable();
> > }
> 
> Of course i had to go and paste the code i was working on. The original 
> isn't wrapped in preempt_disable/enable.
> 
> 	Zwane (who really needs to get to bed now)

void flush_tlb_all(void)
{
	BUG_ON(preempt_count() == 0);
	smp_call_function (flush_tlb_all_ipi,0,1,1);

	do_flush_tlb_all_local();
}


------------[ cut here ]------------
kernel BUG at arch/i386/kernel/smp.c:455!
invalid operand: 0000
CPU:    0
EIP:    0060:[<c01166d8>]    Not tainted
EFLAGS: 00000246
EIP is at flush_tlb_all+0x88/0xa0
eax: 00000000   ebx: c1552000   ecx: 00040000   edx: 00040000
esi: c1bf7ec4   edi: 00000001   ebp: c1553f6c   esp: c1553f68
ds: 007b   es: 007b   ss: 0068
Process swapoff (pid: 1936, threadinfo=c1552000 task=c143ad40)
Stack: c2811000 c1553f84 c015427b c1bf7ec4 00000000 c2811000 c059a4e0 c1553f94
       c015430e c2811000 00000001 c1553fbc c0156b06 c2811000 c1552000 ffffffff
       c1ad0a34 c1bfb2e4 bfffff1a bfffff18 080493eb c1552000 c0109957 bfffff1a
Call Trace:
 [<c015427b>] __vunmap+0x2b/0xa0
 [<c015430e>] vfree+0x1e/0x30
 [<c0156b06>] sys_swapoff+0x3c6/0x4d0
 [<c0109957>] syscall_call+0x7/0xb

Code: 0f 0b c7 01 2b f8 43 c0 eb 80 8d b4 26 00 00 00 00 8d bc 27

	Zwane
-- 
function.linuxpower.ca

Re: [PATCH][2.5] flush_tlb_all is not preempt safe.

[Thomas Schlichter]

[-- Attachment #1.1: body text --]
[-- Type: text/plain, Size: 354 bytes --]

Hi,

I've created a patch based on yours to solve the flush_tlb_all preempt-issue 
for x86_64 and the i386/mach-voyager subarchitecture. I'm not sure if the 
ia64 architecture would need to be patched, too...?

Wouldn't it even have been possible to solve this problem just by swapping the 
two original lines?

Best regards
  Thomas Schlichter

[-- Attachment #1.2: flush_tlb_all_preempt.patch --]
[-- Type: text/x-diff, Size: 787 bytes --]

--- linux-2.5.62/arch/i386/mach-voyager/voyager_smp.c.orig	Wed Feb 19 16:43:22 2003
+++ linux-2.5.62/arch/i386/mach-voyager/voyager_smp.c	Wed Feb 19 16:43:57 2003
@@ -1230,9 +1230,11 @@
 void
 flush_tlb_all(void)
 {
+	preempt_disable();
 	smp_call_function (flush_tlb_all_function, 0, 1, 1);
 
 	do_flush_tlb_all_local();
+	preempt_enable();
 }
 
 /* used to set up the trampoline for other CPUs when the memory manager
--- linux-2.5.62/arch/x86_64/kernel/smp.c.orig	Wed Feb 19 21:08:20 2003
+++ linux-2.5.62/arch/x86_64/kernel/smp.c	Wed Feb 19 21:09:40 2003
@@ -344,9 +344,11 @@
 
 void flush_tlb_all(void)
 {
+	preempt_disable();
 	smp_call_function (flush_tlb_all_ipi,0,1,1);
 
 	do_flush_tlb_all_local();
+	preempt_enable();
 }
 
 void smp_kdb_stop(void)

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[PATCH][2.5] flush_tlb_all is not preempt safe in x86_64 and i386/mach-voyager

[Thomas Schlichter]

[-- Attachment #1.1: body text --]
[-- Type: text/plain, Size: 282 bytes --]

This patch is based on Changeset 1.914.160.6.
It solves the flush_tlb_all preempt-issue for x86_64 and the i386/mach-voyager subarchitecture.

Best regards
  Thomas Schlichter

P.S.: Wouldn't it even have been possible to solve this problem just by swapping the original two lines?

[-- Attachment #1.2: flush_tlb_all_preempt.patch --]
[-- Type: text/x-diff, Size: 787 bytes --]

--- linux-2.5.62/arch/i386/mach-voyager/voyager_smp.c.orig	Wed Feb 19 16:43:22 2003
+++ linux-2.5.62/arch/i386/mach-voyager/voyager_smp.c	Wed Feb 19 16:43:57 2003
@@ -1230,9 +1230,11 @@
 void
 flush_tlb_all(void)
 {
+	preempt_disable();
 	smp_call_function (flush_tlb_all_function, 0, 1, 1);
 
 	do_flush_tlb_all_local();
+	preempt_enable();
 }
 
 /* used to set up the trampoline for other CPUs when the memory manager
--- linux-2.5.62/arch/x86_64/kernel/smp.c.orig	Wed Feb 19 21:08:20 2003
+++ linux-2.5.62/arch/x86_64/kernel/smp.c	Wed Feb 19 21:09:40 2003
@@ -344,9 +344,11 @@
 
 void flush_tlb_all(void)
 {
+	preempt_disable();
 	smp_call_function (flush_tlb_all_ipi,0,1,1);
 
 	do_flush_tlb_all_local();
+	preempt_enable();
 }
 
 void smp_kdb_stop(void)

[-- Attachment #2: signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]