From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS31976 209.132.180.0/23 X-Spam-Status: No, score=-3.6 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, SPF_HELO_PASS,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.1 Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 253221F597 for ; Tue, 24 Jul 2018 16:05:07 +0000 (UTC) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:references:from:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; q=dns; s=default; b=I1qUsYnswZNHsFWQ ZFzX6XmXj0u5nQqpxPZtF88zqtTU9wmOEMZgw9U09364g5FymzjcTgKvYj/FdoJP sBVWLqKcgw9wMKbuXuyDeSWha7xCY+Iq9hgD9KgCjXj8a1JTnaLKKx2drHUdJsrj N/IuQ+Vdjo52IIzl22pypUR46Ho= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:subject:to:references:from:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; s=default; bh=lTha+Jykl4J9gPRizl5JTC VvtwY=; b=cFrOqA6orKfL48Jy2MRf1HRg0VLfASX2eDPoDrqi4GLy2IRo47PGRd Wmyjy6CTN6M4lmWrzHsah0Hrkq7kRqlwS/B3ir14fHqZ0Qg89g6A5AfuNuQJzN+w KtZMqSJgZWhMR9vL0dSURmK6tHsgRCDhk0B/J/0Edm6NsTQSAiVP8= Received: (qmail 97471 invoked by alias); 24 Jul 2018 16:05:04 -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 Received: (qmail 96612 invoked by uid 89); 24 Jul 2018 16:05:03 -0000 Authentication-Results: sourceware.org; auth=none X-HELO: mail-qt0-f193.google.com Subject: Re: [PATCH v8 2/8] nptl: Add C11 threads mtx_* functions To: Adhemerval Zanella , libc-alpha@sourceware.org References: <1517591084-11347-1-git-send-email-adhemerval.zanella@linaro.org> <1517591084-11347-3-git-send-email-adhemerval.zanella@linaro.org> <56dc5757-845a-72f8-458e-849491ddbb97@linaro.org> From: Carlos O'Donell Message-ID: <0d40a416-a27e-9df2-941a-21f2a3af1934@redhat.com> Date: Tue, 24 Jul 2018 12:04:56 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <56dc5757-845a-72f8-458e-849491ddbb97@linaro.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 07/24/2018 08:04 AM, Adhemerval Zanella wrote: > Updated patch from Carlos suggestion. Changes from previous version: > > - Remove an extra mtx_destroy on nptl/Versions. > - Adjusted the wording on cnd_init static asserts. > - Add bugzilla entry and adjusted glibc version on CL. > > --- > > This patch adds the cnd_* definitions from C11 threads (ISO/IEC 9899:2011), > more specifically cnd_broadcast, cnd_destroy, cnd_init, cnd_signal, > cnd_timedwait, cnd_wait, and required types. > OK. I confirm going through the standard and I see: cnd_t, cnd_broadcast, cnd_destroy, cnd_init, cnd_signal, cnd_timedwait, and cnd_wait as the only condvar functions in use. The rest of the usage requires the mutex support which has already been reviewed. > Mostly of the definitions are composed based on POSIX conterparts, and > cnd_t is also based on internal pthreads fields, but with distinct internal > layout to avoid possible issues with code interchange (such as trying to pass > POSIX structure on C11 functions and to avoid inclusion of pthread.h). The > idea is to make it possible to share POSIX internal implementation for mostly > of the code making adjust where only required. OK. > Checked with a build for all major ABI (aarch64-linux-gnu, alpha-linux-gnu, > arm-linux-gnueabi, i386-linux-gnu, ia64-linux-gnu, m68k-linux-gnu, > microblaze-linux-gnu [1], mips{64}-linux-gnu, nios2-linux-gnu, > powerpc{64le}-linux-gnu, s390{x}-linux-gnu, sparc{64}-linux-gnu, > tile{pro,gx}-linux-gnu, and x86_64-linux-gnu). OK. > Also ran a full check on aarch64-linux-gnu, x86_64-linux-gnu, i686-linux-gnu, > arm-linux-gnueabhf, and powerpc64le-linux-gnu. > > [BZ #14092] > * conform/data/threads.h-data (cnd_t): New type. > (cnd_init): New function. > (cnd_signal): Likewise. > (cnd_broadcast): Likewise. > (cnd_wait): Likewise. > (cnd_timedwait): Likewise. > (cnd_destroy): Likewise. > * nptl/Makefile (libpthread-routines): Add cnd_broadcast, > cnd_destroy, cnd_init, cnd_signal, cnd_timedwait, and cnd_wait > object. > * nptl/Versions (libpthread) [GLIBC_2.28]: Likewise. > * nptl/cnd_broadcast.c: New file. > * nptl/cnd_destroy.c: Likewise. > * nptl/cnd_init.c: Likewise. > * nptl/cnd_signal.c: Likewise. > * nptl/cnd_timedwait.c: Likewise. > * nptl/cnd_wait.c: Likewise. > * sysdeps/nptl/threads.h (cnd_t): New type. > (cnd_init): New prototype. > (cnd_signa): Likewise. > (cnd_broadcast): Likewise. > (cnd_wait): Likewise. > (cnd_timedwait): Likewise. > (cnd_destroy): Likewise. This looks good to me. Reviewed-by: Carlos O'Donell > --- > ChangeLog | 26 ++++++++++++++++++++++++++ > conform/data/threads.h-data | 8 ++++++++ > nptl/Makefile | 3 ++- > nptl/Versions | 3 ++- > nptl/cnd_broadcast.c | 26 ++++++++++++++++++++++++++ > nptl/cnd_destroy.c | 26 ++++++++++++++++++++++++++ > nptl/cnd_init.c | 33 +++++++++++++++++++++++++++++++++ > nptl/cnd_signal.c | 26 ++++++++++++++++++++++++++ > nptl/cnd_timedwait.c | 29 +++++++++++++++++++++++++++++ > nptl/cnd_wait.c | 27 +++++++++++++++++++++++++++ > nptl/threads.h | 34 ++++++++++++++++++++++++++++++++++ > 11 files changed, 239 insertions(+), 2 deletions(-) > create mode 100644 nptl/cnd_broadcast.c > create mode 100644 nptl/cnd_destroy.c > create mode 100644 nptl/cnd_init.c > create mode 100644 nptl/cnd_signal.c > create mode 100644 nptl/cnd_timedwait.c > create mode 100644 nptl/cnd_wait.c This looks good to me. It looks good to me for 2.28. Reviewed-by: Carlos O'Donell > diff --git a/ChangeLog b/ChangeLog > index 1b1d79e..f92f25a 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,6 +1,32 @@ > 2018-07-21 Adhemerval Zanella > > [BZ #14092] > + * conform/data/threads.h-data (cnd_t): New type. > + (cnd_init): New function. > + (cnd_signal): Likewise. > + (cnd_broadcast): Likewise. > + (cnd_wait): Likewise. > + (cnd_timedwait): Likewise. > + (cnd_destroy): Likewise. > + * nptl/Makefile (libpthread-routines): Add cnd_broadcast, > + cnd_destroy, cnd_init, cnd_signal, cnd_timedwait, and cnd_wait > + object. > + * nptl/Versions (libpthread) [GLIBC_2.28]: Likewise. > + * nptl/cnd_broadcast.c: New file. > + * nptl/cnd_destroy.c: Likewise. > + * nptl/cnd_init.c: Likewise. > + * nptl/cnd_signal.c: Likewise. > + * nptl/cnd_timedwait.c: Likewise. > + * nptl/cnd_wait.c: Likewise. > + * sysdeps/nptl/threads.h (cnd_t): New type. > + (cnd_init): New prototype. > + (cnd_signa): Likewise. > + (cnd_broadcast): Likewise. > + (cnd_wait): Likewise. > + (cnd_timedwait): Likewise. > + (cnd_destroy): Likewise. > + OK. > + [BZ #14092] > * conform/data/threads.h-data (ONCE_FLAG_INIT): New macro. > (once_flag): New type. > (call_once): New function. > diff --git a/conform/data/threads.h-data b/conform/data/threads.h-data > index 70b2fe0..d7c562e 100644 > --- a/conform/data/threads.h-data > +++ b/conform/data/threads.h-data > @@ -16,6 +16,7 @@ type thrd_t > type thrd_start_t > type mtx_t > type once_flag > +type cnd_t > > function int thrd_create (thrd_t*, thrd_start_t, void*) > function int thrd_equal (thrd_t, thrd_t) > @@ -35,6 +36,13 @@ function void mtx_destroy (mtx_t*) > > function void call_once (once_flag*, void (*)(void)) > > +function int cnd_init (cnd_t*) OK. > +function int cnd_signal (cnd_t*) OK. > +function int cnd_broadcast (cnd_t*) OK. > +function int cnd_wait (cnd_t*, mtx_t*) OK. > +function int cnd_timedwait (cnd_t*, mtx_t*, const struct timespec*) OK. > +function void cnd_destroy (cnd_t*) OK. > + > #include "time.h-data" > > #endif > diff --git a/nptl/Makefile b/nptl/Makefile > index d55d24b..0864aee 100644 > --- a/nptl/Makefile > +++ b/nptl/Makefile > @@ -143,7 +143,8 @@ libpthread-routines = nptl-init nptlfreeres vars events version pt-interp \ > pthread_setattr_default_np pthread_getattr_default_np \ > thrd_create thrd_detach thrd_exit thrd_join \ > mtx_destroy mtx_init mtx_lock mtx_timedlock \ > - mtx_trylock mtx_unlock call_once > + mtx_trylock mtx_unlock call_once cnd_broadcast \ > + cnd_destroy cnd_init cnd_signal cnd_timedwait cnd_wait OK. 6 interfaces. > # pthread_setuid pthread_seteuid pthread_setreuid \ > # pthread_setresuid \ > # pthread_setgid pthread_setegid pthread_setregid \ > diff --git a/nptl/Versions b/nptl/Versions > index 0fdba18..9c38d67 100644 > --- a/nptl/Versions > +++ b/nptl/Versions > @@ -273,7 +273,8 @@ libpthread { > GLIBC_2.28 { > thrd_create; thrd_detach; thrd_exit; thrd_join; > mtx_init; mtx_lock; mtx_timedlock; mtx_trylock; mtx_unlock; mtx_destroy; > - call_once; > + call_once; cnd_broadcast; cnd_destroy; cnd_init; cnd_signal; > + cnd_timedwait; cnd_wait; OK. 6. > } > > GLIBC_PRIVATE { > diff --git a/nptl/cnd_broadcast.c b/nptl/cnd_broadcast.c > new file mode 100644 > index 0000000..889f88e > --- /dev/null > +++ b/nptl/cnd_broadcast.c > @@ -0,0 +1,26 @@ > +/* C11 thread conditional broadcast implementation. > + Copyright (C) 2018 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + . */ > + > +#include "thrd_priv.h" > + > +int > +cnd_broadcast (cnd_t *cond) > +{ > + int err_code = __pthread_cond_broadcast ((pthread_cond_t*) cond); > + return thrd_err_map (err_code); > +} OK. > diff --git a/nptl/cnd_destroy.c b/nptl/cnd_destroy.c > new file mode 100644 > index 0000000..37df9e9 > --- /dev/null > +++ b/nptl/cnd_destroy.c > @@ -0,0 +1,26 @@ > +/* C11 threads conditional destroy implementation. > + Copyright (C) 2018 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + . */ > + > +#include "thrd_priv.h" > +#include "pthreadP.h" > + > +void > +cnd_destroy (cnd_t *cond) > +{ > + __pthread_cond_destroy ((pthread_cond_t *) cond); > +} OK. > diff --git a/nptl/cnd_init.c b/nptl/cnd_init.c > new file mode 100644 > index 0000000..13f0f80 > --- /dev/null > +++ b/nptl/cnd_init.c > @@ -0,0 +1,33 @@ > +/* C11 thread conditional initialization implementation. > + Copyright (C) 2018 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + . */ > + > +#include > + > +#include "thrd_priv.h" > + > +int > +cnd_init (cnd_t *cond) > +{ > + _Static_assert (sizeof (cnd_t) == sizeof (pthread_cond_t), > + "(sizeof (cnd_t) != sizeof (pthread_cond_t)"); > + _Static_assert (alignof (cnd_t) == alignof (pthread_cond_t), > + "alignof (cnd_t) != alignof (pthread_cond_t)"); OK. > + > + int err_code = __pthread_cond_init ((pthread_cond_t *)cond, NULL); > + return thrd_err_map (err_code); OK. > +} > diff --git a/nptl/cnd_signal.c b/nptl/cnd_signal.c > new file mode 100644 > index 0000000..8ae650b > --- /dev/null > +++ b/nptl/cnd_signal.c > @@ -0,0 +1,26 @@ > +/* C11 threads conditional signal implementation. > + Copyright (C) 2018 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + . */ > + > +#include "thrd_priv.h" > + > +int > +cnd_signal (cnd_t *cond) > +{ > + int err_code = __pthread_cond_signal ((pthread_cond_t *) cond); > + return thrd_err_map (err_code); > +} OK. > diff --git a/nptl/cnd_timedwait.c b/nptl/cnd_timedwait.c > new file mode 100644 > index 0000000..75358fe > --- /dev/null > +++ b/nptl/cnd_timedwait.c > @@ -0,0 +1,29 @@ > +/* C11 threads conditional timed wait implementation. > + Copyright (C) 2018 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + . */ > + > +#include "thrd_priv.h" > + > +int > +cnd_timedwait (cnd_t *restrict cond, mtx_t *restrict mutex, > + const struct timespec* restrict time_point) > +{ > + int err_code = __pthread_cond_timedwait ((pthread_cond_t *) cond, > + (pthread_mutex_t *) mutex, > + time_point); > + return thrd_err_map (err_code); OK. > +} > diff --git a/nptl/cnd_wait.c b/nptl/cnd_wait.c > new file mode 100644 > index 0000000..727d9bb > --- /dev/null > +++ b/nptl/cnd_wait.c > @@ -0,0 +1,27 @@ > +/* C11 threads conditional wait implementaiton. > + Copyright (C) 2018 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + . */ > + > +#include "thrd_priv.h" > + > +int > +cnd_wait (cnd_t *cond, mtx_t *mutex) > +{ > + int err_code = __pthread_cond_wait ((pthread_cond_t *) cond, > + (pthread_mutex_t *) mutex); > + return thrd_err_map (err_code); OK. > +} > diff --git a/nptl/threads.h b/nptl/threads.h > index 32f7cf8..e46b1b7 100644 > --- a/nptl/threads.h > +++ b/nptl/threads.h > @@ -60,6 +60,12 @@ typedef union > long int __align __LOCK_ALIGNMENT; > } mtx_t; > > +typedef union > +{ > + char __size[__SIZEOF_PTHREAD_COND_T]; > + __extension__ long long int __align __LOCK_ALIGNMENT; > +} cnd_t; OK. > + > /* Threads functions. */ > > /* Create a new thread executing the function __FUNC. Arguments for __FUNC > @@ -140,6 +146,34 @@ extern void mtx_destroy (mtx_t *__mutex); > All calls must be made with the same __FLAGS object. */ > extern void call_once (once_flag *__flag, void (*__func)(void)); > > + > +/* Condition variable functions. */ > + > +/* Initialize new condition variable pointed by __COND. */ > +extern int cnd_init (cnd_t *__cond); > + > +/* Unblock one thread that currently waits on condition variable pointed > + by __COND. */ > +extern int cnd_signal (cnd_t *__cond); > + > +/* Unblock all threads currently waiting on condition variable pointed by > + __COND. */ > +extern int cnd_broadcast (cnd_t *__cond); > + > +/* Block current thread on the condition variable pointed by __COND. */ > +extern int cnd_wait (cnd_t *__cond, mtx_t *__mutex); > + > +/* Block current thread on the condition variable until condition variable > + pointed by __COND is signaled or time pointed by __TIME_POINT is > + reached. */ > +extern int cnd_timedwait (cnd_t *__restrict __cond, > + mtx_t *__restrict __mutex, > + const struct timespec *__restrict __time_point); > + > +/* Destroy condition variable pointed by __cond and free all of its > + resources. */ > +extern void cnd_destroy (cnd_t *__COND); OK. > + > __END_DECLS > > #endif /* _THREADS_H */ > Cheers, Carlos.