locking problem with mips atomicity

locking problem with mips atomicity

[Michael Eager]

[-- Attachment #1: Type: text/plain, Size: 2254 bytes --]

The MIPS architecture spec says that there should not be a load,
store or prefetch between issuing a LL instruction and a following SC
instruction.  (MIPS32 Arch, Vol 2, rev 0.95, SC instruction description, p 186)

The macros in libstdc++-v3/config/cpu/mips/atomicity.h (and similar code
in glibc) could generate a load between the LL and SC if they were inlined.
The comments in the macros still mentione inline, but "inline" was removed 
in the most recent mod to this header (2/27/04).  The functions generated 
without inline do not appear to have this problem, but previous versions of 
gcc inlined the code do.  (Offhand, I think removing inline for locking 
code will have an adverse affect on performance.)

This program (adapted from mcount.c in glibc) shows the problem:

mcount.c:
typedef int _Atomic_word;

#include "atomicity.h"
using namespace __gnu_cxx;

struct gmonparam {
        _Atomic_word state;
};
extern struct gmonparam _gmonparam ;

static void __attribute__ ((__used__)) __mcount ()
{
        register struct gmonparam *p;
        p = &_gmonparam;
        if (! __exchange_and_add(&p->state, 1))
          return;
        p->state = 0;
}

The code generated for exchange_and_add is (lwc0 == LL, swc0 == SC):

  10:   8f830000        lw      v1,0(gp)
  14:   00000000        nop
  18:   c0630000        lwc0    $3,0(v1)
  1c:   00432021        addu    a0,v0,v1
  20:   8f810000        lw      at,0(gp)    <<<<<< BAD
  24:   00000000        nop
  28:   e0240000        swc0    $4,0(at)
  2c:   1080fff8        beqz    a0,10 <_Z8__mcountv+0x10>
  30:   00000000        nop
  34:   10600004        beqz    v1,48 <_Z8__mcountv+0x48>

The attached patch loads the address of the location being
modified into a reg before the LL, and references it explicitly,
so that the following is generated instead:

  10:   8f840000        lw      a0,0(gp)
  14:   c0830000        lwc0    $3,0(a0)
  18:   00432821        addu    a1,v0,v1
  1c:   e0850000        swc0    $5,0(a0)
  20:   10a0fffb        beqz    a1,10 <_Z8__mcountv+0x10>
  24:   00000000        nop
  28:   10600004        beqz    v1,3c <_Z8__mcountv+0x3c>


--
Michael Eager     eager@mvista.com	408-328-8426	
MontaVista Software, Inc. 1237 E. Arques Ave., Sunnyvale, CA  94085

[-- Attachment #2: FSF-gcc.patch --]
[-- Type: text/plain, Size: 1921 bytes --]

Index: libstdc++-v3/config/cpu/mips/atomicity.h
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/config/cpu/mips/atomicity.h,v
retrieving revision 1.9
diff -u -r1.9 atomicity.h
--- libstdc++-v3/config/cpu/mips/atomicity.h	27 Feb 2004 00:49:48 -0000	1.9
+++ libstdc++-v3/config/cpu/mips/atomicity.h	15 Mar 2004 23:55:31 -0000
@@ -35,7 +35,7 @@
   __attribute__ ((__unused__))
   __exchange_and_add(volatile _Atomic_word* __mem, int __val)
   {
-    _Atomic_word __result, __tmp;
+    _Atomic_word __result, __tmp, temp;
     
     __asm__ __volatile__
       ("/* Inline exchange & add */\n\t"
@@ -44,13 +44,14 @@
 #if _MIPS_SIM == _ABIO32
        ".set	mips2\n\t"
 #endif
-       "ll	%0,%3\n\t"
-       "addu	%1,%4,%0\n\t"
-       "sc	%1,%2\n\t"
+       "la      %3,%2\n\t"
+       "ll	%0,0(%3)\n\t"
+       "addu	%1,%5,%0\n\t"
+       "sc	%1,0(%3)\n\t"
        ".set	pop\n\t"
        "beqz	%1,1b\n\t"
        "/* End exchange & add */"
-       : "=&r"(__result), "=&r"(__tmp), "=m"(*__mem)
+       : "=&r"(__result), "=&r"(__tmp), "=m"(*__mem), "=r" (temp)
        : "m" (*__mem), "r"(__val));
     
     return __result;
@@ -60,7 +61,7 @@
   __attribute__ ((__unused__))
   __atomic_add(volatile _Atomic_word* __mem, int __val)
   {
-    _Atomic_word __result;
+    _Atomic_word __result, temp;
     
     __asm__ __volatile__
       ("/* Inline atomic add */\n\t"
@@ -69,13 +70,14 @@
 #if _MIPS_SIM == _ABIO32
        ".set	mips2\n\t"
 #endif
-       "ll	%0,%2\n\t"
-       "addu	%0,%3,%0\n\t"
-       "sc	%0,%1\n\t"
+       "la      %2,%1\n\t"
+       "ll	%0,0(%2)\n\t"
+       "addu	%0,%4,%0\n\t"
+       "sc	%0,0(%2)\n\t"
        ".set	pop\n\t"
        "beqz	%0,1b\n\t"
        "/* End atomic add */"
-       : "=&r"(__result), "=m"(*__mem)
+       : "=&r"(__result), "=m"(*__mem), "=r" (temp)
      : "m" (*__mem), "r"(__val));
   }
 } // namespace __gnu_cxx

Re: locking problem with mips atomicity

[Phil Edwards]

Things dealing with libstdc++-v3 should also be sent to that list.

I'll leave the guts of the patch to a MIPS maintainer to approve, but here
are some things you'll need to change:

> -    _Atomic_word __result, __tmp;
> +    _Atomic_word __result, __tmp, temp;

The name must have two leading underscores, so it would have to be __temp,
not temp.  Likewise for the second function.

Please consider renaming these to something more descriptive.  We know
they're temporary.  :-)  We don't know what they're used for without
studying the assembly.  (Feel free to rename tmp as well.)


> +       "la      %3,%2\n\t"
> +       "ll	%0,0(%3)\n\t"
> +       "addu	%1,%5,%0\n\t"
> +       "sc	%1,0(%3)\n\t"

Watch the extra space in the first line.  (Might as well get the cosmetic
aspects right from the beginning.)


Phil

-- 
Behind everything some further thing is found, forever; thus the tree behind
the bird, stone beneath soil, the sun behind Urth.  Behind our efforts, let
there be found our efforts.
              - Ascian saying, as related by Loyal to the Group of Seventeen

Re: locking problem with mips atomicity

[Michael Eager]

[-- Attachment #1: Type: text/plain, Size: 1225 bytes --]

Phil Edwards wrote:
> 
> Things dealing with libstdc++-v3 should also be sent to that list.

Thanks.

> 
> I'll leave the guts of the patch to a MIPS maintainer to approve, but here
> are some things you'll need to change:
> 
> > -    _Atomic_word __result, __tmp;
> > +    _Atomic_word __result, __tmp, temp;
> 
> The name must have two leading underscores, so it would have to be __temp,
> not temp.  Likewise for the second function.
> 
> Please consider renaming these to something more descriptive.  We know
> they're temporary.  :-)  We don't know what they're used for without
> studying the assembly.  (Feel free to rename tmp as well.)

Renamed temp to __addr, which should be more descriptive.  Renamed __tmp
to __sum, since it is the sum of the __mem value and __val.

Also added a changelog entry, which I'd forgotten.

> > +       "la      %3,%2\n\t"
> > +       "ll   %0,0(%3)\n\t"
> > +       "addu %1,%5,%0\n\t"
> > +       "sc   %1,0(%3)\n\t"
> 
> Watch the extra space in the first line.  (Might as well get the cosmetic
> aspects right from the beginning.)

Replaced spaces with a tab.

--
Michael Eager     eager@mvista.com	408-328-8426	
MontaVista Software, Inc. 1237 E. Arques Ave., Sunnyvale, CA  94085

[-- Attachment #2: FSF-gcc.patch --]
[-- Type: text/plain, Size: 2064 bytes --]

2004-03-10  Michael Eager  <eager@mvista.com>

	* libstdc++-v3/config/cpu/mips/atomicity.h:  Prevent reg
	loads between LL and SC instructions.

Index: libstdc++-v3/config/cpu/mips/atomicity.h
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/config/cpu/mips/atomicity.h,v
retrieving revision 1.9
diff -u -r1.9 atomicity.h
--- libstdc++-v3/config/cpu/mips/atomicity.h	27 Feb 2004 00:49:48 -0000	1.9
+++ libstdc++-v3/config/cpu/mips/atomicity.h	15 Mar 2004 23:55:31 -0000
@@ -35,7 +35,7 @@
   __attribute__ ((__unused__))
   __exchange_and_add(volatile _Atomic_word* __mem, int __val)
   {
-    _Atomic_word __result, __tmp;
+    _Atomic_word __result, __sum, __addr;
     
     __asm__ __volatile__
       ("/* Inline exchange & add */\n\t"
@@ -44,13 +44,14 @@
 #if _MIPS_SIM == _ABIO32
        ".set	mips2\n\t"
 #endif
-       "ll	%0,%3\n\t"
-       "addu	%1,%4,%0\n\t"
-       "sc	%1,%2\n\t"
+       "la	%3,%2\n\t"
+       "ll	%0,0(%3)\n\t"
+       "addu	%1,%5,%0\n\t"
+       "sc	%1,0(%3)\n\t"
        ".set	pop\n\t"
        "beqz	%1,1b\n\t"
        "/* End exchange & add */"
-       : "=&r"(__result), "=&r"(__tmp), "=m"(*__mem)
+       : "=&r"(__result), "=&r"(__sum), "=m"(*__mem), "=r" (__addr)
        : "m" (*__mem), "r"(__val));
     
     return __result;
@@ -60,7 +61,7 @@
   __attribute__ ((__unused__))
   __atomic_add(volatile _Atomic_word* __mem, int __val)
   {
-    _Atomic_word __result;
+    _Atomic_word __result, __addr;
     
     __asm__ __volatile__
       ("/* Inline atomic add */\n\t"
@@ -69,13 +70,14 @@
 #if _MIPS_SIM == _ABIO32
        ".set	mips2\n\t"
 #endif
-       "ll	%0,%2\n\t"
-       "addu	%0,%3,%0\n\t"
-       "sc	%0,%1\n\t"
+       "la	%2,%1\n\t"
+       "ll	%0,0(%2)\n\t"
+       "addu	%0,%4,%0\n\t"
+       "sc	%0,0(%2)\n\t"
        ".set	pop\n\t"
        "beqz	%0,1b\n\t"
        "/* End atomic add */"
-       : "=&r"(__result), "=m"(*__mem)
+       : "=&r"(__result), "=m"(*__mem), "=r" (__addr)
      : "m" (*__mem), "r"(__val));
   }
 } // namespace __gnu_cxx

Re: locking problem with mips atomicity

[Eric Christopher]

> 
> ______________________________________________________________________
> 2004-03-10  Michael Eager  <eager@mvista.com>
> 
> 	* libstdc++-v3/config/cpu/mips/atomicity.h:  Prevent reg
> 	loads between LL and SC instructions.

This is OK.

-eric

-- 
Eric Christopher <echristo@redhat.com>

Re: locking problem with mips atomicity

[Richard Henderson]

On Mon, Mar 15, 2004 at 03:57:02PM -0800, Michael Eager wrote:
> -       "ll	%0,%3\n\t"
> -       "addu	%1,%4,%0\n\t"
> -       "sc	%1,%2\n\t"
> +       "la      %3,%2\n\t"
> +       "ll	%0,0(%3)\n\t"
> +       "addu	%1,%5,%0\n\t"
> +       "sc	%1,0(%3)\n\t"
>         ".set	pop\n\t"
>         "beqz	%1,1b\n\t"
>         "/* End exchange & add */"
> -       : "=&r"(__result), "=&r"(__tmp), "=m"(*__mem)
> +       : "=&r"(__result), "=&r"(__tmp), "=m"(*__mem), "=r" (temp)

Better is to make the compiler force the address into a register.
It may already be that way, resulting in "la r3,0(r4)".


r~

Re: locking problem with mips atomicity

[Michael Eager]

Richard Henderson wrote:
> 
> On Mon, Mar 15, 2004 at 03:57:02PM -0800, Michael Eager wrote:
> > -       "ll   %0,%3\n\t"
> > -       "addu %1,%4,%0\n\t"
> > -       "sc   %1,%2\n\t"
> > +       "la      %3,%2\n\t"
> > +       "ll   %0,0(%3)\n\t"
> > +       "addu %1,%5,%0\n\t"
> > +       "sc   %1,0(%3)\n\t"
> >         ".set pop\n\t"
> >         "beqz %1,1b\n\t"
> >         "/* End exchange & add */"
> > -       : "=&r"(__result), "=&r"(__tmp), "=m"(*__mem)
> > +       : "=&r"(__result), "=&r"(__tmp), "=m"(*__mem), "=r" (temp)
> 
> Better is to make the compiler force the address into a register.
> It may already be that way, resulting in "la r3,0(r4)".

This is exactly what this code does.   

--
Michael Eager     eager@mvista.com	408-328-8426	
MontaVista Software, Inc. 1237 E. Arques Ave., Sunnyvale, CA  94085

Re: locking problem with mips atomicity

[Eric Christopher]

> > Better is to make the compiler force the address into a register.
> > It may already be that way, resulting in "la r3,0(r4)".
> 
> This is exactly what this code does.   

He means to force it in without using the la macro.

In fact, I think I'd prefer this.

-eric

-- 
Eric Christopher <echristo@redhat.com>

Re: locking problem with mips atomicity

[Michael Eager]

Eric Christopher wrote:
> 
> > > Better is to make the compiler force the address into a register.
> > > It may already be that way, resulting in "la r3,0(r4)".
> >
> > This is exactly what this code does.
> 
> He means to force it in without using the la macro.
> 
> In fact, I think I'd prefer this.

How?


--
Michael Eager     eager@mvista.com	408-328-8426	
MontaVista Software, Inc. 1237 E. Arques Ave., Sunnyvale, CA  94085

Re: locking problem with mips atomicity

[Richard Henderson]

On Tue, Mar 16, 2004 at 09:46:55AM -0800, Michael Eager wrote:
> How?

Add an additional "r"(__mem).


r~

Re: locking problem with mips atomicity

[Michael Eager]

[-- Attachment #1: Type: text/plain, Size: 362 bytes --]

Richard Henderson wrote:
> 
> On Tue, Mar 16, 2004 at 09:46:55AM -0800, Michael Eager wrote:
> > How?
> 
> Add an additional "r"(__mem).

Well, that's surprising.  I was able to get rid of all of the 
other changes. 

Revised patch attached.

--
Michael Eager     eager@mvista.com	408-328-8426	
MontaVista Software, Inc. 1237 E. Arques Ave., Sunnyvale, CA  94085

[-- Attachment #2: FSF-gcc.patch --]
[-- Type: text/plain, Size: 1009 bytes --]

2004-03-10  Michael Eager  <eager@mvista.com>

	* libstdc++-v3/config/cpu/mips/atomicity.h:  Prevent reg
	loads between LL and SC instructions.

Index: ./libstdc++-v3/config/cpu/mips/atomicity.h
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/config/cpu/mips/atomicity.h,v
retrieving revision 1.9
diff -u -r1.9 atomicity.h
--- ./libstdc++-v3/config/cpu/mips/atomicity.h	27 Feb 2004 00:49:48 -0000	1.9
+++ ./libstdc++-v3/config/cpu/mips/atomicity.h	16 Mar 2004 22:43:34 -0000
@@ -51,7 +51,7 @@
        "beqz	%1,1b\n\t"
        "/* End exchange & add */"
        : "=&r"(__result), "=&r"(__tmp), "=m"(*__mem)
-       : "m" (*__mem), "r"(__val));
+       : "m" (*__mem), "r"(__val), "r"(__mem));
     
     return __result;
   }
@@ -76,6 +76,6 @@
        "beqz	%0,1b\n\t"
        "/* End atomic add */"
        : "=&r"(__result), "=m"(*__mem)
-     : "m" (*__mem), "r"(__val));
+     : "m" (*__mem), "r"(__val), "r"(__mem));
   }
 } // namespace __gnu_cxx

Re: locking problem with mips atomicity

[Eric Christopher]

On Tue, 2004-03-16 at 14:44, Michael Eager wrote:
> Richard Henderson wrote:
> > 
> > On Tue, Mar 16, 2004 at 09:46:55AM -0800, Michael Eager wrote:
> > > How?
> > 
> > Add an additional "r"(__mem).
> 
> Well, that's surprising.  I was able to get rid of all of the 
> other changes. 

Yup. I blame myself for not noticing earlier.

OK. And thanks.

-eric

-- 
Eric Christopher <echristo@redhat.com>

Re: locking problem with mips atomicity

[Richard Henderson]

On Tue, Mar 16, 2004 at 02:44:15PM -0800, Michael Eager wrote:
> Well, that's surprising.  I was able to get rid of all of the 
> other changes. 

Not ok.  It's not *required* that the compiler reduce all of the
addresses for you.  In particular, this is more likely to fail
without optimization.

> -       : "m" (*__mem), "r"(__val));
> +       : "m" (*__mem), "r"(__val), "r"(__mem));

In addition to just adding the new argument, you have to actually
use it in the assembly.


r~

Re: locking problem with mips atomicity

[Eric Christopher]

On Tue, 2004-03-16 at 15:22, Richard Henderson wrote:
> On Tue, Mar 16, 2004 at 02:44:15PM -0800, Michael Eager wrote:
> > Well, that's surprising.  I was able to get rid of all of the 
> > other changes. 
> 
> Not ok.  It's not *required* that the compiler reduce all of the
> addresses for you.  In particular, this is more likely to fail
> without optimization.
> 
> > -       : "m" (*__mem), "r"(__val));
> > +       : "m" (*__mem), "r"(__val), "r"(__mem));
> 
> In addition to just adding the new argument, you have to actually
> use it in the assembly.

*shakes head*

I think i'm going to give up for today.

Thanks richard.

-eric

-- 
Eric Christopher <echristo@redhat.com>

Re: locking problem with mips atomicity

[Michael Eager]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]

Richard Henderson wrote:
> 
> On Tue, Mar 16, 2004 at 02:44:15PM -0800, Michael Eager wrote:
> > Well, that's surprising.  I was able to get rid of all of the
> > other changes.
> 
> Not ok.  It's not *required* that the compiler reduce all of the
> addresses for you.  In particular, this is more likely to fail
> without optimization.
> 
> > -       : "m" (*__mem), "r"(__val));
> > +       : "m" (*__mem), "r"(__val), "r"(__mem));
> 
> In addition to just adding the new argument, you have to actually
> use it in the assembly.

Here's yet one more version of the patch.

--
Michael Eager     eager@mvista.com	408-328-8426	
MontaVista Software, Inc. 1237 E. Arques Ave., Sunnyvale, CA  94085

[-- Attachment #2: FSF-gcc.patch --]
[-- Type: text/plain, Size: 1429 bytes --]

2004-03-16  Michael Eager  <eager@mvista.com>

	* libstdc++-v3/config/cpu/mips/atomicity.h:  Prevent reg
	loads between LL and SC instructions.

Index: ./libstdc++-v3/config/cpu/mips/atomicity.h
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/config/cpu/mips/atomicity.h,v
retrieving revision 1.9
diff -u -r1.9 atomicity.h
--- ./libstdc++-v3/config/cpu/mips/atomicity.h	27 Feb 2004 00:49:48 -0000	1.9
+++ ./libstdc++-v3/config/cpu/mips/atomicity.h	17 Mar 2004 00:17:48 -0000
@@ -44,14 +44,14 @@
 #if _MIPS_SIM == _ABIO32
        ".set	mips2\n\t"
 #endif
-       "ll	%0,%3\n\t"
+       "ll	%0,0(%5)\n\t"
        "addu	%1,%4,%0\n\t"
-       "sc	%1,%2\n\t"
+       "sc	%1,0(%5)\n\t"
        ".set	pop\n\t"
        "beqz	%1,1b\n\t"
        "/* End exchange & add */"
        : "=&r"(__result), "=&r"(__tmp), "=m"(*__mem)
-       : "m" (*__mem), "r"(__val));
+       : "m" (*__mem), "r"(__val), "r"(__mem));
     
     return __result;
   }
@@ -69,13 +69,13 @@
 #if _MIPS_SIM == _ABIO32
        ".set	mips2\n\t"
 #endif
-       "ll	%0,%2\n\t"
+       "ll	%0,0(%4)\n\t"
        "addu	%0,%3,%0\n\t"
-       "sc	%0,%1\n\t"
+       "sc	%0,0(%4)\n\t"
        ".set	pop\n\t"
        "beqz	%0,1b\n\t"
        "/* End atomic add */"
        : "=&r"(__result), "=m"(*__mem)
-     : "m" (*__mem), "r"(__val));
+     : "m" (*__mem), "r"(__val), "r"(__mem));
   }
 } // namespace __gnu_cxx

Re: locking problem with mips atomicity

[Phil Edwards]

Added libstdc++ to the cc list.


On Tue, Mar 16, 2004 at 04:18:10PM -0800, Michael Eager wrote:
> Here's yet one more version of the patch.
> 
> 	* libstdc++-v3/config/cpu/mips/atomicity.h:  Prevent reg
> 	loads between LL and SC instructions.

1)  libstdc++-v3 is temporarily frozen.  (This is /why/ you need to cc: the
    list, so that you'll know when it's open for commits again.)

2)  Remove 'libstdc++-v3/' from the front of the pathname, please, and
    add the log entry to libstdc++-v3/ChangeLog, not the toplevel file
    (when the library is unfrozen, of course).


Thanks.

-- 
Behind everything some further thing is found, forever; thus the tree behind
the bird, stone beneath soil, the sun behind Urth.  Behind our efforts, let
there be found our efforts.
              - Ascian saying, as related by Loyal to the Group of Seventeen

Re: locking problem with mips atomicity

[Michael Eager]

[-- Attachment #1: Type: text/plain, Size: 838 bytes --]

Phil Edwards wrote:
> 
> Added libstdc++ to the cc list.
> 
> On Tue, Mar 16, 2004 at 04:18:10PM -0800, Michael Eager wrote:
> > Here's yet one more version of the patch.
> >
> >       * libstdc++-v3/config/cpu/mips/atomicity.h:  Prevent reg
> >       loads between LL and SC instructions.
> 
> 1)  libstdc++-v3 is temporarily frozen.  (This is /why/ you need to cc: the
>     list, so that you'll know when it's open for commits again.)
> 
> 2)  Remove 'libstdc++-v3/' from the front of the pathname, please, and
>     add the log entry to libstdc++-v3/ChangeLog, not the toplevel file
>     (when the library is unfrozen, of course).

Updated patch attached.

I don't have check-in privs, so who can/should do this?

--
Michael Eager     eager@mvista.com	408-328-8426	
MontaVista Software, Inc. 1237 E. Arques Ave., Sunnyvale, CA  94085

[-- Attachment #2: FSF-gcc.patch --]
[-- Type: text/plain, Size: 1402 bytes --]

2004-03-16  Michael Eager  <eager@mvista.com>

	* config/cpu/mips/atomicity.h:  Prevent reg loads 
	between LL and SC instructions.

Index: config/cpu/mips/atomicity.h
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/config/cpu/mips/atomicity.h,v
retrieving revision 1.9
diff -u -r1.9 atomicity.h
--- ./libstdc++-v3/config/cpu/mips/atomicity.h	27 Feb 2004 00:49:48 -0000	1.9
+++ ./libstdc++-v3/config/cpu/mips/atomicity.h	17 Mar 2004 00:17:48 -0000
@@ -44,14 +44,14 @@
 #if _MIPS_SIM == _ABIO32
        ".set	mips2\n\t"
 #endif
-       "ll	%0,%3\n\t"
+       "ll	%0,0(%5)\n\t"
        "addu	%1,%4,%0\n\t"
-       "sc	%1,%2\n\t"
+       "sc	%1,0(%5)\n\t"
        ".set	pop\n\t"
        "beqz	%1,1b\n\t"
        "/* End exchange & add */"
        : "=&r"(__result), "=&r"(__tmp), "=m"(*__mem)
-       : "m" (*__mem), "r"(__val));
+       : "m" (*__mem), "r"(__val), "r"(__mem));
     
     return __result;
   }
@@ -69,13 +69,13 @@
 #if _MIPS_SIM == _ABIO32
        ".set	mips2\n\t"
 #endif
-       "ll	%0,%2\n\t"
+       "ll	%0,0(%4)\n\t"
        "addu	%0,%3,%0\n\t"
-       "sc	%0,%1\n\t"
+       "sc	%0,0(%4)\n\t"
        ".set	pop\n\t"
        "beqz	%0,1b\n\t"
        "/* End atomic add */"
        : "=&r"(__result), "=m"(*__mem)
-     : "m" (*__mem), "r"(__val));
+     : "m" (*__mem), "r"(__val), "r"(__mem));
   }
 } // namespace __gnu_cxx

Re: locking problem with mips atomicity

[Phil Edwards]

On Tue, Mar 16, 2004 at 04:51:26PM -0800, Michael Eager wrote:
> Phil Edwards wrote:
> > 
> > 1)  libstdc++-v3 is temporarily frozen.  (This is /why/ you need to cc: the
> >     list, so that you'll know when it's open for commits again.)
> > 
> > 2)  Remove 'libstdc++-v3/' from the front of the pathname, please, and
> >     add the log entry to libstdc++-v3/ChangeLog, not the toplevel file
> >     (when the library is unfrozen, of course).
> 
> Updated patch attached.
> 
> I don't have check-in privs, so who can/should do this?

I can do it.  Things should be thawed again in a day or so.


Phil

-- 
Behind everything some further thing is found, forever; thus the tree behind
the bird, stone beneath soil, the sun behind Urth.  Behind our efforts, let
there be found our efforts.
              - Ascian saying, as related by Loyal to the Group of Seventeen

Re: locking problem with mips atomicity

[Richard Henderson]

On Tue, Mar 16, 2004 at 04:18:10PM -0800, Michael Eager wrote:
> 	* libstdc++-v3/config/cpu/mips/atomicity.h:  Prevent reg
> 	loads between LL and SC instructions.

Looks good now.


r~

Re: locking problem with mips atomicity

[Michael Eager]

[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]

Phil Edwards wrote:
> 
> On Tue, Mar 16, 2004 at 04:51:26PM -0800, Michael Eager wrote:
> > Phil Edwards wrote:
> > >
> > > 1)  libstdc++-v3 is temporarily frozen.  (This is /why/ you need to cc: the
> > >     list, so that you'll know when it's open for commits again.)
> > >
> > > 2)  Remove 'libstdc++-v3/' from the front of the pathname, please, and
> > >     add the log entry to libstdc++-v3/ChangeLog, not the toplevel file
> > >     (when the library is unfrozen, of course).
> >
> > Updated patch attached.
> >
> > I don't have check-in privs, so who can/should do this?
> 
> I can do it.  Things should be thawed again in a day or so.

Thanks to a bit help from Paul Koenig and a bit of head scratching,
I've revised the patch.  It now only has constraints which are referenced.
I added a "memory" constraint.

Revised patch attached, and revised test program to invoke __atomic_add.

--
Michael Eager     eager@mvista.com	408-328-8426	
MontaVista Software, Inc. 1237 E. Arques Ave., Sunnyvale, CA  94085

[-- Attachment #2: FSF-gcc.patch --]
[-- Type: text/plain, Size: 1541 bytes --]

2004-03-16  Michael Eager  <eager@mvista.com>

	* config/cpu/mips/atomicity.h:  Prevent reg loads 
	between LL and SC instructions.

Index: config/cpu/mips/atomicity.h
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/config/cpu/mips/atomicity.h,v
retrieving revision 1.9
diff -u -r1.9 atomicity.h
--- ./libstdc++-v3/config/cpu/mips/atomicity.h	27 Feb 2004 00:49:48 -0000	1.9
+++ ./libstdc++-v3/config/cpu/mips/atomicity.h	17 Mar 2004 22:34:09 -0000
@@ -44,14 +44,15 @@
 #if _MIPS_SIM == _ABIO32
        ".set	mips2\n\t"
 #endif
-       "ll	%0,%3\n\t"
-       "addu	%1,%4,%0\n\t"
-       "sc	%1,%2\n\t"
+       "ll	%0,0(%2)\n\t"
+       "addu	%1,%3,%0\n\t"
+       "sc	%1,0(%2)\n\t"
        ".set	pop\n\t"
        "beqz	%1,1b\n\t"
        "/* End exchange & add */"
-       : "=&r"(__result), "=&r"(__tmp), "=m"(*__mem)
-       : "m" (*__mem), "r"(__val));
+       : "=&r"(__result), "=&r"(__tmp), "+r"(__mem)
+       : "r"(__val)
+       :  "memory" );
     
     return __result;
   }
@@ -69,13 +70,14 @@
 #if _MIPS_SIM == _ABIO32
        ".set	mips2\n\t"
 #endif
-       "ll	%0,%2\n\t"
-       "addu	%0,%3,%0\n\t"
-       "sc	%0,%1\n\t"
+       "ll	%0,0(%1)\n\t"
+       "addu	%0,%2,%0\n\t"
+       "sc	%0,0(%1)\n\t"
        ".set	pop\n\t"
        "beqz	%0,1b\n\t"
        "/* End atomic add */"
-       : "=&r"(__result), "=m"(*__mem)
-     : "m" (*__mem), "r"(__val));
+       : "=&r"(__result), "+r"(__mem)
+       : "r"(__val)
+       : "memory" );
   }
 } // namespace __gnu_cxx

[-- Attachment #3: mcount.c --]
[-- Type: text/plain, Size: 324 bytes --]

#include "atomicity.h"

struct gmonparam {
        long int state;
};

extern struct gmonparam _gmonparam ;


static void __attribute__ ((__used__)) __mcount ()
{
        register struct gmonparam *p;

        p = &_gmonparam;


        if (! compare_and_swap (&p->state, 0, 1))
          return;

        p->state = 0;
}

Re: locking problem with mips atomicity

[Michael Eager]

[-- Attachment #1: Type: text/plain, Size: 1053 bytes --]

Phil Edwards wrote:
> 
> On Tue, Mar 16, 2004 at 04:51:26PM -0800, Michael Eager wrote:
> > Phil Edwards wrote:
> > >
> > > 1)  libstdc++-v3 is temporarily frozen.  (This is /why/ you need to cc: the
> > >     list, so that you'll know when it's open for commits again.)
> > >
> > > 2)  Remove 'libstdc++-v3/' from the front of the pathname, please, and
> > >     add the log entry to libstdc++-v3/ChangeLog, not the toplevel file
> > >     (when the library is unfrozen, of course).
> >
> > Updated patch attached.
> >
> > I don't have check-in privs, so who can/should do this?
> 
> I can do it.  Things should be thawed again in a day or so.

With a bit of help from Paul Koning and a bit of head scratching, I've
modified the patch to only have constraints which are referenced.  I also
added a "memory" clobber.

Revised patch attached (hopefully, the last) and revised test case which
also invokes __atomic_add.

Thanks, Paul!

--
Michael Eager     eager@mvista.com	408-328-8426	
MontaVista Software, Inc. 1237 E. Arques Ave., Sunnyvale, CA  94085

[-- Attachment #2: FSF-gcc.patch --]
[-- Type: text/plain, Size: 1541 bytes --]

2004-03-16  Michael Eager  <eager@mvista.com>

	* config/cpu/mips/atomicity.h:  Prevent reg loads 
	between LL and SC instructions.

Index: config/cpu/mips/atomicity.h
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/config/cpu/mips/atomicity.h,v
retrieving revision 1.9
diff -u -r1.9 atomicity.h
--- ./libstdc++-v3/config/cpu/mips/atomicity.h	27 Feb 2004 00:49:48 -0000	1.9
+++ ./libstdc++-v3/config/cpu/mips/atomicity.h	17 Mar 2004 22:34:09 -0000
@@ -44,14 +44,15 @@
 #if _MIPS_SIM == _ABIO32
        ".set	mips2\n\t"
 #endif
-       "ll	%0,%3\n\t"
-       "addu	%1,%4,%0\n\t"
-       "sc	%1,%2\n\t"
+       "ll	%0,0(%2)\n\t"
+       "addu	%1,%3,%0\n\t"
+       "sc	%1,0(%2)\n\t"
        ".set	pop\n\t"
        "beqz	%1,1b\n\t"
        "/* End exchange & add */"
-       : "=&r"(__result), "=&r"(__tmp), "=m"(*__mem)
-       : "m" (*__mem), "r"(__val));
+       : "=&r"(__result), "=&r"(__tmp), "+r"(__mem)
+       : "r"(__val)
+       :  "memory" );
     
     return __result;
   }
@@ -69,13 +70,14 @@
 #if _MIPS_SIM == _ABIO32
        ".set	mips2\n\t"
 #endif
-       "ll	%0,%2\n\t"
-       "addu	%0,%3,%0\n\t"
-       "sc	%0,%1\n\t"
+       "ll	%0,0(%1)\n\t"
+       "addu	%0,%2,%0\n\t"
+       "sc	%0,0(%1)\n\t"
        ".set	pop\n\t"
        "beqz	%0,1b\n\t"
        "/* End atomic add */"
-       : "=&r"(__result), "=m"(*__mem)
-     : "m" (*__mem), "r"(__val));
+       : "=&r"(__result), "+r"(__mem)
+       : "r"(__val)
+       : "memory" );
   }
 } // namespace __gnu_cxx

[-- Attachment #3: mcount.c --]
[-- Type: text/plain, Size: 324 bytes --]

#include "atomicity.h"

struct gmonparam {
        long int state;
};

extern struct gmonparam _gmonparam ;


static void __attribute__ ((__used__)) __mcount ()
{
        register struct gmonparam *p;

        p = &_gmonparam;


        if (! compare_and_swap (&p->state, 0, 1))
          return;

        p->state = 0;
}

Re: locking problem with mips atomicity

[Richard Henderson]

On Wed, Mar 17, 2004 at 02:45:08PM -0800, Michael Eager wrote:
> +       "ll	%0,0(%2)\n\t"
> +       "addu	%1,%3,%0\n\t"
> +       "sc	%1,0(%2)\n\t"
>         ".set	pop\n\t"
>         "beqz	%1,1b\n\t"
>         "/* End exchange & add */"
> +       : "=&r"(__result), "=&r"(__tmp), "+r"(__mem)

Why the in-out constraint on __mem?  You don't modify %2.


r~

Re: locking problem with mips atomicity

[Michael Eager]

[-- Attachment #1: Type: text/plain, Size: 555 bytes --]

Richard Henderson wrote:
> 
> On Wed, Mar 17, 2004 at 02:45:08PM -0800, Michael Eager wrote:
> > +       "ll   %0,0(%2)\n\t"
> > +       "addu %1,%3,%0\n\t"
> > +       "sc   %1,0(%2)\n\t"
> >         ".set pop\n\t"
> >         "beqz %1,1b\n\t"
> >         "/* End exchange & add */"
> > +       : "=&r"(__result), "=&r"(__tmp), "+r"(__mem)
> 
> Why the in-out constraint on __mem?  You don't modify %2.

OK, changed it to only input.

--
Michael Eager     eager@mvista.com	408-328-8426	
MontaVista Software, Inc. 1237 E. Arques Ave., Sunnyvale, CA  94085

[-- Attachment #2: FSF-gcc.patch --]
[-- Type: text/plain, Size: 1554 bytes --]

2004-03-16  Michael Eager  <eager@mvista.com>

	* config/cpu/mips/atomicity.h:  Prevent reg loads 
	between LL and SC instructions.

Index: ./libstdc++-v3/config/cpu/mips/atomicity.h
===================================================================
RCS file: /cvs/gcc/gcc/libstdc++-v3/config/cpu/mips/atomicity.h,v
retrieving revision 1.9
diff -u -r1.9 atomicity.h
--- ./libstdc++-v3/config/cpu/mips/atomicity.h	27 Feb 2004 00:49:48 -0000	1.9
+++ ./libstdc++-v3/config/cpu/mips/atomicity.h	18 Mar 2004 01:01:26 -0000
@@ -44,14 +44,15 @@
 #if _MIPS_SIM == _ABIO32
        ".set	mips2\n\t"
 #endif
-       "ll	%0,%3\n\t"
-       "addu	%1,%4,%0\n\t"
-       "sc	%1,%2\n\t"
+       "ll	%0,0(%2)\n\t"
+       "addu	%1,%3,%0\n\t"
+       "sc	%1,0(%2)\n\t"
        ".set	pop\n\t"
        "beqz	%1,1b\n\t"
        "/* End exchange & add */"
-       : "=&r"(__result), "=&r"(__tmp), "=m"(*__mem)
-       : "m" (*__mem), "r"(__val));
+       : "=&r"(__result), "=&r"(__tmp)
+       : "r"(__mem), "r"(__val)
+       :  "memory" );
     
     return __result;
   }
@@ -69,13 +70,14 @@
 #if _MIPS_SIM == _ABIO32
        ".set	mips2\n\t"
 #endif
-       "ll	%0,%2\n\t"
-       "addu	%0,%3,%0\n\t"
-       "sc	%0,%1\n\t"
+       "ll	%0,0(%1)\n\t"
+       "addu	%0,%2,%0\n\t"
+       "sc	%0,0(%1)\n\t"
        ".set	pop\n\t"
        "beqz	%0,1b\n\t"
        "/* End atomic add */"
-       : "=&r"(__result), "=m"(*__mem)
-     : "m" (*__mem), "r"(__val));
+       : "=&r"(__result)
+       : "r"(__mem), "r"(__val)
+       : "memory" );
   }
 } // namespace __gnu_cxx

Re: locking problem with mips atomicity

[Richard Sandiford]

Michael Eager <eager@mvista.com> writes:
> I've revised the patch.  It now only has constraints which are referenced.
> I added a "memory" constraint.

Sorry if I've missed the explanation, but why is using a memory clobber
an improvement over an unused memory operand?

Richard

Re: locking problem with mips atomicity

[Michael Eager]

Richard Sandiford wrote:
> 
> Michael Eager <eager@mvista.com> writes:
> > I've revised the patch.  It now only has constraints which are referenced.
> > I added a "memory" constraint.
> 
> Sorry if I've missed the explanation, but why is using a memory clobber
> an improvement over an unused memory operand?

The location which is clobbered is unknown.  From what I understand, an
unused memory operand is ignored.  

--
Michael Eager     eager@mvista.com	408-328-8426	
MontaVista Software, Inc. 1237 E. Arques Ave., Sunnyvale, CA  94085

Re: locking problem with mips atomicity

[Richard Henderson]

On Thu, Mar 18, 2004 at 11:04:19AM +0000, Richard Sandiford wrote:
> Sorry if I've missed the explanation, but why is using a memory clobber
> an improvement over an unused memory operand?

To make sure that any memory protected by the mutex is not
moved across the acquisition of the lock.


r~

Re: locking problem with mips atomicity

[Richard Sandiford]

Richard Henderson <rth@redhat.com> writes:
> On Thu, Mar 18, 2004 at 11:04:19AM +0000, Richard Sandiford wrote:
>> Sorry if I've missed the explanation, but why is using a memory clobber
>> an improvement over an unused memory operand?
>
> To make sure that any memory protected by the mutex is not
> moved across the acquisition of the lock.

Ah, right, sorry for being dense ;)

Re: locking problem with mips atomicity

[Phil Edwards]

On Wed, Mar 17, 2004 at 05:01:33PM -0800, Michael Eager wrote:
> > 
> > Why the in-out constraint on __mem?  You don't modify %2.
> 
> OK, changed it to only input.

Since it seems to be the latest, this is the version I'll check in to the
trunk.  There isn't an open regression PR for this (is there?), so it'll
be queued for 3.4.1.

oPhil

-- 
Behind everything some further thing is found, forever; thus the tree behind
the bird, stone beneath soil, the sun behind Urth.  Behind our efforts, let
there be found our efforts.
              - Ascian saying, as related by Loyal to the Group of Seventeen

Re: locking problem with mips atomicity

[Michael Eager]

Phil Edwards wrote:
> 
> On Wed, Mar 17, 2004 at 05:01:33PM -0800, Michael Eager wrote:
> > >
> > > Why the in-out constraint on __mem?  You don't modify %2.
> >
> > OK, changed it to only input.
> 
> Since it seems to be the latest, this is the version I'll check in to the
> trunk.  There isn't an open regression PR for this (is there?), so it'll
> be queued for 3.4.1.

It's not a regression.   No PR.  I can create one if you want.

--
Michael Eager     eager@mvista.com	408-328-8426	
MontaVista Software, Inc. 1237 E. Arques Ave., Sunnyvale, CA  94085

Re: locking problem with mips atomicity

[Phil Edwards]

On Fri, Mar 19, 2004 at 03:01:33PM -0800, Michael Eager wrote:
> Phil Edwards wrote:
> > 
> > On Wed, Mar 17, 2004 at 05:01:33PM -0800, Michael Eager wrote:
> > > >
> > > > Why the in-out constraint on __mem?  You don't modify %2.
> > >
> > > OK, changed it to only input.
> > 
> > Since it seems to be the latest, this is the version I'll check in to the
> > trunk.  There isn't an open regression PR for this (is there?), so it'll
> > be queued for 3.4.1.
> 
> It's not a regression.   No PR.  I can create one if you want.

If there's no regression, then I (personally) wouldn't bother, but one of
the bugmasters may request otherwise.

If I don't remember to apply it once 3.4 is out, please remind me.
I'm trying to keep all the queued-up patches in one place, but some may
slip through.

I've just applied the patch to the trunk.

-- 
Behind everything some further thing is found, forever; thus the tree behind
the bird, stone beneath soil, the sun behind Urth.  Behind our efforts, let
there be found our efforts.
              - Ascian saying, as related by Loyal to the Group of Seventeen