[PATCH v2 05/19] nptl: Move setxid flag out of cancelhandling
Adhemerval Zanella
adhemerval.zanella@linaro.org
Thu Aug 26 16:39:53 GMT 2021
On 26/08/2021 12:21, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> On 26/08/2021 08:34, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> Now that the thread state is tracked by 'joinstate' field, there is no
>>>> need to keep the setxid flag within the 'cancelhandling'. It simplifies
>>>> the atomic code to set and reset it (since there is no need to handle
>>>> the thread state concurrent update).
>>>
>>> Some of this functionality reimplements the tidlock.
>>>
>>> There is some functionality around thread creation, but I do not know if
>>> the synchronization is entirely correct. This makes it rather difficult
>>> to review changes.
>>>
>>> It's also not clear to me if we need to do anything to prevent early
>>> setxid calls:
>>>
>>>> /* Don't allow setxid until cloned. */
>>>
>>> I suspect this should just look at the TID instead. We create the
>>> thread with all signals blocked, so once there is a TID (and the thread
>>> has not exited), it is safe to send the signal.
>>
>> I agree that we might improve the setxid code, however what I intended
>> with this changes is to keep its semantic as-is and just decouple it
>> from cancelhandling. I might revise it on a later patch to follow your
>> suggestions.
>
> I wonder if there is a new race here:
>
> @@ -109,41 +100,23 @@ setxid_mark_thread (struct xid_command *cmdp, struct pthread *t)
> /* Don't let the thread exit before the setxid handler runs. */
> t->setxid_futex = 0;
>
> + /* If thread is exiting right now, ignore it. */
> + if (atomic_load_acquire (&t->joinstate) == THREAD_STATE_EXITING)
> + return;
>
> + /* Release the futex if there is no other setxid in progress. */
> + if (atomic_exchange_acquire (&t->setxid_flag, 1) == 0)
> + {
> + t->setxid_futex = 1;
> + futex_wake (&t->setxid_futex, 1, FUTEX_PRIVATE);
> }
>
> That is, if the setxid-sending thread can now wait for a futex wake that
> never comes.
Wouldn't be handled by setxid_unmark_thread, which wakes the futex
unconditionally?
>
> There's also a use of atomic_exchange_release whose result is not used:
>
> + atomic_exchange_release (&t->setxid_flag, 0);
>
> That should probably be store.
Ack.
More information about the Libc-alpha
mailing list