[06/15] nptl: Use exit_lock when accessing TID on pthread_getaffinity_np
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
Also return ESRCH if the thread is already terminated at the time of
the call. This is slight better than returning the calling thread
affinity (current behaviour), since the thread lifetime is defined.
Checked on x86_64-linux-gnu.
---
nptl/pthread_getaffinity.c | 28 +++++++++------
sysdeps/pthread/Makefile | 1 +
sysdeps/pthread/tst-pthread-exited.c | 51 ++++++++++++++++++++++++++++
3 files changed, 69 insertions(+), 11 deletions(-)
create mode 100644 sysdeps/pthread/tst-pthread-exited.c
Comments
* Adhemerval Zanella via Libc-alpha:
> Also return ESRCH if the thread is already terminated at the time of
> the call. This is slight better than returning the calling thread
> affinity (current behaviour), since the thread lifetime is defined.
I still think this should produce an empty affinity set instead of ESRCH
because POSIX means something different for ESRCH (thread ID invalid,
something that we cannot detect in the glibc implementation).
Thanks,
Florian
Hi!
> > Also return ESRCH if the thread is already terminated at the time of
> > the call. This is slight better than returning the calling thread
> > affinity (current behaviour), since the thread lifetime is defined.
>
> I still think this should produce an empty affinity set instead of ESRCH
> because POSIX means something different for ESRCH (thread ID invalid,
> something that we cannot detect in the glibc implementation).
The RATIONALE for most pthread_{get,set}sched*() functions has:
If an implementation detects use of a thread ID after the end of its
lifetime, it is recommended that the function should fail and report an
[ESRCH] error.
See for example:
https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/
* Cyril Hrubis:
> Hi!
>> > Also return ESRCH if the thread is already terminated at the time of
>> > the call. This is slight better than returning the calling thread
>> > affinity (current behaviour), since the thread lifetime is defined.
>>
>> I still think this should produce an empty affinity set instead of ESRCH
>> because POSIX means something different for ESRCH (thread ID invalid,
>> something that we cannot detect in the glibc implementation).
>
> The RATIONALE for most pthread_{get,set}sched*() functions has:
>
> If an implementation detects use of a thread ID after the end of its
> lifetime, it is recommended that the function should fail and report an
> [ESRCH] error.
>
> See for example:
>
> https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/
Exactly. If the thread has exited, but is still joinable, the thread
ID lifetime has not ended, so we should not return ESRCH.
On 04/10/2021 16:27, Florian Weimer wrote:
> * Cyril Hrubis:
>
>> Hi!
>>>> Also return ESRCH if the thread is already terminated at the time of
>>>> the call. This is slight better than returning the calling thread
>>>> affinity (current behaviour), since the thread lifetime is defined.
>>>
>>> I still think this should produce an empty affinity set instead of ESRCH
>>> because POSIX means something different for ESRCH (thread ID invalid,
>>> something that we cannot detect in the glibc implementation).
>>
>> The RATIONALE for most pthread_{get,set}sched*() functions has:
>>
>> If an implementation detects use of a thread ID after the end of its
>> lifetime, it is recommended that the function should fail and report an
>> [ESRCH] error.
>>
>> See for example:
>>
>> https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/
>
> Exactly. If the thread has exited, but is still joinable, the thread
> ID lifetime has not ended, so we should not return ESRCH.
>
But in this case the information can not be queried from the kernel,
any information that glibc returns will not make sense (even an
empty affinity mask). We can try to cache the pthread_setaffinity_np()
value, but I really want to avoid such complexity and the extra space
required in struct pthread.
I think this is QoI to indicate the caller such information can not
be obtained, instead of returning an bogus value.
Hi!
> >> The RATIONALE for most pthread_{get,set}sched*() functions has:
> >>
> >> If an implementation detects use of a thread ID after the end of its
> >> lifetime, it is recommended that the function should fail and report an
> >> [ESRCH] error.
> >>
> >> See for example:
> >>
> >> https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/
> >
> > Exactly. If the thread has exited, but is still joinable, the thread
> > ID lifetime has not ended, so we should not return ESRCH.
> >
>
> But in this case the information can not be queried from the kernel,
> any information that glibc returns will not make sense (even an
> empty affinity mask). We can try to cache the pthread_setaffinity_np()
> value, but I really want to avoid such complexity and the extra space
> required in struct pthread.
>
> I think this is QoI to indicate the caller such information can not
> be obtained, instead of returning an bogus value.
I guess that if we want to return an error it should be something
different from ESRCH in order not to confuse, however there does not
seem to be a good match for this condition as far as I can tell.
On 05/10/2021 04:35, Cyril Hrubis wrote:
> Hi!
>>>> The RATIONALE for most pthread_{get,set}sched*() functions has:
>>>>
>>>> If an implementation detects use of a thread ID after the end of its
>>>> lifetime, it is recommended that the function should fail and report an
>>>> [ESRCH] error.
>>>>
>>>> See for example:
>>>>
>>>> https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/
>>>
>>> Exactly. If the thread has exited, but is still joinable, the thread
>>> ID lifetime has not ended, so we should not return ESRCH.
>>>
>>
>> But in this case the information can not be queried from the kernel,
>> any information that glibc returns will not make sense (even an
>> empty affinity mask). We can try to cache the pthread_setaffinity_np()
>> value, but I really want to avoid such complexity and the extra space
>> required in struct pthread.
>>
>> I think this is QoI to indicate the caller such information can not
>> be obtained, instead of returning an bogus value.
>
> I guess that if we want to return an error it should be something
> different from ESRCH in order not to confuse, however there does not
> seem to be a good match for this condition as far as I can tell.
>
Maybe return EINVAL then, but returning an failure would still tie
its semantic to check for pthread_t validity. So the question is
whether would be better to no tie the pthread_setaffinity_np() (or
any other interface that requires the pthread tid valid value)
to check for pthread lifetime and return a bogus value (with possible
side-effects) or return an error to make it clear to caller.
* Adhemerval Zanella:
> On 04/10/2021 16:27, Florian Weimer wrote:
>> * Cyril Hrubis:
>>
>>> Hi!
>>>>> Also return ESRCH if the thread is already terminated at the time of
>>>>> the call. This is slight better than returning the calling thread
>>>>> affinity (current behaviour), since the thread lifetime is defined.
>>>>
>>>> I still think this should produce an empty affinity set instead of ESRCH
>>>> because POSIX means something different for ESRCH (thread ID invalid,
>>>> something that we cannot detect in the glibc implementation).
>>>
>>> The RATIONALE for most pthread_{get,set}sched*() functions has:
>>>
>>> If an implementation detects use of a thread ID after the end of its
>>> lifetime, it is recommended that the function should fail and report an
>>> [ESRCH] error.
>>>
>>> See for example:
>>>
>>> https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/
>>
>> Exactly. If the thread has exited, but is still joinable, the thread
>> ID lifetime has not ended, so we should not return ESRCH.
>>
>
> But in this case the information can not be queried from the kernel,
> any information that glibc returns will not make sense (even an
> empty affinity mask). We can try to cache the pthread_setaffinity_np()
> value, but I really want to avoid such complexity and the extra space
> required in struct pthread.
Nah, I agree that caching this information in user space does not make
sense at this time.
I don't see much of a problem with an empty affinity mask.
> I think this is QoI to indicate the caller such information can not
> be obtained, instead of returning an bogus value.
Sure, but ESRCH seems to be the wrong error code, and some language in
POSIX strongly suggests that future POSIX versions are going to be
more strict about the cases in which it can be used. I do not want
applications to check for an error code that is encountered in most
cases only after undefined behavior has occurred.
@@ -15,25 +15,31 @@
License along with the GNU C Library; if not, see
<https://www.gnu.org/licenses/>. */
-#include <errno.h>
-#include <limits.h>
+#include <libc-lock.h>
#include <pthreadP.h>
-#include <string.h>
-#include <sysdep.h>
-#include <sys/param.h>
-#include <sys/types.h>
#include <shlib-compat.h>
int
__pthread_getaffinity_np (pthread_t th, size_t cpusetsize, cpu_set_t *cpuset)
{
- const struct pthread *pd = (const struct pthread *) th;
+ struct pthread *pd = (struct pthread *) th;
- int res = INTERNAL_SYSCALL_CALL (sched_getaffinity, pd->tid,
- MIN (INT_MAX, cpusetsize), cpuset);
- if (INTERNAL_SYSCALL_ERROR_P (res))
- return INTERNAL_SYSCALL_ERRNO (res);
+ /* Block all signals, as required by pd->exit_lock. */
+ sigset_t old_mask;
+ __libc_signal_block_all (&old_mask);
+ __libc_lock_lock (pd->exit_lock);
+
+ int res = pd->tid == 0
+ ? -ESRCH
+ : INTERNAL_SYSCALL_CALL (sched_getaffinity, pd->tid,
+ MIN (INT_MAX, cpusetsize), cpuset);
+
+ __libc_lock_unlock (pd->exit_lock);
+ __libc_signal_restore_set (&old_mask);
+
+ if (res < 0)
+ return -res;
/* Clean the rest of the memory the kernel didn't do. */
memset ((char *) cpuset + res, '\0', cpusetsize - res);
@@ -123,6 +123,7 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
tst-pthread_cancel-select-loop \
tst-pthread_kill-exited \
tst-pthread_kill-exiting \
+ tst-pthread-exited
# tests
tests-time64 := \
new file mode 100644
@@ -0,0 +1,51 @@
+/* Test pthread interface which should return ESRCH when issues along
+ with a terminated pthread_t.
+ Copyright (C) 2021 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <errno.h>
+#include <signal.h>
+#include <stddef.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/xthread.h>
+
+static void *
+noop_thread (void *closure)
+{
+ return NULL;
+}
+
+static int
+do_test (void)
+{
+ pthread_t thr = xpthread_create (NULL, noop_thread, NULL);
+
+ support_wait_for_thread_exit ();
+
+ {
+ cpu_set_t cpuset;
+ int r = pthread_getaffinity_np (thr, sizeof (cpuset), &cpuset);
+ TEST_COMPARE (r, ESRCH);
+ }
+
+ xpthread_join (thr);
+
+ return 0;
+}
+
+#include <support/test-driver.c>