Re: [PATCH v2 06/19] nptl: Replace struct thread cancelhandling field

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella:

> Now that both thread state and setxid is being tracked by two different
> fields ('joinstate' and 'setxid_flag'), there is no need to keep track
> of exiting (EXITING_BIT) and terminated (TERMINATED_BIT) state in
> 'cancelhandling'.
>
> The 'cancelhandling' field is renamed to 'cancelstatus' and it only
> signals whether the thread has been cancelled (set by pthread_cancel()).
> It allows simplify the atomic operation required to avoid CAS
> operations.

“cancelstatus” is a bad name because “cancel state” is POSIX terminology
with different meaning.  Please choose a different name.

I think pending_cancel or cancel_requested would work.

> There is no need to check the thread state on
> __pthread_enable_asynccancel() nor on pthread_testcancel () anymore,
> since cancellation is explicit disabled when thread start the exit code
> on __do_cancel().

“explict[ly]”

>
> On __nptl_free_tcp(), the 'joinstate' now defines whether it is the

“__nptl_free_tcb”.

> creating thread or the created thread that calls it.  So there is
> no concurrent call within the function and thus no need to set the
> TERMINATED_BIT.
>
> For SIGCANCEL handler, sigcancel_handler(), 'joinstate' is used instead
> (pthread_cancel() might still be called concurrenty in assynchronous
> mode).

“as[]ynchronous”

> diff --git a/nptl/nptl_free_tcb.c b/nptl/nptl_free_tcb.c
> index cbf3580f59..15e1a18562 100644
> --- a/nptl/nptl_free_tcb.c
> +++ b/nptl/nptl_free_tcb.c
> @@ -24,22 +24,12 @@
>  void
>  __nptl_free_tcb (struct pthread *pd)
>  {
> -  /* The thread is exiting now.  */
> -  if (atomic_bit_test_set (&pd->cancelhandling, TERMINATED_BIT) == 0)
> -    {
> -      /* Free TPP data.  */
> -      if (pd->tpp != NULL)
> -        {
> -          struct priority_protection_data *tpp = pd->tpp;
> +  free (pd->tpp);
> +  pd->tpp = NULL;

For detached threads, this is called on the thread after signals have
been blocked.  I don't think this is entirely correct.  And things
become rather interesting if malloc needs pd->tpp.

It's not clear to me whether this could happen before.

> +  /* Queue the stack memory block for reuse and exit the process.  The kernel
> +     will signal via writing to the address returned by QUEUE-STACK when the
> +     stack is available.  */
> +  __nptl_deallocate_stack (pd);
>  }

That old comment seems to be out-of-date.

>  libc_hidden_def (__nptl_free_tcb)
> diff --git a/nptl/pthread_cancel.c b/nptl/pthread_cancel.c
> index 67e00ef007..aed6c1ea47 100644
> --- a/nptl/pthread_cancel.c
> +++ b/nptl/pthread_cancel.c

> @@ -93,8 +90,9 @@ __pthread_cancel (pthread_t th)
>    }
>  #endif
>  
> -  int oldch = atomic_fetch_or_acquire (&pd->cancelhandling, CANCELED_BITMASK);
> -  if ((oldch & CANCELED_BITMASK) != 0)
> +  /* If already cancelled just return (cancellation will be acted upon in next
> +     cancellation entrypoint).  */
> +  if (atomic_exchange_acquire (&pd->cancelstatus, 1) == 1)
>      return 0;

This should probably use relaxed MO.

The libthreaddb changes look okay to me.

Thanks,
Florian