2003-02-14 10:57:46

by Zwane Mwaikambo

[permalink] [raw]
Subject: [PATCH][2.5] flush_tlb_all is not preempt safe.

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


2003-02-14 11:04:47

by Zwane Mwaikambo

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

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

2003-02-15 13:33:22

by Zwane Mwaikambo

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

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

2003-02-19 20:14:23

by Thomas Schlichter

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

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


Attachments:
(No filename) (0.00 B)
signed data
(No filename) (189.00 B)
signature
Download all attachments

2003-02-20 19:52:42

by Thomas Schlichter

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

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?


Attachments:
(No filename) (0.00 B)
signed data
(No filename) (189.00 B)
signature
Download all attachments