From patchwork Tue Feb 12 12:53:13 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Stefan Liebler X-Patchwork-Id: 31407 Received: (qmail 16913 invoked by alias); 12 Feb 2019 12:53:24 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 16695 invoked by uid 89); 12 Feb 2019 12:53:23 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-27.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mx0a-001b2d01.pphosted.com Subject: Re: [PATCH] misc/tst-clone3: Fix waiting for exited thread. To: libc-alpha@sourceware.org References: <0bffc1e1-9cde-181b-cf10-4cc3530fa9cb@linux.ibm.com> <3a9bc23d-c5c6-2a6f-890e-efb9f476e104@linaro.org> <87zhr2fv5c.fsf@oldenburg2.str.redhat.com> <2e275f03-d8c8-693a-0836-bc6e74700927@linaro.org> From: Stefan Liebler Date: Tue, 12 Feb 2019 13:53:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <2e275f03-d8c8-693a-0836-bc6e74700927@linaro.org> x-cbid: 19021212-0028-0000-0000-00000347A57A X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 19021212-0029-0000-0000-00002405C511 Message-Id: <494a9561-9e1b-1dc9-f5c0-dd2e1ca24d59@linux.ibm.com> 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 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 >> , the GCC __atomic builtins, and . >> >> 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 ). 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. commit 4fe43564677deb86f4b404d20bc48a2128f918a9 Author: Stefan Liebler 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 /* For _STACK_GROWS_{UP,DOWN}. */ #include +#include /* 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; }