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: revamp sdt.h


> I've ported my pthread probe changes against your systemtap glibc tree.
> I think the interface of the new sdt.h and that of the LIBC_PROBE()
> marco is almost the identical, but I am not sure if we are going to have
> macros like PROBE2() in the new-sdt -- not an issue at all as it is just
> a handy macro.

Aside from asm support, the sdt.h macro interface will not change.

> I've inline-attached the diff, and the biggest change is that
> "pthread_probe.h" used with the old sdt.h is now gone.

Good.  You are almost there on cosmetic issues for glibc submissions.
Put a space before a paren, as you see in the rest of the code.
Use full sentences with proper punctuation in comments, and follow
the comment indentation style you see in the rest of the code.

> I have not attached the diffs for the probes in the inline assembly code
> yet, I will do that soon.

Ok.  I have only skimmed these patches as to the semantics of the probes.
So I only have a few comments, but the choices of other probe locations and
parameters still needs a more thorough review.

> @@ -55,6 +57,8 @@ pthread_join (threadid, thread_return)
>    struct pthread *self = THREAD_SELF;
>    int result = 0;
>  
> +  LIBC_PROBE(pthread_join, 1, threadid);

A vanilla probe for non-bogus entry to pthread_join is probably fine.  But
perhaps it's more useful to have (either only, or a separate additional)
probe when pthread_join actually blocks (or close to, i.e. lll_wait_tid
call).

> +  LIBC_PROBE(pthread_join_ret, 2, threadid, result);  

The error code is useful to give here, sure.  But I think pd->result
is the most obviously interesting and important value, so don't miss that.
That might mean different probe names for success/error cases, or perhaps
just a dummy pd->result value of NULL for the error case.

> @@ -32,6 +33,8 @@ __pthread_mutex_destroy (mutex)
>    /* Set to an invalid value.  */
>    mutex->__data.__kind = -1;
>  
> +  LIBC_PROBE(pthread_mutex_destroy, 1, mutex);

For any probe that is named simply with the function name, it should be
before it does its work, not after.  Here, the probe fires after the mutex
has been destroyed, so it's no longer possible to inspect its (former) state.

> @@ -47,6 +49,8 @@ __pthread_mutex_init (mutex, mutexattr)
>  
>    imutexattr = (const struct pthread_mutexattr *) mutexattr ?:
> &default_attr;
>  
> +  LIBC_PROBE(pthread_mutex_init, 2, mutex, mutexattr);

Here might be exception where the opposite makes more sense.  For a *_init
function, the mutex is known to be garbage beforehand, so there is nothing
there to inspect.  OTOH, if tracing a bug wherein it gets reinitialized
wrongly, perhaps you want to see it first.

> +  /* systemtap marker */
> +  LIBC_PROBE(pthread_mutex_lock, 1, mutex);

Omit useless comments.

> @@ -94,6 +108,7 @@ __pthread_mutex_lock (mutex)
>  	  int cnt = 0;
>  	  int max_cnt = MIN (MAX_ADAPTIVE_COUNT,
>  			     mutex->__data.__spins * 2 + 10);
> +
>  	  do
>  	    {
>  	      if (cnt++ >= max_cnt)

Do not make any changes even to whitespace outside your additions.


Thanks,
Roland


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