[PATCH] nptl: Add pthread_thread_number_np function

[Florian Weimer]

On 12/15/2017 05:08 AM, Carlos O'Donell wrote:
> On 12/14/2017 10:56 AM, Florian Weimer wrote:
>> The implementation is actually in libc.so.  With a full implementation
>> of pthread_self in libc.so, pthread_thread_number_np is completely
>> usable without libpthread.
> 
> High level:
> 
> The addition of a new API to libc.so needs a strong justification.
> 
> Do you have a use in mind for this API?
> 
> When would users use this number?
> 
> Why would they use this number?

Logging, and discriminating between threads, for example in random bit 
generators.

> Do any of the uses you envision overlap with the longstanding
> requests for gettid()?

Barely, because gettid does not return a unique number over time, and 
the number returned from pthread_thread_number_np cannot be used with 
existing interfaces.

> Design level:
> 
> We make claims that the thread number is never reused, but then the
> implementation wraps the uint64_t global counter, and reuses the
> number. If we really don't allow reuse, then we limit the implementation
> to only ever starting 2^64-1 threads, similar to the discussions around
> dlopen and void* cookies, I don't know if this kind of limit is a good
> idea or not. My instinct tells me we should not limit the implementation
> in such ways.

The implementation of condition variables uses a 64-bit counter in a 
similar way.  I assumed that we had consensus that we can assume that a 
simple 64-bit counter would never overflow.

> We could allocate the thread numbers lazily, and that would certainly
> avoid limiting ourselves to only allocating 2^64-1 threads. On top of
> that if the function could return an error then we could return such
> an error at overflow:
> 
>   int pthread_thread_number_np (uint64_t* thread_number, pthread_t @var{thread})
> 
> Returns 0 if the thread has a unique number, otherwise -1 if it does not.

That's a bad interface.  If you are worried about 64-bit overflow, we 
should use a 128-bit counter instead, but I'm not convinced this is 
necessary.

Lazy allocation would make the function not safe for use in signal handlers.

>> +This function returns a number that uniquely identifies @var{thread}
>> +among all past, current, and future running threads.  This number does
>> +not change during the life-time of the thread.  Once returned by this
>> +function, a number will not be reused after the thread terminates.
> 
> We should be clear here that uint64_t (the return type) only has 64-bits,
> and while theoretically difficult to reach, this has a limit. The only way
> not to reuse the numbers is to limit the implementation to only allowing
> uint64_t worth of threads to ever be started. That's a limit that never
> existed before the existence of this API.

This  restriction already existed:

    We use two 64b counters: __wseq and __g1_start.  They are
    monotonically increasing and single-writer-multiple-readers
    counters, so we can implement load, fetch-and-add, and
    fetch-and-xor operations even when we just have 32b atomics.
    Values we add or xor are less than or equal to 1<<31 (*), so we
    only have to make overflow-and-addition atomic wrt. to concurrent
    load operations and xor operations.  To do that, we split each
    counter into two 32b values of which we reserve the MSB of each to
    represent an overflow from the lower-order half to the higher-order
    half.

> My worry here is that once you open pandoras box and say that they thread
> number *might* be reused after 2^64-1 pthread_create's, then users will
> start writing code *not* to treat it as unique.

I'm going to put in a __libc_fatal, just to be on the safe side.

> Thus, see my notes above about changing the interface slightly.
> 
>> +
>> +The returned number is only unique with regards to the current process.
>> +It may be shared by subprocesses and other processes in the system.
>> +
>> +The initial (main) thread has number 1.  Thread numbers are not
>> +necessarily assigned in a consecutive fashion.  They bear no
>> +relationship to process IDs or thread IDs assigned by the kernel.
> 
> Suggest:
> 
> They bear no relationship to POSIX thread IDs (pthread_t), process IDs,
> or Linux kernel thread IDs.

“
They bear no relationship to POSIX thread IDs (@code{pthread_t} values), 
process IDs or thread IDs assigned by the kernel.
”

(This is in the generic part of the manual, so we shouldn't reference 
Linux unless absolutely necessary.)

>> +    pthread_thread_number_np;
>> +  }
>>     GLIBC_PRIVATE {
>>       __libc_alloca_cutoff;
>>       # Internal libc interface to libpthread
>> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
>> index 1cc7893195..0e1cf3939e 100644
>> --- a/nptl/allocatestack.c
>> +++ b/nptl/allocatestack.c
>> @@ -413,16 +413,21 @@ allocate_stack (const struct pthread_attr *attr, struct pthread **pdp,
>>     assert (powerof2 (pagesize_m1 + 1));
>>     assert (TCB_ALIGNMENT >= STACK_ALIGN);
>>   
>> -  /* Get the stack size from the attribute if it is set.  Otherwise we
>> -     use the default we determined at start time.  */
>> -  if (attr->stacksize != 0)
>> -    size = attr->stacksize;
>> -  else
>> -    {
>> -      lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
>> +  uint64_t thread_number;
>> +  lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
> 
> You need to add some P&C comments explaining that __default_pthread_attr_lock
> is now *also* protecting the global_thread_number global and why.

I now have:

   uint64_t thread_number;
   lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
   {
     /* Number 1 is reserved for the initial thread.  Reuse
        __default_pthread_attr_lock to avoid concurrent updates of this
        counter.  */
     static uint64_t global_thread_number = 1;
     thread_number = ++global_thread_number;

     /* Check for counter wrap-around.  This should never happen
        because 2**64 is such a large value.  */
     if (thread_number == 0)
       __libc_fatal ("Fatal glibc error: maximum number of threads 
exceeded\n");

>> +static int
>> +do_test (void)
>> +{
>> +  TEST_COMPARE (pthread_thread_number_np (pthread_self ()), 1U);
>> +  support_isolate_in_subprocess (subprocess, NULL);
> 
> Why do we isolate this in a subprocess?

To check that the main thread has thread number 1 in a subprocess.  I'll 
add a comment.

New patch attached.

Thanks,
Florian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: thread_number.patch
Type: text/x-patch
Size: 35339 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20171215/1ec3f401/attachment.bin>