Re: [PATCH] nptl: Add pthread_thread_number_np function

[Carlos O'Donell <carlos@redhat.com>]

On 12/21/2017 03:03 AM, Florian Weimer wrote:
> On 12/21/2017 10:26 AM, Carlos O'Donell wrote:
> 
>>> +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 POSIX thread IDs (@code{pthread_t} values), process IDs
>>> +or thread IDs assigned by the kernel.
>>
>> I would like us to add something like this:
>> ~~~
>> While the return type of this function is only 64-bits wide, the intent
>> is not to impose an artificial limit on the number of threads that can be
>> created by the runtime. In the future this interface may be extended
>> to 128-bits to support creating as many threads as a user may need
>> for the lifetime of the process.
>> ~~~
>>
>> That way the intent of the interface and future changes are clear.
> 
> So how would a programmer use this interface in a future-proof way?
> I think such a statement would raise more questions than it answers.

I went to bed thinking much the same thing and worried that perhaps this
text was not appropriate for the manual, but could serve as a comment in
the source code for future maintainers. Since this is really a question
about GNU ethos and avoiding artificial limits.

Would you be opposed to adding the comment to the new function sources?

>>> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
>>> index 1cc7893195..454df7740b 100644
>>> --- a/nptl/allocatestack.c
>>> +++ b/nptl/allocatestack.c
>>> @@ -413,16 +413,28 @@ 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);
>>> +  {
>>> +    /* 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;
>>
>> Alright, here comes serious worry #1.
>>
>> If we say "Thread numbers are not necessarily assigned in a consecutive fashion.",
>> and we assign them in a consecutive fashion, users will ignore this statement
>> and use what empirically appears to be true.
> 
> The above does not actually assign thread numbers in a consecutive
> fashion, from an application perspective because the implementation
> can create its own threads for its own internal use.  (librt and
> libanl do this.)

That's a good point.

I know that aio has a helper thraed, so does the notification, so does some
of the related timer routines.

All of those would help break this notion, but these are also rarer features.
 
>> People start relying on this counter incrementing from 1 upwards.
>>
>> People start using this monotonic property for indexing.
>>
>> Soon we can't change it because it's implied API behaviour.
>>
>> I think we should disabuse them from doing something low cost to roll the value:
>>
>> * Do nothing for thread 1, leaving it 1.
>> * Check global_thread_number for overflow instead.
>> * Pick a random number of bits to roll between 0-63 (picked at process startup)
>> * Roll the value by some that number of bits.
>> * Use the rolled value as the thread_number
> 
> Not sure if I understand this.  Do you want us to start at a random
> value?  Or assign IDs randomly?  The latter will have a collision
> much sooner.
> 
> I can switch the thread numbers to a fixed, but random-looking
> permutation of the integers in [0, 2**64), but this looks excessive.

I want a low cost solution that avoids abuses of the interface for
indexing into arrays, or other issues that would break when we change
this in the future. We do not want users to make assumptions about the
values we hand out.

The lowest cost option I can think of is to circular shift the global
counter value by some number of bits.

I don't care for it to be random.

> In my opinion, we need to assume at one point that programmers read
> the documentation.

The implementation should be robust against abuse. That's what makes
it high quality.

-- 
Cheers,
Carlos.