This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Adding systemtap probe points in pthread library (slightly revised again)


Thanks a lot for the comment.

- fixed missing space before paren for the MARCOs
- removed comments (most of the comments were originally written to
explain why the probes were placed in the specific locations...)


What is it?
This is a patch against the "remotes/origin/roland/systemtap" branch. It
adds SystemTap probe points to the Pthread library to allow developers
and system administrators to trace and debug pthread programs.

ChangeLog:

2011-01-12  Rayson Ho <rho@redhat.com>
        
	* DESIGN-systemtap-probes.txt: New file. (documentation)
	* pthread_create.c: Added SystemTap Pthreads probes.
	* pthread_join.c: Likewise.
	* pthread_mutex_destroy.c: Likewise.
	* pthread_mutex_init.c: Likewise.
	* pthread_mutex_lock.c: Likewise.
	* pthread_mutex_unlock.c: Likewise.
	* pthread_rwlock_destroy.c: Likewise.
	* pthread_rwlock_rdlock.c: Likewise.
	* pthread_rwlock_unlock.c: Likewise.
	* pthread_rwlock_wrlock.c: Likewise.
	* sysdeps/unix/sysv/linux/x86_64/lowlevellock.S: Likewise.

Testing:
- no regressions with or without systemtap probes compiled into pthread
- compilation works with or without systemtap enabled

diff --git a/nptl/DESIGN-systemtap-probes.txt
b/nptl/DESIGN-systemtap-probes.txt
index e69de29..17ec433 100644
--- a/nptl/DESIGN-systemtap-probes.txt
+++ b/nptl/DESIGN-systemtap-probes.txt
@@ -0,0 +1,33 @@
+Systemtap is a dynamic tracing/instrumenting tool available on Linux.
Probes that are not fired have close to zero runtime overhead.
+
+The following probes are available for NPTL:
+
+Thread creation & Join Probes
+=============================
+create   - probe for pthread_create(3) - arg1 = thread ID, arg2 =
start_routine, arg3 = arguments
+start    - probe for actual thread creation, arg1 = struct pthread
(members include thread ID, process ID)
+join     - probe for pthread_join(3)   - arg1 = thread ID
+join_ret - probe for pthread_join(3) return - arg1 = thread ID, arg2 =
return value
+
+Lock-related Probes
+===================
+mutex_init    - probe for pthread_mutex_init(3) - arg1 = address of
mutex lock
+mutex_acquired- probe for pthread_mutex_lock(3) - arg1 = address of
mutex lock
+mutex_block   - probe for resume from _possible_ mutex block event -
arg1 = address of mutex lock
+mutex_entry   - probe for entry to the pthread_mutex_lock(3) function,
- arg1 = address of mutex lock
+mutex_release - probe for pthread_mutex_unlock(3) after the successful
release of a mutex lock - arg1 = address of mutex lock
+mutex_destroy - probe for pthread_mutex_destroy(3) - arg1 = address of
mutex lock
+rwlock_destroy- probe for pthread_rwlock_destroy(3) - arg1 = address of
rw lock
+rwlock_acquire_write-probe for pthread_rwlock_wrlock(3) - arg1 =
address of rw lock
+rwlock_unlock - probe for pthread_rwlock_unlock(3) - arg1 = address of
rw lock
+
+lock_wait         - probe in low-level lock code, only fired when futex
is called (i.e. when trying to acquire a contented lock)
+lock_wait_private - probe in low-level lock code, only fired when futex
is called (i.e. when trying to acquire a contented lock)
+
+Condition variable Probes
+=========================
+cond_init - probe for pthread_cond_init(3) - arg1 = condition, arg2 =
attr
+cond_destroy- probe for pthread_condattr_destroy(3) - arg1 = attr
+cond_wait - probe for pthread_cond_wait(3) - arg1 = condition, arg2 =
mutex lock
+cond_signal - probe for pthread_cond_signal(3) - arg1 = condition
+cond_broadcast - probe for pthread_cond_broadcast(3) - arg1 = condition
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index 4075dd9..5fdf2c9 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -556,6 +556,8 @@ __pthread_create_2_1 (newthread, attr,
start_routine, arg)
   /* Pass the descriptor to the caller.  */
   *newthread = (pthread_t) pd;
 
+  LIBC_PROBE (newthread, 2, start_routine, arg);
+
   /* Start the thread.  */
   return create_thread (pd, iattr, STACK_VARIABLES_ARGS);
 }
diff --git a/nptl/pthread_join.c b/nptl/pthread_join.c
index 6a87a8b..7cbeb1d 100644
--- a/nptl/pthread_join.c
+++ b/nptl/pthread_join.c
@@ -23,6 +23,8 @@
 #include <atomic.h>
 #include "pthreadP.h"
 
+#include <stap-probe.h>
+
 
 static void
 cleanup (void *arg)
@@ -55,6 +57,8 @@ pthread_join (threadid, thread_return)
   struct pthread *self = THREAD_SELF;
   int result = 0;
 
+  LIBC_PROBE (join,1, threadid);
+
   /* During the wait we change to asynchronous cancellation.  If we
      are canceled the thread we are waiting for must be marked as
      un-wait-ed for again.  */
@@ -110,5 +114,7 @@ pthread_join (threadid, thread_return)
       __free_tcb (pd);
     }
 
+  LIBC_PROBE (join_ret, 3, threadid, result, pd->result);
+
   return result;
 }
diff --git a/nptl/pthread_mutex_destroy.c b/nptl/pthread_mutex_destroy.c
index e2c9f8a..00037c5 100644
--- a/nptl/pthread_mutex_destroy.c
+++ b/nptl/pthread_mutex_destroy.c
@@ -20,11 +20,15 @@
 #include <errno.h>
 #include "pthreadP.h"
 
+#include <stap-probe.h>
+
 
 int
 __pthread_mutex_destroy (mutex)
      pthread_mutex_t *mutex;
 {
+  LIBC_PROBE (cond_destroy, 1, mutex);
+
   if ((mutex->__data.__kind & PTHREAD_MUTEX_ROBUST_NORMAL_NP) == 0
       && mutex->__data.__nusers != 0)
     return EBUSY;
diff --git a/nptl/pthread_mutex_init.c b/nptl/pthread_mutex_init.c
index d9b1ef0..a025a38 100644
--- a/nptl/pthread_mutex_init.c
+++ b/nptl/pthread_mutex_init.c
@@ -24,6 +24,8 @@
 #include <kernel-features.h>
 #include "pthreadP.h"
 
+#include <stap-probe.h>
+
 static const struct pthread_mutexattr default_attr =
   {
     /* Default is a normal mutex, not shared between processes.  */
@@ -135,6 +137,8 @@ __pthread_mutex_init (mutex, mutexattr)
   // mutex->__spins = 0;	already done by memset
   // mutex->__next = NULL;	already done by memset
 
+  LIBC_PROBE (mutex_init, 1, mutex);
+
   return 0;
 }
 strong_alias (__pthread_mutex_init, pthread_mutex_init)
diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
index 50dc188..fdec784 100644
--- a/nptl/pthread_mutex_lock.c
+++ b/nptl/pthread_mutex_lock.c
@@ -24,6 +24,7 @@
 #include <not-cancel.h>
 #include "pthreadP.h"
 #include <lowlevellock.h>
+#include <stap-probe.h>
 
 
 #ifndef LLL_MUTEX_LOCK
@@ -48,6 +49,9 @@ __pthread_mutex_lock (mutex)
   assert (sizeof (mutex->__size) >= sizeof (mutex->__data));
 
   unsigned int type = PTHREAD_MUTEX_TYPE (mutex);
+
+  LIBC_PROBE (mutex_entry, 1, mutex);
+
   if (__builtin_expect (type & ~PTHREAD_MUTEX_KIND_MASK_NP, 0))
     return __pthread_mutex_lock_full (mutex);
 
@@ -60,6 +64,8 @@ __pthread_mutex_lock (mutex)
       /* Normal mutex.  */
       LLL_MUTEX_LOCK (mutex);
       assert (mutex->__data.__owner == 0);
+
+      LIBC_PROBE (mutex_block, 1, mutex);
     }
   else if (__builtin_expect (type == PTHREAD_MUTEX_RECURSIVE_NP, 1))
     {
@@ -83,6 +89,8 @@ __pthread_mutex_lock (mutex)
 
       assert (mutex->__data.__owner == 0);
       mutex->__data.__count = 1;
+
+      LIBC_PROBE (mutex_block, 1, mutex);
     }
   else if (__builtin_expect (type == PTHREAD_MUTEX_ADAPTIVE_NP, 1))
     {
@@ -108,6 +116,8 @@ __pthread_mutex_lock (mutex)
 	    }
 	  while (LLL_MUTEX_TRYLOCK (mutex) != 0);
 
+          LIBC_PROBE (mutex_block, 1, mutex);
+
 	  mutex->__data.__spins += (cnt - mutex->__data.__spins) / 8;
 	}
       assert (mutex->__data.__owner == 0);
@@ -127,6 +137,8 @@ __pthread_mutex_lock (mutex)
   ++mutex->__data.__nusers;
 #endif
 
+  LIBC_PROBE (mutex_acquire, 1, mutex);
+
   return 0;
 }
 
@@ -451,6 +463,8 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
 	  }
 	while ((oldval & PTHREAD_MUTEX_PRIO_CEILING_MASK) != ceilval);
 
+        LIBC_PROBE (mutex_block, 1, mutex);
+
 	assert (mutex->__data.__owner == 0);
 	mutex->__data.__count = 1;
       }
@@ -467,6 +481,8 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
   ++mutex->__data.__nusers;
 #endif
 
+  LIBC_PROBE (mutex_acquire, 1, mutex);
+
   return 0;
 }
 #ifndef __pthread_mutex_lock
diff --git a/nptl/pthread_mutex_unlock.c b/nptl/pthread_mutex_unlock.c
index f9fe10b..479e500 100644
--- a/nptl/pthread_mutex_unlock.c
+++ b/nptl/pthread_mutex_unlock.c
@@ -22,6 +22,7 @@
 #include <stdlib.h>
 #include "pthreadP.h"
 #include <lowlevellock.h>
+#include <stap-probe.h>
 
 static int
 internal_function
@@ -50,6 +51,9 @@ __pthread_mutex_unlock_usercnt (mutex, decr)
 
       /* Unlock.  */
       lll_unlock (mutex->__data.__lock, PTHREAD_MUTEX_PSHARED (mutex));
+
+      LIBC_PROBE (mutex_release, 1, mutex);
+
       return 0;
     }
   else if (__builtin_expect (type == PTHREAD_MUTEX_RECURSIVE_NP, 1))
@@ -272,6 +276,9 @@ __pthread_mutex_unlock_full (pthread_mutex_t *mutex,
int decr)
 			PTHREAD_MUTEX_PSHARED (mutex));
 
       int oldprio = newval >> PTHREAD_MUTEX_PRIO_CEILING_SHIFT;
+
+      LIBC_PROBE (mutex_release, 1, mutex);
+
       return __pthread_tpp_change_priority (oldprio, -1);
 
     default:
@@ -279,6 +286,7 @@ __pthread_mutex_unlock_full (pthread_mutex_t *mutex,
int decr)
       return EINVAL;
     }
 
+  LIBC_PROBE (mutex_release, 1, mutex);
   return 0;
 }
 
diff --git a/nptl/pthread_rwlock_destroy.c
b/nptl/pthread_rwlock_destroy.c
index 28fd24b..84aa693 100644
--- a/nptl/pthread_rwlock_destroy.c
+++ b/nptl/pthread_rwlock_destroy.c
@@ -18,12 +18,15 @@
    02111-1307 USA.  */
 
 #include "pthreadP.h"
+#include <stap-probe.h>
 
 
 int
 __pthread_rwlock_destroy (rwlock)
      pthread_rwlock_t *rwlock;
 {
+  LIBC_PROBE (rwlock_destroy, 1, rwlock);
+
   /* Nothing to be done.  For now.  */
   return 0;
 }
diff --git a/nptl/pthread_rwlock_rdlock.c b/nptl/pthread_rwlock_rdlock.c
index 31eb508..610f223 100644
--- a/nptl/pthread_rwlock_rdlock.c
+++ b/nptl/pthread_rwlock_rdlock.c
@@ -22,6 +22,7 @@
 #include <lowlevellock.h>
 #include <pthread.h>
 #include <pthreadP.h>
+#include <stap-probe.h>
 
 
 /* Acquire read lock for RWLOCK.  */
@@ -31,6 +32,8 @@ __pthread_rwlock_rdlock (rwlock)
 {
   int result = 0;
 
+  LIBC_PROBE (rlock_entry, 1, rwlock);
+
   /* Make sure we are along.  */
   lll_lock (rwlock->__data.__lock, rwlock->__data.__shared);
 
@@ -49,6 +52,8 @@ __pthread_rwlock_rdlock (rwlock)
 	      --rwlock->__data.__nr_readers;
 	      result = EAGAIN;
 	    }
+          else
+            LIBC_PROBE (rwlock_acquire_read, 1, rwlock);
 
 	  break;
 	}
diff --git a/nptl/pthread_rwlock_unlock.c b/nptl/pthread_rwlock_unlock.c
index a7ef71a..a6e8d87 100644
--- a/nptl/pthread_rwlock_unlock.c
+++ b/nptl/pthread_rwlock_unlock.c
@@ -22,11 +22,14 @@
 #include <lowlevellock.h>
 #include <pthread.h>
 #include <pthreadP.h>
+#include <stap-probe.h>
 
 /* Unlock RWLOCK.  */
 int
 __pthread_rwlock_unlock (pthread_rwlock_t *rwlock)
 {
+  LIBC_PROBE (rwlock_unlock, 1, rwlock);
+
   lll_lock (rwlock->__data.__lock, rwlock->__data.__shared);
   if (rwlock->__data.__writer)
     rwlock->__data.__writer = 0;
diff --git a/nptl/pthread_rwlock_wrlock.c b/nptl/pthread_rwlock_wrlock.c
index 64fe970..748062e 100644
--- a/nptl/pthread_rwlock_wrlock.c
+++ b/nptl/pthread_rwlock_wrlock.c
@@ -22,6 +22,7 @@
 #include <lowlevellock.h>
 #include <pthread.h>
 #include <pthreadP.h>
+#include <stap-probe.h>
 
 
 /* Acquire write lock for RWLOCK.  */
@@ -31,6 +32,8 @@ __pthread_rwlock_wrlock (rwlock)
 {
   int result = 0;
 
+  LIBC_PROBE (wlock_entry, 1, rwlock);
+
   /* Make sure we are along.  */
   lll_lock (rwlock->__data.__lock, rwlock->__data.__shared);
 
@@ -41,6 +44,9 @@ __pthread_rwlock_wrlock (rwlock)
 	{
 	  /* Mark self as writer.  */
 	  rwlock->__data.__writer = THREAD_GETMEM (THREAD_SELF, tid);
+
+          LIBC_PROBE (rwlock_acquire_write, 1, rwlock);
+
 	  break;
 	}
 
diff --git a/nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S
b/nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S
index 3195db2..f0664f9 100644
--- a/nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S
+++ b/nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S
@@ -22,6 +22,15 @@
 #include <kernel-features.h>
 #include <lowlevellock.h>
 
+#if defined USE_STAP_PROBE && (defined IN_LIB || !defined NOT_IN_libc)
+ #include <stap-probe.h>
+ #define PTHREAD_PROBE_LL_LOCKWAIT_PRIVATE(arg1)
LIBC_PROBE_ASM(lll_lock_wait_private, arg1)
+ #define PTHREAD_PROBE_LL_LOCKWAIT(arg1)
LIBC_PROBE_ASM(lll_lock_wait, arg1)
+#else
+ #define PTHREAD_PROBE_LL_LOCKWAIT_PRIVATE(arg1)
+ #define PTHREAD_PROBE_LL_LOCKWAIT(arg1)
+#endif
+
 	.text
 
 #ifdef __ASSUME_PRIVATE_FUTEX
@@ -91,7 +100,8 @@ __lll_lock_wait_private:
 	cmpl	%edx, %eax	/* NB:	 %edx == 2 */
 	jne	2f
 
-1:	movl	$SYS_futex, %eax
+1:	PTHREAD_PROBE_LL_LOCKWAIT_PRIVATE (%rdi)
+	movl	$SYS_futex, %eax
 	syscall
 
 2:	movl	%edx, %eax
@@ -130,7 +140,8 @@ __lll_lock_wait:
 	cmpl	%edx, %eax	/* NB:	 %edx == 2 */
 	jne	2f
 
-1:	movl	$SYS_futex, %eax
+1:	PTHREAD_PROBE_LL_LOCKWAIT (%rdi)
+	movl	$SYS_futex, %eax
 	syscall
 
 2:	movl	%edx, %eax

Rayson





On Wed, 2011-01-12 at 10:18 -0800, Roland McGrath wrote:
> > +  LIBC_PROBE(join,1, threadid);
> 
> Missing space before paren, missing space after comma.
> Half the additions have similar whitespace errors.
> Please just read over ALL YOUR CODE before you submit.
> 
> > +          else
> > +            {
> > +	      /* systemtap pthread probe - this is the only place where
> > +	         we get this read-write lock */
> > +              LIBC_PROBE (rwlock_acquire_read, 1, rwlock);
> > +            }
> 
> Don't add braces around a single statement.  This comment is not formatted
> properly.  Look at the existing code and see how we write comments, please.
> This comment itself doesn't seem to say anything useful anyway.
> 
> > +1:	PTHREAD_PROBE_LL_LOCKWAIT_PRIVATE(%rdi)
> 
> Again, missing space before paren.
> 
> This stuff is not rocket science, and this is the nth time in a review of
> the same code I've told you about the same errors.  Just look at the
> existing code and match the formatting conventions you see there.
> 
> 
> Thanks,
> Roland



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]