New condvar implementation that provides stronger ordering guarantees.
Commit Message
On Tue, 2016-06-14 at 22:53 +0200, Torvald Riegel wrote:
> I've now tested this patch successfully using our existing tests on ppc,
> ppc64, ppc64le, s390x, and aarch64. I couldn't test on s390 due to
> https://www.sourceware.org/ml/libc-alpha/2016-06/msg00545.html.
>
> The attached patch is a minor revision that just fixes some formatting
> and adds an include (revealing by testing on s390x).
I have attached a followup patch that adds support for the pretty
printers added in the meantime. For ease-of-review, I'm sending this as
a separate path and not a revision of the previous patch; I will commit
it as a single patch though.
Comments
On 12/23/2016 11:33 AM, Torvald Riegel wrote:
> On Tue, 2016-06-14 at 22:53 +0200, Torvald Riegel wrote:
>> I've now tested this patch successfully using our existing tests on ppc,
>> ppc64, ppc64le, s390x, and aarch64. I couldn't test on s390 due to
>> https://www.sourceware.org/ml/libc-alpha/2016-06/msg00545.html.
>>
>> The attached patch is a minor revision that just fixes some formatting
>> and adds an include (revealing by testing on s390x).
> I have attached a followup patch that adds support for the pretty
> printers added in the meantime. For ease-of-review, I'm sending this as
> a separate path and not a revision of the previous patch; I will commit
> it as a single patch though.
This looks good to me, though the implementation is fairly rudimentary.
I would like us to continue to make this better _before_ the release to
help developers working on the various ports that need to make this work.
When I say "better" I mean expose more algorithm details.
So this should get checked in with the new condvar, but we should cricle
back to this before the release.
Cheers,
Carlos.
> commit 157cc3cc3e7c6656337f23079272d4247dd53814
> Author: Torvald Riegel <triegel@redhat.com>
> Date: Fri Dec 23 14:52:08 2016 +0100
>
> Additional support for pretty printers.
>
> diff --git a/nptl/nptl-printers.py b/nptl/nptl-printers.py
> index e402f23..c207e9c 100644
> --- a/nptl/nptl-printers.py
> +++ b/nptl/nptl-printers.py
> @@ -293,16 +293,6 @@ class MutexAttributesPrinter(object):
> elif protocol == PTHREAD_PRIO_PROTECT:
> self.values.append(('Protocol', 'Priority protect'))
>
> -CLOCK_IDS = {
> - CLOCK_REALTIME: 'CLOCK_REALTIME',
> - CLOCK_MONOTONIC: 'CLOCK_MONOTONIC',
> - CLOCK_PROCESS_CPUTIME_ID: 'CLOCK_PROCESS_CPUTIME_ID',
> - CLOCK_THREAD_CPUTIME_ID: 'CLOCK_THREAD_CPUTIME_ID',
> - CLOCK_MONOTONIC_RAW: 'CLOCK_MONOTONIC_RAW',
> - CLOCK_REALTIME_COARSE: 'CLOCK_REALTIME_COARSE',
> - CLOCK_MONOTONIC_COARSE: 'CLOCK_MONOTONIC_COARSE'
> -}
> -
> class ConditionVariablePrinter(object):
> """Pretty printer for pthread_cond_t."""
>
> @@ -313,24 +303,8 @@ class ConditionVariablePrinter(object):
> cond: A gdb.value representing a pthread_cond_t.
> """
>
> - # Since PTHREAD_COND_SHARED is an integer, we need to cast it to void *
> - # to be able to compare it to the condvar's __data.__mutex member.
> - #
> - # While it looks like self.shared_value should be a class variable,
> - # that would result in it having an incorrect size if we're loading
> - # these printers through .gdbinit for a 64-bit objfile in AMD64.
> - # This is because gdb initially assumes the pointer size to be 4 bytes,
> - # and only sets it to 8 after loading the 64-bit objfiles. Since
> - # .gdbinit runs before any objfiles are loaded, this would effectively
> - # make self.shared_value have a size of 4, thus breaking later
> - # comparisons with pointers whose types are looked up at runtime.
> - void_ptr_type = gdb.lookup_type('void').pointer()
> - self.shared_value = gdb.Value(PTHREAD_COND_SHARED).cast(void_ptr_type)
> -
> data = cond['__data']
> - self.total_seq = data['__total_seq']
> - self.mutex = data['__mutex']
> - self.nwaiters = data['__nwaiters']
> + self.wrefs = data['__wrefs']
> self.values = []
>
> self.read_values()
> @@ -360,7 +334,6 @@ class ConditionVariablePrinter(object):
>
> self.read_status()
> self.read_attributes()
> - self.read_mutex_info()
>
> def read_status(self):
> """Read the status of the condvar.
> @@ -369,41 +342,22 @@ class ConditionVariablePrinter(object):
> are waiting for it.
> """
>
> - if self.total_seq == PTHREAD_COND_DESTROYED:
> - self.values.append(('Status', 'Destroyed'))
> -
> - self.values.append(('Threads waiting for this condvar',
> - self.nwaiters >> COND_NWAITERS_SHIFT))
> + self.values.append(('Threads known to still execute a wait function',
> + self.wrefs >> PTHREAD_COND_WREFS_SHIFT))
>
> def read_attributes(self):
> """Read the condvar's attributes."""
>
> - clock_id = self.nwaiters & ((1 << COND_NWAITERS_SHIFT) - 1)
> -
> - # clock_id must be casted to int because it's a gdb.Value
> - self.values.append(('Clock ID', CLOCK_IDS[int(clock_id)]))
> + if (self.wrefs & PTHREAD_COND_CLOCK_MONOTONIC_MASK) != 0:
> + self.values.append(('Clock ID', 'CLOCK_MONOTONIC'))
> + else:
> + self.values.append(('Clock ID', 'CLOCK_REALTIME'))
>
> - shared = (self.mutex == self.shared_value)
> -
> - if shared:
> + if (self.wrefs & PTHREAD_COND_SHARED_MASK) != 0:
> self.values.append(('Shared', 'Yes'))
> else:
> self.values.append(('Shared', 'No'))
>
> - def read_mutex_info(self):
> - """Read the data of the mutex this condvar is bound to.
> -
> - A pthread_cond_t's __data.__mutex member is a void * which
> - must be casted to pthread_mutex_t *. For shared condvars, this
> - member isn't recorded and has a special value instead.
> - """
> -
> - if self.mutex and self.mutex != self.shared_value:
> - mutex_type = gdb.lookup_type('pthread_mutex_t')
> - mutex = self.mutex.cast(mutex_type.pointer()).dereference()
> -
> - self.values.append(('Mutex', mutex))
> -
> class ConditionVariableAttributesPrinter(object):
> """Pretty printer for pthread_condattr_t.
>
> @@ -453,10 +407,12 @@ class ConditionVariableAttributesPrinter(object):
> created in self.children.
> """
>
> - clock_id = self.condattr & ((1 << COND_NWAITERS_SHIFT) - 1)
> + clock_id = (self.condattr >> 1) & ((1 << COND_CLOCK_BITS) - 1)
>
> - # clock_id must be casted to int because it's a gdb.Value
> - self.values.append(('Clock ID', CLOCK_IDS[int(clock_id)]))
> + if clock_id != 0:
> + self.values.append(('Clock ID', 'CLOCK_MONOTONIC'))
> + else:
> + self.values.append(('Clock ID', 'CLOCK_REALTIME'))
>
> if self.condattr & 1:
> self.values.append(('Shared', 'Yes'))
> diff --git a/nptl/nptl_lock_constants.pysym b/nptl/nptl_lock_constants.pysym
> index 303ec61..2ab3179 100644
> --- a/nptl/nptl_lock_constants.pysym
> +++ b/nptl/nptl_lock_constants.pysym
> @@ -44,26 +44,13 @@ PTHREAD_PRIO_NONE
> PTHREAD_PRIO_INHERIT
> PTHREAD_PRIO_PROTECT
>
> --- These values are hardcoded as well:
> --- Value of __mutex for shared condvars.
> -PTHREAD_COND_SHARED (void *)~0l
> -
> --- Value of __total_seq for destroyed condvars.
> -PTHREAD_COND_DESTROYED -1ull
> -
> --- __nwaiters encodes the number of threads waiting on a condvar
> --- and the clock ID.
> --- __nwaiters >> COND_NWAITERS_SHIFT gives us the number of waiters.
> -COND_NWAITERS_SHIFT
> -
> --- Condvar clock IDs
> -CLOCK_REALTIME
> -CLOCK_MONOTONIC
> -CLOCK_PROCESS_CPUTIME_ID
> -CLOCK_THREAD_CPUTIME_ID
> -CLOCK_MONOTONIC_RAW
> -CLOCK_REALTIME_COARSE
> -CLOCK_MONOTONIC_COARSE
> +-- Condition variable
> +-- FIXME Why do macros prefixed with __ cannot be used directly?
> +PTHREAD_COND_SHARED_MASK __PTHREAD_COND_SHARED_MASK
> +PTHREAD_COND_CLOCK_MONOTONIC_MASK __PTHREAD_COND_CLOCK_MONOTONIC_MASK
> +COND_CLOCK_BITS
> +-- These values are hardcoded:
> +PTHREAD_COND_WREFS_SHIFT 3
>
> -- Rwlock attributes
> PTHREAD_RWLOCK_PREFER_READER_NP
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 6e0dd09..92a9992 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -167,6 +167,13 @@ enum
> #define __PTHREAD_ONCE_FORK_GEN_INCR 4
>
>
> +/* Condition variable definitions. See __pthread_cond_wait_common.
> + Need to be defined here so there is one place from which
> + nptl_lock_constants can grab them. */
> +#define __PTHREAD_COND_CLOCK_MONOTONIC_MASK 2
> +#define __PTHREAD_COND_SHARED_MASK 1
> +
> +
> /* Internal variables. */
>
>
> diff --git a/nptl/pthread_cond_init.c b/nptl/pthread_cond_init.c
> index cdd9b7d..c1eac5f 100644
> --- a/nptl/pthread_cond_init.c
> +++ b/nptl/pthread_cond_init.c
> @@ -29,6 +29,10 @@ __pthread_cond_init (pthread_cond_t *cond, const pthread_condattr_t *cond_attr)
> struct pthread_condattr *icond_attr = (struct pthread_condattr *) cond_attr;
>
> memset (cond, 0, sizeof (pthread_cond_t));
> +
> + /* Update the pretty printers if the internal representation of icond_attr
> + is changed. */
> +
> /* Iff not equal to ~0l, this is a PTHREAD_PROCESS_PRIVATE condvar. */
> if (icond_attr != NULL && (icond_attr->value & 1) != 0)
> cond->__data.__wrefs |= __PTHREAD_COND_SHARED_MASK;
> diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c
> index 984f01f..2b43402 100644
> --- a/nptl/pthread_cond_wait.c
> +++ b/nptl/pthread_cond_wait.c
> @@ -293,6 +293,8 @@ __condvar_cleanup_waiting (void *arg)
> * Bit 1 is the clock ID (0 == CLOCK_REALTIME, 1 == CLOCK_MONOTONIC).
> * Bit 0 is true iff this is a process-shared condvar.
> * Simple reference count used by both waiters and pthread_cond_destroy.
> + (If the format of __wrefs is changed, update nptl_lock_constants.pysym
> + and the pretty printers.)
> For each of the two groups, we have:
> __g_refs: Futex waiter reference count.
> * LSB is true if waiters should run futex_wake when they remove the
> diff --git a/nptl/test-cond-printers.py b/nptl/test-cond-printers.py
> index af0e12e..9e807c9 100644
> --- a/nptl/test-cond-printers.py
> +++ b/nptl/test-cond-printers.py
> @@ -35,7 +35,7 @@ try:
>
> break_at(test_source, 'Test status (destroyed)')
> continue_cmd() # Go to test_status_destroyed
> - test_printer(var, to_string, {'Status': 'Destroyed'})
> + test_printer(var, to_string, {'Threads known to still execute a wait function': '0'})
>
> continue_cmd() # Exit
>
> diff --git a/scripts/test_printers_common.py b/scripts/test_printers_common.py
> index c79d7e3..91cf5ec 100644
> --- a/scripts/test_printers_common.py
> +++ b/scripts/test_printers_common.py
> @@ -157,7 +157,7 @@ def init_test(test_bin, printer_files, printer_names):
>
> # Load all the pretty printer files. We're assuming these are safe.
> for printer_file in printer_files:
> - test('source {0}'.format(printer_file))
> + test('source {0}'.format(printer_file), '')
>
> # Disable all the pretty printers.
> test('disable pretty-printer', r'0 of [0-9]+ printers enabled')
> diff --git a/sysdeps/x86/bits/pthreadtypes.h b/sysdeps/x86/bits/pthreadtypes.h
> index 59b1d0d..38bc454 100644
> --- a/sysdeps/x86/bits/pthreadtypes.h
> +++ b/sysdeps/x86/bits/pthreadtypes.h
> @@ -161,8 +161,6 @@ typedef union
> unsigned int __g1_orig_size;
> unsigned int __wrefs;
> unsigned int __g_signals[2];
> -#define __PTHREAD_COND_CLOCK_MONOTONIC_MASK 2
> -#define __PTHREAD_COND_SHARED_MASK 1
> } __data;
> char __size[__SIZEOF_PTHREAD_COND_T];
> __extension__ long long int __align;
commit 157cc3cc3e7c6656337f23079272d4247dd53814
Author: Torvald Riegel <triegel@redhat.com>
Date: Fri Dec 23 14:52:08 2016 +0100
Additional support for pretty printers.
@@ -293,16 +293,6 @@ class MutexAttributesPrinter(object):
elif protocol == PTHREAD_PRIO_PROTECT:
self.values.append(('Protocol', 'Priority protect'))
-CLOCK_IDS = {
- CLOCK_REALTIME: 'CLOCK_REALTIME',
- CLOCK_MONOTONIC: 'CLOCK_MONOTONIC',
- CLOCK_PROCESS_CPUTIME_ID: 'CLOCK_PROCESS_CPUTIME_ID',
- CLOCK_THREAD_CPUTIME_ID: 'CLOCK_THREAD_CPUTIME_ID',
- CLOCK_MONOTONIC_RAW: 'CLOCK_MONOTONIC_RAW',
- CLOCK_REALTIME_COARSE: 'CLOCK_REALTIME_COARSE',
- CLOCK_MONOTONIC_COARSE: 'CLOCK_MONOTONIC_COARSE'
-}
-
class ConditionVariablePrinter(object):
"""Pretty printer for pthread_cond_t."""
@@ -313,24 +303,8 @@ class ConditionVariablePrinter(object):
cond: A gdb.value representing a pthread_cond_t.
"""
- # Since PTHREAD_COND_SHARED is an integer, we need to cast it to void *
- # to be able to compare it to the condvar's __data.__mutex member.
- #
- # While it looks like self.shared_value should be a class variable,
- # that would result in it having an incorrect size if we're loading
- # these printers through .gdbinit for a 64-bit objfile in AMD64.
- # This is because gdb initially assumes the pointer size to be 4 bytes,
- # and only sets it to 8 after loading the 64-bit objfiles. Since
- # .gdbinit runs before any objfiles are loaded, this would effectively
- # make self.shared_value have a size of 4, thus breaking later
- # comparisons with pointers whose types are looked up at runtime.
- void_ptr_type = gdb.lookup_type('void').pointer()
- self.shared_value = gdb.Value(PTHREAD_COND_SHARED).cast(void_ptr_type)
-
data = cond['__data']
- self.total_seq = data['__total_seq']
- self.mutex = data['__mutex']
- self.nwaiters = data['__nwaiters']
+ self.wrefs = data['__wrefs']
self.values = []
self.read_values()
@@ -360,7 +334,6 @@ class ConditionVariablePrinter(object):
self.read_status()
self.read_attributes()
- self.read_mutex_info()
def read_status(self):
"""Read the status of the condvar.
@@ -369,41 +342,22 @@ class ConditionVariablePrinter(object):
are waiting for it.
"""
- if self.total_seq == PTHREAD_COND_DESTROYED:
- self.values.append(('Status', 'Destroyed'))
-
- self.values.append(('Threads waiting for this condvar',
- self.nwaiters >> COND_NWAITERS_SHIFT))
+ self.values.append(('Threads known to still execute a wait function',
+ self.wrefs >> PTHREAD_COND_WREFS_SHIFT))
def read_attributes(self):
"""Read the condvar's attributes."""
- clock_id = self.nwaiters & ((1 << COND_NWAITERS_SHIFT) - 1)
-
- # clock_id must be casted to int because it's a gdb.Value
- self.values.append(('Clock ID', CLOCK_IDS[int(clock_id)]))
+ if (self.wrefs & PTHREAD_COND_CLOCK_MONOTONIC_MASK) != 0:
+ self.values.append(('Clock ID', 'CLOCK_MONOTONIC'))
+ else:
+ self.values.append(('Clock ID', 'CLOCK_REALTIME'))
- shared = (self.mutex == self.shared_value)
-
- if shared:
+ if (self.wrefs & PTHREAD_COND_SHARED_MASK) != 0:
self.values.append(('Shared', 'Yes'))
else:
self.values.append(('Shared', 'No'))
- def read_mutex_info(self):
- """Read the data of the mutex this condvar is bound to.
-
- A pthread_cond_t's __data.__mutex member is a void * which
- must be casted to pthread_mutex_t *. For shared condvars, this
- member isn't recorded and has a special value instead.
- """
-
- if self.mutex and self.mutex != self.shared_value:
- mutex_type = gdb.lookup_type('pthread_mutex_t')
- mutex = self.mutex.cast(mutex_type.pointer()).dereference()
-
- self.values.append(('Mutex', mutex))
-
class ConditionVariableAttributesPrinter(object):
"""Pretty printer for pthread_condattr_t.
@@ -453,10 +407,12 @@ class ConditionVariableAttributesPrinter(object):
created in self.children.
"""
- clock_id = self.condattr & ((1 << COND_NWAITERS_SHIFT) - 1)
+ clock_id = (self.condattr >> 1) & ((1 << COND_CLOCK_BITS) - 1)
- # clock_id must be casted to int because it's a gdb.Value
- self.values.append(('Clock ID', CLOCK_IDS[int(clock_id)]))
+ if clock_id != 0:
+ self.values.append(('Clock ID', 'CLOCK_MONOTONIC'))
+ else:
+ self.values.append(('Clock ID', 'CLOCK_REALTIME'))
if self.condattr & 1:
self.values.append(('Shared', 'Yes'))
@@ -44,26 +44,13 @@ PTHREAD_PRIO_NONE
PTHREAD_PRIO_INHERIT
PTHREAD_PRIO_PROTECT
-PTHREAD_COND_SHARED (void *)~0l
-
-PTHREAD_COND_DESTROYED -1ull
-
-COND_NWAITERS_SHIFT
-
-CLOCK_REALTIME
-CLOCK_MONOTONIC
-CLOCK_PROCESS_CPUTIME_ID
-CLOCK_THREAD_CPUTIME_ID
-CLOCK_MONOTONIC_RAW
-CLOCK_REALTIME_COARSE
-CLOCK_MONOTONIC_COARSE
+-- Condition variable
+-- FIXME Why do macros prefixed with __ cannot be used directly?
+PTHREAD_COND_SHARED_MASK __PTHREAD_COND_SHARED_MASK
+PTHREAD_COND_CLOCK_MONOTONIC_MASK __PTHREAD_COND_CLOCK_MONOTONIC_MASK
+COND_CLOCK_BITS
+-- These values are hardcoded:
+PTHREAD_COND_WREFS_SHIFT 3
-- Rwlock attributes
PTHREAD_RWLOCK_PREFER_READER_NP
@@ -167,6 +167,13 @@ enum
#define __PTHREAD_ONCE_FORK_GEN_INCR 4
+/* Condition variable definitions. See __pthread_cond_wait_common.
+ Need to be defined here so there is one place from which
+ nptl_lock_constants can grab them. */
+#define __PTHREAD_COND_CLOCK_MONOTONIC_MASK 2
+#define __PTHREAD_COND_SHARED_MASK 1
+
+
/* Internal variables. */
@@ -29,6 +29,10 @@ __pthread_cond_init (pthread_cond_t *cond, const pthread_condattr_t *cond_attr)
struct pthread_condattr *icond_attr = (struct pthread_condattr *) cond_attr;
memset (cond, 0, sizeof (pthread_cond_t));
+
+ /* Update the pretty printers if the internal representation of icond_attr
+ is changed. */
+
/* Iff not equal to ~0l, this is a PTHREAD_PROCESS_PRIVATE condvar. */
if (icond_attr != NULL && (icond_attr->value & 1) != 0)
cond->__data.__wrefs |= __PTHREAD_COND_SHARED_MASK;
@@ -293,6 +293,8 @@ __condvar_cleanup_waiting (void *arg)
* Bit 1 is the clock ID (0 == CLOCK_REALTIME, 1 == CLOCK_MONOTONIC).
* Bit 0 is true iff this is a process-shared condvar.
* Simple reference count used by both waiters and pthread_cond_destroy.
+ (If the format of __wrefs is changed, update nptl_lock_constants.pysym
+ and the pretty printers.)
For each of the two groups, we have:
__g_refs: Futex waiter reference count.
* LSB is true if waiters should run futex_wake when they remove the
@@ -35,7 +35,7 @@ try:
break_at(test_source, 'Test status (destroyed)')
continue_cmd() # Go to test_status_destroyed
- test_printer(var, to_string, {'Status': 'Destroyed'})
+ test_printer(var, to_string, {'Threads known to still execute a wait function': '0'})
continue_cmd() # Exit
@@ -157,7 +157,7 @@ def init_test(test_bin, printer_files, printer_names):
# Load all the pretty printer files. We're assuming these are safe.
for printer_file in printer_files:
- test('source {0}'.format(printer_file))
+ test('source {0}'.format(printer_file), '')
# Disable all the pretty printers.
test('disable pretty-printer', r'0 of [0-9]+ printers enabled')
@@ -161,8 +161,6 @@ typedef union
unsigned int __g1_orig_size;
unsigned int __wrefs;
unsigned int __g_signals[2];
-#define __PTHREAD_COND_CLOCK_MONOTONIC_MASK 2
-#define __PTHREAD_COND_SHARED_MASK 1
} __data;
char __size[__SIZEOF_PTHREAD_COND_T];
__extension__ long long int __align;