Re: [PATCH] nptl: Add pthread_thread_number_np function

[Florian Weimer <fweimer at redhat dot com>]

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.

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

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

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

Thanks,
Florian