Re: [PATCH] nptl: Add pthread_thread_number_np function

[Carlos O'Donell <carlos at redhat dot com>]

On 12/14/2017 11:47 PM, Florian Weimer wrote:
> 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.

Good. You should mention these uses in the NEWS entry for the new
function. Please add a NEWS entry in v2.

If by "random bit generators" we are talking about arc4random, we could
add this as an internal interface for direct use by arc4random? That
wouldn't be a problem. I would not want this review to block arc4random.

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

I like that answer. Thanks.
 
>> 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.

The comparison is not valid.

One can create and destroy an infinite number of condition variables.

The design of the pthread_thread_number_np API immediately limits the
number of creatable threads to the size of the return type. If we allow
creating more threads than that then we break the API.

My objection is not with the internal implementation of a 64-bit counter.

My objection is to the external exposing of a limit on number of threads.

This is the same problem with using an integer cookie for dlopen returns.
 
>> 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.

I *might* be convinced a 128-bit return is enough.

I would like to see a quick discussion of why we can't make an interface
that doesn't limit the number of threads?

We should not impose arbitrary limits on our interfaces.

Anything that uniquely identifies an object is going to, by the nature of
the requirement, be a variable-sized opaque value.

The implementation can have an internal 64-bit limit, that's fine, but the
external interface would then support an unlimited number of threads.

We can even provide a convenience function to turn the opaque value into
an ASCII string, and such an implementation can just print a 64-bit value
to the string buffer.

Would such an interface be truly terrible?

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

Are you going to mark this function as AS-safe in the documentation?

Yes, you used '@assafe{}' in the manual, OK.

Note that your '@acsafe{}' markup in the manual is wrong, since this function
holds locks and is therefore not AS-safe.

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

That's a good idea.

>> 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.)

Looks good.

>>> +    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.  */

OK.

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

Changes requested:

- Please add another comment in nptl/vars.c adding notes that show that 
  __default_pthread_attr_lock is being used to protect the thread number.

- Explore an interface that doesn't limit the number of threads we can
  create, or continue to convince me I'm crazy. Yes, with a 128-bit counter
  you could have zeptsecond thread creation times and still not overflow
  before the sun expires. That is to say that I'm looking to be convinced
  that a variable sized cookie interface for external uses is a bad idea.

-- 
Cheers,
Carlos.