[review v2] nptl: Add struct_mutex.h

["Florian Weimer (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>]

Florian Weimer has posted comments on this change.

Change URL: https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/518
......................................................................


Patch Set 2:

(11 comments)

| --- /dev/null
| +++ /COMMIT_MSG
| @@ -1,0 +11,29 @@ C11 on pthreadtypes-arch.h (added by commit 06be6368da16104be5) is
| +not really the best options for newer ports.  It requires define some
| +misleading flags that should be always defined as 0
| +(__PTHREAD_COMPAT_PADDING_MID and __PTHREAD_COMPAT_PADDING_END), it
| +exposes options used solely for linuxthreads compat mode
| +(__PTHREAD_MUTEX_USE_UNION and __PTHREAD_MUTEX_NUSERS_AFTER_KIND), and
| +requires newer ports to explicit define them (adding more boilerplate
| +code).
| +
| +This patch adds a new default __pthread_mutex_s definition meant to
| +be used by newer ports.  Its layout mimic the current usage on both

PS2, Line 20:

“mimics”

| +32 and 64 bits ports and it allows most ports use the generic

PS2, Line 21:

“most ports to use”

| +definition.  Only ports that use some arch-specific definition (such
| +as hardware lock-elision or linuxthread compat) requires specific

PS2, Line 23:

“linuxthreads”

| +headers.
| +
| +For 32 bits, the generic definitions mimics the other 32 bits ports

PS2, Line 26:

“For 32 bit, the generic definitions mimic the other 32-bit ports”
(I think)

| +of using an union to define the fields uses on adaptive and robust
| +mutexes (thus not allowing both usage at same time) and by using a
| +single linked-list for robust mutexes.  Both decisions seemed to
| +follow what recent ports has done and make the resulting

PS2, Line 30:

“have done”

| +pthread_mutex_t/mtx_t object smaller.
| +
| +Also the static intialization macro for pthread_mutex_t is set to use
| +an arch defined on (__PTHREAD_MUTEX_INITIALIZER) which simplifies its

PS2, Line 34:

Not sure what “an arch defined on” means in this context.

| +implementation.
| +
| +Checked with a build on affected abis.
| +
| +Change-Id: I30a22c3e3497805fd6e52994c5925897cffcfe13
| --- /dev/null
| +++ sysdeps/nptl/bits/struct_mutex.h
| @@ -1,0 +1,10 @@ 
| +/* Default internal mutex struct definitions.  Linux version.

PS2, Line 1:

Maybe “/* Default mutex implementation struct definitions”? As a bits/
header, this is not really internal.

This also affects the sysdeps overrides.

| +   Copyright (C) 2019 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,

 ...

| @@ -1,0 +41,20 @@ #endif
| +
| +     After a mutex has been initialized, the __kind of a mutex is usually not
| +     changed.  BUT it can be set to -1 in pthread_mutex_destroy or elision can
| +     be enabled.  This is done concurrently in the pthread_mutex_*lock
| +     functions by using the macro FORCE_ELISION. This macro is only defined
| +     for architectures which supports lock elision.
| +
| +     For elision, there are the flags PTHREAD_MUTEX_ELISION_NP and
| +     PTHREAD_MUTEX_NO_ELISION_NP which can be set in addition to the already
| +     set type of a mutex.  Before a mutex is initialized, only
| +     PTHREAD_MUTEX_NO_ELISION_NP can be set with pthread_mutexattr_settype.

PS2, Line 51:

I don't understand the last comment (the reference to
pthread_mutexattr_settype).

| +
| +     After a mutex has been initialized, the functions pthread_mutex_*lock can
| +     enable elision - if the mutex-type and the machine supports it - by
| +     setting the flag PTHREAD_MUTEX_ELISION_NP. This is done concurrently.
| +     Afterwards the lock / unlock functions are using specific elision
| +     code-paths.  */
| +  int __kind;
| +#if __WORDSIZE != 64
| +  unsigned int __nusers;
| --- sysdeps/nptl/bits/thread-shared-types.h
| +++ sysdeps/nptl/bits/thread-shared-types.h
| @@ -160,11 +62,20 @@ #else
| -    __PTHREAD_SPINS_DATA;
| -    __pthread_slist_t __list;
| -  };
| -# define __PTHREAD_MUTEX_HAVE_PREV      0
| -#endif
| -  __PTHREAD_COMPAT_PADDING_END
| -};
| +
| +/* Arch-specific mutex definitions.  A generic implementation is provided
| +   by sysdeps/nptl/bits/struct_mutex.h.  If required, an architecture

PS2, Line 64:

I think it is inappropriate to refer to sysdeps paths in installed
headers because they are not meaningful after installation.

Maybe this information should go into Makefile, where bits/thread-
shared-types.h is added to headers?

| +   can override it by defining:
| +
| +   1. struct __pthread_mutex_s (used on both pthread_mutex_t and mtx_t
| +      definition).  It should contains at least the internal members
| +      defined in the generic version.

PS2, Line 69:

“used in the definition of pthread_mutex_t and mtx_t”?

| +
| +   2. __LOCK_ALIGNMENT for any extra attribute for internal lock used with
| +      atomic operations.

PS2, Line 72:

__LOCK_ALIGNMENT should probably removed (replaced by arch-specific
headers) because it makes the types ABI-incompatible if a non-GNU
compiler used (missing __attribute__ support).  So we really should
not encourage its use. But this is separate change.

| +
| +   3. The macro __PTHREAD_MUTEX_INITIALIZER used for static initialization.
| +      It should initialize the mutex internal flag.  */
| +
| +#include <bits/struct_mutex.h>
|  
|  
|  /* Common definition of pthread_cond_t. */
|  

-- 
Gerrit-Project: glibc
Gerrit-Branch: master
Gerrit-Change-Id: I30a22c3e3497805fd6e52994c5925897cffcfe13
Gerrit-Change-Number: 518
Gerrit-PatchSet: 2
Gerrit-Owner: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Gerrit-Reviewer: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Gerrit-Reviewer: Florian Weimer <fweimer@redhat.com>
Gerrit-Comment-Date: Thu, 14 Nov 2019 14:37:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment