-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[llvm-exegesis] Add thread IDs to subprocess memory names #84451
[llvm-exegesis] Add thread IDs to subprocess memory names #84451
Conversation
This patch adds the thread ID to the subprocess memory shared memory names. This avoids conflicts for downstream consumers that might want to consume llvm-exegesis across multiple threads, which would otherwise run into conflicts due to the same PID running multiple instances.
@llvm/pr-subscribers-tools-llvm-exegesis Author: Aiden Grossman (boomanaiden154) ChangesThis patch adds the thread ID to the subprocess memory shared memory names. This avoids conflicts for downstream consumers that might want to consume llvm-exegesis across multiple threads, which would otherwise run into conflicts due to the same PID running multiple instances. Full diff: https://github.com/llvm/llvm-project/pull/84451.diff 2 Files Affected:
diff --git a/llvm/tools/llvm-exegesis/lib/SubprocessMemory.cpp b/llvm/tools/llvm-exegesis/lib/SubprocessMemory.cpp
index a49fa077257d00..28dc4488a2a004 100644
--- a/llvm/tools/llvm-exegesis/lib/SubprocessMemory.cpp
+++ b/llvm/tools/llvm-exegesis/lib/SubprocessMemory.cpp
@@ -14,6 +14,7 @@
#ifdef __linux__
#include <fcntl.h>
#include <sys/mman.h>
+#include <sys/syscall.h>
#include <unistd.h>
#endif
@@ -22,12 +23,21 @@ namespace exegesis {
#if defined(__linux__) && !defined(__ANDROID__)
+long getCurrentTID() {
+ // We're using the raw syscall here rather than the gettid() function provided
+ // by most libcs for compatibility as gettid() was only added to glibc in
+ // version 2.30.
+ return syscall(SYS_gettid);
+}
+
Error SubprocessMemory::initializeSubprocessMemory(pid_t ProcessID) {
// Add the PID to the shared memory name so that if we're running multiple
// processes at the same time, they won't interfere with each other.
// This comes up particularly often when running the exegesis tests with
- // llvm-lit
- std::string AuxiliaryMemoryName = "/auxmem" + std::to_string(ProcessID);
+ // llvm-lit. Additionally add the TID so that downstream consumers
+ // using multiple threads don't run into conflicts.
+ std::string AuxiliaryMemoryName = "/" + std::to_string(getCurrentTID()) +
+ "auxmem" + std::to_string(ProcessID);
int AuxiliaryMemoryFD = shm_open(AuxiliaryMemoryName.c_str(),
O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
if (AuxiliaryMemoryFD == -1)
@@ -47,7 +57,8 @@ Error SubprocessMemory::addMemoryDefinition(
pid_t ProcessPID) {
SharedMemoryNames.reserve(MemoryDefinitions.size());
for (auto &[Name, MemVal] : MemoryDefinitions) {
- std::string SharedMemoryName = "/" + std::to_string(ProcessPID) + "memdef" +
+ std::string SharedMemoryName = "/" + std::to_string(ProcessPID) + "t" +
+ std::to_string(getCurrentTID()) + "memdef" +
std::to_string(MemVal.Index);
SharedMemoryNames.push_back(SharedMemoryName);
int SharedMemoryFD =
diff --git a/llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp b/llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp
index c07ec188a602c4..7c23e7b7e9c5a5 100644
--- a/llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp
+++ b/llvm/unittests/tools/llvm-exegesis/X86/SubprocessMemoryTest.cpp
@@ -17,6 +17,7 @@
#include <endian.h>
#include <fcntl.h>
#include <sys/mman.h>
+#include <sys/syscall.h>
#include <unistd.h>
#endif // __linux__
@@ -49,7 +50,9 @@ class SubprocessMemoryTest : public X86TestBase {
std::string getSharedMemoryName(const unsigned TestNumber,
const unsigned DefinitionNumber) {
- return "/" + std::to_string(getSharedMemoryNumber(TestNumber)) + "memdef" +
+ long CurrentTID = syscall(SYS_gettid);
+ return "/" + std::to_string(getSharedMemoryNumber(TestNumber)) + "t" +
+ std::to_string(CurrentTID) + "memdef" +
std::to_string(DefinitionNumber);
}
|
This is aimed at enabling the use of multi threads within Gematria, where we want to do memory annotations and benchmarking in parallel to process a large number of snippets (on the order of hundreds of millions now) at a decent rate. There are some other things that additionally need to be added that I have planned for follow-up patches, including core pinning. This patch isn't particularly useful to upstream exegesis, but others, like core pinning, definitely would be. |
✅ With the latest revision this PR passed the C/C++ code formatter. |
…4451)" This reverts commit 6bbe8a2. This breaks building LLVM on macOS, failing with llvm/tools/llvm-exegesis/lib/SubprocessMemory.cpp:146:33: error: out-of-line definition of 'setupAuxiliaryMemoryInSubprocess' does not match any declaration in 'llvm::exegesis::SubprocessMemory' Expected<int> SubprocessMemory::setupAuxiliaryMemoryInSubprocess(
It looks like this breaks building at least on macOS, so I went ahead and revert the change for now in aefad27 This is the error I am seeing:
|
I also got the same error, and it also seems the pre-commit CI was failing with the same error: https://buildkite.com/llvm-project/github-pull-requests/builds/46349#018e31bf-f1a5-4c5e-86f3-47632f4abc15/414-12754. |
Thanks for bringing the issue to my attention and reverting in the meantime. I've relanded it in 8003f55 with a fix. |
…ames (#84451)"" This reverts commit 8003f55. This (and/or a related commit) was causing build failures on one of the buildbots that needs more investigation. More information is available at https://lab.llvm.org/buildbot/#/builders/178/builds/7015.
…4451)" This reverts commit 1fe9c41. This relands commit 6bbe8a2. This was causing build failures on one of the ARMv8 builders. Still not completely sure why, but relanding it to see if the failure pops up again. If it does, the plan is to fix forward by disabling tests on ARM temporarily as llvm-exegesis does not currently use SubprocessMemory on ARM.
…vm#84451)" This reverts commit 1fe9c41. This relands commit 6bbe8a2. This was causing build failures on one of the ARMv8 builders. Still not completely sure why, but relanding it to see if the failure pops up again. If it does, the plan is to fix forward by disabling tests on ARM temporarily as llvm-exegesis does not currently use SubprocessMemory on ARM.
When building for Android, I get this
Nvm I fixed by patching |
This should be fixed in a29e85d. If you/others care about that configuration though, you should stand up a buildbot so that we can address these breakages soon after they land rather than a month later. |
This patch adds the thread ID to the subprocess memory shared memory names. This avoids conflicts for downstream consumers that might want to consume llvm-exegesis across multiple threads, which would otherwise run into conflicts due to the same PID running multiple instances.