[parisc-linux] missing barrier in _raw_spin_lock?

[parisc-linux] missing barrier in _raw_spin_lock?

[Arnd Bergmann]

We stumbled over a problem on s390 that can cause random memory corruption
under high load on SMP. It turned out to be a missing on :"memory" clobber
on the _raw_spin_lock primitive.

As far as I can see, the same problem is in the parisc spinlock definition
in linux-2.6.1, but none of the other architectures.

The code below demonstrates the problem. With the broken spinlock, the
compiler does not emit code for the second "if" or for the assignment.
Similar code can be found in mempool_free().

	Arnd <><

----
#include <linux/spinlock.h>

static int x;
static spinlock_t lock;

void test(void)
{
        if (x) {
                spin_lock(&lock);
                if (!x)
                        x = 0x1234;
                spin_unlock(&lock);
        }
}

Re: [parisc-linux] missing barrier in _raw_spin_lock?

[Joel Soete]

Arnd Bergmann wrote:
> We stumbled over a problem on s390 that can cause random memory corruption
> under high load on SMP. It turned out to be a missing on :"memory" clobber
> on the _raw_spin_lock primitive.
> 
> As far as I can see, the same problem is in the parisc spinlock definition
> in linux-2.6.1, but none of the other architectures.
> 
> The code below demonstrates the problem. With the broken spinlock, the
> compiler does not emit code for the second "if" or for the assignment.
> Similar code can be found in mempool_free().
> 
> 	Arnd <><
> 
> ----
> #include <linux/spinlock.h>
> 
hmm yet another stupid question of mine: what would we have to use _linux_/spinlock.h or _asm_/spinlock.h

Thanks in advance,
	Joel


> static int x;
> static spinlock_t lock;
> 
> void test(void)
> {
>         if (x) {
>                 spin_lock(&lock);
>                 if (!x)
>                         x = 0x1234;
>                 spin_unlock(&lock);
>         }
> }
> 
> _______________________________________________
> parisc-linux mailing list
> parisc-linux@lists.parisc-linux.org
> http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
> 

Re: [parisc-linux] missing barrier in _raw_spin_lock?

[Joel Soete]

Joel Soete wrote:
> 
> 
> Arnd Bergmann wrote:
> 
>> We stumbled over a problem on s390 that can cause random memory 
>> corruption
>> under high load on SMP. It turned out to be a missing on :"memory" 
>> clobber
>> on the _raw_spin_lock primitive.
>>
>> As far as I can see, the same problem is in the parisc spinlock 
>> definition
>> in linux-2.6.1, but none of the other architectures.
>>
>> The code below demonstrates the problem. With the broken spinlock, the
>> compiler does not emit code for the second "if" or for the assignment.
>> Similar code can be found in mempool_free().
>>
>>     Arnd <><
>>
>> ----
>> #include <linux/spinlock.h>
>>
> hmm yet another stupid question of mine: what would we have to use 
> _linux_/spinlock.h or _asm_/spinlock.h
Oops appologies: the answer is in linux/spinlock (i was just confused because for my c110 I didn't configure SMP)

Thanks for your understand,
	Joel

> 
> Thanks in advance,
>     Joel
> 
> 
>> static int x;
>> static spinlock_t lock;
>>
>> void test(void)
>> {
>>         if (x) {
>>                 spin_lock(&lock);
>>                 if (!x)
>>                         x = 0x1234;
>>                 spin_unlock(&lock);
>>         }
>> }
>>
>> _______________________________________________
>> parisc-linux mailing list
>> parisc-linux@lists.parisc-linux.org
>> http://lists.parisc-linux.org/mailman/listinfo/parisc-linux
>>
> 

Re: [parisc-linux] missing barrier in _raw_spin_lock?

[John David Anglin]

> >> static int x;
> >> static spinlock_t lock;
> >>
> >> void test(void)
> >> {
> >>         if (x) {
> >>                 spin_lock(&lock);
> >>                 if (!x)
> >>                         x = 0x1234;
> >>                 spin_unlock(&lock);
> >>         }
> >> }

The test is wrong.  Static variables are initialized to 0.  So, the first
`if' is never true and the code will optimize to a return instruction.

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)

Re: [parisc-linux] missing barrier in _raw_spin_lock?

[Grant Grundler]

On Sat, Jan 24, 2004 at 04:15:44PM -0500, John David Anglin wrote:
> > >> static int x;
> > >> static spinlock_t lock;
...
> The test is wrong.  Static variables are initialized to 0.  So, the first
> `if' is never true and the code will optimize to a return instruction.

In any case, that should be
	static spinlock_t lock = SPIN_LOCK_UNLOCKED;

(On parisc UNLOCKED == 1, not 0 like the rest of the world)

Is gcc really that smart that it knows static variables are 0?
The would be amazing to me because I thought linker magic could
twiddle stuff like this. Anyway, gcc doesn't seem to be that smart:

# diff t1.c t2.c
3c3
< static int x=1;
---
> static int x;

# diff t1.S t2.S
3,7d2
<       .align 4
<       .type   x, @object
<       .size   x, 4
< x:
<       .word   1
54a50,51
>       .local  x
>       .comm   x,4,4


BTW, the .S diff is the same for both -O2 and -O3 output.
Maybe other compile options would further optimize.


Arndt,
In any case, thanks for raising the issue.
I hadn't looked at the asm code in a long time.

The key bit is __ldcw() in asm/system.h:

/* LDCW, the only atomic read-write operation PA-RISC has. *sigh*.  */
#define __ldcw(a) ({ \
        unsigned __ret; \
        __asm__ __volatile__("ldcw 0(%1),%0" : "=r" (__ret) : "r" (a)); \
        __ret; \
})

I suspect (but am not certain) Arnd is right.
We indicate __ret is getting clobbered (=r) and that 'a' is an input ("r").
I don't see anything indicating we changed the memory location.

But I don't see the problem.  The t2.S output looks correct.
The "if (!x) {...}" code is generated:

...
.L18:
        ldw RR'x-$global$(%r21),%r20
        comib,<> 0,%r20,.L13
        ldi 4660,%r25
        stw %r25,RR'x-$global$(%r21)
...

BTW, PARISC still doesn't support CONFIG_SMP (cpu init sequence)
on 2.6 kernels. SMP works (mostly) on 2.4 kernels.

Here's how I generated t1.S output and it's not completely trivial.
One has to:
	o cp /usr/src/linux-2.6/include/linux/autoconf.h ./linux/
	o vi linux/autoconf.hl
		- change "#undef CONFIG_SMP" to #define
		- add "#define CONFIG_NR_CPUS 4"
	o gcc -O3 -I. -I/usr/src/linux-2.6/include/ -S t1.c -o t1.S

I've put the whole mess on
	http://iou.parisc-linux.org/~grundler/asm-memory/


BTW, I notice several other asm() in linux/asm-parisc/system.h which
specify ': "memory"' but the instructions used are register only.
Notably the ssm/rsm/mtsm insn's.
Any comment if those asm() *do NOT* need ': "memory"'?

thanks,
grant

Re: [parisc-linux] missing barrier in _raw_spin_lock?

[John David Anglin]

>  Anyway, gcc doesn't seem to be that smart:

Right, it not that smart but it does optimize certain memory references.

> The key bit is __ldcw() in asm/system.h:
> 
> /* LDCW, the only atomic read-write operation PA-RISC has. *sigh*.  */
> #define __ldcw(a) ({ \
>         unsigned __ret; \
>         __asm__ __volatile__("ldcw 0(%1),%0" : "=r" (__ret) : "r" (a)); \
>         __ret; \
> })

In some version of pthreads, we used

#define __ldcw(a) ({ \
  unsigned int __ret;                                                   \
  __asm__ __volatile__("ldcw 0(%2),%0"                                  \
		      : "=r" (__ret), "=m" (*(a)) : "r" (a));           \
  __ret;                                                                \
})

This tells gcc explicitly what memory is affected by the asm.  However,
it doesn't provide a memory barrier.  I think this isn't necessary in
pthreads because it provides a separate macro define for this.

This is what the gcc documentation says about asms:

If your assembler instructions access memory in an unpredictable
fashion, add `memory' to the list of clobbered registers.  This
will cause GCC to not keep memory values cached in registers across the
assembler instruction and not optimize stores or loads to that memory.
You will also want to add the `volatile' keyword if the memory
affected is not listed in the inputs or outputs of the `asm', as
the `memory' clobber does not count as a side-effect of the
`asm'.  If you know how large the accessed memory is, you can add
it as input or output but if this is not known, you should add
`memory'.  As an example, if you access ten bytes of a string, you
can use a memory input like:

> I suspect (but am not certain) Arnd is right.
> We indicate __ret is getting clobbered (=r) and that 'a' is an input ("r").
> I don't see anything indicating we changed the memory location.

It's not really the memory accessed by the ldcw that's at issue here.
If `x' is a location that can change between the first test and a successful
lock (SMP), then you want a memory barrier in __ldcw so that the memory
value of x is not cached in a register across the lock.

The following revision to the test program demonstrates the problem
of a memory value carried into the lock.

#include <linux/spinlock.h>

static spinlock_t lock = SPIN_LOCK_UNLOCKED;

void test(int *x)
{
  if (*x)
    {
      spin_lock(&lock);
      if (!*x)
	*x = 0x1234;
      spin_unlock(&lock);
    }
}

We need the "memory" clobber in SMP.  I think the macro should be:

#define __ldcw(a) ({ \
  unsigned int __ret;							 \
  __asm__ __volatile__("ldcw 0(%2),%0"					 \
		      : "=r" (__ret), "=m" (*(a)) : "r" (a) : "memory"); \
  __ret;								 \
})

Dave
-- 
J. David Anglin                                  dave.anglin@nrc-cnrc.gc.ca
National Research Council of Canada              (613) 990-0752 (FAX: 952-6602)