: Fix blocking pthread_join.

  Hi,

On s390 (31bit) if glibc is build with -Os, pthread_join sometimes
blocks indefinitely. This is e.g. observable with
testcase intl/tst-gettext6.

pthread_join is calling lll_wait_tid(tid), which performs the futex-wait
syscall in a loop as long as tid != 0 (thread is alive).

On s390 (and build with -Os), tid is loaded from memory before
comparing against zero and then the tid is loaded a second time
in order to pass it to the futex-wait-syscall.
If the thread exits in between, then the futex-wait-syscall is
called with the value zero and it waits until a futex-wake occurs.
As the thread is already exited, there won't be a futex-wake.

In lll_wait_tid, the tid is stored to the local variable __tid,
which is then used as argument for the futex-wait-syscall.
But unfortunately the compiler is allowed to reload the value
from memory.

With this patch, the tid is loaded by dereferencing a volatile pointer.
Then the compiler is not allowed to reload the value for __tid from memory.

Okay to commit?

Bye
Stefan

---

ChangeLog:

	* sysdeps/nptl/lowlevellock.h (lll_wait_tid):
	Use a volatile pointer to load __tid.
  

On 04/25/2018 06:27 AM, Stefan Liebler wrote:
> Hi,
> 
> On s390 (31bit) if glibc is build with -Os, pthread_join sometimes
> blocks indefinitely. This is e.g. observable with
> testcase intl/tst-gettext6.
> 
> pthread_join is calling lll_wait_tid(tid), which performs the futex-wait
> syscall in a loop as long as tid != 0 (thread is alive).
> 
> On s390 (and build with -Os), tid is loaded from memory before
> comparing against zero and then the tid is loaded a second time
> in order to pass it to the futex-wait-syscall.
> If the thread exits in between, then the futex-wait-syscall is
> called with the value zero and it waits until a futex-wake occurs.
> As the thread is already exited, there won't be a futex-wake.
> 
> In lll_wait_tid, the tid is stored to the local variable __tid,
> which is then used as argument for the futex-wait-syscall.
> But unfortunately the compiler is allowed to reload the value
> from memory.
> 
> With this patch, the tid is loaded by dereferencing a volatile pointer.
> Then the compiler is not allowed to reload the value for __tid from memory.
> 
> Okay to commit?

Would using an atomic type and an atomic load MO relaxed prevent the
compiler from reloading from memory?

I'm unhappy with the use of volatile here because it's not quite
the real semantics. Sure, the memory is volatile, it may change at
any point, but that's not what matters. What matters is that we load
from that memory once and only once.
  

On Wed, 2018-04-25 at 07:39 -0500, Carlos O'Donell wrote:
> On 04/25/2018 06:27 AM, Stefan Liebler wrote:
> > With this patch, the tid is loaded by dereferencing a volatile pointer.
> > Then the compiler is not allowed to reload the value for __tid from memory.

We always use atomic accesses when it comes to concurrently accessed
data (there are exceptions, but these are tightly controlled).
We never use volatile to "fix" concurrent accesses.

> > Okay to commit?
> 
> Would using an atomic type and an atomic load MO relaxed prevent the
> compiler from reloading from memory?

That's the right fix, and it should be an acquire MO load to synchronize
with the kernel's store to 0.  (We should make it a requirement for the
kernel to use a release store; IIRC, it is on many archs, but it isn't
documented.)

The accesses to the TID should be changed to use atomics everywhere, and
some (simple) concurrency notes should be added.

> I'm unhappy with the use of volatile here because it's not quite
> the real semantics. Sure, the memory is volatile, it may change at
> any point, but that's not what matters. What matters is that we load
> from that memory once and only once.

It's a normal concurrent access, so we're using atomics for it.
Volatile but non-atomic is for cases where one would communicate with an
external device or sth like that, and those device's memory accesses
would appear to interrupt the thread that's using the volatile accesses.
IOW, it's like sequential code from a memory-model perspective, just
that the device's accesses can interleave with the CPU thread's
accesses.  There's no such simple interleaving when it comes to
concurrent accesses.
  

On Wed, Apr 25, 2018 at 01:27:07PM +0200, Stefan Liebler wrote:
> Hi,
> 
> On s390 (31bit) if glibc is build with -Os, pthread_join sometimes
> blocks indefinitely. This is e.g. observable with
> testcase intl/tst-gettext6.
> 
> pthread_join is calling lll_wait_tid(tid), which performs the futex-wait
> syscall in a loop as long as tid != 0 (thread is alive).
> 
> On s390 (and build with -Os), tid is loaded from memory before
> comparing against zero and then the tid is loaded a second time
> in order to pass it to the futex-wait-syscall.
> If the thread exits in between, then the futex-wait-syscall is
> called with the value zero and it waits until a futex-wake occurs.
> As the thread is already exited, there won't be a futex-wake.
> 
> In lll_wait_tid, the tid is stored to the local variable __tid,
> which is then used as argument for the futex-wait-syscall.
> But unfortunately the compiler is allowed to reload the value
> from memory.
> 
> With this patch, the tid is loaded by dereferencing a volatile pointer.
> Then the compiler is not allowed to reload the value for __tid from memory.
> 
> Okay to commit?

There should probably be a bugzilla issue for this, no?

Rich
  

On 05/02/2018 12:29 PM, Rich Felker wrote:
> On Wed, Apr 25, 2018 at 01:27:07PM +0200, Stefan Liebler wrote:
>> Hi,
>>
>> On s390 (31bit) if glibc is build with -Os, pthread_join sometimes
>> blocks indefinitely. This is e.g. observable with
>> testcase intl/tst-gettext6.
>>
>> pthread_join is calling lll_wait_tid(tid), which performs the futex-wait
>> syscall in a loop as long as tid != 0 (thread is alive).
>>
>> On s390 (and build with -Os), tid is loaded from memory before
>> comparing against zero and then the tid is loaded a second time
>> in order to pass it to the futex-wait-syscall.
>> If the thread exits in between, then the futex-wait-syscall is
>> called with the value zero and it waits until a futex-wake occurs.
>> As the thread is already exited, there won't be a futex-wake.
>>
>> In lll_wait_tid, the tid is stored to the local variable __tid,
>> which is then used as argument for the futex-wait-syscall.
>> But unfortunately the compiler is allowed to reload the value
>> from memory.
>>
>> With this patch, the tid is loaded by dereferencing a volatile pointer.
>> Then the compiler is not allowed to reload the value for __tid from memory.
>>
>> Okay to commit?
> 
> There should probably be a bugzilla issue for this, no?

Yes. Publicly visible bugs need one.