[2/2] New pthread rwlock that is more scalable.

On Wed, 2016-07-27 at 23:44 +0200, Torvald Riegel wrote:
> This replaces the pthread rwlock with a new implementation that uses a

> more scalable algorithm (primarily through not using a critical section

> anymore to make state changes).  The fast path for rdlock acquisition

> and release is now basically a single atomic read-modify write or CAS

> and a few branches.  See nptl/pthread_rwlock_common.c for details.


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.

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

On Tue, 2017-01-10 at 01:36 -0500, Carlos O'Donell wrote:
> Is it really necessary to switch from unlocked/locked

> to 'not acquired'/acquired?


I think it's better (eg, it works better with having an owner and having
reader/writer ownership (do you lock "as a reader"? "for reading"?...),
we say "lock acquisiton" or "mutex acquisition" instead of "lock
locking", etc.).  It also matches that we use "acquire" in our internal
documentation (though one may say that this is a biased argument, given
that I've been pushing for this).  It matches acquire and release MO.

The pretty printers are new this release, so the change shouldn't break
existing uses.  I should adapt the mutex printers too for consistency, I
guess.