[PATCH 1/2] tls: Add debug logging for TLS and TCB management

[DJ Delorie]

Overall OK if you can reasonably discount my last two comments ;-)

Frédéric Bérat <fberat@redhat.com> writes:
> +  else if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_TLS))
> +    _dl_debug_printf ("TCB allocated: 0x%lx\n", (unsigned long int) result);

Ideally this should be %p for pointers and the cast wouldn't be needed.
However, _dl_debug_printf doesn't support that :-(

> diff --git a/nptl/nptl-stack.c b/nptl/nptl-stack.c
> index c049c5133c..c8f2b3505f 100644
> --- a/nptl/nptl-stack.c
> +++ b/nptl/nptl-stack.c
> @@ -77,6 +77,10 @@ __nptl_free_stacks (size_t limit)
>  
>  	  /* Free the memory associated with the ELF TLS.  */
>  	  _dl_deallocate_tls (TLS_TPADJ (curr), false);
> +	  if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_TLS))
> +	    GLRO (dl_debug_printf) (
> +		"TCB cache full, deallocating: TID=%ld, TCB=0x%lx\n",
> +		(long int) curr->tid, (unsigned long int) curr);

In general, when deallocating or destroying things, you should put the
prints before the calls in case the information you want to print is
lost during the deallocation/destruction.  In *this* case it's safe, but
it might not have been.

> @@ -99,6 +103,9 @@ queue_stack (struct pthread *stack)
>    /* We unconditionally add the stack to the list.  The memory may
>       still be in use but it will not be reused until the kernel marks
>       the stack as not used anymore.  */
> +  if (__glibc_unlikely (GLRO (dl_debug_mask) & DL_DEBUG_TLS))
> +    GLRO (dl_debug_printf) ("TCB deallocated into cache: TID=%ld, TCB=0x%lx\n",
> +			    (long int) stack->tid, (unsigned long int) stack);

The text says TCB= but the value is the tcb's stack?  It's actually
right, but the variable name is misleading...

> diff --git a/nptl/tst-dl-debug-tid.c b/nptl/tst-dl-debug-tid.c
> +  /* Give the detached thread time to finish.  */
> +  sleep (1);

This is known to be unreliable on heavily loaded systems, and wastes a
whole second on lightly loaded ones.  Is there any other way to
busy-wait or syscall to detect the non-existence of that thread?

> diff --git a/nptl/tst-dl-debug-tid.sh b/nptl/tst-dl-debug-tid.sh
> +# Arguments are from Makefile.
> +common_objpfx=$1
> +test_wrapper=$2
> +rtld_prefix=$3
> +test_wrapper_env=$4
> +run_program_env=$5
> +test_program=$6

These should be quoted, as well as their uses below.  Most are safe but
in case the user builds in a directory with spaces :-P