[PATCH] Fix mutex pretty printer test and pretty printer output.

[PATCH] Fix mutex pretty printer test and pretty printer output.

[Torvald Riegel]

[-- Attachment #1: Type: text/plain, Size: 363 bytes --]

This fixes the test for the mutex pretty printers in that it does not
assume that the owner field is set even though the mutex is acquired;
this is not guaranteed under the current lock elision implementation.
Also, it improves the wording of some of the pretty printer output (eg,
changing 'Locked' to 'Acquired').

Tested on x86_64 with --enable-lock-elision. 

[-- Attachment #2: mutex-pp-fix.patch --]
[-- Type: text/x-patch, Size: 6201 bytes --]

commit 661a8a8d766747367314f848733804f22cef825e
Author: Torvald Riegel <triegel@redhat.com>
Date:   Mon Jan 9 20:40:57 2017 +0100

    Fix mutex pretty printer test and pretty printer output.
    
    This fixes the test for the mutex pretty printers in that it does not
    assume that the owner field is set even though the mutex is acquired;
    this is not guaranteed under the current lock elision implementation.
    Also, it improves the wording of some of the pretty printer output (eg,
    changing 'Locked' to 'Acquired').
    
    	* nptl/nptl-printers.py (MutexPrinter): Change output.
    	* nptl/test-mutex-printers.py: Fix test and adapt to changed output.

diff --git a/nptl/nptl-printers.py b/nptl/nptl-printers.py
index 9d67865..54d4c84 100644
--- a/nptl/nptl-printers.py
+++ b/nptl/nptl-printers.py
@@ -124,18 +124,21 @@ class MutexPrinter(object):
         """
 
         if self.lock == PTHREAD_MUTEX_UNLOCKED:
-            self.values.append(('Status', 'Unlocked'))
+            self.values.append(('Status', 'Not acquired'))
         else:
             if self.lock & FUTEX_WAITERS:
-                self.values.append(('Status', 'Locked, possibly with waiters'))
+                self.values.append(('Status',
+                                    'Acquired, possibly with waiters'))
             else:
                 self.values.append(('Status',
-                                    'Locked, possibly with no waiters'))
+                                    'Acquired, possibly with no waiters'))
 
             if self.lock & FUTEX_OWNER_DIED:
-                self.values.append(('Owner ID', '%d (dead)' % self.owner))
+                self.values.append(('Owner ID (if known)',
+                                    '%d (dead)' % self.owner))
             else:
-                self.values.append(('Owner ID', self.lock & FUTEX_TID_MASK))
+                self.values.append(('Owner ID (if known)',
+                                    self.lock & FUTEX_TID_MASK))
 
         if self.owner == PTHREAD_MUTEX_INCONSISTENT:
             self.values.append(('State protected by this mutex',
@@ -157,7 +160,7 @@ class MutexPrinter(object):
             lock_value &= ~(PTHREAD_MUTEX_PRIO_CEILING_MASK)
 
         if lock_value == PTHREAD_MUTEX_UNLOCKED:
-            self.values.append(('Status', 'Unlocked'))
+            self.values.append(('Status', 'Not acquired'))
         else:
             if self.kind & PTHREAD_MUTEX_PRIO_INHERIT_NP:
                 waiters = self.lock & FUTEX_WAITERS
@@ -168,12 +171,13 @@ class MutexPrinter(object):
                 owner = self.owner
 
             if waiters:
-                self.values.append(('Status', 'Locked, possibly with waiters'))
+                self.values.append(('Status',
+                                    'Acquired, possibly with waiters'))
             else:
                 self.values.append(('Status',
-                                    'Locked, possibly with no waiters'))
+                                    'Acquired, possibly with no waiters'))
 
-            self.values.append(('Owner ID', owner))
+            self.values.append(('Owner ID (if known)', owner))
 
     def read_attributes(self):
         """Read the mutex's attributes."""
@@ -215,7 +219,7 @@ class MutexPrinter(object):
         mutex_type = self.kind & PTHREAD_MUTEX_KIND_MASK
 
         if mutex_type == PTHREAD_MUTEX_RECURSIVE and self.count > 1:
-            self.values.append(('Times locked recursively', self.count))
+            self.values.append(('Times acquired by the owner', self.count))
 
 class MutexAttributesPrinter(object):
     """Pretty printer for pthread_mutexattr_t.
diff --git a/nptl/test-mutex-printers.py b/nptl/test-mutex-printers.py
index 23f16b0..d0600b7 100644
--- a/nptl/test-mutex-printers.py
+++ b/nptl/test-mutex-printers.py
@@ -39,15 +39,17 @@ try:
 
     break_at(test_source, 'Test status (non-robust)')
     continue_cmd() # Go to test_status_no_robust
-    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': 'Locked, possibly with no waiters',
-                                  'Owner ID': thread_id})
+    # Don't check Owner ID here because the owner may not always be set
+    # (e.g., if using lock elision).
+    test_printer(var, to_string,
+                 {'Status': 'Acquired, possibly with no waiters'})
 
     break_at(test_source, 'Test status (robust)')
     continue_cmd() # Go to test_status_robust
-    test_printer(var, to_string, {'Status': 'Unlocked'})
+    test_printer(var, to_string, {'Status': 'Not acquired'})
 
     # We'll now test the robust mutex locking states.  We'll create a new
     # thread that will lock a robust mutex and exit without unlocking it.
@@ -69,21 +71,22 @@ try:
     # The new thread should be dead by now.
     break_at(test_source, 'Test locking (robust)')
     continue_cmd()
-    test_printer(var, to_string, {'Owner ID': r'{0} \(dead\)'.format(child_id)})
+    test_printer(var, to_string,
+                 {'Owner ID \(if known\)': r'{0} \(dead\)'.format(child_id)})
     # Try to lock and unlock the mutex.
     next_cmd()
-    test_printer(var, to_string, {'Owner ID': thread_id,
+    test_printer(var, to_string, {'Owner ID \(if known\)': thread_id,
                            'State protected by this mutex': 'Inconsistent'})
     next_cmd()
-    test_printer(var, to_string, {'Status': 'Unlocked',
+    test_printer(var, to_string, {'Status': 'Not acquired',
                         'State protected by this mutex': 'Not recoverable'})
     set_scheduler_locking(False)
 
     break_at(test_source, 'Test recursive locks')
     continue_cmd() # Go to test_recursive_locks
-    test_printer(var, to_string, {'Times locked recursively': '2'})
+    test_printer(var, to_string, {'Times acquired by the owner': '2'})
     next_cmd()
-    test_printer(var, to_string, {'Times locked recursively': '3'})
+    test_printer(var, to_string, {'Times acquired by the owner': '3'})
     continue_cmd() # Exit
 
 except (NoLineError, pexpect.TIMEOUT) as exception:

Re: [PATCH] Fix mutex pretty printer test and pretty printer output.

[Martin Galvan]

Thanks a lot for doing this; I was about to start working on something similar.

If I might make a suggestion: "Owner ID (if known)" doesn't really
help the user understand what's happening when the mutex is acquired
but the owner isn't recorded. I'd say we should at least document this
case somewhere (perhaps in the README?). I can write that myself if
you want to.

Re: [PATCH] Fix mutex pretty printer test and pretty printer output.

[Torvald Riegel]

On Tue, 2017-01-10 at 13:38 -0300, Martin Galvan wrote:
> Thanks a lot for doing this; I was about to start working on something similar.
> 
> If I might make a suggestion: "Owner ID (if known)" doesn't really
> help the user understand what's happening when the mutex is acquired
> but the owner isn't recorded.

If it's zero, it may not be known.  Understanding why would require
explaining details of the mutex implementation.
Do you have a concrete suggestion for alternative wording, or do you
think that "(if known)" is okay?

> I'd say we should at least document this
> case somewhere (perhaps in the README?). I can write that myself if
> you want to.

Maybe.  If you want to go ahead and explain what the pretty printers
reveal, and how they should not be misunderstood (which is an important
part of this -- users should be aware that they get simplified
information), then please propose a patch.

It might also be worth to clearly state the design goals for the pretty
printers, which in my opinion is roughly:
* Provide simplified information about the state of synchronization
constructs to users.
* There is no guarantee that this information is complete and covers
everything a particular thread might do (e.g., the pretty printers do
not show if a thread is spin-waiting in an attempt to acquire a mutex).
* It is not aimed at understanding the details of the implementation of
the synchronization construct.

Re: [PATCH] Fix mutex pretty printer test and pretty printer output.

[Martin Galvan]

2017-01-11 7:32 GMT-03:00 Torvald Riegel <triegel@redhat.com>:
> Do you have a concrete suggestion for alternative wording, or do you
> think that "(if known)" is okay?

Perhaps there could be a check for __owner being zero, and in that
case print "unknown" instead of "0". That way we could drop the "(if
known)". Maybe we could check whether __elision  exists and inform the
user that lock elision is enabled, so that they have a better idea of
what's going on. What do you think?

> Maybe.  If you want to go ahead and explain what the pretty printers
> reveal, and how they should not be misunderstood (which is an important
> part of this -- users should be aware that they get simplified
> information), then please propose a patch.

There wouldn't be a need for further explanation if we just said that
the owner is unknown to us when __owner is zero.

> It might also be worth to clearly state the design goals for the pretty
> printers, which in my opinion is roughly:
> * Provide simplified information about the state of synchronization
> constructs to users.
> * There is no guarantee that this information is complete and covers
> everything a particular thread might do (e.g., the pretty printers do
> not show if a thread is spin-waiting in an attempt to acquire a mutex).
> * It is not aimed at understanding the details of the implementation of
> the synchronization construct.

I thought that was implicitly clear. Printers exist precisely to
abstract away the implementation details of the sync constructs, and
yes, some details might be left off. If the user wants to know what's
going on under the hood, they can just disable the printers and try to
make sense of what gdb shows.

Re: [PATCH] Fix mutex pretty printer test and pretty printer output.

[Martin Galvan]

2017-01-11 9:45 GMT-03:00 Martin Galvan <omgalvan.86@gmail.com>:
> Maybe we could check whether __elision  exists and inform the
> user that lock elision is enabled

My mistake, I thought __elision only existed when lock elision was
enabled, but I guess I was wrong.

Re: [PATCH] Fix mutex pretty printer test and pretty printer output.

[Torvald Riegel]

On Wed, 2017-01-11 at 09:45 -0300, Martin Galvan wrote:
> 2017-01-11 7:32 GMT-03:00 Torvald Riegel <triegel@redhat.com>:
> > Do you have a concrete suggestion for alternative wording, or do you
> > think that "(if known)" is okay?
> 
> Perhaps there could be a check for __owner being zero, and in that
> case print "unknown" instead of "0".

Works for me, though I guess it should take the current state into
account too (ie, if it's not acquired, it shouldn't print unknown).

> That way we could drop the "(if
> known)". Maybe we could check whether __elision  exists and inform the
> user that lock elision is enabled, so that they have a better idea of
> what's going on. What do you think?

I'd like to have as little dependency on implementation internals as
possible.  One reason is that the internals might change, and although
we don't promise stability of the pretty printer output, every change
there causes at least some cost on the users' side.

Also, for example, if there is a performance advantage for not setting
the owner field on mutex types where this is possible, I would stop
setting it.

> > It might also be worth to clearly state the design goals for the pretty
> > printers, which in my opinion is roughly:
> > * Provide simplified information about the state of synchronization
> > constructs to users.
> > * There is no guarantee that this information is complete and covers
> > everything a particular thread might do (e.g., the pretty printers do
> > not show if a thread is spin-waiting in an attempt to acquire a mutex).
> > * It is not aimed at understanding the details of the implementation of
> > the synchronization construct.
> 
> I thought that was implicitly clear. Printers exist precisely to
> abstract away the implementation details of the sync constructs, and
> yes, some details might be left off. If the user wants to know what's
> going on under the hood, they can just disable the printers and try to
> make sense of what gdb shows.

I'm not quite sure it's implicitly clear.  I suppose many people will
try to interpret it and use it to understand why there program doesn't
work.  I don't want to get bug reports caused by people reading more
into the pretty printers' output than what we promise; that's why I
suggested to make this explicit.

Re: [PATCH] Fix mutex pretty printer test and pretty printer output.

[Martin Galvan]

2017-01-11 11:01 GMT-03:00 Torvald Riegel <triegel@redhat.com>:
> Works for me, though I guess it should take the current state into
> account too (ie, if it's not acquired, it shouldn't print unknown).

Right. Will send a patch during this week.

> I'd like to have as little dependency on implementation internals as
> possible.  One reason is that the internals might change, and although
> we don't promise stability of the pretty printer output, every change
> there causes at least some cost on the users' side.

Makes sense.

>> > It might also be worth to clearly state the design goals for the pretty
>> > printers, which in my opinion is roughly:
>> > * Provide simplified information about the state of synchronization
>> > constructs to users.
>> > * There is no guarantee that this information is complete and covers
>> > everything a particular thread might do (e.g., the pretty printers do
>> > not show if a thread is spin-waiting in an attempt to acquire a mutex).
>> > * It is not aimed at understanding the details of the implementation of
>> > the synchronization construct.
>>
>> I thought that was implicitly clear.
>
> I'm not quite sure it's implicitly clear.  I suppose many people will
> try to interpret it and use it to understand why there program doesn't
> work.  I don't want to get bug reports caused by people reading more
> into the pretty printers' output than what we promise; that's why I
> suggested to make this explicit.

Ok. Will make it clear on the README, probably using the lock printers
as just an example (since hopefully in the future we'll have printers
for other modules as well :) )

Re: [PATCH] Fix mutex pretty printer test and pretty printer output.

[Torvald Riegel]

On Wed, 2017-01-11 at 11:07 -0300, Martin Galvan wrote:
> 2017-01-11 11:01 GMT-03:00 Torvald Riegel <triegel@redhat.com>:
> > Works for me, though I guess it should take the current state into
> > account too (ie, if it's not acquired, it shouldn't print unknown).
> 
> Right. Will send a patch during this week.

Thanks.  It might be easiest if you just take my patch and improve it
with what we discussed so far.

> > I'd like to have as little dependency on implementation internals as
> > possible.  One reason is that the internals might change, and although
> > we don't promise stability of the pretty printer output, every change
> > there causes at least some cost on the users' side.
> 
> Makes sense.
> 
> >> > It might also be worth to clearly state the design goals for the pretty
> >> > printers, which in my opinion is roughly:
> >> > * Provide simplified information about the state of synchronization
> >> > constructs to users.
> >> > * There is no guarantee that this information is complete and covers
> >> > everything a particular thread might do (e.g., the pretty printers do
> >> > not show if a thread is spin-waiting in an attempt to acquire a mutex).
> >> > * It is not aimed at understanding the details of the implementation of
> >> > the synchronization construct.
> >>
> >> I thought that was implicitly clear.
> >
> > I'm not quite sure it's implicitly clear.  I suppose many people will
> > try to interpret it and use it to understand why there program doesn't
> > work.  I don't want to get bug reports caused by people reading more
> > into the pretty printers' output than what we promise; that's why I
> > suggested to make this explicit.
> 
> Ok. Will make it clear on the README, probably using the lock printers
> as just an example (since hopefully in the future we'll have printers
> for other modules as well :) )

I don't know whether other modules may make more promises (in
particular, mostly sequential code, where the state is described fully
by what a single thread can observe).  I was thinking only about pretty
printers for concurrent stuff when I wrote the goals above.