diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 1abfe5bac..7b1ccf114 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -112,7 +112,7 @@ jobs: -D CMAKE_CXX_COMPILER=clang++ - run: cmake --build build - run: cmake --install build --prefix /tmp - - run: ctest --parallel --output-on-failure -E UnknownFunction --test-dir build/ + - run: ctest --parallel --output-on-failure -E "UnknownFunction|GetUnknownPhysicalDeviceFunctionLoop|GetUnknownDeviceFunctionWhileCreatingDevices" --test-dir build/ linux-32: needs: codegen @@ -198,7 +198,7 @@ jobs: # https://gitlab.kitware.com/cmake/cmake/-/issues/25317 PKG_CONFIG_PATH: /usr/lib/i386-linux-gnu/pkgconfig - run: cmake --build build - - run: ctest --parallel --output-on-failure -E UnknownFunction --test-dir build/ + - run: ctest --parallel --output-on-failure -E "UnknownFunction|GetUnknownPhysicalDeviceFunctionLoop|GetUnknownDeviceFunctionWhileCreatingDevices" --test-dir build/ linux-arm: needs: codegen @@ -310,7 +310,7 @@ jobs: -A ${{ matrix.arch }} ` -D BUILD_WERROR=ON - run: cmake --build build/ --config Release - - run: ctest --parallel --output-on-failure -C Release -E UnknownFunction --test-dir build/ + - run: ctest --parallel --output-on-failure -C Release -E "UnknownFunction|GetUnknownPhysicalDeviceFunctionLoop|GetUnknownDeviceFunctionWhileCreatingDevices" --test-dir build/ windows_arm: # Native Windows on Arm: covers the arm64 and arm64ec MARMASM paths the x64 runners can't. @@ -531,7 +531,7 @@ jobs: env: LDFLAGS: -Wl,-fatal_warnings - run: cmake --build build --config Release - - run: ctest --parallel --output-on-failure --build-config Release -E UnknownFunction --test-dir build/ + - run: ctest --parallel --output-on-failure --build-config Release -E "UnknownFunction|GetUnknownPhysicalDeviceFunctionLoop|GetUnknownDeviceFunctionWhileCreatingDevices" --test-dir build/ - run: cmake --install build --config Release --prefix /tmp - name: Verify Universal Binary if: ${{ matrix.static == 'OFF' }} @@ -568,7 +568,7 @@ jobs: env: LDFLAGS: -Wl,-fatal_warnings - run: cmake --build build --config Release - - run: ctest --parallel --output-on-failure --build-config Release -E UnknownFunction --test-dir build/ + - run: ctest --parallel --output-on-failure --build-config Release -E "UnknownFunction|GetUnknownPhysicalDeviceFunctionLoop|GetUnknownDeviceFunctionWhileCreatingDevices" --test-dir build/ - run: cmake --install build --config Release --prefix /tmp - name: Verify Universal Binary if: ${{ matrix.static == 'OFF' }} diff --git a/loader/unknown_function_handling.c b/loader/unknown_function_handling.c index 23e75fac8..09f2e57d5 100644 --- a/loader/unknown_function_handling.c +++ b/loader/unknown_function_handling.c @@ -63,6 +63,7 @@ void loader_free_phys_dev_ext_table(struct loader_instance *inst) { (void)inst; #else #include "allocation.h" +#include "loader.h" #include "log.h" // Forward declarations @@ -210,12 +211,22 @@ void *loader_dev_ext_gpa_impl(struct loader_instance *inst, const char *funcName return out_function; } +// Main interface functions. Both hold loader_lock while registering a function: the implementation appends to the +// instance's unknown function name table and walks the driver and logical device lists, all of which +// vkCreateDevice/vkDestroyDevice/vkEnumeratePhysicalDevices mutate under that lock, while vkGetInstanceProcAddr takes +// no lock of its own. loader_lock is recursive, so callers which already hold it are unaffected. void *loader_dev_ext_gpa_tramp(struct loader_instance *inst, const char *funcName) { - return loader_dev_ext_gpa_impl(inst, funcName, true); + loader_platform_thread_lock_mutex(&loader_lock); + void *addr = loader_dev_ext_gpa_impl(inst, funcName, true); + loader_platform_thread_unlock_mutex(&loader_lock); + return addr; } void *loader_dev_ext_gpa_term(struct loader_instance *inst, const char *funcName) { - return loader_dev_ext_gpa_impl(inst, funcName, false); + loader_platform_thread_lock_mutex(&loader_lock); + void *addr = loader_dev_ext_gpa_impl(inst, funcName, false); + loader_platform_thread_unlock_mutex(&loader_lock); + return addr; } // Physical Device function handling @@ -367,12 +378,19 @@ void *loader_phys_dev_ext_gpa_impl(struct loader_instance *inst, const char *fun } return loader_get_phys_dev_ext_termin(new_function_index); } -// Main interface functions, makes it clear whether it is getting a terminator or trampoline +// Main interface functions, makes it clear whether it is getting a terminator or trampoline. Both hold loader_lock for +// the same reason as the device variants above. void *loader_phys_dev_ext_gpa_tramp(struct loader_instance *inst, const char *funcName) { - return loader_phys_dev_ext_gpa_impl(inst, funcName, true); + loader_platform_thread_lock_mutex(&loader_lock); + void *addr = loader_phys_dev_ext_gpa_impl(inst, funcName, true); + loader_platform_thread_unlock_mutex(&loader_lock); + return addr; } void *loader_phys_dev_ext_gpa_term(struct loader_instance *inst, const char *funcName) { - return loader_phys_dev_ext_gpa_impl(inst, funcName, false); + loader_platform_thread_lock_mutex(&loader_lock); + void *addr = loader_phys_dev_ext_gpa_impl(inst, funcName, false); + loader_platform_thread_unlock_mutex(&loader_lock); + return addr; } #endif diff --git a/tests/loader_threading_tests.cpp b/tests/loader_threading_tests.cpp index 499cfae55..65206ccb4 100644 --- a/tests/loader_threading_tests.cpp +++ b/tests/loader_threading_tests.cpp @@ -199,3 +199,87 @@ TEST(Threading, SurfaceCreateDestroyLoop) { surface_threads[i].join(); } } + +// The driver reports these through vk_icdGetPhysicalDeviceProcAddr, so vkGetInstanceProcAddr has to append each one to +// the loader's instance wide unknown function tables before it can hand back a trampoline. +VKAPI_ATTR uint32_t VKAPI_CALL test_unknown_phys_dev_function(VkPhysicalDevice, uint32_t foo) { return foo; } + +void get_unknown_function_loop(InstWrapper* inst, std::vector const* func_names) { + for (auto const& name : *func_names) { + PFN_vkVoidFunction func = inst->load(name.c_str()); + ASSERT_NE(func, nullptr); + } +} + +TEST(Threading, GetUnknownPhysicalDeviceFunctionLoop) { + // Capped so that thread_count * funcs_per_thread stays under MAX_NUM_UNKNOWN_EXTS on a machine with many cores + uint32_t thread_count = std::thread::hardware_concurrency(); + if (thread_count < 2) thread_count = 2; + if (thread_count > 8) thread_count = 8; + const uint32_t funcs_per_thread = 16; + + FrameworkEnvironment env{FrameworkSettings{}.set_log_filter("")}; + auto& phys_dev = env.add_icd(TEST_ICD_PATH_VERSION_2_EXPORT_ICD_GPDPA).add_and_get_physical_device({}); + + // Each thread queries its own set of names, so every query appends a new entry rather than finding an existing one + std::vector> func_names{thread_count}; + for (uint32_t i = 0; i < thread_count; i++) { + for (uint32_t j = 0; j < funcs_per_thread; j++) { + func_names[i].push_back("vkNotRealFuncTEST_" + std::to_string(i) + "_" + std::to_string(j)); + phys_dev.custom_physical_device_functions.push_back( + VulkanFunction{func_names[i].back(), to_vkVoidFunction(test_unknown_phys_dev_function)}); + } + } + + InstWrapper inst{env.vulkan_functions}; + inst.CheckCreate(); + + std::vector function_query_threads; + for (uint32_t i = 0; i < thread_count; i++) { + function_query_threads.emplace_back(get_unknown_function_loop, &inst, &func_names[i]); + } + for (uint32_t i = 0; i < thread_count; i++) { + function_query_threads[i].join(); + } +} + +// Reported through the driver's vkGetInstanceProcAddr, which puts them on the unknown device function path. Registering +// one walks every driver's logical device list, which vkCreateDevice and vkDestroyDevice are editing in the other threads. +VKAPI_ATTR uint32_t VKAPI_CALL test_unknown_device_function(VkDevice, uint32_t foo) { return foo; } + +void create_destroy_device_only_loop(InstWrapper* inst, uint32_t num_loops) { + for (uint32_t i = 0; i < num_loops; i++) { + DeviceWrapper dev{*inst}; + dev.CheckCreate(inst->GetPhysDev()); + } +} + +TEST(Threading, GetUnknownDeviceFunctionWhileCreatingDevices) { + const uint32_t thread_count = 4; + const uint32_t funcs_per_thread = 16; + const uint32_t num_loops_create_destroy_device = 50; + + FrameworkEnvironment env{FrameworkSettings{}.set_log_filter("")}; + auto& phys_dev = env.add_icd(TEST_ICD_PATH_VERSION_2_EXPORT_ICD_GPDPA).add_and_get_physical_device({}); + + std::vector> func_names{thread_count}; + for (uint32_t i = 0; i < thread_count; i++) { + for (uint32_t j = 0; j < funcs_per_thread; j++) { + func_names[i].push_back("vkNotRealDeviceFuncTEST_" + std::to_string(i) + "_" + std::to_string(j)); + phys_dev.known_device_functions.push_back( + VulkanFunction{func_names[i].back(), to_vkVoidFunction(test_unknown_device_function)}); + } + } + + InstWrapper inst{env.vulkan_functions}; + inst.CheckCreate(); + + std::vector threads; + for (uint32_t i = 0; i < thread_count; i++) { + threads.emplace_back(get_unknown_function_loop, &inst, &func_names[i]); + threads.emplace_back(create_destroy_device_only_loop, &inst, num_loops_create_destroy_device); + } + for (auto& thread : threads) { + thread.join(); + } +}