[libc-commits] [libc] [libc] add simplified tid cache (PR #101620)
Schrodinger ZHU Yifan via libc-commits
libc-commits at lists.llvm.org
Fri Aug 2 10:00:35 PDT 2024
https://github.com/SchrodingerZhu updated https://github.com/llvm/llvm-project/pull/101620
>From 8e426cda8f617fb208e833fc8f25384885e8a8fb Mon Sep 17 00:00:00 2001
From: Schrodinger ZHU Yifan <i at zhuyi.fan>
Date: Thu, 1 Aug 2024 23:34:18 -0700
Subject: [PATCH 1/3] [libc] add simplified tid cache
---
libc/config/linux/aarch64/entrypoints.txt | 1 +
libc/config/linux/riscv/entrypoints.txt | 1 +
libc/config/linux/x86_64/entrypoints.txt | 1 +
libc/newhdrgen/yaml/unistd.yaml | 6 +++
libc/spec/posix.td | 5 ++
libc/src/__support/threads/CMakeLists.txt | 16 ++++++
libc/src/__support/threads/identifier.h | 52 +++++++++++++++++++
.../__support/threads/linux/CMakeLists.txt | 1 +
libc/src/__support/threads/linux/rwlock.h | 9 ++--
libc/src/unistd/CMakeLists.txt | 10 ++++
libc/src/unistd/gettid.cpp | 17 ++++++
libc/src/unistd/gettid.h | 21 ++++++++
libc/src/unistd/linux/CMakeLists.txt | 1 +
libc/src/unistd/linux/fork.cpp | 10 +++-
.../integration/src/unistd/CMakeLists.txt | 4 ++
.../test/integration/src/unistd/fork_test.cpp | 21 ++++++++
16 files changed, 170 insertions(+), 6 deletions(-)
create mode 100644 libc/src/__support/threads/identifier.h
create mode 100644 libc/src/unistd/gettid.cpp
create mode 100644 libc/src/unistd/gettid.h
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..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();
>From 0f2e1a680e6669d722be9fc4dfb9838216519999 Mon Sep 17 00:00:00 2001
From: Yifan Zhu <yifzhu at nvidia.com>
Date: Fri, 2 Aug 2024 09:41:36 -0700
Subject: [PATCH 2/3] address CR
---
libc/src/__support/threads/identifier.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/libc/src/__support/threads/identifier.h b/libc/src/__support/threads/identifier.h
index 36a7e3eef2d11..a30b044475dda 100644
--- a/libc/src/__support/threads/identifier.h
+++ b/libc/src/__support/threads/identifier.h
@@ -1,5 +1,4 @@
-//===--- Thread Identifier Header ---------------------------------*- C++
-//-*-===//
+//===--- 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.
>From e115c65a49257272d634a1fdb31ed9fd71daf504 Mon Sep 17 00:00:00 2001
From: Yifan Zhu <yifzhu at nvidia.com>
Date: Fri, 2 Aug 2024 10:00:21 -0700
Subject: [PATCH 3/3] fix invalidation position
---
libc/src/unistd/linux/fork.cpp | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/libc/src/unistd/linux/fork.cpp b/libc/src/unistd/linux/fork.cpp
index c41ef24975715..f1c4394ac2f86 100644
--- a/libc/src/unistd/linux/fork.cpp
+++ b/libc/src/unistd/linux/fork.cpp
@@ -26,6 +26,11 @@ 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);
#elif defined(SYS_clone)
@@ -33,11 +38,6 @@ 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.
More information about the libc-commits
mailing list