* [PATCH] misc/tst-clone3: Fix waiting for exited thread.
@ 2019-02-08 15:12 Stefan Liebler
2019-02-08 18:37 ` Adhemerval Zanella
0 siblings, 1 reply; 8+ messages in thread
From: Stefan Liebler @ 2019-02-08 15:12 UTC (permalink / raw
To: GNU C Library
[-- Attachment #1: Type: text/plain, Size: 977 bytes --]
Hi,
from time to time the test misc/tst-clone3 fails with an timeout. Then
futex_wait is blocking. Usually ctid should be set to zero due to
CLONE_CHILD_CLEARTID and the futex should be waken up. But the fail
occures if the thread has already exited before ctid is set to the
return value of clone(). Then futex_wait() will block as there will be
nobody who wakes the futex up again.
This patch initializes ctid to a known value before calling clone and
the kernel is the only one who updates the value to zero after clone.
If futex_wait is called then it is either waked up due to the exited
thread or the futex syscall fails as *ctid_ptr is already zero instead
of the specified value 1.
Okay to commit?
Bye
Stefan
ChangeLog:
* sysdeps/unix/sysv/linux/tst-clone3.c (do_test):
Initialize ctid with a known value and remove update of ctid
after clone.
(wait_tid): Adjust arguments and call futex_wait with ctid_val
as assumed current value of ctid_ptr.
[-- Attachment #2: 20190208_misc_tst-clone3.patch --]
[-- Type: text/x-patch, Size: 2582 bytes --]
commit 5a8f80973dbbf06a0eebc2064d2a14521c6a6131
Author: Stefan Liebler <stli@linux.ibm.com>
Date: Fri Feb 8 12:42:52 2019 +0100
misc/tst-clone3: Fix waiting for exited thread.
From time to time the test misc/tst-clone3 fails with an timeout.
Then futex_wait is blocking. Usually ctid should be set to zero
due to CLONE_CHILD_CLEARTID and the futex should be waken up.
But the fail occures if the thread has already exited before
ctid is set to the return value of clone(). Then futex_wait() will
block as there will be nobody who wakes the futex up again.
This patch initializes ctid to a known value before calling clone
and the kernel is the only one who updates the value to zero after clone.
If futex_wait is called then it is either waked up due to the exited thread
or the futex syscall fails as *ctid_ptr is already zero instead of the
specified value 1.
ChangeLog:
* sysdeps/unix/sysv/linux/tst-clone3.c (do_test):
Initialize ctid with a known value and remove update of ctid
after clone.
(wait_tid): Adjust arguments and call futex_wait with ctid_val
as assumed current value of ctid_ptr.
diff --git a/sysdeps/unix/sysv/linux/tst-clone3.c b/sysdeps/unix/sysv/linux/tst-clone3.c
index aa8e718afe..ffa2056eb6 100644
--- a/sysdeps/unix/sysv/linux/tst-clone3.c
+++ b/sysdeps/unix/sysv/linux/tst-clone3.c
@@ -42,11 +42,11 @@ f (void *a)
/* Futex wait for TID argument, similar to pthread_join internal
implementation. */
-#define wait_tid(tid) \
+#define wait_tid(ctid_ptr, ctid_val) \
do { \
- __typeof (tid) __tid; \
- while ((__tid = (tid)) != 0) \
- futex_wait (&(tid), __tid); \
+ __typeof (*(ctid_ptr)) __tid; \
+ while ((__tid = *(ctid_ptr)) != 0) \
+ futex_wait (ctid_ptr, ctid_val); \
} while (0)
static inline int
@@ -64,7 +64,11 @@ do_test (void)
clone_flags |= CLONE_VM | CLONE_SIGHAND;
/* We will used ctid to call on futex to wait for thread exit. */
clone_flags |= CLONE_CHILD_CLEARTID;
- pid_t ctid, tid;
+ /* Initialize with a known value. ctid is set to zero by the kernel after the
+ cloned thread has exited. */
+#define CTID_INIT_VAL 1
+ pid_t ctid = CTID_INIT_VAL;
+ pid_t tid;
#ifdef __ia64__
extern int __clone2 (int (*__fn) (void *__arg), void *__child_stack_base,
@@ -86,8 +90,7 @@ do_test (void)
if (tid == -1)
FAIL_EXIT1 ("clone failed: %m");
- ctid = tid;
- wait_tid (ctid);
+ wait_tid (&ctid, CTID_INIT_VAL);
return 2;
}
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] misc/tst-clone3: Fix waiting for exited thread.
2019-02-08 15:12 [PATCH] misc/tst-clone3: Fix waiting for exited thread Stefan Liebler
@ 2019-02-08 18:37 ` Adhemerval Zanella
2019-02-11 10:23 ` Stefan Liebler
0 siblings, 1 reply; 8+ messages in thread
From: Adhemerval Zanella @ 2019-02-08 18:37 UTC (permalink / raw
To: libc-alpha
On 08/02/2019 13:12, Stefan Liebler wrote:
> Hi,
>
> from time to time the test misc/tst-clone3 fails with an timeout. Then futex_wait is blocking. Usually ctid should be set to zero due to CLONE_CHILD_CLEARTID and the futex should be waken up. But the fail occures if the thread has already exited before ctid is set to the return value of clone(). Then futex_wait() will block as there will be nobody who wakes the futex up again.
>
> This patch initializes ctid to a known value before calling clone and the kernel is the only one who updates the value to zero after clone. If futex_wait is called then it is either waked up due to the exited thread or the futex syscall fails as *ctid_ptr is already zero instead of the specified value 1.
>
> Okay to commit?
>
> Bye
> Stefan
>
> ChangeLog:
>
> * sysdeps/unix/sysv/linux/tst-clone3.c (do_test):
> Initialize ctid with a known value and remove update of ctid
> after clone.
> (wait_tid): Adjust arguments and call futex_wait with ctid_val
> as assumed current value of ctid_ptr.
>
> 20190208_misc_tst-clone3.patch
>
> commit 5a8f80973dbbf06a0eebc2064d2a14521c6a6131
> Author: Stefan Liebler <stli@linux.ibm.com>
> Date: Fri Feb 8 12:42:52 2019 +0100
>
> misc/tst-clone3: Fix waiting for exited thread.
>
> From time to time the test misc/tst-clone3 fails with an timeout.
> Then futex_wait is blocking. Usually ctid should be set to zero
> due to CLONE_CHILD_CLEARTID and the futex should be waken up.
> But the fail occures if the thread has already exited before
> ctid is set to the return value of clone(). Then futex_wait() will
> block as there will be nobody who wakes the futex up again.
>
> This patch initializes ctid to a known value before calling clone
> and the kernel is the only one who updates the value to zero after clone.
> If futex_wait is called then it is either waked up due to the exited thread
> or the futex syscall fails as *ctid_ptr is already zero instead of the
> specified value 1.
>
> ChangeLog:
>
> * sysdeps/unix/sysv/linux/tst-clone3.c (do_test):
> Initialize ctid with a known value and remove update of ctid
> after clone.
> (wait_tid): Adjust arguments and call futex_wait with ctid_val
> as assumed current value of ctid_ptr.
Thanks for catching it.
>
> diff --git a/sysdeps/unix/sysv/linux/tst-clone3.c b/sysdeps/unix/sysv/linux/tst-clone3.c
> index aa8e718afe..ffa2056eb6 100644
> --- a/sysdeps/unix/sysv/linux/tst-clone3.c
> +++ b/sysdeps/unix/sysv/linux/tst-clone3.c
> @@ -42,11 +42,11 @@ f (void *a)
>
> /* Futex wait for TID argument, similar to pthread_join internal
> implementation. */
> -#define wait_tid(tid) \
> +#define wait_tid(ctid_ptr, ctid_val) \
> do { \
> - __typeof (tid) __tid; \
> - while ((__tid = (tid)) != 0) \
> - futex_wait (&(tid), __tid); \
> + __typeof (*(ctid_ptr)) __tid; \
> + while ((__tid = *(ctid_ptr)) != 0) \
> + futex_wait (ctid_ptr, ctid_val); \
> } while (0)
lll_wait_tid uses an atomic load with acquire semantic, I think we should
use it as well. We can either include the atomic header or use c11 atomic.
>
> static inline int
> @@ -64,7 +64,11 @@ do_test (void)
> clone_flags |= CLONE_VM | CLONE_SIGHAND;
> /* We will used ctid to call on futex to wait for thread exit. */
> clone_flags |= CLONE_CHILD_CLEARTID;
> - pid_t ctid, tid;
> + /* Initialize with a known value. ctid is set to zero by the kernel after the
> + cloned thread has exited. */
> +#define CTID_INIT_VAL 1
> + pid_t ctid = CTID_INIT_VAL;
> + pid_t tid;
>
> #ifdef __ia64__
> extern int __clone2 (int (*__fn) (void *__arg), void *__child_stack_base,
> @@ -86,8 +90,7 @@ do_test (void)
> if (tid == -1)
> FAIL_EXIT1 ("clone failed: %m");
>
> - ctid = tid;
> - wait_tid (ctid);
> + wait_tid (&ctid, CTID_INIT_VAL);
>
> return 2;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] misc/tst-clone3: Fix waiting for exited thread.
2019-02-08 18:37 ` Adhemerval Zanella
@ 2019-02-11 10:23 ` Stefan Liebler
2019-02-11 10:59 ` Florian Weimer
0 siblings, 1 reply; 8+ messages in thread
From: Stefan Liebler @ 2019-02-11 10:23 UTC (permalink / raw
To: libc-alpha
[-- Attachment #1: Type: text/plain, Size: 6223 bytes --]
Attached the updated patch.
See also notes below.
On 02/08/2019 07:37 PM, Adhemerval Zanella wrote:
>
>
> On 08/02/2019 13:12, Stefan Liebler wrote:
>> Hi,
>>
>> from time to time the test misc/tst-clone3 fails with an timeout. Then futex_wait is blocking. Usually ctid should be set to zero due to CLONE_CHILD_CLEARTID and the futex should be waken up. But the fail occures if the thread has already exited before ctid is set to the return value of clone(). Then futex_wait() will block as there will be nobody who wakes the futex up again.
>>
>> This patch initializes ctid to a known value before calling clone and the kernel is the only one who updates the value to zero after clone. If futex_wait is called then it is either waked up due to the exited thread or the futex syscall fails as *ctid_ptr is already zero instead of the specified value 1.
>>
>> Okay to commit?
>>
>> Bye
>> Stefan
>>
>> ChangeLog:
>>
>> * sysdeps/unix/sysv/linux/tst-clone3.c (do_test):
>> Initialize ctid with a known value and remove update of ctid
>> after clone.
>> (wait_tid): Adjust arguments and call futex_wait with ctid_val
>> as assumed current value of ctid_ptr.
>>
>> 20190208_misc_tst-clone3.patch
>>
>> commit 5a8f80973dbbf06a0eebc2064d2a14521c6a6131
>> Author: Stefan Liebler <stli@linux.ibm.com>
>> Date: Fri Feb 8 12:42:52 2019 +0100
>>
>> misc/tst-clone3: Fix waiting for exited thread.
>>
>> From time to time the test misc/tst-clone3 fails with an timeout.
>> Then futex_wait is blocking. Usually ctid should be set to zero
>> due to CLONE_CHILD_CLEARTID and the futex should be waken up.
>> But the fail occures if the thread has already exited before
>> ctid is set to the return value of clone(). Then futex_wait() will
>> block as there will be nobody who wakes the futex up again.
>>
>> This patch initializes ctid to a known value before calling clone
>> and the kernel is the only one who updates the value to zero after clone.
>> If futex_wait is called then it is either waked up due to the exited thread
>> or the futex syscall fails as *ctid_ptr is already zero instead of the
>> specified value 1.
>>
>> ChangeLog:
>>
>> * sysdeps/unix/sysv/linux/tst-clone3.c (do_test):
>> Initialize ctid with a known value and remove update of ctid
>> after clone.
>> (wait_tid): Adjust arguments and call futex_wait with ctid_val
>> as assumed current value of ctid_ptr.
>
> Thanks for catching it.
>
>>
>> diff --git a/sysdeps/unix/sysv/linux/tst-clone3.c b/sysdeps/unix/sysv/linux/tst-clone3.c
>> index aa8e718afe..ffa2056eb6 100644
>> --- a/sysdeps/unix/sysv/linux/tst-clone3.c
>> +++ b/sysdeps/unix/sysv/linux/tst-clone3.c
>> @@ -42,11 +42,11 @@ f (void *a)
>>
>> /* Futex wait for TID argument, similar to pthread_join internal
>> implementation. */
>> -#define wait_tid(tid) \
>> +#define wait_tid(ctid_ptr, ctid_val) \
>> do { \
>> - __typeof (tid) __tid; \
>> - while ((__tid = (tid)) != 0) \
>> - futex_wait (&(tid), __tid); \
>> + __typeof (*(ctid_ptr)) __tid; \
>> + while ((__tid = *(ctid_ptr)) != 0) \
>> + futex_wait (ctid_ptr, ctid_val); \
>> } while (0)
>
> lll_wait_tid uses an atomic load with acquire semantic, I think we should
> use it as well. We can either include the atomic header or use c11 atomic.
>
I've first tried to include atomic.h, but it failed building on x86_64.
Thus I'm using the c11 atomic load in the updated patch.
Okay to commit?
As information, I've observed those gcc errors on x86_64:
In file included from ../sysdeps/unix/sysv/linux/x86_64/sysdep.h:30,
from ../sysdeps/x86_64/nptl/tls.h:28,
from ../sysdeps/x86/atomic-machine.h:23,
from ../include/atomic.h:50,
from ../sysdeps/unix/sysv/linux/tst-clone3.c:29:
../sysdeps/unix/sysv/linux/dl-sysdep.h: In function
‘_dl_discover_osversion’:
../sysdeps/unix/sysv/linux/dl-sysdep.h:31:42: error: expected
declaration specifiers before ‘attribute_hidden’
extern int _dl_discover_osversion (void) attribute_hidden;
^~~~~~~~~~~~~~~~
In file included from ../sysdeps/x86_64/nptl/tls.h:31,
from ../sysdeps/x86/atomic-machine.h:23,
from ../include/atomic.h:50,
from ../sysdeps/unix/sysv/linux/tst-clone3.c:29:
../sysdeps/generic/dl-dtv.h:22:1: error: empty declaration [-Werror]
struct dtv_pointer
^~~~~~
../sysdeps/generic/dl-dtv.h:33:3: error: storage class specified for
parameter ‘dtv_t’
} dtv_t;
^~~~~
...
many many more errors
...
I've also tried to add tst-clone3 to tests_internal instead of tests in
the Makefile, but then I've got:
In file included from ../sysdeps/x86_64/nptl/tls.h:130,
from ../sysdeps/x86/atomic-machine.h:23,
from ../include/atomic.h:50,
from ../sysdeps/unix/sysv/linux/tst-clone3.c:29:
../nptl/descr.h:104:8: error: redefinition of ‘struct robust_list_head’
struct robust_list_head
^~~~~~~~~~~~~~~~
In file included from ../sysdeps/unix/sysv/linux/tst-clone3.c:26:
/usr/include/linux/futex.h:70:8: note: originally defined here
struct robust_list_head {
^~~~~~~~~~~~~~~~
>>
>> static inline int
>> @@ -64,7 +64,11 @@ do_test (void)
>> clone_flags |= CLONE_VM | CLONE_SIGHAND;
>> /* We will used ctid to call on futex to wait for thread exit. */
>> clone_flags |= CLONE_CHILD_CLEARTID;
>> - pid_t ctid, tid;
>> + /* Initialize with a known value. ctid is set to zero by the kernel after the
>> + cloned thread has exited. */
>> +#define CTID_INIT_VAL 1
>> + pid_t ctid = CTID_INIT_VAL;
>> + pid_t tid;
>>
>> #ifdef __ia64__
>> extern int __clone2 (int (*__fn) (void *__arg), void *__child_stack_base,
>> @@ -86,8 +90,7 @@ do_test (void)
>> if (tid == -1)
>> FAIL_EXIT1 ("clone failed: %m");
>>
>> - ctid = tid;
>> - wait_tid (ctid);
>> + wait_tid (&ctid, CTID_INIT_VAL);
>>
>> return 2;
>> }
>>
>
[-- Attachment #2: 20190211_1030_misc_tst-clone3.patch --]
[-- Type: text/x-patch, Size: 2773 bytes --]
commit 3672d05bd3c5c51831ef8b91ee2b0ff491fd2986
Author: Stefan Liebler <stli@linux.ibm.com>
Date: Fri Feb 8 12:42:52 2019 +0100
misc/tst-clone3: Fix waiting for exited thread.
From time to time the test misc/tst-clone3 fails with a timeout.
Then futex_wait is blocking. Usually ctid should be set to zero
due to CLONE_CHILD_CLEARTID and the futex should be waken up.
But the fail occures if the thread has already exited before
ctid is set to the return value of clone(). Then futex_wait() will
block as there will be nobody who wakes the futex up again.
This patch initializes ctid to a known value before calling clone
and the kernel is the only one who updates the value to zero after clone.
If futex_wait is called then it is either waked up due to the exited thread
or the futex syscall fails as *ctid_ptr is already zero instead of the
specified value 1.
ChangeLog:
* sysdeps/unix/sysv/linux/tst-clone3.c (do_test):
Initialize ctid with a known value and remove update of ctid
after clone.
(wait_tid): Adjust arguments and call futex_wait with ctid_val
as assumed current value of ctid_ptr.
diff --git a/sysdeps/unix/sysv/linux/tst-clone3.c b/sysdeps/unix/sysv/linux/tst-clone3.c
index aa8e718afe..b345d04b4d 100644
--- a/sysdeps/unix/sysv/linux/tst-clone3.c
+++ b/sysdeps/unix/sysv/linux/tst-clone3.c
@@ -42,11 +42,13 @@ f (void *a)
/* Futex wait for TID argument, similar to pthread_join internal
implementation. */
-#define wait_tid(tid) \
- do { \
- __typeof (tid) __tid; \
- while ((__tid = (tid)) != 0) \
- futex_wait (&(tid), __tid); \
+#define wait_tid(ctid_ptr, ctid_val) \
+ do { \
+ __typeof (*(ctid_ptr)) __tid; \
+ /* We need acquire MO here so that we synchronize with the \
+ kernel's store to 0 when the clone terminates. */ \
+ while ((__tid = __atomic_load_n (ctid_ptr, __ATOMIC_ACQUIRE)) != 0) \
+ futex_wait (ctid_ptr, ctid_val); \
} while (0)
static inline int
@@ -64,7 +66,11 @@ do_test (void)
clone_flags |= CLONE_VM | CLONE_SIGHAND;
/* We will used ctid to call on futex to wait for thread exit. */
clone_flags |= CLONE_CHILD_CLEARTID;
- pid_t ctid, tid;
+ /* Initialize with a known value. ctid is set to zero by the kernel after the
+ cloned thread has exited. */
+#define CTID_INIT_VAL 1
+ pid_t ctid = CTID_INIT_VAL;
+ pid_t tid;
#ifdef __ia64__
extern int __clone2 (int (*__fn) (void *__arg), void *__child_stack_base,
@@ -86,8 +92,7 @@ do_test (void)
if (tid == -1)
FAIL_EXIT1 ("clone failed: %m");
- ctid = tid;
- wait_tid (ctid);
+ wait_tid (&ctid, CTID_INIT_VAL);
return 2;
}
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] misc/tst-clone3: Fix waiting for exited thread.
2019-02-11 10:23 ` Stefan Liebler
@ 2019-02-11 10:59 ` Florian Weimer
2019-02-11 12:11 ` Adhemerval Zanella
0 siblings, 1 reply; 8+ messages in thread
From: Florian Weimer @ 2019-02-11 10:59 UTC (permalink / raw
To: Stefan Liebler; +Cc: libc-alpha
* Stefan Liebler:
> I've first tried to include atomic.h, but it failed building on
> x86_64. Thus I'm using the c11 atomic load in the updated patch.
> Okay to commit?
>
>
> As information, I've observed those gcc errors on x86_64:
> In file included from ../sysdeps/unix/sysv/linux/x86_64/sysdep.h:30,
> from ../sysdeps/x86_64/nptl/tls.h:28,
> from ../sysdeps/x86/atomic-machine.h:23,
> from ../include/atomic.h:50,
> from ../sysdeps/unix/sysv/linux/tst-clone3.c:29:
> ../sysdeps/unix/sysv/linux/dl-sysdep.h: In function
> ‘_dl_discover_osversion’:
> ../sysdeps/unix/sysv/linux/dl-sysdep.h:31:42: error: expected
> declaration specifiers before ‘attribute_hidden’
> extern int _dl_discover_osversion (void) attribute_hidden;
> ^~~~~~~~~~~~~~~~
That's because the test isn't in tests-internal.
> + while ((__tid = __atomic_load_n (ctid_ptr, __ATOMIC_ACQUIRE)) != 0) \
Actually, that's not a C11 atomic construct, but I think it's okay to
use that here. (The C11 stuff lives in <stdatomic.h> and should be
functionally equivalent.)
Sorry, this is a pet peeve of mine. We have three different atomic
access facilities that people refer to as C11 atomics: Our own
<atomic.h>, the GCC __atomic builtins, and <stdatomic.h>.
I still think this contributes to cognitive load, and we should
eliminate all but one (leaving us with new-style atomics and the old
macros in <atomic.h>). The GCC __atomic builtins have the best freely
available documentation, so they are a natural candidate IMHO.
Thanks,
Florian
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] misc/tst-clone3: Fix waiting for exited thread.
2019-02-11 10:59 ` Florian Weimer
@ 2019-02-11 12:11 ` Adhemerval Zanella
2019-02-12 12:53 ` Stefan Liebler
0 siblings, 1 reply; 8+ messages in thread
From: Adhemerval Zanella @ 2019-02-11 12:11 UTC (permalink / raw
To: libc-alpha
On 11/02/2019 08:59, Florian Weimer wrote:
> * Stefan Liebler:
>
>> I've first tried to include atomic.h, but it failed building on
>> x86_64. Thus I'm using the c11 atomic load in the updated patch.
>> Okay to commit?
>>
>>
>> As information, I've observed those gcc errors on x86_64:
>> In file included from ../sysdeps/unix/sysv/linux/x86_64/sysdep.h:30,
>> from ../sysdeps/x86_64/nptl/tls.h:28,
>> from ../sysdeps/x86/atomic-machine.h:23,
>> from ../include/atomic.h:50,
>> from ../sysdeps/unix/sysv/linux/tst-clone3.c:29:
>> ../sysdeps/unix/sysv/linux/dl-sysdep.h: In function
>> ‘_dl_discover_osversion’:
>> ../sysdeps/unix/sysv/linux/dl-sysdep.h:31:42: error: expected
>> declaration specifiers before ‘attribute_hidden’
>> extern int _dl_discover_osversion (void) attribute_hidden;
>> ^~~~~~~~~~~~~~~~
>
> That's because the test isn't in tests-internal.
>
>> + while ((__tid = __atomic_load_n (ctid_ptr, __ATOMIC_ACQUIRE)) != 0) \
>
> Actually, that's not a C11 atomic construct, but I think it's okay to
> use that here. (The C11 stuff lives in <stdatomic.h> and should be
> functionally equivalent.)
>
> Sorry, this is a pet peeve of mine. We have three different atomic
> access facilities that people refer to as C11 atomics: Our own
> <atomic.h>, the GCC __atomic builtins, and <stdatomic.h>.
>
> I still think this contributes to cognitive load, and we should
> eliminate all but one (leaving us with new-style atomics and the old
> macros in <atomic.h>). The GCC __atomic builtins have the best freely
> available documentation, so they are a natural candidate IMHO.
It really annoying that C11 standard is not freely available from ISO,
although the working draft is still open [1]. I don't have a strong
opinion, but since there is support in the language itself and they
are fully supported by the compiler I also don't see why not prefer it.
[1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] misc/tst-clone3: Fix waiting for exited thread.
2019-02-11 12:11 ` Adhemerval Zanella
@ 2019-02-12 12:53 ` Stefan Liebler
2019-02-18 9:27 ` Stefan Liebler
0 siblings, 1 reply; 8+ messages in thread
From: Stefan Liebler @ 2019-02-12 12:53 UTC (permalink / raw
To: libc-alpha
[-- Attachment #1: Type: text/plain, Size: 2260 bytes --]
On 02/11/2019 01:11 PM, Adhemerval Zanella wrote:
>
>
> On 11/02/2019 08:59, Florian Weimer wrote:
>> * Stefan Liebler:
>>
>>> I've first tried to include atomic.h, but it failed building on
>>> x86_64. Thus I'm using the c11 atomic load in the updated patch.
>>> Okay to commit?
>>>
>>>
>>> As information, I've observed those gcc errors on x86_64:
>>> In file included from ../sysdeps/unix/sysv/linux/x86_64/sysdep.h:30,
>>> from ../sysdeps/x86_64/nptl/tls.h:28,
>>> from ../sysdeps/x86/atomic-machine.h:23,
>>> from ../include/atomic.h:50,
>>> from ../sysdeps/unix/sysv/linux/tst-clone3.c:29:
>>> ../sysdeps/unix/sysv/linux/dl-sysdep.h: In function
>>> ‘_dl_discover_osversion’:
>>> ../sysdeps/unix/sysv/linux/dl-sysdep.h:31:42: error: expected
>>> declaration specifiers before ‘attribute_hidden’
>>> extern int _dl_discover_osversion (void) attribute_hidden;
>>> ^~~~~~~~~~~~~~~~
>>
>> That's because the test isn't in tests-internal.
>>
>>> + while ((__tid = __atomic_load_n (ctid_ptr, __ATOMIC_ACQUIRE)) != 0) \
>>
>> Actually, that's not a C11 atomic construct, but I think it's okay to
>> use that here. (The C11 stuff lives in <stdatomic.h> and should be
>> functionally equivalent.)
>>
>> Sorry, this is a pet peeve of mine. We have three different atomic
>> access facilities that people refer to as C11 atomics: Our own
>> <atomic.h>, the GCC __atomic builtins, and <stdatomic.h>.
>>
>> I still think this contributes to cognitive load, and we should
>> eliminate all but one (leaving us with new-style atomics and the old
>> macros in <atomic.h>). The GCC __atomic builtins have the best freely
>> available documentation, so they are a natural candidate IMHO.
>
> It really annoying that C11 standard is not freely available from ISO,
> although the working draft is still open [1]. I don't have a strong
> opinion, but since there is support in the language itself and they
> are fully supported by the compiler I also don't see why not prefer it.
>
> [1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf
>
Then I have a further update of the patch which uses stdatomic.h and
atomic_load_explicit.
[-- Attachment #2: 20190212_1245_misc_tst-clone3.patch --]
[-- Type: text/x-patch, Size: 3077 bytes --]
commit 4fe43564677deb86f4b404d20bc48a2128f918a9
Author: Stefan Liebler <stli@linux.ibm.com>
Date: Fri Feb 8 12:42:52 2019 +0100
misc/tst-clone3: Fix waiting for exited thread.
From time to time the test misc/tst-clone3 fails with a timeout.
Then futex_wait is blocking. Usually ctid should be set to zero
due to CLONE_CHILD_CLEARTID and the futex should be waken up.
But the fail occures if the thread has already exited before
ctid is set to the return value of clone(). Then futex_wait() will
block as there will be nobody who wakes the futex up again.
This patch initializes ctid to a known value before calling clone
and the kernel is the only one who updates the value to zero after clone.
If futex_wait is called then it is either waked up due to the exited thread
or the futex syscall fails as *ctid_ptr is already zero instead of the
specified value 1.
ChangeLog:
* sysdeps/unix/sysv/linux/tst-clone3.c (do_test):
Initialize ctid with a known value and remove update of ctid
after clone.
(wait_tid): Adjust arguments and call futex_wait with ctid_val
as assumed current value of ctid_ptr.
diff --git a/sysdeps/unix/sysv/linux/tst-clone3.c b/sysdeps/unix/sysv/linux/tst-clone3.c
index aa8e718afe..e78755fcd3 100644
--- a/sysdeps/unix/sysv/linux/tst-clone3.c
+++ b/sysdeps/unix/sysv/linux/tst-clone3.c
@@ -27,6 +27,7 @@
#include <stackinfo.h> /* For _STACK_GROWS_{UP,DOWN}. */
#include <support/check.h>
+#include <stdatomic.h>
/* Test if clone call with CLONE_THREAD does not call exit_group. The 'f'
function returns '1', which will be used by clone thread to call the
@@ -42,11 +43,14 @@ f (void *a)
/* Futex wait for TID argument, similar to pthread_join internal
implementation. */
-#define wait_tid(tid) \
- do { \
- __typeof (tid) __tid; \
- while ((__tid = (tid)) != 0) \
- futex_wait (&(tid), __tid); \
+#define wait_tid(ctid_ptr, ctid_val) \
+ do { \
+ __typeof (*(ctid_ptr)) __tid; \
+ /* We need acquire MO here so that we synchronize with the \
+ kernel's store to 0 when the clone terminates. */ \
+ while ((__tid = atomic_load_explicit (ctid_ptr, \
+ memory_order_acquire)) != 0) \
+ futex_wait (ctid_ptr, ctid_val); \
} while (0)
static inline int
@@ -64,7 +68,11 @@ do_test (void)
clone_flags |= CLONE_VM | CLONE_SIGHAND;
/* We will used ctid to call on futex to wait for thread exit. */
clone_flags |= CLONE_CHILD_CLEARTID;
- pid_t ctid, tid;
+ /* Initialize with a known value. ctid is set to zero by the kernel after the
+ cloned thread has exited. */
+#define CTID_INIT_VAL 1
+ pid_t ctid = CTID_INIT_VAL;
+ pid_t tid;
#ifdef __ia64__
extern int __clone2 (int (*__fn) (void *__arg), void *__child_stack_base,
@@ -86,8 +94,7 @@ do_test (void)
if (tid == -1)
FAIL_EXIT1 ("clone failed: %m");
- ctid = tid;
- wait_tid (ctid);
+ wait_tid (&ctid, CTID_INIT_VAL);
return 2;
}
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] misc/tst-clone3: Fix waiting for exited thread.
2019-02-12 12:53 ` Stefan Liebler
@ 2019-02-18 9:27 ` Stefan Liebler
2019-02-18 9:36 ` Florian Weimer
0 siblings, 1 reply; 8+ messages in thread
From: Stefan Liebler @ 2019-02-18 9:27 UTC (permalink / raw
To: libc-alpha
On 02/12/2019 01:53 PM, Stefan Liebler wrote:
> On 02/11/2019 01:11 PM, Adhemerval Zanella wrote:
>>
>>
>> On 11/02/2019 08:59, Florian Weimer wrote:
>>> * Stefan Liebler:
>>>
>>>> I've first tried to include atomic.h, but it failed building on
>>>> x86_64. Thus I'm using the c11 atomic load in the updated patch.
>>>> Okay to commit?
>>>>
>>>>
>>>> As information, I've observed those gcc errors on x86_64:
>>>> In file included from ../sysdeps/unix/sysv/linux/x86_64/sysdep.h:30,
>>>> from ../sysdeps/x86_64/nptl/tls.h:28,
>>>> from ../sysdeps/x86/atomic-machine.h:23,
>>>> from ../include/atomic.h:50,
>>>> from ../sysdeps/unix/sysv/linux/tst-clone3.c:29:
>>>> ../sysdeps/unix/sysv/linux/dl-sysdep.h: In function
>>>> ‘_dl_discover_osversion’:
>>>> ../sysdeps/unix/sysv/linux/dl-sysdep.h:31:42: error: expected
>>>> declaration specifiers before ‘attribute_hidden’
>>>> extern int _dl_discover_osversion (void) attribute_hidden;
>>>> ^~~~~~~~~~~~~~~~
>>>
>>> That's because the test isn't in tests-internal.
>>>
>>>> + while ((__tid = __atomic_load_n (ctid_ptr, __ATOMIC_ACQUIRE))
>>>> != 0) \
>>>
>>> Actually, that's not a C11 atomic construct, but I think it's okay to
>>> use that here. (The C11 stuff lives in <stdatomic.h> and should be
>>> functionally equivalent.)
>>>
>>> Sorry, this is a pet peeve of mine. We have three different atomic
>>> access facilities that people refer to as C11 atomics: Our own
>>> <atomic.h>, the GCC __atomic builtins, and <stdatomic.h>.
>>>
>>> I still think this contributes to cognitive load, and we should
>>> eliminate all but one (leaving us with new-style atomics and the old
>>> macros in <atomic.h>). The GCC __atomic builtins have the best freely
>>> available documentation, so they are a natural candidate IMHO.
>>
>> It really annoying that C11 standard is not freely available from ISO,
>> although the working draft is still open [1]. I don't have a strong
>> opinion, but since there is support in the language itself and they
>> are fully supported by the compiler I also don't see why not prefer it.
>>
>> [1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf
>>
> Then I have a further update of the patch which uses stdatomic.h and
> atomic_load_explicit.
ping
okay to commit?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] misc/tst-clone3: Fix waiting for exited thread.
2019-02-18 9:27 ` Stefan Liebler
@ 2019-02-18 9:36 ` Florian Weimer
0 siblings, 0 replies; 8+ messages in thread
From: Florian Weimer @ 2019-02-18 9:36 UTC (permalink / raw
To: Stefan Liebler; +Cc: libc-alpha
* Stefan Liebler:
> On 02/12/2019 01:53 PM, Stefan Liebler wrote:
>> On 02/11/2019 01:11 PM, Adhemerval Zanella wrote:
>>>
>>>
>>> On 11/02/2019 08:59, Florian Weimer wrote:
>>>> * Stefan Liebler:
>>>>
>>>>> I've first tried to include atomic.h, but it failed building on
>>>>> x86_64. Thus I'm using the c11 atomic load in the updated patch.
>>>>> Okay to commit?
>>>>>
>>>>>
>>>>> As information, I've observed those gcc errors on x86_64:
>>>>> In file included from ../sysdeps/unix/sysv/linux/x86_64/sysdep.h:30,
>>>>> from ../sysdeps/x86_64/nptl/tls.h:28,
>>>>> from ../sysdeps/x86/atomic-machine.h:23,
>>>>> from ../include/atomic.h:50,
>>>>> from ../sysdeps/unix/sysv/linux/tst-clone3.c:29:
>>>>> ../sysdeps/unix/sysv/linux/dl-sysdep.h: In function
>>>>> ‘_dl_discover_osversion’:
>>>>> ../sysdeps/unix/sysv/linux/dl-sysdep.h:31:42: error: expected
>>>>> declaration specifiers before ‘attribute_hidden’
>>>>> extern int _dl_discover_osversion (void) attribute_hidden;
>>>>> ^~~~~~~~~~~~~~~~
>>>>
>>>> That's because the test isn't in tests-internal.
>>>>
>>>>> + while ((__tid = __atomic_load_n (ctid_ptr,
>>>>> __ATOMIC_ACQUIRE)) != 0) \
>>>>
>>>> Actually, that's not a C11 atomic construct, but I think it's okay to
>>>> use that here. (The C11 stuff lives in <stdatomic.h> and should be
>>>> functionally equivalent.)
>>>>
>>>> Sorry, this is a pet peeve of mine. We have three different atomic
>>>> access facilities that people refer to as C11 atomics: Our own
>>>> <atomic.h>, the GCC __atomic builtins, and <stdatomic.h>.
>>>>
>>>> I still think this contributes to cognitive load, and we should
>>>> eliminate all but one (leaving us with new-style atomics and the old
>>>> macros in <atomic.h>). The GCC __atomic builtins have the best freely
>>>> available documentation, so they are a natural candidate IMHO.
>>>
>>> It really annoying that C11 standard is not freely available from ISO,
>>> although the working draft is still open [1]. I don't have a strong
>>> opinion, but since there is support in the language itself and they
>>> are fully supported by the compiler I also don't see why not prefer it.
>>>
>>> [1] http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf
>>>
>> Then I have a further update of the patch which uses stdatomic.h and
>> atomic_load_explicit.
>
>
> ping
> okay to commit?
I think so. Patch looks reasonable to me. Thanks.
Florian
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-02-18 9:36 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-08 15:12 [PATCH] misc/tst-clone3: Fix waiting for exited thread Stefan Liebler
2019-02-08 18:37 ` Adhemerval Zanella
2019-02-11 10:23 ` Stefan Liebler
2019-02-11 10:59 ` Florian Weimer
2019-02-11 12:11 ` Adhemerval Zanella
2019-02-12 12:53 ` Stefan Liebler
2019-02-18 9:27 ` Stefan Liebler
2019-02-18 9:36 ` Florian Weimer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).