From 8994f1c1d641d5036d66392b56babadb607f3137 Mon Sep 17 00:00:00 2001 From: "Sv. Lockal" Date: Sun, 15 Dec 2024 17:50:23 +0000 Subject: [PATCH 1/3] Fix build issues for musl libc This fixes few issues, which allow to build and pass kfdtest on musl-based systems: 1) result of https://github.com/ROCm/ROCR-Runtime/commit/daad183bf8a3e1153a652b9e8fda06306b7c94ef#diff-681b7d40f71f20573413d5072e1c381fde6c02aed03a56307afcbe82cf3f5e5eR36-R37f - `extern int hsakmt_page_shift` is under `ifndef`, while it should not ``` /var/tmp/portage/dev-libs/roct-thunk-interface-6.3.0/work/ROCR-Runtime-rocm-6.3.0/libhsakmt/src/openclose.c: In function 'init_page_size': /var/tmp/portage/dev-libs/roct-thunk-interface-6.3.0/work/ROCR-Runtime-rocm-6.3.0/libhsakmt/src/openclose.c:117:9: error: 'hsakmt_page_size' undeclared (first use in this function); did you mean 'hsakmt_page_shift'? 117 | hsakmt_page_size = sysconf(_SC_PAGESIZE); | ^~~~~~~~~~~~~~~~ | hsakmt_page_shift ``` 2) warning of `PAGE_SIZE` macro redefinition (defined previously in `limits.h`) 3) no operator to compare 0 with std::nullptr_t (null is defined as nullptr_t, following https://eel.is/c++draft/support.types.nullptr) ``` In file included from /var/tmp/portage/dev-libs/roct-thunk-interface-6.3.0/work/ROCR-Runtime-rocm-6.3.0/libhsakmt/tests/kfdtest/src/KFDTestUtil.hpp:27, from /var/tmp/portage/dev-libs/roct-thunk-interface-6.3.0/work/ROCR-Runtime-rocm-6.3.0/libhsakmt/tests/kfdtest/src/BaseQueue.hpp:28, from /var/tmp/portage/dev-libs/roct-thunk-interface-6.3.0/work/ROCR-Runtime-rocm-6.3.0/libhsakmt/tests/kfdtest/src/SDMAQueue.hpp:27, from /var/tmp/portage/dev-libs/roct-thunk-interface-6.3.0/work/ROCR-Runtime-rocm-6.3.0/libhsakmt/tests/kfdtest/src/KFDTestUtilQueue.cpp:28: /var/tmp/portage/dev-libs/roct-thunk-interface-6.3.0/work/ROCR-Runtime-rocm-6.3.0/libhsakmt/tests/kfdtest/gtest-1.6.0/gtest/gtest.h: In instantiation of 'testing::AssertionResult testing::internal::CmpHelperNE(const char*, const char*, const T1&, const T2&) [with T1 = long unsigned int; T2 = std::nullptr_t]': /var/tmp/portage/dev-libs/roct-thunk-interface-6.3.0/work/ROCR-Runtime-rocm-6.3.0/libhsakmt/tests/kfdtest/src/KFDTestUtilQueue.cpp:60:29: required from here 19072 | GTEST_ASSERT_(pred_format(#v1, #v2, v1, v2),\ | ^ /var/tmp/portage/dev-libs/roct-thunk-interface-6.3.0/work/ROCR-Runtime-rocm-6.3.0/libhsakmt/tests/kfdtest/gtest-1.6.0/gtest/gtest.h:18573:28: error: invalid operands of types 'const long unsigned int' and 'std::nullptr_t' to binary 'operator!=' 18573 | GTEST_IMPL_CMP_HELPER_(NE, !=); | ^ /var/tmp/portage/dev-libs/roct-thunk-interface-6.3.0/work/ROCR-Runtime-rocm-6.3.0/libhsakmt/tests/kfdtest/gtest-1.6.0/gtest/gtest.h:18558:12: note: in definition of macro 'GTEST_IMPL_CMP_HELPER_' 18558 | if (val1 op val2) {\ | ^~ ``` Closes: https://github.com/ROCm/ROCR-Runtime/issues/267 Signed-off-by: Sv. Lockal --- libhsakmt/src/libhsakmt.h | 5 +++-- libhsakmt/tests/kfdtest/src/KFDTestUtilQueue.cpp | 6 +++--- libhsakmt/tests/kfdtest/src/OSWrapper.hpp | 4 ++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/libhsakmt/src/libhsakmt.h b/libhsakmt/src/libhsakmt.h index 63ef17d96..70a9eed4e 100644 --- a/libhsakmt/src/libhsakmt.h +++ b/libhsakmt/src/libhsakmt.h @@ -64,14 +64,15 @@ extern HsaVersionInfo hsakmt_kfd_version_info; do { if ((minor) > hsakmt_kfd_version_info.KernelInterfaceMinorVersion)\ return HSAKMT_STATUS_NOT_SUPPORTED; } while (0) +extern int hsakmt_page_size; +extern int hsakmt_page_shift; + /* Might be defined in limits.h on platforms where it is constant (used by musl) */ /* See also: https://pubs.opengroup.org/onlinepubs/7908799/xsh/limits.h.html */ #ifndef PAGE_SIZE -extern int hsakmt_page_size; #define PAGE_SIZE hsakmt_page_size #endif #ifndef PAGE_SHIFT -extern int hsakmt_page_shift; #define PAGE_SHIFT hsakmt_page_shift #endif diff --git a/libhsakmt/tests/kfdtest/src/KFDTestUtilQueue.cpp b/libhsakmt/tests/kfdtest/src/KFDTestUtilQueue.cpp index 8f9b857a7..47f66e8ef 100644 --- a/libhsakmt/tests/kfdtest/src/KFDTestUtilQueue.cpp +++ b/libhsakmt/tests/kfdtest/src/KFDTestUtilQueue.cpp @@ -57,13 +57,13 @@ class AsyncMPSQ { void PlacePacketOnNode(PacketList &packetList, int node, TSPattern tsp); /* Run the packets placed on nodes and return immediately.*/ - void Submit(void) { ASSERT_NE((HSAuint64)m_queue, NULL); m_queue->SubmitPacket(); } + void Submit(void) { ASSERT_NE(m_queue, NULL); m_queue->SubmitPacket(); } /* Return only when all packets are consumed. * If there is any packet issues some IO operations, wait these IO to complete too. */ void Wait(void) { - ASSERT_NE((HSAuint64)m_queue, NULL); + ASSERT_NE(m_queue, NULL); m_queue->Wait4PacketConsumption(m_event, std::max((unsigned int)6000, g_TestTimeOut)); } @@ -244,7 +244,7 @@ HSAuint64 AsyncMPSQ::Report(int indexOfPacket, HSAuint64 &begin, HSAuint64 &end) if (m_ts_pattern == HEAD_TAIL) indexOfPacket = 0; - EXPECT_NE((HSAuint64)m_ts, NULL) + EXPECT_NE(m_ts, NULL) << " Error " << ++error << ": No timestamp buf!" << std::endl; /* m_ts_count is equal to packets count + 1, see PlacePacketOnNode(). * So the max index of a packet is m_ts_count - 2. diff --git a/libhsakmt/tests/kfdtest/src/OSWrapper.hpp b/libhsakmt/tests/kfdtest/src/OSWrapper.hpp index 6c3b24f1d..ce4bfecba 100644 --- a/libhsakmt/tests/kfdtest/src/OSWrapper.hpp +++ b/libhsakmt/tests/kfdtest/src/OSWrapper.hpp @@ -23,6 +23,8 @@ #include #include +#include +#include #include #include "KFDTestFlags.hpp" @@ -33,10 +35,8 @@ #ifndef PAGE_SIZE #define PAGE_SIZE (1<<12) -#define PAGE_SHIFT (12) #endif #ifndef PAGE_SHIFT -#define PAGE_SIZE (1<<12) #define PAGE_SHIFT (12) #endif From 363805b6d2656967aa389f45f9e387d6c591df06 Mon Sep 17 00:00:00 2001 From: "Sv. Lockal" Date: Sun, 15 Dec 2024 21:29:13 +0000 Subject: [PATCH 2/3] Fix musl compilation for hsa-runtime This commit applies a similar change, that was reverted in 1cee8656df62e13f83dc2dfda4f13ad5be2e8747, with major issues fixed. 1) reverted commit checked if `pthread_attr_setaffinity_np` symbol exists, but this check did not work (and effectively disabled better path), because this symbol requires `#define _GNU_SOURCE` (see https://man7.org/linux/man-pages/man3/pthread_attr_setaffinity_np.3.html) 2) non-reverted part implied that `pthread_rwlockattr_setkind` function replaces `pthread_rwlockattr_setkind_np`, which is incorrect. This function is non-portable, serves as a hint and has no alternatives. Signed-off-by: Sv. Lockal --- runtime/hsa-ext-finalize/CMakeLists.txt | 12 ++++++++++ runtime/hsa-runtime/CMakeLists.txt | 12 ++++++++++ .../hsa-runtime/core/util/lnx/os_linux.cpp | 22 +++++++++++++------ 3 files changed, 39 insertions(+), 7 deletions(-) diff --git a/runtime/hsa-ext-finalize/CMakeLists.txt b/runtime/hsa-ext-finalize/CMakeLists.txt index 6c6dbdcba..f18b09912 100755 --- a/runtime/hsa-ext-finalize/CMakeLists.txt +++ b/runtime/hsa-ext-finalize/CMakeLists.txt @@ -101,6 +101,18 @@ if( NOT DEFINED OPEN_SOURCE_DIR ) set ( OPEN_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/.." ) endif() +## Check for _GNU_SOURCE pthread extensions +set(CMAKE_REQUIRED_DEFINITIONS -D_GNU_SOURCE) +CHECK_SYMBOL_EXISTS ( "pthread_attr_setaffinity_np" "pthread.h" HAVE_PTHREAD_ATTR_SETAFFINITY_NP ) +CHECK_SYMBOL_EXISTS ( "pthread_rwlockattr_setkind_np" "pthread.h" HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP ) +unset(CMAKE_REQUIRED_DEFINITIONS) +if ( HAVE_PTHREAD_ATTR_SETAFFINITY_NP ) + target_compile_definitions(${CORE_RUNTIME_TARGET} PRIVATE HAVE_PTHREAD_ATTR_SETAFFINITY_NP ) +endif() +if ( HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP ) + target_compile_definitions(${CORE_RUNTIME_TARGET} PRIVATE HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP ) +endif() + ## ------------------------- Linux Compiler and Linker options ------------------------- set ( CMAKE_CXX_FLAGS "-std=c++11 " ) diff --git a/runtime/hsa-runtime/CMakeLists.txt b/runtime/hsa-runtime/CMakeLists.txt index fbbcaf032..03a36a50c 100644 --- a/runtime/hsa-runtime/CMakeLists.txt +++ b/runtime/hsa-runtime/CMakeLists.txt @@ -109,6 +109,18 @@ if ( HAVE_MEMFD_CREATE ) target_compile_definitions(${CORE_RUNTIME_TARGET} PRIVATE HAVE_MEMFD_CREATE ) endif() +## Check for _GNU_SOURCE pthread extensions +set(CMAKE_REQUIRED_DEFINITIONS -D_GNU_SOURCE) +CHECK_SYMBOL_EXISTS ( "pthread_attr_setaffinity_np" "pthread.h" HAVE_PTHREAD_ATTR_SETAFFINITY_NP ) +CHECK_SYMBOL_EXISTS ( "pthread_rwlockattr_setkind_np" "pthread.h" HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP ) +unset(CMAKE_REQUIRED_DEFINITIONS) +if ( HAVE_PTHREAD_ATTR_SETAFFINITY_NP ) + target_compile_definitions(${CORE_RUNTIME_TARGET} PRIVATE HAVE_PTHREAD_ATTR_SETAFFINITY_NP ) +endif() +if ( HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP ) + target_compile_definitions(${CORE_RUNTIME_TARGET} PRIVATE HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP ) +endif() + ## Set include directories for ROCr runtime target_include_directories( ${CORE_RUNTIME_TARGET} PUBLIC diff --git a/runtime/hsa-runtime/core/util/lnx/os_linux.cpp b/runtime/hsa-runtime/core/util/lnx/os_linux.cpp index 72e189be2..04c627a76 100644 --- a/runtime/hsa-runtime/core/util/lnx/os_linux.cpp +++ b/runtime/hsa-runtime/core/util/lnx/os_linux.cpp @@ -135,12 +135,14 @@ class os_thread { for (int i = 0; i < cores; i++) { CPU_SET_S(i, CPU_ALLOC_SIZE(cores), cpuset); } +#ifdef HAVE_PTHREAD_ATTR_SETAFFINITY_NP err = pthread_attr_setaffinity_np(&attrib, CPU_ALLOC_SIZE(cores), cpuset); CPU_FREE(cpuset); if (err != 0) { fprintf(stderr, "pthread_setaffinity_np failed: %s\n", strerror(err)); return; } +#endif } do { @@ -162,6 +164,18 @@ class os_thread { return; } } while (stackSize < 20 * 1024 * 1024); + +#ifndef HAVE_PTHREAD_ATTR_SETAFFINITY_NP + if (cores && cpuset) { + err = pthread_setaffinity_np(thread, CPU_ALLOC_SIZE(cores), cpuset); + CPU_FREE(cpuset); + if (err != 0) { + fprintf(stderr, "pthread_setaffinity_np failed: %s\n", strerror(err)); + thread = 0; + return; + } + } +#endif } os_thread(os_thread&& rhs) { @@ -661,18 +675,12 @@ SharedMutex CreateSharedMutex() { return nullptr; } -#ifdef __GLIBC__ +#ifdef HAVE_PTHREAD_RWLOCKATTR_SETKIND_NP err = pthread_rwlockattr_setkind_np(&attrib, PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP); if (err != 0) { fprintf(stderr, "Set rw lock attribute failure: %s\n", strerror(err)); return nullptr; } -#else - err = pthread_rwlockattr_setkind(&attrib, PTHREAD_RWLOCK_PREFER_WRITER_NONRECURSIVE_NP); - if (err != 0) { - fprintf(stderr, "Set rw lock attribute failure: %s\n", strerror(err)); - return nullptr; - } #endif pthread_rwlock_t* lock = new pthread_rwlock_t; From 09d789c721b55bb6605967277ae6f5074b13aff4 Mon Sep 17 00:00:00 2001 From: "Sv. Lockal" Date: Sun, 15 Dec 2024 22:05:43 +0000 Subject: [PATCH 3/3] Fix comparisons of pointers with nulls The macro NULL sometimes is defined as 0, sometimes as nullptr_t-typed value (following [support.types.nullptr]), so to compare with pointers without warnings nullptr should be used --- libhsakmt/tests/kfdtest/src/KFDTestUtilQueue.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/libhsakmt/tests/kfdtest/src/KFDTestUtilQueue.cpp b/libhsakmt/tests/kfdtest/src/KFDTestUtilQueue.cpp index 47f66e8ef..aa709f3ab 100644 --- a/libhsakmt/tests/kfdtest/src/KFDTestUtilQueue.cpp +++ b/libhsakmt/tests/kfdtest/src/KFDTestUtilQueue.cpp @@ -57,13 +57,13 @@ class AsyncMPSQ { void PlacePacketOnNode(PacketList &packetList, int node, TSPattern tsp); /* Run the packets placed on nodes and return immediately.*/ - void Submit(void) { ASSERT_NE(m_queue, NULL); m_queue->SubmitPacket(); } + void Submit(void) { ASSERT_NE(m_queue, nullptr); m_queue->SubmitPacket(); } /* Return only when all packets are consumed. * If there is any packet issues some IO operations, wait these IO to complete too. */ void Wait(void) { - ASSERT_NE(m_queue, NULL); + ASSERT_NE(m_queue, nullptr); m_queue->Wait4PacketConsumption(m_event, std::max((unsigned int)6000, g_TestTimeOut)); } @@ -244,7 +244,7 @@ HSAuint64 AsyncMPSQ::Report(int indexOfPacket, HSAuint64 &begin, HSAuint64 &end) if (m_ts_pattern == HEAD_TAIL) indexOfPacket = 0; - EXPECT_NE(m_ts, NULL) + EXPECT_NE(m_ts, nullptr) << " Error " << ++error << ": No timestamp buf!" << std::endl; /* m_ts_count is equal to packets count + 1, see PlacePacketOnNode(). * So the max index of a packet is m_ts_count - 2.