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

[Bert Wesarg]

On Wed, Jan 12, 2011 at 19:50, Rayson Ho <rho@redhat.com> wrote:
> 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);

I'm not familiar with this macro, but I suspect that the first
argument will be the name of the probe. In this case 'newthread'. Is
this intended? There is no 'newthread' probe documented in
nptl/DESIGN-systemtap-probes.txt but a 'create' probe. Also, there is
a parameter named 'newthread'. I must also missed the 'start' probe.

> +
>   /* 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);
> +

Again, there is no 'rlock_entry' probe documented, and should it
probably called 'rwlock_entry'?

>   /* 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);

This one is also not documented, isn't it?

>
>          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);
> +

Also not documented, and name should be 'rwlock_entry_write' or something.

>   /* 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)

If 'lll_lock_wait_private' will be the name of the probe, than the
documentation should be updated.

> + #define PTHREAD_PROBE_LL_LOCKWAIT(arg1)
> LIBC_PROBE_ASM(lll_lock_wait, arg1)

Likewise.

> +#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

Bert

>
> Rayson