[Bug nptl/16432] New: pthread_spin_unlock should imply a memory barrier

[Bug nptl/16432] New: pthread_spin_unlock should imply a memory barrier

[mikulas at artax dot karlin.mff.cuni.cz]

https://sourceware.org/bugzilla/show_bug.cgi?id=16432

            Bug ID: 16432
           Summary: pthread_spin_unlock should imply a memory barrier
           Product: glibc
           Version: 2.17
            Status: NEW
          Severity: normal
          Priority: P2
         Component: nptl
          Assignee: unassigned at sourceware dot org
          Reporter: mikulas at artax dot karlin.mff.cuni.cz
                CC: drepper.fsp at gmail dot com

This specification lists functions that should synchronize memory:
http://www.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_11

One of the functions is pthrad_spin_unlock. In glibc on i386, x86-64 and x32,
pthread_spin_unlock is an alias for pthread_spin_init. pthread_spin_init
doesn't synchronize memory, so it violates the specification.



Note: in the Linux kernel (or other custom environments), there are acquire and
release semantics for locking functions defined in the following way:

mutex or spinlock locking has "acquire" consistency semantics - that means that
access_memory();spin_lock() can be reordered to spin_lock();access_memory()

mutex or spinlock unlocking has "release" consistency semantics - that means
that spin_unlock();access_memory() can be reordered to
access_memory();spin_unlock()


The current implementation of pthread_spin_unlock in glibc conforms to the
"release" consistency semantics (that is, the processor can move memory
accesses before the call to pthread_spin_unlock, but it will not move any
memory access past the call to pthread_spin_unlock).

However, the posix document doesn't mention "acquire" or "release" semantics,
it just states that all specified functions should synchronize memory, so you
should implement them to perform a full memory barrier.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16432] pthread_spin_unlock should imply a memory barrier

[triegel at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16432

Torvald Riegel <triegel at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bugdal at aerifal dot cx,
                   |                            |triegel at redhat dot com

--- Comment #1 from Torvald Riegel <triegel at redhat dot com> ---
IMO, POSIX is wrong in requiring all synchronization functions to "synchronize
memory".  POSIX essentially requires all of these functions to contain a full
memory barrier, including lock acquisition and lock release functions.  This
would slow them down significantly in the uncontended case, just for the
benefit of use cases that aren't about locking but "abusing" locks for barriers
(e.g., to do Dekker(-like) synchronization).

I've raised this topic on the C11/POSIX aligment list.  It hasn't been
discussed yet, but I strongly believe that lock acquire/release should not have
full barrier (ie, memory_order_seq_cst semantics).

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16432] pthread_spin_unlock should imply a memory barrier

[mikulas at artax dot karlin.mff.cuni.cz]

https://sourceware.org/bugzilla/show_bug.cgi?id=16432

--- Comment #2 from Mikulas Patocka <mikulas at artax dot karlin.mff.cuni.cz> ---
Yes, I was looking at this function for the purpose of "abusing" it for a
portable memory barrier.

Unfortunatelly, POSIX doesn't provide any portable memory barrier. C11 has
botched barriers - even the strongest atomic model __ATOMIC_SEQ_CST
synchronizes only with locks an other atomic accesses, it doesn't synchronize
with normal memory references. Currently, there is no way to do a memory
barrier portably.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16432] pthread_spin_unlock should imply a memory barrier

[triegel at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16432

--- Comment #3 from Torvald Riegel <triegel at redhat dot com> ---
(In reply to Mikulas Patocka from comment #2)
> Yes, I was looking at this function for the purpose of "abusing" it for a
> portable memory barrier.

Please don't :)  It won't even help you do that because ...

> Unfortunatelly, POSIX doesn't provide any portable memory barrier. C11 has
> botched barriers - even the strongest atomic model __ATOMIC_SEQ_CST
> synchronizes only with locks an other atomic accesses, it doesn't
> synchronize with normal memory references. Currently, there is no way to do
> a memory barrier portably.

... you always have to consider the compiler too.  C11 requires data-race-free
programs -- without that, the compiler would be *severely* restricted in which
optimizations it can apply to normal code.  Thus, C11's memory model isn't
"botched" at all but is really what you want to use.  I suggest to learn how it
works and why it's built that way.  For example, if you use proper atomic
accesses for the data that is accessed concurrently by several threads, C11's
fences are perfectly fine.
Even glibc will likely use the C11 model in the future.

I'm keeping the bug open because the practice vs. POSIX wording issue still
stands.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16432] pthread_spin_unlock should imply a memory barrier

[mikulas at artax dot karlin.mff.cuni.cz]

https://sourceware.org/bugzilla/show_bug.cgi?id=16432

--- Comment #4 from Mikulas Patocka <mikulas at artax dot karlin.mff.cuni.cz> ---
Suppose that I need to create some structure using non-atomic accesses, then
execute a memory barrier and then publish pointer to the structure using atomic
operation. Something like this:

#include <stdatomic.h>
#include <stdlib.h>

struct s {
        int a;
        int b;
        int c;
        int d;
        struct something1 *e;
        struct something2 *f;
};

struct s * _Atomic ptr = NULL;

void fn(struct something1 *q1, struct something2 *q2)
{
        struct s *s = malloc(sizeof(struct s));
        s->a = 1;
        s->b = 2;
        s->c = 3;
        s->d = 4;
        s->e = q1;
        s->f = q2;
        atomic_store_explicit(&ptr, s, __ATOMIC_SEQ_CST);
}

According to the C11 standard this program is wrong. If I wanted to write the
program correctly according to the standard, then every member of the structure
s would have to be _Atomic. Furthermore, every member of structures something1
and something2 would have to be _Atomic. Formally speaking, every variable
reachable via pointers from the structure s would have to be declared with the
_Atomic prefix.

So, the standard is really botched and unusable for this purpose :-(

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16432] pthread_spin_unlock should imply a memory barrier

[triegel at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16432

--- Comment #5 from Torvald Riegel <triegel at redhat dot com> ---
(In reply to Mikulas Patocka from comment #4)
> Suppose that I need to create some structure using non-atomic accesses, then
> execute a memory barrier and then publish pointer to the structure using
> atomic operation.

Sure you can do that.

>         s->f = q2;
>         atomic_store_explicit(&ptr, s, __ATOMIC_SEQ_CST);

And that's a release store too, so you can do
  p = atomic_load_explicit(&ptr, memory_order_acquire);
  if (p!=NULL) foo = p->a;

> According to the C11 standard this program is wrong. If I wanted to write
> the program correctly according to the standard, then every member of the
> structure s would have to be _Atomic. Furthermore, every member of
> structures something1 and something2 would have to be _Atomic. Formally
> speaking, every variable reachable via pointers from the structure s would
> have to be declared with the _Atomic prefix.

No.  The load of ptr has to be atomic, but happens-before holds for nonatomic
accesses too.  Please learn about how the model works.

> So, the standard is really botched and unusable for this purpose :-(

In any case, this bug report is not a place to discuss the C11 memory model.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16432] pthread_spin_unlock should imply a memory barrier

[mikulas at artax dot karlin.mff.cuni.cz]

https://sourceware.org/bugzilla/show_bug.cgi?id=16432

--- Comment #6 from Mikulas Patocka <mikulas at artax dot karlin.mff.cuni.cz> ---
Another example, see this:

#include <stdatomic.h>
#include <stdlib.h>
#include <string.h>

struct s {
        char str[100];
};

struct s * _Atomic ptr = NULL;

void fn(void)
{
        struct s *s = malloc(sizeof(struct s));
        strcpy(s->str, "Hello World!");
        atomic_store_explicit(&ptr, s, __ATOMIC_SEQ_CST);
}

... according to the C11 standard, the strcpy instruction uses non-atomic
memory accesses, so it doesn't synchronize with the "atomic_store_explicit"
statement. So, the compiler can well move the "strcpy" call after
"atomic_store_explicit" and the other thread that reads the pointer will see
uninitialized memory in the field "str".

So, to use the C11 memory model correctly, you can't even use library functions
that reference memory. You have to write your own function for copying strings:
_Atomic char *atomic_strcpy(_Atomic char *dst, _Atomic char *src)
{
        size_t idx = 0;
        char c;
        do {
                c = atomic_load_explicit(&src[idx], __ATOMIC_RELAXED);
                atomic_store_explicit(&dst[idx], c, __ATOMIC_RELAXED);
                idx++;
        } while (c);
        return dst;
}

I wish someone explained these problems to the committee and made them change
the standard so that atomic operations (except __ATOMIC_RELAXED) work like real
barriers and synchronize with all memory accesses, atomic or not. That would
make the standard much more useable.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug nptl/16432] pthread_spin_unlock should imply a memory barrier

[triegel at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16432

--- Comment #7 from Torvald Riegel <triegel at redhat dot com> ---
I repeat: This bug is not a venue to discuss your (mis)understanding of the C11
memory model.

What you write is incorrect.  Please read the standard again, or Batty et al's
formalization.  Understand the difference between the sequenced-before and
synchronizes-with relations, and how they are combined and influence
happens-before.

-- 
You are receiving this mail because:
You are on the CC list for the bug.