| Message ID | 1484064691.5606.208.camel@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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.
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.
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.
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 :) )
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.
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: