[libc-commits] [libc] 301db3d - [libc] add simplified tid cache (#101620)
via libc-commits
libc-commits at lists.llvm.org
Fri Aug 2 10:27:53 PDT 2024
Author: Schrodinger ZHU Yifan
Date: 2024-08-02T10:27:49-07:00
New Revision: 301db3dee53b5afbc9813c5fcd3cce25a0655f5f
URL: https://github.com/llvm/llvm-project/commit/301db3dee53b5afbc9813c5fcd3cce25a0655f5f
DIFF: https://github.com/llvm/llvm-project/commit/301db3dee53b5afbc9813c5fcd3cce25a0655f5f.diff
LOG: [libc] add simplified tid cache (#101620)
According to discussions on monthly meeting, we probably don't want to
cache `getpid` anymore. glibc removes their cache. bionic is hesitating
whether such cache is to be removed. `getpid` is async-signal-safe, so
we must make sure it always work.
However, for `gettid`, we have more freedom. Moreover, we are using
`gettid` to examine deadlock such that the performance penalty is not
negligible here. Thus, this patch is separated from previous patch to
provide only `tid` caching. It is much more simplified. Hopefully,
previous build issues can be resolved easily.
Added:
libc/src/__support/threads/identifier.h
libc/src/unistd/gettid.cpp
libc/src/unistd/gettid.h
Modified:
libc/config/linux/aarch64/entrypoints.txt
libc/config/linux/riscv/entrypoints.txt
libc/config/linux/x86_64/entrypoints.txt
libc/newhdrgen/yaml/unistd.yaml
libc/spec/posix.td
libc/src/__support/threads/CMakeLists.txt
libc/src/__support/threads/linux/CMakeLists.txt
libc/src/__support/threads/linux/rwlock.h
libc/src/unistd/CMakeLists.txt
libc/src/unistd/linux/CMakeLists.txt
libc/src/unistd/linux/fork.cpp
libc/test/integration/src/unistd/CMakeLists.txt
libc/test/integration/src/unistd/fork_test.cpp
Removed:
################################################################################
diff --git a/libc/config/linux/aarch64/entrypoints.txt b/libc/config/linux/aarch64/entrypoints.txt
index 2c449c49d912a..ebdaa0f6de7fd 100644
--- a/libc/config/linux/aarch64/entrypoints.txt
+++ b/libc/config/linux/aarch64/entrypoints.txt
@@ -299,6 +299,7 @@ set(TARGET_LIBC_ENTRYPOINTS
libc.src.unistd.geteuid
libc.src.unistd.getpid
libc.src.unistd.getppid
+ libc.src.unistd.gettid
libc.src.unistd.getuid
libc.src.unistd.isatty
libc.src.unistd.link
diff --git a/libc/config/linux/riscv/entrypoints.txt b/libc/config/linux/riscv/entrypoints.txt
index 771f169332fcb..29969fce6adf8 100644
--- a/libc/config/linux/riscv/entrypoints.txt
+++ b/libc/config/linux/riscv/entrypoints.txt
@@ -318,6 +318,7 @@ set(TARGET_LIBC_ENTRYPOINTS
libc.src.unistd.geteuid
libc.src.unistd.getpid
libc.src.unistd.getppid
+ libc.src.unistd.gettid
libc.src.unistd.getuid
libc.src.unistd.isatty
libc.src.unistd.link
diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt
index 044852b49c75f..2ed287dc8542e 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -318,6 +318,7 @@ set(TARGET_LIBC_ENTRYPOINTS
libc.src.unistd.geteuid
libc.src.unistd.getpid
libc.src.unistd.getppid
+ libc.src.unistd.gettid
libc.src.unistd.getuid
libc.src.unistd.isatty
libc.src.unistd.link
diff --git a/libc/newhdrgen/yaml/unistd.yaml b/libc/newhdrgen/yaml/unistd.yaml
index c698c6b1d64ef..42aecb2f31e5e 100644
--- a/libc/newhdrgen/yaml/unistd.yaml
+++ b/libc/newhdrgen/yaml/unistd.yaml
@@ -307,3 +307,9 @@ functions:
- type: const void *__restrict
- type: void *
- type: ssize_t
+ - name: gettid
+ standards:
+ - Linux
+ return_type: pid_t
+ arguments:
+ - type: void
diff --git a/libc/spec/posix.td b/libc/spec/posix.td
index 1b7e18e999600..0edf9080c712e 100644
--- a/libc/spec/posix.td
+++ b/libc/spec/posix.td
@@ -546,6 +546,11 @@ def POSIX : StandardSpec<"POSIX"> {
RetValSpec<PidT>,
[ArgSpec<VoidType>]
>,
+ FunctionSpec<
+ "gettid",
+ RetValSpec<PidT>,
+ [ArgSpec<VoidType>]
+ >,
FunctionSpec<
"getuid",
RetValSpec<UidT>,
diff --git a/libc/src/__support/threads/CMakeLists.txt b/libc/src/__support/threads/CMakeLists.txt
index d2e46b8e2574e..bd49bbb5ad2fe 100644
--- a/libc/src/__support/threads/CMakeLists.txt
+++ b/libc/src/__support/threads/CMakeLists.txt
@@ -89,3 +89,19 @@ if(TARGET libc.src.__support.threads.${LIBC_TARGET_OS}.CndVar)
.${LIBC_TARGET_OS}.CndVar
)
endif()
+
+if (LLVM_LIBC_FULL_BUILD)
+ set(identifier_dependency_on_thread libc.src.__support.threads.thread)
+endif()
+
+add_header_library(
+ identifier
+ HDRS
+ identifier.h
+ DEPENDS
+ libc.src.__support.OSUtil.osutil
+ libc.src.__support.common
+ libc.include.sys_syscall
+ libc.hdr.types.pid_t
+ ${identifier_dependency_on_thread}
+)
diff --git a/libc/src/__support/threads/identifier.h b/libc/src/__support/threads/identifier.h
new file mode 100644
index 0000000000000..5103c11f6eb86
--- /dev/null
+++ b/libc/src/__support/threads/identifier.h
@@ -0,0 +1,49 @@
+//===--- Thread Identifier Header --------------------------------*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC___SUPPORT_THREADS_IDENTIFIER_H
+#define LLVM_LIBC_SRC___SUPPORT_THREADS_IDENTIFIER_H
+
+#ifdef LIBC_FULL_BUILD
+#include "src/__support/threads/thread.h"
+#endif // LIBC_FULL_BUILD
+
+#include "hdr/types/pid_t.h"
+#include "src/__support/OSUtil/syscall.h"
+#include <sys/syscall.h>
+
+namespace LIBC_NAMESPACE_DECL {
+namespace internal {
+
+LIBC_INLINE pid_t *get_tid_cache() {
+#ifdef LIBC_FULL_BUILD
+ return &self.attrib->tid;
+#else
+ // in non-full build mode, we do not control the fork routine. Therefore,
+ // we do not cache tid at all.
+ return nullptr;
+#endif
+}
+
+LIBC_INLINE pid_t gettid() {
+ pid_t *cache = get_tid_cache();
+ if (LIBC_UNLIKELY(!cache || *cache <= 0))
+ return syscall_impl<pid_t>(SYS_gettid);
+ return *cache;
+}
+
+LIBC_INLINE void force_set_tid(pid_t tid) {
+ pid_t *cache = get_tid_cache();
+ if (cache)
+ *cache = tid;
+}
+
+} // namespace internal
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC___SUPPORT_THREADS_IDENTIFIER_H
diff --git a/libc/src/__support/threads/linux/CMakeLists.txt b/libc/src/__support/threads/linux/CMakeLists.txt
index 8b7971584e77e..c2f0ed0cb233d 100644
--- a/libc/src/__support/threads/linux/CMakeLists.txt
+++ b/libc/src/__support/threads/linux/CMakeLists.txt
@@ -55,6 +55,7 @@ add_header_library(
libc.src.__support.common
libc.src.__support.OSUtil.osutil
libc.src.__support.CPP.limits
+ libc.src.__support.threads.identifier
COMPILE_OPTIONS
-DLIBC_COPT_RWLOCK_DEFAULT_SPIN_COUNT=${LIBC_CONF_RWLOCK_DEFAULT_SPIN_COUNT}
${monotonicity_flags}
diff --git a/libc/src/__support/threads/linux/rwlock.h b/libc/src/__support/threads/linux/rwlock.h
index d2fb0ce1a3c08..57fcc7bb67a6a 100644
--- a/libc/src/__support/threads/linux/rwlock.h
+++ b/libc/src/__support/threads/linux/rwlock.h
@@ -19,6 +19,7 @@
#include "src/__support/macros/attributes.h"
#include "src/__support/macros/config.h"
#include "src/__support/macros/optimization.h"
+#include "src/__support/threads/identifier.h"
#include "src/__support/threads/linux/futex_utils.h"
#include "src/__support/threads/linux/futex_word.h"
#include "src/__support/threads/linux/raw_mutex.h"
@@ -336,8 +337,6 @@ class RwLock {
LIBC_INLINE Role get_preference() const {
return static_cast<Role>(preference);
}
- // TODO: use cached thread id once implemented.
- LIBC_INLINE static pid_t gettid() { return syscall_impl<pid_t>(SYS_gettid); }
template <Role role> LIBC_INLINE LockResult try_lock(RwState &old) {
if constexpr (role == Role::Reader) {
@@ -359,7 +358,7 @@ class RwLock {
if (LIBC_LIKELY(old.compare_exchange_weak_with(
state, old.set_writer_bit(), cpp::MemoryOrder::ACQUIRE,
cpp::MemoryOrder::RELAXED))) {
- writer_tid.store(gettid(), cpp::MemoryOrder::RELAXED);
+ writer_tid.store(internal::gettid(), cpp::MemoryOrder::RELAXED);
return LockResult::Success;
}
// Notice that old is updated by the compare_exchange_weak_with
@@ -394,7 +393,7 @@ class RwLock {
unsigned spin_count = LIBC_COPT_RWLOCK_DEFAULT_SPIN_COUNT) {
// Phase 1: deadlock detection.
// A deadlock happens if this is a RAW/WAW lock in the same thread.
- if (writer_tid.load(cpp::MemoryOrder::RELAXED) == gettid())
+ if (writer_tid.load(cpp::MemoryOrder::RELAXED) == internal::gettid())
return LockResult::Deadlock;
#if LIBC_COPT_TIMEOUT_ENSURE_MONOTONICITY
@@ -520,7 +519,7 @@ class RwLock {
if (old.has_active_writer()) {
// The lock is held by a writer.
// Check if we are the owner of the lock.
- if (writer_tid.load(cpp::MemoryOrder::RELAXED) != gettid())
+ if (writer_tid.load(cpp::MemoryOrder::RELAXED) != internal::gettid())
return LockResult::PermissionDenied;
// clear writer tid.
writer_tid.store(0, cpp::MemoryOrder::RELAXED);
diff --git a/libc/src/unistd/CMakeLists.txt b/libc/src/unistd/CMakeLists.txt
index ddafcd7c92f21..64e07725010b3 100644
--- a/libc/src/unistd/CMakeLists.txt
+++ b/libc/src/unistd/CMakeLists.txt
@@ -333,3 +333,13 @@ add_entrypoint_external(
add_entrypoint_external(
opterr
)
+
+add_entrypoint_object(
+ gettid
+ SRCS
+ gettid.cpp
+ HDRS
+ gettid.h
+ DEPENDS
+ libc.src.__support.threads.identifier
+)
diff --git a/libc/src/unistd/gettid.cpp b/libc/src/unistd/gettid.cpp
new file mode 100644
index 0000000000000..f3d87ef64b795
--- /dev/null
+++ b/libc/src/unistd/gettid.cpp
@@ -0,0 +1,17 @@
+//===-- Implementation file for gettid --------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/unistd/gettid.h"
+#include "src/__support/common.h"
+#include "src/__support/threads/identifier.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+LLVM_LIBC_FUNCTION(pid_t, gettid, ()) { return internal::gettid(); }
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/src/unistd/gettid.h b/libc/src/unistd/gettid.h
new file mode 100644
index 0000000000000..48f5d226638f7
--- /dev/null
+++ b/libc/src/unistd/gettid.h
@@ -0,0 +1,21 @@
+//===-- Implementation header for gettid ------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_UNISTD_GETTID_H
+#define LLVM_LIBC_SRC_UNISTD_GETTID_H
+
+#include "hdr/types/pid_t.h"
+#include "src/__support/common.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+pid_t gettid();
+
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC_UNISTD_GETTID_H
diff --git a/libc/src/unistd/linux/CMakeLists.txt b/libc/src/unistd/linux/CMakeLists.txt
index 7e733d7f002c3..9b0d752cefbd8 100644
--- a/libc/src/unistd/linux/CMakeLists.txt
+++ b/libc/src/unistd/linux/CMakeLists.txt
@@ -103,6 +103,7 @@ add_entrypoint_object(
libc.src.__support.OSUtil.osutil
libc.src.__support.threads.thread
libc.src.errno.errno
+ libc.src.__support.threads.identifier
)
add_entrypoint_object(
diff --git a/libc/src/unistd/linux/fork.cpp b/libc/src/unistd/linux/fork.cpp
index 7d47665b16d3f..8aa0477a15d58 100644
--- a/libc/src/unistd/linux/fork.cpp
+++ b/libc/src/unistd/linux/fork.cpp
@@ -12,6 +12,7 @@
#include "src/__support/common.h"
#include "src/__support/macros/config.h"
#include "src/__support/threads/fork_callbacks.h"
+#include "src/__support/threads/identifier.h"
#include "src/__support/threads/thread.h" // For thread self object
#include "src/errno/libc_errno.h"
@@ -25,19 +26,25 @@ namespace LIBC_NAMESPACE_DECL {
LLVM_LIBC_FUNCTION(pid_t, fork, (void)) {
invoke_prepare_callbacks();
+ pid_t parent_tid = internal::gettid();
+ // Invalidate parent's tid cache before forking. We cannot do this in child
+ // process because in the post-fork instruction windows, there may be a signal
+ // handler triggered which may get the wrong tid.
+ internal::force_set_tid(0);
#ifdef SYS_fork
- pid_t ret = LIBC_NAMESPACE::syscall_impl<pid_t>(SYS_fork);
+ pid_t ret = syscall_impl<pid_t>(SYS_fork);
#elif defined(SYS_clone)
- pid_t ret = LIBC_NAMESPACE::syscall_impl<pid_t>(SYS_clone, SIGCHLD, 0);
+ pid_t ret = syscall_impl<pid_t>(SYS_clone, SIGCHLD, 0);
#else
#error "fork and clone syscalls not available."
#endif
+
if (ret == 0) {
// Return value is 0 in the child process.
// The child is created with a single thread whose self object will be a
// copy of parent process' thread which called fork. So, we have to fix up
// the child process' self object with the new process' tid.
- self.attrib->tid = LIBC_NAMESPACE::syscall_impl<pid_t>(SYS_gettid);
+ internal::force_set_tid(syscall_impl<pid_t>(SYS_gettid));
invoke_child_callbacks();
return 0;
}
@@ -47,7 +54,8 @@ LLVM_LIBC_FUNCTION(pid_t, fork, (void)) {
libc_errno = static_cast<int>(-ret);
return -1;
}
-
+ // recover parent's tid.
+ internal::force_set_tid(parent_tid);
invoke_parent_callbacks();
return ret;
}
diff --git a/libc/test/integration/src/unistd/CMakeLists.txt b/libc/test/integration/src/unistd/CMakeLists.txt
index 3f18231209512..644f16fa46347 100644
--- a/libc/test/integration/src/unistd/CMakeLists.txt
+++ b/libc/test/integration/src/unistd/CMakeLists.txt
@@ -31,6 +31,10 @@ add_integration_test(
libc.src.sys.wait.wait4
libc.src.sys.wait.waitpid
libc.src.unistd.fork
+ libc.src.unistd.gettid
+ libc.src.__support.OSUtil.osutil
+ libc.src.stdlib.exit
+ libc.include.sys_syscall
)
if((${LIBC_TARGET_OS} STREQUAL "linux") AND (${LIBC_TARGET_ARCHITECTURE_IS_X86}))
diff --git a/libc/test/integration/src/unistd/fork_test.cpp b/libc/test/integration/src/unistd/fork_test.cpp
index 9c9213ed46316..87f314f305101 100644
--- a/libc/test/integration/src/unistd/fork_test.cpp
+++ b/libc/test/integration/src/unistd/fork_test.cpp
@@ -6,17 +6,21 @@
//
//===----------------------------------------------------------------------===//
+#include "src/__support/OSUtil/syscall.h"
#include "src/pthread/pthread_atfork.h"
#include "src/signal/raise.h"
+#include "src/stdlib/exit.h"
#include "src/sys/wait/wait.h"
#include "src/sys/wait/wait4.h"
#include "src/sys/wait/waitpid.h"
#include "src/unistd/fork.h"
+#include "src/unistd/gettid.h"
#include "test/IntegrationTest/test.h"
#include <errno.h>
#include <signal.h>
+#include <sys/syscall.h>
#include <sys/wait.h>
#include <unistd.h>
@@ -140,7 +144,24 @@ void fork_with_atfork_callbacks() {
ASSERT_NE(child, DONE);
}
+void gettid_test() {
+ // fork and verify tid is consistent with the syscall result.
+ int pid = LIBC_NAMESPACE::fork();
+ ASSERT_EQ(LIBC_NAMESPACE::gettid(),
+ LIBC_NAMESPACE::syscall_impl<pid_t>(SYS_gettid));
+ if (pid == 0)
+ LIBC_NAMESPACE::exit(0);
+ // make sure child process exits normally
+ int status;
+ pid_t cpid = LIBC_NAMESPACE::waitpid(pid, &status, 0);
+ ASSERT_TRUE(cpid > 0);
+ ASSERT_EQ(cpid, pid);
+ ASSERT_TRUE(WIFEXITED(status));
+ ASSERT_EQ(WEXITSTATUS(status), 0);
+}
+
TEST_MAIN(int argc, char **argv, char **envp) {
+ gettid_test();
fork_and_wait_normal_exit();
fork_and_wait4_normal_exit();
fork_and_waitpid_normal_exit();
More information about the libc-commits
mailing list