This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 2/2] Move shared pthread definitions to common headers
Thanks for the review.
On 18/04/2017 08:32, Torvald Riegel wrote:
> On Mon, 2017-04-03 at 12:08 -0300, Adhemerval Zanella wrote:
>> This patch removes all the replicated pthread definition accross the
>> architectures and consolidates it on shared headers. The new
>> organization is as follow:
>>
>> * Architecture specific definition (such as pthread types sizes) are
>> place in the new pthreadtypes-arch.h header in arch specific path.
>>
>> * All shared structure definition are moved to a common NPTL header
>> at sysdeps/nptl/bits/pthreadtypes.h (with now includes the arch
>> specific one for internal definitions).
>>
>> * Also, for C11 future thread support, both mutex and conditional
>> definition are placed in a common header at
>> sysdeps/nptl/bits/thread-shared-types.h.
>>
>> It is also a refactor patch without expected functional changes.
>> 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, 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 with the changes below.
>
> Patch 1/2 is OK.
>
>> ---
>> diff --git a/sysdeps/ia64/nptl/bits/pthreadtypes.h b/sysdeps/nptl/bits/pthreadtypes.h
>> similarity index 53%
>> rename from sysdeps/ia64/nptl/bits/pthreadtypes.h
>> rename to sysdeps/nptl/bits/pthreadtypes.h
>> index c67ee86..6348cf5 100644
>> --- a/sysdeps/ia64/nptl/bits/pthreadtypes.h
>> +++ b/sysdeps/nptl/bits/pthreadtypes.h
>> @@ -1,6 +1,6 @@
>> -/* Copyright (C) 2003-2017 Free Software Foundation, Inc.
>> +/* Declaration of common pthread types for all architectures.
>> + Copyright (C) 2017 Free Software Foundation, Inc.
>> This file is part of the GNU C Library.
>> - Contributed by Jakub Jelinek <jakub@redhat.com>, 2003.
>>
>> The GNU C Library is free software; you can redistribute it and/or
>> modify it under the terms of the GNU Lesser General Public
>> @@ -16,69 +16,21 @@
>> License along with the GNU C Library; if not, see
>> <http://www.gnu.org/licenses/>. */
>>
>> -#ifndef _BITS_PTHREADTYPES_H
>> -#define _BITS_PTHREADTYPES_H 1
>> -
>> -#define __SIZEOF_PTHREAD_ATTR_T 56
>> -#define __SIZEOF_PTHREAD_MUTEX_T 40
>> -#define __SIZEOF_PTHREAD_MUTEXATTR_T 4
>> -#define __SIZEOF_PTHREAD_COND_T 48
>> -#define __SIZEOF_PTHREAD_CONDATTR_T 4
>> -#define __SIZEOF_PTHREAD_RWLOCK_T 56
>> -#define __SIZEOF_PTHREAD_RWLOCKATTR_T 8
>> -#define __SIZEOF_PTHREAD_BARRIER_T 32
>> -#define __SIZEOF_PTHREAD_BARRIERATTR_T 4
>> +#ifndef _BITS_PTHREADTYPES_COMMON_H
>> +# define _BITS_PTHREADTYPES_COMMON_H 1
>>
>> +/* For internal mutex and conditional definitions. */
>
> s/conditional/condition variable/
Ack.
>
>> @@ -131,33 +53,43 @@ typedef unsigned int pthread_key_t;
>> typedef int pthread_once_t;
>>
>>
>> +union pthread_attr_t
>> +{
>> + char __size[__SIZEOF_PTHREAD_ATTR_T];
>> + long int __align;
>> +};
>> +#ifndef __have_pthread_attr_t
>> +typedef union pthread_attr_t pthread_attr_t;
>> +# define __have_pthread_attr_t1
>> +#endif
>> +
>> +
>> +typedef union
>> +{
>> + struct __pthread_mutex_s __data;
>> + char __size[__SIZEOF_PTHREAD_MUTEX_T];
>> + long int __align;
>> +} pthread_mutex_t;
>> +
>> +
>> +typedef union
>> +{
>> + struct __pthread_cond_s __data;
>> + char __size[__SIZEOF_PTHREAD_COND_T];
>> + __extension__ long long int __align;
>> +} pthread_cond_t;
>> +
>> +
>> #if defined __USE_UNIX98 || defined __USE_XOPEN2K
>> /* Data structure for read-write lock variable handling. The
>
> s/read-write/reader--writer/
Ack.
>
>> - structure of the attribute type is not exposed on purpose. */
>> + structure of the attribute type is deliberately not exposed. */
>> typedef union
>> {
>> - struct
>> - {
>> - unsigned int __readers;
>> - unsigned int __writers;
>> - unsigned int __wrphase_futex;
>> - unsigned int __writers_futex;
>> - unsigned int __pad3;
>> - unsigned int __pad4;
>> - int __cur_writer;
>> - int __shared;
>> - unsigned long int __pad1;
>> - unsigned long int __pad2;
>> - /* FLAGS must stay at this position in the structure to maintain
>> - binary compatibility. */
>> - unsigned int __flags;
>> - } __data;
>> + struct __pthread_rwlock_arch_t __data;
>> char __size[__SIZEOF_PTHREAD_RWLOCK_T];
>> long int __align;
>> } pthread_rwlock_t;
>>
>> -#define __PTHREAD_RWLOCK_ELISION_EXTRA 0
>> -
>> typedef union
>> {
>> char __size[__SIZEOF_PTHREAD_RWLOCKATTR_T];
>> @@ -186,5 +118,4 @@ typedef union
>> } pthread_barrierattr_t;
>> #endif
>>
>> -
>> -#endif /* bits/pthreadtypes.h */
>> +#endif
>> diff --git a/sysdeps/nptl/bits/thread-shared-types.h b/sysdeps/nptl/bits/thread-shared-types.h
>> new file mode 100644
>> index 0000000..82085f1
>> --- /dev/null
>> +++ b/sysdeps/nptl/bits/thread-shared-types.h
>> @@ -0,0 +1,121 @@
>> +/* Common thread definition for both POSIX and C11.
>
> s/thread definition/threading primitives definitions/
Ack.
>
>> + Copyright (C) 2017 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
>> + <http://www.gnu.org/licenses/>. */
>> +
>> +#ifndef _THREAD_SHARED_TYPES_H
>> +#define _THREAD_SHARED_TYPES_H 1
>> +
>> +/* Arch-specific definitions. */
>> +#include <bits/pthreadtypes-arch.h>
>> +
>> +/* Common definition of pthread_mutex_t. */
>> +
>> +#if __WORDSIZE == 64
>> +typedef struct __pthread_internal_list
>> +{
>> + struct __pthread_internal_list *__prev;
>> + struct __pthread_internal_list *__next;
>> +} __pthread_list_t;
>> +#else
>> +typedef struct __pthread_internal_slist
>> +{
>> + struct __pthread_internal_slist *__next;
>> +} __pthread_slist_t;
>> +#endif
>> +
>> +/* Lock elision support. */
>> +#if __PTHREAD_MUTEX_LOCK_ELISION
>> +# if __WORDSIZE == 64
>> +# define __PTHREAD_SPINS_DATA \
>> + short __spins; \
>> + short __elision;
>> +# define __PTHREAD_SPINS 0, 0
>> +# else
>> +# define __PTHREAD_SPINS_DATA \
>> + struct \
>> + { \
>> + short __espins; \
>> + short __eelision; \
>> + } __elision_data
>> +# define __PTHREAD_SPINS { 0, 0 }
>> +# define __spins __elision_data.__espins
>> +# define __elision __elision_data.__eelision
>
> These two macros seem rather risky to me. Is there another way to make
> this work? For which archs would struct vs. fields make a difference in
> terms of alignment etc?
For this consolidation I replicated current architecture code that uses
lock elision (they differ only in the internal struct's name and all use
the macro for accessing the fields).
Another option would to move the elision field definition at each
architecture specific header (pthreadtypes-arch.h), so it can guarantee
any alignment or other constraint. I do not have a preference here, so
if you would prefer this approach I can change the patch.
>
>> +# endif
>> +#else
>> +# define __PTHREAD_SPINS_DATA int __spins
>> +/* Mutex __spins initializer used by PTHREAD_MUTEX_INITIALIZER. */
>> +# define __PTHREAD_SPINS 0
>> +#endif
>
> Likewise, and you don't undefined __spins and __elision
The idea is __spins will be define by __PTHREAD_SPINS_DATA and since this
snippet will be defined iff the architecture explicit do not support lock
elision, using __elision field should and will thrown a compilation error.
>
>> +
>> +struct __pthread_mutex_s
>> +{
>> + int __lock __PTHREAD_LOCK_ALIGNMENT;
>> + unsigned int __count;
>> + int __owner;
>> +#if __WORDSIZE == 64
>> + unsigned int __nusers;
>> +#endif
>> + /* KIND must stay at this position in the structure to maintain
>> + binary compatibility with static initializers. */
>> + int __kind;
>> + __PTHREAD_COMPAT_PADDING_MID;
>> +#if __WORDSIZE == 64
>> + __PTHREAD_SPINS_DATA;
>> + __pthread_list_t __list;
>> +# define __PTHREAD_MUTEX_HAVE_PREV 1
>> +#else
>> + unsigned int __nusers;
>> + __extension__ union
>> + {
>> + __PTHREAD_SPINS_DATA;
>> + __pthread_slist_t __list;
>> + }
>> +#endif
>> + __PTHREAD_COMPAT_PADDING_END;
>> +};
>> +
>> +
>> +/* Common definition of pthread_cond_t. */
>> +
>> +struct __pthread_cond_s
>> +{
>> + __extension__ union
>> + {
>> + __extension__ unsigned long long int __wseq;
>> + struct
>> + {
>> + unsigned int __low;
>> + unsigned int __high;
>> + } __wseq32;
>> + };
>> + __extension__ union
>> + {
>> + __extension__ unsigned long long int __g1_start;
>> + struct
>> + {
>> + unsigned int __low;
>> + unsigned int __high;
>> + } __g1_start32;
>> + };
>> + unsigned int __g_refs[2] __PTHREAD_LOCK_ALIGNMENT;
>
> If __PTHREAD_LOCK_ALIGNMENT is not just for locks, but more generally
> captures alignment requirements for futex words, I think it should be
> named differently, and there should be a comment somewhere explaining
> it's use. This is probably also the case for the other macros that this
> file uses (though perhaps the __SIZEOF_PTHREAD_* macros are
> self-explainatory). A stub pthreadtypes-arch.h may be a way to do it,
> though I'm not sure this works as well in this case as it works in the
> case of futex-internal.h.
I think we can use pthreadtypes-arch.h, the idea is exactly to grab all
the arch specific constraint and it should be an installed header.
Otherwise we will need to create another installed header to have this
definition and I am not sure if this is worth.
In any way, I will change the macro name to __LOCK_ALIGNMENT and I will
add comments on sysdeps/nptl/bits/thread-shared-types.h explaining the
requires defines the arch-specific header should have (__LOCK_ALIGNMENT,
__PTHREAD_COMPAT_PADDING_MID, __PTHREAD_COMPAT_PADDING_END, and
__PTHREAD_MUTEX_LOCK_ELISION).
>
>> + unsigned int __g_size[2];
>> + unsigned int __g1_orig_size;
>> + unsigned int __wrefs;
>> + unsigned int __g_signals[2];
>> +};
>> +
>> +#endif /* _THREAD_SHARED_TYPES_H */
>