nptl: Remove cancellation checks from sem_{timed}wait (BZ #23006)
Commit Message
The Austin group has decided that sem_wait and sem_timedwait do not need to
be cancellation points. This patch removes the cancellation checks in order
to improve performance.
As for comment #2 in BZ#23006, the old implementation which explicit check
for cancellation is moved to a compat symbol.
Checked on x86_64-linux-gnu and i686-linux-gnu.
[BZ #23006]
* nptl/Version (GLIBC_2.30): Add sem_wait and sem_timedwait.
* nptl/sem_timedwait.c (__new_sem_timedwait): New default symbol.
(sem_timedwait): Define as __old_sem_timedwait and make compat symbol.
* nptl/sem_wait.c (__new_sem_wait): Remove cancellation handling and
add new default version.
(__old_sem_wait_cancel): New compat symbol.
* nptl/tst-cancel12.c (cleanup, tf, do_test): Adapt to check if
sem_{timed}wait is not a cancellation entrypoint and use libsupport.
* nptl/tst-cancel14.c: Likewise.
* sysdeps/unix/sysv/linux/aarch64/libpthread.abilist (GLIBC_2.30):
Add sem_timedwait and sem_wait.
* sysdeps/unix/sysv/linux/alpha/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/arm/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/csky/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/hppa/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/i386/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/ia64/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/m68k/coldfire/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/m68k/m680x0/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/microblaze/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/mips/mips32/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/mips/mips64/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/nios2/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/powerpc/powerpc32/libpthread.abilist:
Likewise.
* sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libpthread.abilist:
Likewise.
* sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libpthread.abilist:
Likewise.
* sysdeps/unix/sysv/linux/riscv/rv64/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/s390/s390-32/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/s390/s390-64/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/sh/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/sparc/sparc32/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/sparc/sparc64/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/x86_64/64/libpthread.abilist: Likewise.
* sysdeps/unix/sysv/linux/x86_64/x32/libpthread.abilist: Likewise.
[1] http://austingroupbugs.net/view.php?id=1076#c3938
---
ChangeLog | 41 +++++++++
nptl/Versions | 4 +
nptl/sem_timedwait.c | 28 +++++-
nptl/sem_wait.c | 18 +++-
nptl/tst-cancel12.c | 77 ++++------------
nptl/tst-cancel14.c | 92 +++++--------------
.../sysv/linux/aarch64/libpthread.abilist | 2 +
.../unix/sysv/linux/alpha/libpthread.abilist | 2 +
.../unix/sysv/linux/arm/libpthread.abilist | 2 +
.../unix/sysv/linux/csky/libpthread.abilist | 2 +
.../unix/sysv/linux/hppa/libpthread.abilist | 2 +
.../unix/sysv/linux/i386/libpthread.abilist | 2 +
.../unix/sysv/linux/ia64/libpthread.abilist | 2 +
.../linux/m68k/coldfire/libpthread.abilist | 2 +
.../sysv/linux/m68k/m680x0/libpthread.abilist | 2 +
.../sysv/linux/microblaze/libpthread.abilist | 2 +
.../sysv/linux/mips/mips32/libpthread.abilist | 2 +
.../sysv/linux/mips/mips64/libpthread.abilist | 2 +
.../unix/sysv/linux/nios2/libpthread.abilist | 2 +
.../powerpc/powerpc32/libpthread.abilist | 2 +
.../powerpc/powerpc64/be/libpthread.abilist | 2 +
.../powerpc/powerpc64/le/libpthread.abilist | 2 +
.../sysv/linux/riscv/rv64/libpthread.abilist | 2 +
.../linux/s390/s390-32/libpthread.abilist | 2 +
.../linux/s390/s390-64/libpthread.abilist | 2 +
sysdeps/unix/sysv/linux/sh/libpthread.abilist | 2 +
.../linux/sparc/sparc32/libpthread.abilist | 2 +
.../linux/sparc/sparc64/libpthread.abilist | 2 +
.../sysv/linux/x86_64/64/libpthread.abilist | 2 +
.../sysv/linux/x86_64/x32/libpthread.abilist | 2 +
30 files changed, 175 insertions(+), 133 deletions(-)
Comments
* Adhemerval Zanella:
> The Austin group has decided that sem_wait and sem_timedwait do not need to
> be cancellation points. This patch removes the cancellation checks in order
> to improve performance.
>
> As for comment #2 in BZ#23006, the old implementation which explicit check
> for cancellation is moved to a compat symbol.
The code nptl/sem_waitcommon.c still acts on a cancellation request, I
think. The only change is that if we do not wait at all, we do not act
on the cancellation. I don't think this matches the POSIX semantics,
where cancellation points are either there or not, and do not depend on
function implementation details.
Thanks,
Florian
On 24/06/2019 14:17, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> The Austin group has decided that sem_wait and sem_timedwait do not need to
>> be cancellation points. This patch removes the cancellation checks in order
>> to improve performance.
>>
>> As for comment #2 in BZ#23006, the old implementation which explicit check
>> for cancellation is moved to a compat symbol.
>
> The code nptl/sem_waitcommon.c still acts on a cancellation request, I
> think. The only change is that if we do not wait at all, we do not act
> on the cancellation. I don't think this matches the POSIX semantics,
> where cancellation points are either there or not, and do not depend on
> function implementation details.
My understanding is defect #1076 is to make it more clear when exactly the
'may' cancellable entrypoints should act upon cancellation request, more
specifically that cancellation should be used as a mechanism for preventing
indefinite blocking.
Also, imho this specification does not clash from the 'Thread Cancellation
Overview' [1], which states that:
For functions in the "may occur" list, a cancellation check may be performed
on some calls but not others; i.e., whether or not a cancellation point occurs
when one of these functions is being executed can depend on current conditions."
So, following the idea of the first note in defect #1076, sem_wait() and
sem_timedwait() are now moved from cancellation entrypoints 'shall' to 'may'.
The BZ#23006 was created to remove the cancellation checks done even for the fast
paths where the call will not block. My understanding is sem_{timed}wait should
still be cancellation entrypoint for the *blocked* case, more specifically in the
code when futex calls might block indefinitely.
It should also be noted that the 'Thread Cancellation Overview' rationale has a
list of blocking entrypoints that are not listed in 'shall' and 'may' and
sem_{timed}wait are not listed. I think to make sem_{timed}wait to *not* act
as cancellation entrypoint regardless we will need to open a Austin Group
Defect to update this rationale to include the semaphore functions.
[1] https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html
* Adhemerval Zanella:
> On 24/06/2019 14:17, Florian Weimer wrote:
>> * Adhemerval Zanella:
>>
>>> The Austin group has decided that sem_wait and sem_timedwait do not need to
>>> be cancellation points. This patch removes the cancellation checks in order
>>> to improve performance.
>>>
>>> As for comment #2 in BZ#23006, the old implementation which explicit check
>>> for cancellation is moved to a compat symbol.
>>
>> The code nptl/sem_waitcommon.c still acts on a cancellation request, I
>> think. The only change is that if we do not wait at all, we do not act
>> on the cancellation. I don't think this matches the POSIX semantics,
>> where cancellation points are either there or not, and do not depend on
>> function implementation details.
>
> My understanding is defect #1076 is to make it more clear when exactly the
> 'may' cancellable entrypoints should act upon cancellation request, more
> specifically that cancellation should be used as a mechanism for preventing
> indefinite blocking.
>
> Also, imho this specification does not clash from the 'Thread Cancellation
> Overview' [1], which states that:
>
> For functions in the "may occur" list, a cancellation check may be performed
> on some calls but not others; i.e., whether or not a cancellation point occurs
> when one of these functions is being executed can depend on current
> conditions."
My impression is that doesn't mean that the behavior is random (although
the wording allows for that). It's dependent on how things have been
configured, and not on which threads wins a race during the call.
Any cancellation model which makes functions a cancellation points only
if they block has this problem. It also means that future optimizations
(in glibc, the kernel, or the silicon) could effectively remove
cancellation points. I don't think this is desirable.
Thanks,
Florian
On 25/06/2019 09:17, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> On 24/06/2019 14:17, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> The Austin group has decided that sem_wait and sem_timedwait do not need to
>>>> be cancellation points. This patch removes the cancellation checks in order
>>>> to improve performance.
>>>>
>>>> As for comment #2 in BZ#23006, the old implementation which explicit check
>>>> for cancellation is moved to a compat symbol.
>>>
>>> The code nptl/sem_waitcommon.c still acts on a cancellation request, I
>>> think. The only change is that if we do not wait at all, we do not act
>>> on the cancellation. I don't think this matches the POSIX semantics,
>>> where cancellation points are either there or not, and do not depend on
>>> function implementation details.
>>
>> My understanding is defect #1076 is to make it more clear when exactly the
>> 'may' cancellable entrypoints should act upon cancellation request, more
>> specifically that cancellation should be used as a mechanism for preventing
>> indefinite blocking.
>>
>> Also, imho this specification does not clash from the 'Thread Cancellation
>> Overview' [1], which states that:
>>
>> For functions in the "may occur" list, a cancellation check may be performed
>> on some calls but not others; i.e., whether or not a cancellation point occurs
>> when one of these functions is being executed can depend on current
>> conditions."
>
> My impression is that doesn't mean that the behavior is random (although
> the wording allows for that). It's dependent on how things have been
> configured, and not on which threads wins a race during the call.
My understanding is pthread cancellation is a mechanism to abort unbounded block
from a thread. Being random is not meaningful here (where the sem_{timed}wait
would block or not).
>
> Any cancellation model which makes functions a cancellation points only
> if they block has this problem. It also means that future optimizations
> (in glibc, the kernel, or the silicon) could effectively remove
> cancellation points. I don't think this is desirable.
That's why I think to make sem_{timed}wait *not* a cancellation entrypoint
would need to open a Austin Group defect, it is explicit that the only
blocking routines that not act as a cancellation are pthread_mutex_lock,
pthread_barrier_wait, pthread_spin_lock.
* Adhemerval Zanella:
>> My impression is that doesn't mean that the behavior is random (although
>> the wording allows for that). It's dependent on how things have been
>> configured, and not on which threads wins a race during the call.
>
> My understanding is pthread cancellation is a mechanism to abort
> unbounded block from a thread. Being random is not meaningful here
> (where the sem_{timed}wait would block or not).
Not just unbounded blocks, inconveniently long ones as well. (Otherwise
no timed wait function would be a cancellation point.)
What I meant is that if such functions only check for pending
cancellation on slow paths, then you might end up with cases where
cancellation checks disappear from executions due to future
optimizations. As a result, an expected cancellation point may vanish
from some inner loop, causing application breakage.
>> Any cancellation model which makes functions a cancellation points only
>> if they block has this problem. It also means that future optimizations
>> (in glibc, the kernel, or the silicon) could effectively remove
>> cancellation points. I don't think this is desirable.
>
> That's why I think to make sem_{timed}wait *not* a cancellation
> entrypoint would need to open a Austin Group defect, it is explicit
> that the only blocking routines that not act as a cancellation are
> pthread_mutex_lock, pthread_barrier_wait, pthread_spin_lock.
We discussed this before with getentropy/getrandom. I argued in the
same direction (if something can block for a long time, it should be a
cancellation point). Torvald, as our concurrency expert, argued against
this. Existing practice also shows that most of our file system calls
are *not* cancellation points, and these can also wait forever when
hitting network file systems. So all this is a bit inconsistent,
unfortunately.
Thanks,
Florian
On 25/06/2019 09:35, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>>> My impression is that doesn't mean that the behavior is random (although
>>> the wording allows for that). It's dependent on how things have been
>>> configured, and not on which threads wins a race during the call.
>>
>> My understanding is pthread cancellation is a mechanism to abort
>> unbounded block from a thread. Being random is not meaningful here
>> (where the sem_{timed}wait would block or not).
>
> Not just unbounded blocks, inconveniently long ones as well. (Otherwise
> no timed wait function would be a cancellation point.)
Right.
>
> What I meant is that if such functions only check for pending
> cancellation on slow paths, then you might end up with cases where
> cancellation checks disappear from executions due to future
> optimizations. As a result, an expected cancellation point may vanish
> from some inner loop, causing application breakage.
I see such modification being a semantic change rather than just an
optimization and it is most likely a regression. That's why I see
BZ#23006 itself required a Austin group clarification.
>
>>> Any cancellation model which makes functions a cancellation points only
>>> if they block has this problem. It also means that future optimizations
>>> (in glibc, the kernel, or the silicon) could effectively remove
>>> cancellation points. I don't think this is desirable.
>>
>> That's why I think to make sem_{timed}wait *not* a cancellation
>> entrypoint would need to open a Austin Group defect, it is explicit
>> that the only blocking routines that not act as a cancellation are
>> pthread_mutex_lock, pthread_barrier_wait, pthread_spin_lock.
>
> We discussed this before with getentropy/getrandom. I argued in the
> same direction (if something can block for a long time, it should be a
> cancellation point). Torvald, as our concurrency expert, argued against
> this. Existing practice also shows that most of our file system calls
> are *not* cancellation points, and these can also wait forever when
> hitting network file systems. So all this is a bit inconsistent,
> unfortunately.
Another point is see no point if deviate glibc implementation from other
libc regarding sem_{timed}wait (where it is usually a cancellation point).
It would most likely cause application breakage as well.
But I understand Torvarld view, specially with current racy statues of
glibc thread cancellation (BZ#12683). Maybe when we finally make thread
cancellation safer we also focus on make the Linux file system calls
more consistent regarding cancellation.
In any case, do you think we should make sem_{timed}wait not a cancellation
entrypoint for 2.30? I can rework the patch if it is the case.
* Adhemerval Zanella:
> In any case, do you think we should make sem_{timed}wait not a cancellation
> entrypoint for 2.30? I can rework the patch if it is the case.
Do we have any other form of blocking synchronization that is
cancellable? If not, a cancellable semaphore wait operation would make
sense. But then we should perform the cancellation check on the fast
path, too.
Thanks,
Florian
On 25/06/2019 10:36, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> In any case, do you think we should make sem_{timed}wait not a cancellation
>> entrypoint for 2.30? I can rework the patch if it is the case.
>
> Do we have any other form of blocking synchronization that is
> cancellable? If not, a cancellable semaphore wait operation would make
> sense. But then we should perform the cancellation check on the fast
> path, too.
pthread_cond_wait, pthread_cond_timedwait, and pthread_join as well the
gnu extension pthread_timedjoin_np and pthread_cond_clockwait (not yet
upstream). Depending of you definition of synchronization, you can also
include sigtimedwait, sigwait, sigwaitinfo, wait, waitid, and waitpid.
We explicit does not support cancellation for pthread_rwlock_rdlock,
pthread_rwlock_timedrdlock, pthread_rwlock_timedwrlock, and
pthread_rwlock_wrlock. I tend to see that pthread_rwlock_* are similar
to pthread_mutex and the rationale layout on 'Thread Cancellation
Overview' also applies.
So at least we have synchronization functions that explicit does not
act for cancellation, we might extend it to sem_{timed}wait now that
they are also listed on 'shall'.
However, I still think this does not really follow along with the
rationale exposed on the very issue that triggered it [1], which aimed
imho to just remove the requirement of check for cancellation on fast
path only.
[1] http://austingroupbugs.net/view.php?id=1076#c3938
On 25/06/2019 11:29, Adhemerval Zanella wrote:
>
>
> On 25/06/2019 10:36, Florian Weimer wrote:
>> * Adhemerval Zanella:
>>
>>> In any case, do you think we should make sem_{timed}wait not a cancellation
>>> entrypoint for 2.30? I can rework the patch if it is the case.
>>
>> Do we have any other form of blocking synchronization that is
>> cancellable? If not, a cancellable semaphore wait operation would make
>> sense. But then we should perform the cancellation check on the fast
>> path, too.
>
> pthread_cond_wait, pthread_cond_timedwait, and pthread_join as well the
> gnu extension pthread_timedjoin_np and pthread_cond_clockwait (not yet
> upstream). Depending of you definition of synchronization, you can also
> include sigtimedwait, sigwait, sigwaitinfo, wait, waitid, and waitpid.
>
> We explicit does not support cancellation for pthread_rwlock_rdlock,
> pthread_rwlock_timedrdlock, pthread_rwlock_timedwrlock, and
> pthread_rwlock_wrlock. I tend to see that pthread_rwlock_* are similar
> to pthread_mutex and the rationale layout on 'Thread Cancellation
> Overview' also applies.
>
> So at least we have synchronization functions that explicit does not
> act for cancellation, we might extend it to sem_{timed}wait now that
> they are also listed on 'shall'.
>
> However, I still think this does not really follow along with the
> rationale exposed on the very issue that triggered it [1], which aimed
> imho to just remove the requirement of check for cancellation on fast
> path only.
>
> [1] http://austingroupbugs.net/view.php?id=1076#c3938
>
Florian, how should we proceed regarding it? Should we just move
sem_{timed}wait out of the cancellable entrypoints or act uppon just
for the potentially blocked case? For latter do you still think we
should keep a compat symbol for early check?
* Adhemerval Zanella:
> On 25/06/2019 11:29, Adhemerval Zanella wrote:
>>
>>
>> On 25/06/2019 10:36, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> In any case, do you think we should make sem_{timed}wait not a cancellation
>>>> entrypoint for 2.30? I can rework the patch if it is the case.
>>>
>>> Do we have any other form of blocking synchronization that is
>>> cancellable? If not, a cancellable semaphore wait operation would make
>>> sense. But then we should perform the cancellation check on the fast
>>> path, too.
>>
>> pthread_cond_wait, pthread_cond_timedwait, and pthread_join as well the
>> gnu extension pthread_timedjoin_np and pthread_cond_clockwait (not yet
>> upstream). Depending of you definition of synchronization, you can also
>> include sigtimedwait, sigwait, sigwaitinfo, wait, waitid, and waitpid.
>>
>> We explicit does not support cancellation for pthread_rwlock_rdlock,
>> pthread_rwlock_timedrdlock, pthread_rwlock_timedwrlock, and
>> pthread_rwlock_wrlock. I tend to see that pthread_rwlock_* are similar
>> to pthread_mutex and the rationale layout on 'Thread Cancellation
>> Overview' also applies.
>>
>> So at least we have synchronization functions that explicit does not
>> act for cancellation, we might extend it to sem_{timed}wait now that
>> they are also listed on 'shall'.
>>
>> However, I still think this does not really follow along with the
>> rationale exposed on the very issue that triggered it [1], which aimed
>> imho to just remove the requirement of check for cancellation on fast
>> path only.
>>
>> [1] http://austingroupbugs.net/view.php?id=1076#c3938
>>
>
> Florian, how should we proceed regarding it? Should we just move
> sem_{timed}wait out of the cancellable entrypoints or act uppon just
> for the potentially blocked case? For latter do you still think we
> should keep a compat symbol for early check?
Ugh, to be honest, I don't think any code change is required here.
Thanks,
Florian
On 03/07/2019 15:28, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> On 25/06/2019 11:29, Adhemerval Zanella wrote:
>>>
>>>
>>> On 25/06/2019 10:36, Florian Weimer wrote:
>>>> * Adhemerval Zanella:
>>>>
>>>>> In any case, do you think we should make sem_{timed}wait not a cancellation
>>>>> entrypoint for 2.30? I can rework the patch if it is the case.
>>>>
>>>> Do we have any other form of blocking synchronization that is
>>>> cancellable? If not, a cancellable semaphore wait operation would make
>>>> sense. But then we should perform the cancellation check on the fast
>>>> path, too.
>>>
>>> pthread_cond_wait, pthread_cond_timedwait, and pthread_join as well the
>>> gnu extension pthread_timedjoin_np and pthread_cond_clockwait (not yet
>>> upstream). Depending of you definition of synchronization, you can also
>>> include sigtimedwait, sigwait, sigwaitinfo, wait, waitid, and waitpid.
>>>
>>> We explicit does not support cancellation for pthread_rwlock_rdlock,
>>> pthread_rwlock_timedrdlock, pthread_rwlock_timedwrlock, and
>>> pthread_rwlock_wrlock. I tend to see that pthread_rwlock_* are similar
>>> to pthread_mutex and the rationale layout on 'Thread Cancellation
>>> Overview' also applies.
>>>
>>> So at least we have synchronization functions that explicit does not
>>> act for cancellation, we might extend it to sem_{timed}wait now that
>>> they are also listed on 'shall'.
>>>
>>> However, I still think this does not really follow along with the
>>> rationale exposed on the very issue that triggered it [1], which aimed
>>> imho to just remove the requirement of check for cancellation on fast
>>> path only.
>>>
>>> [1] http://austingroupbugs.net/view.php?id=1076#c3938
>>>
>>
>> Florian, how should we proceed regarding it? Should we just move
>> sem_{timed}wait out of the cancellable entrypoints or act uppon just
>> for the potentially blocked case? For latter do you still think we
>> should keep a compat symbol for early check?
>
> Ugh, to be honest, I don't think any code change is required here.
By 'here' do you mean the patch or current implementation? I still think
we can use the Austin clarification to optimize the sem_{timed}wait
and remove the early checks for the non-blocked case. And I also don't
think it is worth to add a compatibility check for this specific
semantic.
* Adhemerval Zanella:
> On 03/07/2019 15:28, Florian Weimer wrote:
>> * Adhemerval Zanella:
>>
>>> On 25/06/2019 11:29, Adhemerval Zanella wrote:
>>>>
>>>>
>>>> On 25/06/2019 10:36, Florian Weimer wrote:
>>>>> * Adhemerval Zanella:
>>>>>
>>>>>> In any case, do you think we should make sem_{timed}wait not a cancellation
>>>>>> entrypoint for 2.30? I can rework the patch if it is the case.
>>>>>
>>>>> Do we have any other form of blocking synchronization that is
>>>>> cancellable? If not, a cancellable semaphore wait operation would make
>>>>> sense. But then we should perform the cancellation check on the fast
>>>>> path, too.
>>>>
>>>> pthread_cond_wait, pthread_cond_timedwait, and pthread_join as well the
>>>> gnu extension pthread_timedjoin_np and pthread_cond_clockwait (not yet
>>>> upstream). Depending of you definition of synchronization, you can also
>>>> include sigtimedwait, sigwait, sigwaitinfo, wait, waitid, and waitpid.
>>>>
>>>> We explicit does not support cancellation for pthread_rwlock_rdlock,
>>>> pthread_rwlock_timedrdlock, pthread_rwlock_timedwrlock, and
>>>> pthread_rwlock_wrlock. I tend to see that pthread_rwlock_* are similar
>>>> to pthread_mutex and the rationale layout on 'Thread Cancellation
>>>> Overview' also applies.
>>>>
>>>> So at least we have synchronization functions that explicit does not
>>>> act for cancellation, we might extend it to sem_{timed}wait now that
>>>> they are also listed on 'shall'.
>>>>
>>>> However, I still think this does not really follow along with the
>>>> rationale exposed on the very issue that triggered it [1], which aimed
>>>> imho to just remove the requirement of check for cancellation on fast
>>>> path only.
>>>>
>>>> [1] http://austingroupbugs.net/view.php?id=1076#c3938
>>>>
>>>
>>> Florian, how should we proceed regarding it? Should we just move
>>> sem_{timed}wait out of the cancellable entrypoints or act uppon just
>>> for the potentially blocked case? For latter do you still think we
>>> should keep a compat symbol for early check?
>>
>> Ugh, to be honest, I don't think any code change is required here.
>
> By 'here' do you mean the patch or current implementation? I still think
> we can use the Austin clarification to optimize the sem_{timed}wait
> and remove the early checks for the non-blocked case. And I also don't
> think it is worth to add a compatibility check for this specific
> semantic.
I think the optimization is invalid, for the reasons I outlined earlier.
Thanks,
Florian
On 04/07/2019 11:19, Florian Weimer wrote:
> * Adhemerval Zanella:
>
>> On 03/07/2019 15:28, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> On 25/06/2019 11:29, Adhemerval Zanella wrote:
>>>>>
>>>>>
>>>>> On 25/06/2019 10:36, Florian Weimer wrote:
>>>>>> * Adhemerval Zanella:
>>>>>>
>>>>>>> In any case, do you think we should make sem_{timed}wait not a cancellation
>>>>>>> entrypoint for 2.30? I can rework the patch if it is the case.
>>>>>>
>>>>>> Do we have any other form of blocking synchronization that is
>>>>>> cancellable? If not, a cancellable semaphore wait operation would make
>>>>>> sense. But then we should perform the cancellation check on the fast
>>>>>> path, too.
>>>>>
>>>>> pthread_cond_wait, pthread_cond_timedwait, and pthread_join as well the
>>>>> gnu extension pthread_timedjoin_np and pthread_cond_clockwait (not yet
>>>>> upstream). Depending of you definition of synchronization, you can also
>>>>> include sigtimedwait, sigwait, sigwaitinfo, wait, waitid, and waitpid.
>>>>>
>>>>> We explicit does not support cancellation for pthread_rwlock_rdlock,
>>>>> pthread_rwlock_timedrdlock, pthread_rwlock_timedwrlock, and
>>>>> pthread_rwlock_wrlock. I tend to see that pthread_rwlock_* are similar
>>>>> to pthread_mutex and the rationale layout on 'Thread Cancellation
>>>>> Overview' also applies.
>>>>>
>>>>> So at least we have synchronization functions that explicit does not
>>>>> act for cancellation, we might extend it to sem_{timed}wait now that
>>>>> they are also listed on 'shall'.
>>>>>
>>>>> However, I still think this does not really follow along with the
>>>>> rationale exposed on the very issue that triggered it [1], which aimed
>>>>> imho to just remove the requirement of check for cancellation on fast
>>>>> path only.
>>>>>
>>>>> [1] http://austingroupbugs.net/view.php?id=1076#c3938
>>>>>
>>>>
>>>> Florian, how should we proceed regarding it? Should we just move
>>>> sem_{timed}wait out of the cancellable entrypoints or act uppon just
>>>> for the potentially blocked case? For latter do you still think we
>>>> should keep a compat symbol for early check?
>>>
>>> Ugh, to be honest, I don't think any code change is required here.
>>
>> By 'here' do you mean the patch or current implementation? I still think
>> we can use the Austin clarification to optimize the sem_{timed}wait
>> and remove the early checks for the non-blocked case. And I also don't
>> think it is worth to add a compatibility check for this specific
>> semantic.
>
> I think the optimization is invalid, for the reasons I outlined earlier.
This discussion seems to be on stalemate and I think this is not really
a blocker for 2.30. I will remove it from the list and we can sort this
when 2.31 opens.
* Adhemerval Zanella:
> This discussion seems to be on stalemate and I think this is not really
> a blocker for 2.30. I will remove it from the list and we can sort this
> when 2.31 opens.
Thanks, this makes sense to me.
Florian
@@ -277,6 +277,10 @@ libpthread {
cnd_timedwait; cnd_wait; tss_create; tss_delete; tss_get; tss_set;
}
+ GLIBC_2.30 {
+ sem_wait; sem_timedwait;
+ }
+
GLIBC_PRIVATE {
__pthread_initialize_minimal;
__pthread_clock_gettime; __pthread_clock_settime;
@@ -17,12 +17,32 @@
License along with the GNU C Library; if not, see
<http://www.gnu.org/licenses/>. */
-#include "sem_waitcommon.c"
-
/* This is in a separate file because because sem_timedwait is only provided
if __USE_XOPEN2K is defined. */
+
+#include "sem_waitcommon.c"
+
+/* The Austin group has decided that sem_wait() and sem_timedwait() do not
+ need to be cancellation points:
+ http://austingroupbugs.net/view.php?id=1076#c3938 */
+int
+__new_sem_timedwait (sem_t *sem, const struct timespec *abstime)
+{
+ if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)
+ {
+ __set_errno (EINVAL);
+ return -1;
+ }
+
+ if (__new_sem_wait_fast ((struct new_sem *) sem, 0) == 0)
+ return 0;
+ return __new_sem_wait_slow((struct new_sem *) sem, abstime);
+}
+versioned_symbol (libpthread, __new_sem_timedwait, sem_timedwait, GLIBC_2_30);
+
+#if SHLIB_COMPAT (libpthread, GLIBC_2_2, GLIBC_2_30)
int
-sem_timedwait (sem_t *sem, const struct timespec *abstime)
+__old_sem_timedwait (sem_t *sem, const struct timespec *abstime)
{
if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)
{
@@ -38,3 +58,5 @@ sem_timedwait (sem_t *sem, const struct timespec *abstime)
else
return __new_sem_wait_slow((struct new_sem *) sem, abstime);
}
+compat_symbol (libpthread, __old_sem_timedwait, sem_timedwait, GLIBC_2_2);
+#endif
@@ -20,8 +20,21 @@
#include <lowlevellock.h> /* lll_futex* used by the old code. */
#include "sem_waitcommon.c"
+/* The Austin group has decided that sem_wait() and sem_timedwait() do not
+ need to be cancellation points:
+ http://austingroupbugs.net/view.php?id=1076#c3938 */
int
__new_sem_wait (sem_t *sem)
+{
+ if (__new_sem_wait_fast ((struct new_sem *) sem, 0) == 0)
+ return 0;
+ return __new_sem_wait_slow ((struct new_sem *) sem, NULL);
+}
+versioned_symbol (libpthread, __new_sem_wait, sem_wait, GLIBC_2_30);
+
+#if SHLIB_COMPAT (libpthread, GLIBC_2_1, GLIBC_2_30)
+int
+__old_sem_wait_cancel (sem_t *sem)
{
/* We need to check whether we need to act upon a cancellation request here
because POSIX specifies that cancellation points "shall occur" in
@@ -39,9 +52,10 @@ __new_sem_wait (sem_t *sem)
if (__new_sem_wait_fast ((struct new_sem *) sem, 0) == 0)
return 0;
else
- return __new_sem_wait_slow((struct new_sem *) sem, NULL);
+ return __new_sem_wait_slow ((struct new_sem *) sem, NULL);
}
-versioned_symbol (libpthread, __new_sem_wait, sem_wait, GLIBC_2_1);
+compat_symbol (libpthread, __old_sem_wait_cancel, sem_wait, GLIBC_2_1);
+#endif
#if SHLIB_COMPAT (libpthread, GLIBC_2_0, GLIBC_2_1)
int
@@ -25,6 +25,8 @@
#include <string.h>
#include <unistd.h>
+#include <support/xthread.h>
+#include <support/check.h>
static pthread_barrier_t bar;
static sem_t sem;
@@ -35,11 +37,7 @@ cleanup (void *arg)
{
static int ncall;
- if (++ncall != 1)
- {
- puts ("second call to cleanup");
- exit (1);
- }
+ TEST_VERIFY_EXIT (++ncall != 1);
}
@@ -48,14 +46,9 @@ tf (void *arg)
{
pthread_cleanup_push (cleanup, NULL);
- int e = pthread_barrier_wait (&bar);
- if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
- {
- puts ("error: tf: 1st barrier_wait failed");
- exit (1);
- }
+ xpthread_barrier_wait (&bar);
- /* This call should be cancelable. */
+ /* This call should not be cancelable. */
sem_wait (&sem);
pthread_cleanup_pop (0);
@@ -67,57 +60,23 @@ tf (void *arg)
static int
do_test (void)
{
- pthread_t th;
-
- if (pthread_barrier_init (&bar, NULL, 2) != 0)
- {
- puts ("error: barrier_init failed");
- exit (1);
- }
+ xpthread_barrier_init (&bar, NULL, 2);
/* A value higher than 0 will check for uncontended pthread cancellation,
where the sem_wait operation will return immediatelly. */
- if (sem_init (&sem, 0, 1) != 0)
- {
- puts ("error: sem_init failed");
- exit (1);
- }
-
- if (pthread_create (&th, NULL, tf, NULL) != 0)
- {
- puts ("error: create failed");
- exit (1);
- }
-
- if (pthread_cancel (th) != 0)
- {
- puts ("error: pthread_cancel failed");
- exit (1);
- }
-
- int e = pthread_barrier_wait (&bar);
- if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
- {
- puts ("error: 1st barrier_wait failed");
- exit (1);
- }
-
- void *r;
- if (pthread_join (th, &r) != 0)
- {
- puts ("error: pthread_join failed");
- exit (1);
- }
-
- if (r != PTHREAD_CANCELED)
- {
- puts ("error: thread not canceled");
- exit (1);
- }
+ TEST_COMPARE (sem_init (&sem, 0, 1), 0);
+
+ pthread_t th = xpthread_create (NULL, tf, NULL);
+ xpthread_cancel (th);
+
+ xpthread_barrier_wait (&bar);
+
+ void *r = xpthread_join (th);
+
+ /* sem_wait is not a cancellation entrypoint. */
+ TEST_VERIFY (r == 0);
return 0;
}
-
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
+#include <support/test-driver.c>
@@ -26,6 +26,9 @@
#include <unistd.h>
#include <sys/time.h>
+#include <support/xthread.h>
+#include <support/timespec.h>
+#include <support/check.h>
static pthread_barrier_t bar;
static sem_t sem;
@@ -36,11 +39,7 @@ cleanup (void *arg)
{
static int ncall;
- if (++ncall != 1)
- {
- puts ("second call to cleanup");
- exit (1);
- }
+ TEST_VERIFY_EXIT (++ncall != 1);
}
@@ -49,23 +48,13 @@ tf (void *arg)
{
pthread_cleanup_push (cleanup, NULL);
- int e = pthread_barrier_wait (&bar);
- if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
- {
- puts ("error: tf: 1st barrier_wait failed");
- exit (1);
- }
-
- struct timeval tv;
- (void) gettimeofday (&tv, NULL);
-
- struct timespec ts;
- TIMEVAL_TO_TIMESPEC (&tv, &ts);
+ xpthread_barrier_wait (&bar);
/* Timeout in 5 seconds. */
- ts.tv_sec += 5;
+ const struct timespec ts = timespec_add (xclock_now (CLOCK_REALTIME),
+ make_timespec (0, 500000000));
- /* This call should block and be cancelable. */
+ /* This call should not be cancelable. */
sem_timedwait (&sem, &ts);
pthread_cleanup_pop (0);
@@ -77,57 +66,22 @@ tf (void *arg)
static int
do_test (void)
{
- pthread_t th;
-
- if (pthread_barrier_init (&bar, NULL, 2) != 0)
- {
- puts ("error: barrier_init failed");
- exit (1);
- }
-
- if (sem_init (&sem, 0, 1) != 0)
- {
- puts ("error: sem_init failed");
- exit (1);
- }
-
- if (pthread_create (&th, NULL, tf, NULL) != 0)
- {
- puts ("error: create failed");
- exit (1);
- }
-
- /* Check whether cancellation is honored even before sem_timedwait does
- anything. */
- if (pthread_cancel (th) != 0)
- {
- puts ("error: 1st cancel failed");
- exit (1);
- }
-
- int e = pthread_barrier_wait (&bar);
- if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
- {
- puts ("1st barrier_wait failed");
- exit (1);
- }
-
- void *r;
- if (pthread_join (th, &r) != 0)
- {
- puts ("join failed");
- exit (1);
- }
-
- if (r != PTHREAD_CANCELED)
- {
- puts ("thread not canceled");
- exit (1);
- }
+ xpthread_barrier_init (&bar, NULL, 2);
+
+ TEST_COMPARE (sem_init (&sem, 0, 1), 0);
+
+ pthread_t th = xpthread_create (NULL, tf, NULL);
+
+ xpthread_cancel (th);
+
+ xpthread_barrier_wait (&bar);
+
+ void *r = xpthread_join (th);
+
+ /* sem_timedwait is not a cancellation entrypoint. */
+ TEST_VERIFY (r == 0);
return 0;
}
-
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
+#include <support/test-driver.c>
@@ -243,3 +243,5 @@ GLIBC_2.28 tss_create F
GLIBC_2.28 tss_delete F
GLIBC_2.28 tss_get F
GLIBC_2.28 tss_set F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
@@ -255,6 +255,8 @@ GLIBC_2.3.4 pthread_attr_setaffinity_np F
GLIBC_2.3.4 pthread_getaffinity_np F
GLIBC_2.3.4 pthread_setaffinity_np F
GLIBC_2.3.4 pthread_setschedprio F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 pthread_mutex_consistent_np F
GLIBC_2.4 pthread_mutex_getprioceiling F
GLIBC_2.4 pthread_mutex_setprioceiling F
@@ -27,6 +27,8 @@ GLIBC_2.28 tss_create F
GLIBC_2.28 tss_delete F
GLIBC_2.28 tss_get F
GLIBC_2.28 tss_set F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 _IO_flockfile F
GLIBC_2.4 _IO_ftrylockfile F
GLIBC_2.4 _IO_funlockfile F
@@ -233,3 +233,5 @@ GLIBC_2.29 tss_set F
GLIBC_2.29 wait F
GLIBC_2.29 waitpid F
GLIBC_2.29 write F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
@@ -245,6 +245,8 @@ GLIBC_2.3.4 pthread_attr_setaffinity_np F
GLIBC_2.3.4 pthread_getaffinity_np F
GLIBC_2.3.4 pthread_setaffinity_np F
GLIBC_2.3.4 pthread_setschedprio F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 pthread_mutex_consistent_np F
GLIBC_2.4 pthread_mutex_getprioceiling F
GLIBC_2.4 pthread_mutex_setprioceiling F
@@ -253,6 +253,8 @@ GLIBC_2.3.4 pthread_attr_setaffinity_np F
GLIBC_2.3.4 pthread_getaffinity_np F
GLIBC_2.3.4 pthread_setaffinity_np F
GLIBC_2.3.4 pthread_setschedprio F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 pthread_mutex_consistent_np F
GLIBC_2.4 pthread_mutex_getprioceiling F
GLIBC_2.4 pthread_mutex_setprioceiling F
@@ -247,6 +247,8 @@ GLIBC_2.3.4 pthread_attr_setaffinity_np F
GLIBC_2.3.4 pthread_getaffinity_np F
GLIBC_2.3.4 pthread_setaffinity_np F
GLIBC_2.3.4 pthread_setschedprio F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 pthread_mutex_consistent_np F
GLIBC_2.4 pthread_mutex_getprioceiling F
GLIBC_2.4 pthread_mutex_setprioceiling F
@@ -27,6 +27,8 @@ GLIBC_2.28 tss_create F
GLIBC_2.28 tss_delete F
GLIBC_2.28 tss_get F
GLIBC_2.28 tss_set F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 _IO_flockfile F
GLIBC_2.4 _IO_ftrylockfile F
GLIBC_2.4 _IO_funlockfile F
@@ -253,6 +253,8 @@ GLIBC_2.3.4 pthread_attr_setaffinity_np F
GLIBC_2.3.4 pthread_getaffinity_np F
GLIBC_2.3.4 pthread_setaffinity_np F
GLIBC_2.3.4 pthread_setschedprio F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 pthread_mutex_consistent_np F
GLIBC_2.4 pthread_mutex_getprioceiling F
GLIBC_2.4 pthread_mutex_setprioceiling F
@@ -243,3 +243,5 @@ GLIBC_2.28 tss_create F
GLIBC_2.28 tss_delete F
GLIBC_2.28 tss_get F
GLIBC_2.28 tss_set F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
@@ -255,6 +255,8 @@ GLIBC_2.3.4 pthread_attr_setaffinity_np F
GLIBC_2.3.4 pthread_getaffinity_np F
GLIBC_2.3.4 pthread_setaffinity_np F
GLIBC_2.3.4 pthread_setschedprio F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 pthread_mutex_consistent_np F
GLIBC_2.4 pthread_mutex_getprioceiling F
GLIBC_2.4 pthread_mutex_setprioceiling F
@@ -255,6 +255,8 @@ GLIBC_2.3.4 pthread_attr_setaffinity_np F
GLIBC_2.3.4 pthread_getaffinity_np F
GLIBC_2.3.4 pthread_setaffinity_np F
GLIBC_2.3.4 pthread_setschedprio F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 pthread_mutex_consistent_np F
GLIBC_2.4 pthread_mutex_getprioceiling F
GLIBC_2.4 pthread_mutex_setprioceiling F
@@ -241,3 +241,5 @@ GLIBC_2.28 tss_create F
GLIBC_2.28 tss_delete F
GLIBC_2.28 tss_get F
GLIBC_2.28 tss_set F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
@@ -255,6 +255,8 @@ GLIBC_2.3.4 pthread_getaffinity_np F
GLIBC_2.3.4 pthread_setaffinity_np F
GLIBC_2.3.4 pthread_setschedprio F
GLIBC_2.3.4 siglongjmp F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 pthread_mutex_consistent_np F
GLIBC_2.4 pthread_mutex_getprioceiling F
GLIBC_2.4 pthread_mutex_setprioceiling F
@@ -246,6 +246,8 @@ GLIBC_2.3.4 pthread_getaffinity_np F
GLIBC_2.3.4 pthread_setaffinity_np F
GLIBC_2.3.4 pthread_setschedprio F
GLIBC_2.3.4 siglongjmp F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 pthread_mutex_consistent_np F
GLIBC_2.4 pthread_mutex_getprioceiling F
GLIBC_2.4 pthread_mutex_setprioceiling F
@@ -243,3 +243,5 @@ GLIBC_2.28 tss_create F
GLIBC_2.28 tss_delete F
GLIBC_2.28 tss_get F
GLIBC_2.28 tss_set F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
@@ -235,3 +235,5 @@ GLIBC_2.28 tss_create F
GLIBC_2.28 tss_delete F
GLIBC_2.28 tss_get F
GLIBC_2.28 tss_set F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
@@ -255,6 +255,8 @@ GLIBC_2.3.4 pthread_attr_setaffinity_np F
GLIBC_2.3.4 pthread_getaffinity_np F
GLIBC_2.3.4 pthread_setaffinity_np F
GLIBC_2.3.4 pthread_setschedprio F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 pthread_mutex_consistent_np F
GLIBC_2.4 pthread_mutex_getprioceiling F
GLIBC_2.4 pthread_mutex_setprioceiling F
@@ -247,6 +247,8 @@ GLIBC_2.3.4 pthread_attr_setaffinity_np F
GLIBC_2.3.4 pthread_getaffinity_np F
GLIBC_2.3.4 pthread_setaffinity_np F
GLIBC_2.3.4 pthread_setschedprio F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 pthread_mutex_consistent_np F
GLIBC_2.4 pthread_mutex_getprioceiling F
GLIBC_2.4 pthread_mutex_setprioceiling F
@@ -245,6 +245,8 @@ GLIBC_2.3.4 pthread_attr_setaffinity_np F
GLIBC_2.3.4 pthread_getaffinity_np F
GLIBC_2.3.4 pthread_setaffinity_np F
GLIBC_2.3.4 pthread_setschedprio F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 pthread_mutex_consistent_np F
GLIBC_2.4 pthread_mutex_getprioceiling F
GLIBC_2.4 pthread_mutex_setprioceiling F
@@ -255,6 +255,8 @@ GLIBC_2.3.4 pthread_attr_setaffinity_np F
GLIBC_2.3.4 pthread_getaffinity_np F
GLIBC_2.3.4 pthread_setaffinity_np F
GLIBC_2.3.4 pthread_setschedprio F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 pthread_mutex_consistent_np F
GLIBC_2.4 pthread_mutex_getprioceiling F
GLIBC_2.4 pthread_mutex_setprioceiling F
@@ -247,6 +247,8 @@ GLIBC_2.3.4 pthread_attr_setaffinity_np F
GLIBC_2.3.4 pthread_getaffinity_np F
GLIBC_2.3.4 pthread_setaffinity_np F
GLIBC_2.3.4 pthread_setschedprio F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 pthread_mutex_consistent_np F
GLIBC_2.4 pthread_mutex_getprioceiling F
GLIBC_2.4 pthread_mutex_setprioceiling F
@@ -245,6 +245,8 @@ GLIBC_2.3.4 pthread_attr_setaffinity_np F
GLIBC_2.3.4 pthread_getaffinity_np F
GLIBC_2.3.4 pthread_setaffinity_np F
GLIBC_2.3.4 pthread_setschedprio F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F
GLIBC_2.4 pthread_mutex_consistent_np F
GLIBC_2.4 pthread_mutex_getprioceiling F
GLIBC_2.4 pthread_mutex_setprioceiling F
@@ -243,3 +243,5 @@ GLIBC_2.28 tss_create F
GLIBC_2.28 tss_delete F
GLIBC_2.28 tss_get F
GLIBC_2.28 tss_set F
+GLIBC_2.30 sem_timedwait F
+GLIBC_2.30 sem_wait F