diff mbox

misc/tst-clone3: Fix waiting for exited thread.

Message ID 494a9561-9e1b-1dc9-f5c0-dd2e1ca24d59@linux.ibm.com
State Committed
Headers show

Commit Message

Stefan Liebler Feb. 12, 2019, 12:53 p.m. UTC
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.

Comments

Stefan Liebler Feb. 18, 2019, 9:27 a.m. UTC | #1
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?
Florian Weimer Feb. 18, 2019, 9:36 a.m. UTC | #2
* 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
diff mbox

Patch

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;
 }