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

[Bert Wesarg <bert dot wesarg at googlemail dot com>]

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