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

[Carlos O'Donell <carlos at redhat dot 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