[libc] add simplified tid cache #101620
Merged
[libc] add simplified tid cache #101620
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-libc Author: Schrodinger ZHU Yifan (SchrodingerZhu) ChangesAccording to discussions on monthly meeting, we probably don't want to cache However, for Full diff: https://github.com/llvm/llvm-project/pull/101620.diff 16 Files Affected:
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..36a7e3eef2d11
--- /dev/null
+++ b/libc/src/__support/threads/identifier.h
@@ -0,0 +1,52 @@
+//===--- 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 of 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))
+ return syscall_impl<pid_t>(SYS_gettid);
+ if (LIBC_UNLIKELY(*cache <= 0))
+ *cache = 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..c41ef24975715 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"
@@ -32,6 +33,12 @@ LLVM_LIBC_FUNCTION(pid_t, fork, (void)) {
#else
#error "fork and clone syscalls not available."
#endif
+ 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);
+
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
@@ -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..d63ae2055edf1 100644
--- a/libc/test/integration/src/unistd/CMakeLists.txt
+++ b/libc/test/integration/src/unistd/CMakeLists.txt
@@ -31,6 +31,9 @@ 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.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..46f58a21e3e95 100644
--- a/libc/test/integration/src/unistd/fork_test.cpp
+++ b/libc/test/integration/src/unistd/fork_test.cpp
@@ -6,17 +6,20 @@
//
//===----------------------------------------------------------------------===//
+#include "src/__support/OSUtil/syscall.h"
#include "src/pthread/pthread_atfork.h"
#include "src/signal/raise.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,6 +143,20 @@ 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));
+ // 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_FALSE(WIFEXITED(status));
+ ASSERT_TRUE(WTERMSIG(status) == SIGUSR1);
+}
+
TEST_MAIN(int argc, char **argv, char **envp) {
fork_and_wait_normal_exit();
fork_and_wait4_normal_exit();
|
SchrodingerZhu
force-pushed
the
libc/tid-cache-rework
branch
from
433b778 to
8e426cd
Compare
lntue
reviewed
Aug 2, 2024
lntue
approved these changes
Aug 2, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/43/builds/3564 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/104/builds/3623 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/93/builds/3469 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/147/builds/3462 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/78/builds/3303 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/188/builds/2414 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/4969 Here is the relevant piece of the build log for the reference: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
According to discussions on monthly meeting, we probably don't want to cache
getpidanymore. glibc removes their cache. bionic is hesitating whether such cache is to be removed.getpidis async-signal-safe, so we must make sure it always work.However, for
gettid, we have more freedom. Moreover, we are usinggettidto examine deadlock such that the performance penalty is not negligible here. Thus, this patch is separated from previous patch to provide onlytidcaching. It is much more simplified. Hopefully, previous build issues can be resolved easily.