This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 2/2] New pthread rwlock that is more scalable.
- From: Carlos O'Donell <carlos at redhat dot com>
- To: Torvald Riegel <triegel at redhat dot com>, GLIBC Devel <libc-alpha at sourceware dot org>, Siddhesh Poyarekar <siddhesh at redhat dot com>
- Date: Tue, 10 Jan 2017 01:36:00 -0500
- Subject: Re: [PATCH 2/2] New pthread rwlock that is more scalable.
- Authentication-results: sourceware.org; auth=none
- References: <1469655533.19224.27.camel@localhost.localdomain> <1469655868.19224.34.camel@localhost.localdomain> <1482525381.14990.834.camel@redhat.com>
On 12/23/2016 03:36 PM, Torvald Riegel wrote:
> Attached is a follow-up patch that adapts the pretty printers added in
> the meantime to work with the new rwlock. For ease-of-review, this is a
> separate patch. I'll commit it as one patch, of course.
This looks good to me.
Needs a ChangeLog, but is otherwise stylistically fine.
Is it really necessary to switch from unlocked/locked
to 'not acquired'/acquired?
> commit 8839c1956594113d1f19bfe3dd13b1f4c01273ea
> Author: Torvald Riegel <triegel@redhat.com>
> Date: Fri Dec 23 21:30:59 2016 +0100
>
> rwlock: Add pretty printers support.
>
> diff --git a/nptl/nptl-printers.py b/nptl/nptl-printers.py
> index e402f23..dddbb06 100644
> --- a/nptl/nptl-printers.py
> +++ b/nptl/nptl-printers.py
> @@ -474,12 +474,10 @@ class RWLockPrinter(object):
> """
>
> data = rwlock['__data']
> - self.readers = data['__nr_readers']
> - self.queued_readers = data['__nr_readers_queued']
> - self.queued_writers = data['__nr_writers_queued']
> - self.writer_id = data['__writer']
> + self.readers = data['__readers']
> + self.cur_writer = data['__cur_writer']
> self.shared = data['__shared']
> - self.prefers_writers = data['__flags']
> + self.flags = data['__flags']
> self.values = []
> self.read_values()
>
> @@ -512,20 +510,19 @@ class RWLockPrinter(object):
> def read_status(self):
> """Read the status of the rwlock."""
>
> - # Right now pthread_rwlock_destroy doesn't do anything, so there's no
> - # way to check if an rwlock is destroyed.
> -
> - if self.writer_id:
> - self.values.append(('Status', 'Locked (Write)'))
> - self.values.append(('Writer ID', self.writer_id))
> - elif self.readers:
> - self.values.append(('Status', 'Locked (Read)'))
> - self.values.append(('Readers', self.readers))
> + if self.readers & PTHREAD_RWLOCK_WRPHASE:
> + if self.readers & PTHREAD_RWLOCK_WRLOCKED:
> + self.values.append(('Status', 'Acquired (Write)'))
> + self.values.append(('Writer ID', self.cur_writer))
> + else:
> + self.values.append(('Status', 'Not acquired'))
> else:
> - self.values.append(('Status', 'Unlocked'))
> -
> - self.values.append(('Queued readers', self.queued_readers))
> - self.values.append(('Queued writers', self.queued_writers))
> + r = self.readers >> PTHREAD_RWLOCK_READER_SHIFT
> + if r > 0:
> + self.values.append(('Status', 'Acquired (Read)'))
> + self.values.append(('Readers', r))
> + else:
> + self.values.append(('Status', 'Not acquired'))
>
> def read_attributes(self):
> """Read the attributes of the rwlock."""
> @@ -535,10 +532,12 @@ class RWLockPrinter(object):
> else:
> self.values.append(('Shared', 'No'))
>
> - if self.prefers_writers:
> + if self.flags == PTHREAD_RWLOCK_PREFER_READER_NP:
> + self.values.append(('Prefers', 'Readers'))
> + elif self.flags == PTHREAD_RWLOCK_PREFER_WRITER_NP:
> self.values.append(('Prefers', 'Writers'))
> else:
> - self.values.append(('Prefers', 'Readers'))
> + self.values.append(('Prefers', 'Writers no recursive readers'))
>
> class RWLockAttributesPrinter(object):
> """Pretty printer for pthread_rwlockattr_t.
> @@ -599,13 +598,12 @@ class RWLockAttributesPrinter(object):
> # PTHREAD_PROCESS_PRIVATE
> self.values.append(('Shared', 'No'))
>
> - if (rwlock_type == PTHREAD_RWLOCK_PREFER_READER_NP or
> - rwlock_type == PTHREAD_RWLOCK_PREFER_WRITER_NP):
> - # This is a known bug. Using PTHREAD_RWLOCK_PREFER_WRITER_NP will
> - # still make the rwlock prefer readers.
> + if rwlock_type == PTHREAD_RWLOCK_PREFER_READER_NP:
> self.values.append(('Prefers', 'Readers'))
> - elif rwlock_type == PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP:
> + elif rwlock_type == PTHREAD_RWLOCK_PREFER_WRITER_NP:
> self.values.append(('Prefers', 'Writers'))
> + else:
> + self.values.append(('Prefers', 'Writers no recursive readers'))
>
> def register(objfile):
> """Register the pretty printers within the given objfile."""
> diff --git a/nptl/nptl_lock_constants.pysym b/nptl/nptl_lock_constants.pysym
> index 303ec61..3251d2e 100644
> --- a/nptl/nptl_lock_constants.pysym
> +++ b/nptl/nptl_lock_constants.pysym
> @@ -70,6 +70,11 @@ PTHREAD_RWLOCK_PREFER_READER_NP
> PTHREAD_RWLOCK_PREFER_WRITER_NP
> PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP
>
> +-- Rwlock
> +PTHREAD_RWLOCK_WRPHASE
> +PTHREAD_RWLOCK_WRLOCKED
> +PTHREAD_RWLOCK_READER_SHIFT
> +
> -- 'Shared' attribute values
> PTHREAD_PROCESS_PRIVATE
> PTHREAD_PROCESS_SHARED
> diff --git a/nptl/test-rwlock-printers.py b/nptl/test-rwlock-printers.py
> index b972fa6..bc58be3 100644
> --- a/nptl/test-rwlock-printers.py
> +++ b/nptl/test-rwlock-printers.py
> @@ -35,9 +35,9 @@ try:
>
> break_at(test_source, 'Test locking (reader)')
> continue_cmd() # Go to test_locking_reader
> - test_printer(var, to_string, {'Status': 'Unlocked'})
> + test_printer(var, to_string, {'Status': 'Not acquired'})
> next_cmd()
> - test_printer(var, to_string, {'Status': r'Locked \(Read\)', 'Readers': '1'})
> + test_printer(var, to_string, {'Status': r'Acquired \(Read\)', 'Readers': '1'})
> next_cmd()
> test_printer(var, to_string, {'Readers': '2'})
> next_cmd()
> @@ -45,10 +45,10 @@ try:
>
> break_at(test_source, 'Test locking (writer)')
> continue_cmd() # Go to test_locking_writer
> - test_printer(var, to_string, {'Status': 'Unlocked'})
> + test_printer(var, to_string, {'Status': 'Not acquired'})
> next_cmd()
> thread_id = get_current_thread_lwpid()
> - test_printer(var, to_string, {'Status': r'Locked \(Write\)',
> + test_printer(var, to_string, {'Status': r'Acquired \(Write\)',
> 'Writer ID': thread_id})
>
> continue_cmd() # Exit
> diff --git a/nptl/test-rwlockattr-printers.c b/nptl/test-rwlockattr-printers.c
> index d12facf..c79956b 100644
> --- a/nptl/test-rwlockattr-printers.c
> +++ b/nptl/test-rwlockattr-printers.c
> @@ -75,6 +75,8 @@ test_setkind_np (pthread_rwlock_t *rwlock, pthread_rwlockattr_t *attr)
>
> if (SET_KIND (attr, PTHREAD_RWLOCK_PREFER_READER_NP) == 0 /* Set kind. */
> && rwlock_reinit (rwlock, attr) == PASS
> + && SET_KIND (attr, PTHREAD_RWLOCK_PREFER_WRITER_NP) == 0
> + && rwlock_reinit (rwlock, attr) == PASS
> && SET_KIND (attr, PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP) == 0
> && rwlock_reinit (rwlock, attr) == PASS)
> result = PASS;
> diff --git a/nptl/test-rwlockattr-printers.py b/nptl/test-rwlockattr-printers.py
> index 1ca2dc6..aa4f964 100644
> --- a/nptl/test-rwlockattr-printers.py
> +++ b/nptl/test-rwlockattr-printers.py
> @@ -46,6 +46,9 @@ try:
> next_cmd(2)
> test_printer(rwlock_var, rwlock_to_string, {'Prefers': 'Writers'})
> test_printer(attr_var, attr_to_string, {'Prefers': 'Writers'})
> + next_cmd(2)
> + test_printer(rwlock_var, rwlock_to_string, {'Prefers': 'Writers no recursive readers'})
> + test_printer(attr_var, attr_to_string, {'Prefers': 'Writers no recursive readers'})
>
> break_at(test_source, 'Set shared')
> continue_cmd() # Go to test_setpshared