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

[Bert Wesarg]

On Wed, Jan 19, 2011 at 18:53, Rayson Ho <rho@redhat.com> wrote:
> On Wed, 2011-01-12 at 21:07 +0100, Bert Wesarg wrote:
>> 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.

And I still miss the 'pthread_start' probe. Do I missed it?

>
> Thanks Bert and Roland for the comments. I've fixed the documentation,
> and I've added new code that uses the ASM probe. Please review my patch
> again:
>

> diff --git a/nptl/DESIGN-systemtap-probes.txt
> b/nptl/DESIGN-systemtap-probes.txt
> index e69de29..0425824 100644
> --- a/nptl/DESIGN-systemtap-probes.txt
> +++ b/nptl/DESIGN-systemtap-probes.txt
> @@ -0,0 +1,40 @@
>
> +Systemtap is a dynamic tracing/instrumenting tool available on Linux.
> Probes that are not fired at run time have close to zero performance
> overhead.
> +
> +The following probes are available for NPTL:
> +
> +Thread creation & Join Probes
> +=============================
> +pthread_create - probe for pthread_create(3) - arg1 = start_routine,
> arg2 = arguments
> +pthread_start - probe for actual thread creation, arg1 = struct pthread
> (members include thread ID, process ID)
> +pthread_join - probe for pthread_join(3) - arg1 = thread ID
> +pthread_join_ret - probe for pthread_join(3) return - arg1 = thread ID,
> arg2 = return value

Why does these probes have the 'pthread_' prefix, but all pthread
related probes below not? Also, 'join_ret' is the only one with an
abbreviated verb. Maybe 'join_return' or simply 'joined'?

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

Could we keep the naming consistent, its 'acquired' but not
'released'. Its' 'entry' (a noun) but all the others aren't nouns. i
would also suggest to keep the name close to the function name.
Something like:

mutex_init
mutex_lock_locked
mutex_lock_blocking
mutex_lock_enter
mutex_unlock

> +
> +wrlock_entry - probe for entry to the pthread_rwlock_wrlock(3) function
> - arg1 = address of rw lock
> +rdlock_entry - probe for entry to the pthread_rwlock_rdlock(3) function
> - arg1 = address of rw 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
> +rdlock_acquire_read - probe for pthread_rwlock_rdlock(3) after
> successfully getting the lock - - arg1 = address of rw lock
> +rwlock_unlock - probe for pthread_rwlock_unlock(3) - arg1 = address of
> rw lock
> +
> +lll_futex_wait         - probe in low-level (assembly language) locking
> code, only fired when futex(2) is called (i.e. when trying to acquire a
> contented lock)
> +lll_futex_wait_private - probe in low-level (assembly language) locking
> code, only fired when futex(2) is called (i.e. when trying to acquire a
> contented lock)
> +lll_futex_wake         - probe in low-level (assembly language) locking
> code, only fired when futex(2) is called (FUTEX_WAIT)
> +
> +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_timedwait - probe for pthread_cond_timedwait(3) - arg1 =
> condition, arg2 = mutex lock, arg3 = timespec
> +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_cond_timedwait.c
> b/nptl/pthread_cond_timedwait.c
> index 7278ec4..4e4e33d 100644
> --- a/nptl/pthread_cond_timedwait.c
> +++ b/nptl/pthread_cond_timedwait.c
> @@ -65,7 +65,7 @@ __pthread_cond_timedwait (cond, mutex, abstime)
>   int pshared = (cond->__data.__mutex == (void *) ~0l)
>                ? LLL_SHARED : LLL_PRIVATE;
>
> -  /* Make sure we are along.  */
> +  /* Make sure we are alone.  */
>   lll_lock (cond->__data.__lock, pshared);
>
>   /* Now we can release the mutex.  */

I would have expected the 'cond_timedwait' probe here.

> diff --git a/nptl/pthread_condattr_destroy.c
> b/nptl/pthread_condattr_destroy.c
> index e6d069e..9c84278 100644
> --- a/nptl/pthread_condattr_destroy.c
> +++ b/nptl/pthread_condattr_destroy.c
> @@ -25,6 +25,8 @@ __pthread_condattr_destroy (attr)
>      pthread_condattr_t *attr;
>  {
>   /* Nothing to be done.  */
> +
> +  LIBC_PROBE (cond_destroy, 1, attr);

Judging from the file and the function name in the hunk header, I
think this probe does not belong here.

>   return 0;
>  }
>  strong_alias (__pthread_condattr_destroy, pthread_condattr_destroy)

> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index 4075dd9..d574cc5 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 (pthread_create, 2, start_routine, arg);
> +
>   /* Start the thread.  */
>   return create_thread (pd, iattr, STACK_VARIABLES_ARGS);
>  }

Bert