* Bug in atomic_sub_if_positive
@ 2008-06-26 6:07 Morten Larsen
2008-06-26 12:11 ` Ralf Baechle
0 siblings, 1 reply; 9+ messages in thread
From: Morten Larsen @ 2008-06-26 6:07 UTC (permalink / raw)
To: linux-mips
As far as I can tell the branch optimization fixes in 2.6.21 introduced
a bug in atomic_sub_if_positive that causes it to return even when the
sc instruction fails. The result is that e.g. down_trylock becomes
unreliable as the semaphore counter is not always decremented.
Below is a patch that fixed the problem on my system.
Morten S. Larsen
--- a/include/asm-mips/atomic.h 2008-06-25 22:38:43.159739000 -0700
+++ b/include/asm-mips/atomic.h 2008-06-25 22:39:07.552065000 -0700
@@ -292,10 +292,10 @@ static __inline__ int atomic_sub_if_posi
" beqz %0, 2f
\n"
" subu %0, %1, %3
\n"
" .set reorder
\n"
- "1:
\n"
" .subsection 2
\n"
"2: b 1b
\n"
" .previous
\n"
+ "1:
\n"
" .set mips0
\n"
: "=&r" (result), "=&r" (temp), "=m" (v->counter)
: "Ir" (i), "m" (v->counter)
@@ -682,10 +682,10 @@ static __inline__ long atomic64_sub_if_p
" beqz %0, 2f
\n"
" dsubu %0, %1, %3
\n"
" .set reorder
\n"
- "1:
\n"
" .subsection 2
\n"
"2: b 1b
\n"
" .previous
\n"
+ "1:
\n"
" .set mips0
\n"
: "=&r" (result), "=&r" (temp), "=m" (v->counter)
: "Ir" (i), "m" (v->counter)
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: Bug in atomic_sub_if_positive
2008-06-26 6:07 Bug in atomic_sub_if_positive Morten Larsen
@ 2008-06-26 12:11 ` Ralf Baechle
2008-06-27 6:00 ` Morten Larsen
0 siblings, 1 reply; 9+ messages in thread
From: Ralf Baechle @ 2008-06-26 12:11 UTC (permalink / raw)
To: Morten Larsen; +Cc: linux-mips
On Wed, Jun 25, 2008 at 11:07:24PM -0700, Morten Larsen wrote:
> As far as I can tell the branch optimization fixes in 2.6.21 introduced
> a bug in atomic_sub_if_positive that causes it to return even when the
> sc instruction fails. The result is that e.g. down_trylock becomes
> unreliable as the semaphore counter is not always decremented.
I did play with a test program and can't reproduce the effect with my
assembler. I have darm memories of gas immitating some obscure behaviour
of the IRIX assembler and I think it doesn't do so for all MIPS targets.
So I'm wandering what toolchain have you been using?
Ralf
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: Bug in atomic_sub_if_positive
2008-06-26 12:11 ` Ralf Baechle
@ 2008-06-27 6:00 ` Morten Larsen
0 siblings, 0 replies; 9+ messages in thread
From: Morten Larsen @ 2008-06-27 6:00 UTC (permalink / raw)
To: Ralf Baechle; +Cc: linux-mips
> I did play with a test program and can't reproduce the effect with my
> assembler. I have darm memories of gas immitating some
> obscure behaviour
> of the IRIX assembler and I think it doesn't do so for all
> MIPS targets.
>
> So I'm wandering what toolchain have you been using?
>
> Ralf
>
The toolchain is from Wind River Systems 2.0 Linux Edition.
The kernel (based on 2.6.21.7) is also supplied by Wind River, and it
has preemption turned on.
The CPU is a Broadcom/SiByte BCM1125H (single core version of BCM1250
Swarm.)
Morten
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: Bug in atomic_sub_if_positive
@ 2008-07-02 0:12 Morten Larsen
2008-07-02 9:59 ` Thiemo Seufer
0 siblings, 1 reply; 9+ messages in thread
From: Morten Larsen @ 2008-07-02 0:12 UTC (permalink / raw)
To: linux-mips
> As far as I can tell the branch optimization fixes in 2.6.21 introduced
> a bug in atomic_sub_if_positive that causes it to return even when the
> sc instruction fails. The result is that e.g. down_trylock becomes
> unreliable as the semaphore counter is not always decremented.
Previous patch was garbled by Outlook - this one should be clean:
--- a/include/asm-mips/atomic.h 2008-06-25 22:38:43.159739000 -0700
+++ b/include/asm-mips/atomic.h 2008-06-25 22:39:07.552065000 -0700
@@ -292,10 +292,10 @@ static __inline__ int atomic_sub_if_posi
" beqz %0, 2f \n"
" subu %0, %1, %3 \n"
" .set reorder \n"
- "1: \n"
" .subsection 2 \n"
"2: b 1b \n"
" .previous \n"
+ "1: \n"
" .set mips0 \n"
: "=&r" (result), "=&r" (temp), "=m" (v->counter)
: "Ir" (i), "m" (v->counter)
@@ -682,10 +682,10 @@ static __inline__ long atomic64_sub_if_p
" beqz %0, 2f \n"
" dsubu %0, %1, %3 \n"
" .set reorder \n"
- "1: \n"
" .subsection 2 \n"
"2: b 1b \n"
" .previous \n"
+ "1: \n"
" .set mips0 \n"
: "=&r" (result), "=&r" (temp), "=m" (v->counter)
: "Ir" (i), "m" (v->counter)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Bug in atomic_sub_if_positive
2008-07-02 0:12 Morten Larsen
@ 2008-07-02 9:59 ` Thiemo Seufer
2008-07-02 10:31 ` Atsushi Nemoto
0 siblings, 1 reply; 9+ messages in thread
From: Thiemo Seufer @ 2008-07-02 9:59 UTC (permalink / raw)
To: Morten Larsen; +Cc: linux-mips
Morten Larsen wrote:
>
> > As far as I can tell the branch optimization fixes in 2.6.21 introduced
> > a bug in atomic_sub_if_positive that causes it to return even when the
> > sc instruction fails. The result is that e.g. down_trylock becomes
> > unreliable as the semaphore counter is not always decremented.
>
> Previous patch was garbled by Outlook - this one should be clean:
>
> --- a/include/asm-mips/atomic.h 2008-06-25 22:38:43.159739000 -0700
> +++ b/include/asm-mips/atomic.h 2008-06-25 22:39:07.552065000 -0700
> @@ -292,10 +292,10 @@ static __inline__ int atomic_sub_if_posi
> " beqz %0, 2f \n"
> " subu %0, %1, %3 \n"
> " .set reorder \n"
> - "1: \n"
> " .subsection 2 \n"
> "2: b 1b \n"
> " .previous \n"
> + "1: \n"
AFAICS this change should make no difference to the generated code. I
suspect you assembler handles .subsection incorrectly. Can you provide
a disassembled exapmle which gets altered by this patch? Also, please
tell us the exact version of the assembler you use.
Thiemo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Bug in atomic_sub_if_positive
2008-07-02 9:59 ` Thiemo Seufer
@ 2008-07-02 10:31 ` Atsushi Nemoto
2008-07-02 10:51 ` Thiemo Seufer
0 siblings, 1 reply; 9+ messages in thread
From: Atsushi Nemoto @ 2008-07-02 10:31 UTC (permalink / raw)
To: ths; +Cc: mlarsen, linux-mips
On Wed, 2 Jul 2008 10:59:56 +0100, Thiemo Seufer <ths@networkno.de> wrote:
> > --- a/include/asm-mips/atomic.h 2008-06-25 22:38:43.159739000 -0700
> > +++ b/include/asm-mips/atomic.h 2008-06-25 22:39:07.552065000 -0700
> > @@ -292,10 +292,10 @@ static __inline__ int atomic_sub_if_posi
> > " beqz %0, 2f \n"
> > " subu %0, %1, %3 \n"
> > " .set reorder \n"
> > - "1: \n"
> > " .subsection 2 \n"
> > "2: b 1b \n"
> > " .previous \n"
> > + "1: \n"
>
> AFAICS this change should make no difference to the generated code. I
> suspect you assembler handles .subsection incorrectly. Can you provide
> a disassembled exapmle which gets altered by this patch? Also, please
> tell us the exact version of the assembler you use.
Why no defference? The '1b' in subsection refer backword '1' label on
the source code (which is a label for LL insn in this case with the
patch) ?
Anyway I can provide them. I'm using vanilla 2.17 and 2.18.
Without the patch:
801233bc <try_acquire_console_sem>:
801233bc: lui v0,0x8038
801233c0: ll a0,31976(v0)
801233c4: addiu v1,a0,-1
801233c8: bltz v1,801233dc <try_acquire_console_sem+0x20>
801233cc: nop
801233d0: sc v1,31976(v0)
801233d4: beqz v1,80124dac <sys_syslog+0x8>
801233d8: addiu v1,a0,-1
801233dc: bltz v1,801233fc <try_acquire_console_sem+0x40>
801233e0: li v0,-1
...
80124dac: b 801233dc <try_acquire_console_sem+0x20>
80124db0: nop
With the patch:
801233bc <try_acquire_console_sem>:
801233bc: lui v0,0x8038
801233c0: ll a0,31976(v0)
801233c4: addiu v1,a0,-1
801233c8: bltz v1,801233dc <try_acquire_console_sem+0x20>
801233cc: nop
801233d0: sc v1,31976(v0)
801233d4: beqz v1,80124dac <sys_syslog+0x8>
801233d8: addiu v1,a0,-1
801233dc: bltz v1,801233fc <try_acquire_console_sem+0x40>
801233e0: li v0,-1
...
80124dac: b 801233c0 <try_acquire_console_sem+0x4>
80124db0: nop
The patch looks correct.
---
Atsushi Nemoto
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Bug in atomic_sub_if_positive
2008-07-02 10:31 ` Atsushi Nemoto
@ 2008-07-02 10:51 ` Thiemo Seufer
2008-07-03 15:04 ` Atsushi Nemoto
0 siblings, 1 reply; 9+ messages in thread
From: Thiemo Seufer @ 2008-07-02 10:51 UTC (permalink / raw)
To: Atsushi Nemoto; +Cc: mlarsen, linux-mips
Atsushi Nemoto wrote:
> On Wed, 2 Jul 2008 10:59:56 +0100, Thiemo Seufer <ths@networkno.de> wrote:
> > > --- a/include/asm-mips/atomic.h 2008-06-25 22:38:43.159739000 -0700
> > > +++ b/include/asm-mips/atomic.h 2008-06-25 22:39:07.552065000 -0700
> > > @@ -292,10 +292,10 @@ static __inline__ int atomic_sub_if_posi
> > > " beqz %0, 2f \n"
> > > " subu %0, %1, %3 \n"
> > > " .set reorder \n"
> > > - "1: \n"
> > > " .subsection 2 \n"
> > > "2: b 1b \n"
> > > " .previous \n"
> > > + "1: \n"
> >
> > AFAICS this change should make no difference to the generated code. I
> > suspect you assembler handles .subsection incorrectly. Can you provide
> > a disassembled exapmle which gets altered by this patch? Also, please
> > tell us the exact version of the assembler you use.
>
> Why no defference? The '1b' in subsection refer backword '1' label on
> the source code (which is a label for LL insn in this case with the
> patch) ?
Oh. Now that you spelled it out I see it, too. :-)
> Anyway I can provide them. I'm using vanilla 2.17 and 2.18.
>
> Without the patch:
> 801233bc <try_acquire_console_sem>:
> 801233bc: lui v0,0x8038
> 801233c0: ll a0,31976(v0)
> 801233c4: addiu v1,a0,-1
> 801233c8: bltz v1,801233dc <try_acquire_console_sem+0x20>
> 801233cc: nop
> 801233d0: sc v1,31976(v0)
> 801233d4: beqz v1,80124dac <sys_syslog+0x8>
> 801233d8: addiu v1,a0,-1
> 801233dc: bltz v1,801233fc <try_acquire_console_sem+0x40>
> 801233e0: li v0,-1
> ...
> 80124dac: b 801233dc <try_acquire_console_sem+0x20>
> 80124db0: nop
>
> With the patch:
> 801233bc <try_acquire_console_sem>:
> 801233bc: lui v0,0x8038
> 801233c0: ll a0,31976(v0)
> 801233c4: addiu v1,a0,-1
> 801233c8: bltz v1,801233dc <try_acquire_console_sem+0x20>
> 801233cc: nop
> 801233d0: sc v1,31976(v0)
> 801233d4: beqz v1,80124dac <sys_syslog+0x8>
> 801233d8: addiu v1,a0,-1
> 801233dc: bltz v1,801233fc <try_acquire_console_sem+0x40>
> 801233e0: li v0,-1
> ...
> 80124dac: b 801233c0 <try_acquire_console_sem+0x4>
> 80124db0: nop
>
>
> The patch looks correct.
Agreed.
Thiemo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Bug in atomic_sub_if_positive
2008-07-02 10:51 ` Thiemo Seufer
@ 2008-07-03 15:04 ` Atsushi Nemoto
2008-07-04 11:54 ` Ralf Baechle
0 siblings, 1 reply; 9+ messages in thread
From: Atsushi Nemoto @ 2008-07-03 15:04 UTC (permalink / raw)
To: ths; +Cc: mlarsen, linux-mips
On Wed, 2 Jul 2008 11:51:55 +0100, Thiemo Seufer <ths@networkno.de> wrote:
> > The patch looks correct.
>
> Agreed.
Ralf, too late for 2.6.26?
Anyway I think it should go into -stable tree too.
---
Atsushi Nemoto
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Bug in atomic_sub_if_positive
2008-07-03 15:04 ` Atsushi Nemoto
@ 2008-07-04 11:54 ` Ralf Baechle
0 siblings, 0 replies; 9+ messages in thread
From: Ralf Baechle @ 2008-07-04 11:54 UTC (permalink / raw)
To: Atsushi Nemoto; +Cc: ths, mlarsen, linux-mips
On Fri, Jul 04, 2008 at 12:04:26AM +0900, Atsushi Nemoto wrote:
> On Wed, 2 Jul 2008 11:51:55 +0100, Thiemo Seufer <ths@networkno.de> wrote:
> > > The patch looks correct.
> >
> > Agreed.
>
> Ralf, too late for 2.6.26?
Pull request to Linus is out.
> Anyway I think it should go into -stable tree too.
It is. This one is important. Heck, I don't get how a kernel can be as
reliable as it was with an elefant like this :-)
Ralf
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2008-07-04 11:56 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-06-26 6:07 Bug in atomic_sub_if_positive Morten Larsen
2008-06-26 12:11 ` Ralf Baechle
2008-06-27 6:00 ` Morten Larsen
-- strict thread matches above, loose matches on Subject: below --
2008-07-02 0:12 Morten Larsen
2008-07-02 9:59 ` Thiemo Seufer
2008-07-02 10:31 ` Atsushi Nemoto
2008-07-02 10:51 ` Thiemo Seufer
2008-07-03 15:04 ` Atsushi Nemoto
2008-07-04 11:54 ` Ralf Baechle
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox