From 287ec26dde9e9769112c72ae7417849a31c4ff9f Mon Sep 17 00:00:00 2001 From: Ricardo Garcia Date: Fri, 10 Mar 2023 16:07:14 +0100 Subject: [PATCH] Fix race condition while initializing shader cache No test results should be affected by this change. Without this patch, I can often (not always) reproduce a crash when running vk-build-programs. The crash happens during the first seconds of the program running, while initializing shader cache variables and using them for the first time. With this patch, I can no longer make vk-build-programs crash. Components: Vulkan VK-GL-CTS issue: 4269 Change-Id: I61db5e9869849813e9bb451ddd4126ca091077da --- external/vulkancts/framework/vulkan/vkPrograms.cpp | 135 ++++++++++++--------- 1 file changed, 77 insertions(+), 58 deletions(-) diff --git a/external/vulkancts/framework/vulkan/vkPrograms.cpp b/external/vulkancts/framework/vulkan/vkPrograms.cpp index 1653ded..2cfb97e 100644 --- a/external/vulkancts/framework/vulkan/vkPrograms.cpp +++ b/external/vulkancts/framework/vulkan/vkPrograms.cpp @@ -39,6 +39,7 @@ #include "tcuCommandLine.hpp" #include +#include namespace vk { @@ -217,9 +218,8 @@ private: // Each cache node takes 4 * 4 = 16 bytes; 1M items takes 16M memory. const deUint32 cacheMaxItems = 1024 * 1024; -cacheMutex* cacheFileMutex = 0; -bool cacheFileFirstRun = true; -deUint32* cacheMempool = 0; +cacheMutex* cacheFileMutex = nullptr; +deUint32* cacheMempool = nullptr; #ifndef DISABLE_SHADERCACHE_IPC ipc_sharedmemory cacheIPCMemory; #endif @@ -344,77 +344,96 @@ void shaderCacheFirstRunCheck (const tcu::CommandLine& commandLine) { bool first = true; - if (!cacheFileFirstRun) - return; + // We need to solve two problems here: + // 1) The cache and cache mutex only have to be initialized once by the first thread that arrives here. + // 2) We must prevent other threads from exiting early from this function thinking they don't have to initialize the cache and + // cache mutex, only to try to lock the cache mutex while the first thread is still initializing it. To prevent this, we must + // hold an initialization mutex (declared below) while initializing the cache and cache mutex, making other threads wait. - cacheFileFirstRun = false; + // Used to check and set cacheFileFirstRun. We make it static, and C++11 guarantees it will only be initialized once. + static std::mutex cacheFileFirstRunMutex; + static bool cacheFileFirstRun = true; -#ifndef DISABLE_SHADERCACHE_IPC - if (commandLine.isShaderCacheIPCEnabled()) - { - // IPC path, allocate shared mutex and shared memory - cacheFileMutex = new cacheMutexIPC; - cacheFileMutex->lock(); - ipc_mem_init(&cacheIPCMemory, "cts_shadercache_memory", sizeof(deUint32) * (cacheMaxItems * 4 + 1)); - if (ipc_mem_open_existing(&cacheIPCMemory) != 0) - { - ipc_mem_create(&cacheIPCMemory); - cacheMempool = (deUint32*)ipc_mem_access(&cacheIPCMemory); - cacheMempool[0] = 0; - } - else - { - cacheMempool = (deUint32*)ipc_mem_access(&cacheIPCMemory); - first = false; - } - atexit(shaderCacheCleanIPC); - } - else -#endif - { - // Non-IPC path, allocate local mutex and memory - cacheFileMutex = new cacheMutex; - cacheFileMutex->lock(); - cacheMempool = new deUint32[cacheMaxItems * 4 + 1]; - cacheMempool[0] = 0; + // Is cacheFileFirstRun true for this thread? + bool needInit = false; - atexit(shaderCacheClean); + // Check cacheFileFirstRun only while holding the mutex, and hold it while initializing the cache. + const std::lock_guard lock(cacheFileFirstRunMutex); + if (cacheFileFirstRun) + { + needInit = true; + cacheFileFirstRun = false; } - if (first) + if (needInit) { - if (commandLine.isShaderCacheTruncateEnabled()) +#ifndef DISABLE_SHADERCACHE_IPC + if (commandLine.isShaderCacheIPCEnabled()) { - // Open file with "w" access to truncate it - FILE* f = fopen(commandLine.getShaderCacheFilename(), "wb"); - if (f) - fclose(f); + // IPC path, allocate shared mutex and shared memory + cacheFileMutex = new cacheMutexIPC; + cacheFileMutex->lock(); + ipc_mem_init(&cacheIPCMemory, "cts_shadercache_memory", sizeof(deUint32) * (cacheMaxItems * 4 + 1)); + if (ipc_mem_open_existing(&cacheIPCMemory) != 0) + { + ipc_mem_create(&cacheIPCMemory); + cacheMempool = (deUint32*)ipc_mem_access(&cacheIPCMemory); + cacheMempool[0] = 0; + } + else + { + cacheMempool = (deUint32*)ipc_mem_access(&cacheIPCMemory); + first = false; + } + atexit(shaderCacheCleanIPC); } else +#endif { - // Parse chunked shader cache file for hashes and offsets - FILE* file = fopen(commandLine.getShaderCacheFilename(), "rb"); - int count = 0; - if (file) + // Non-IPC path, allocate local mutex and memory + cacheFileMutex = new cacheMutex; + cacheFileMutex->lock(); + cacheMempool = new deUint32[cacheMaxItems * 4 + 1]; + cacheMempool[0] = 0; + + atexit(shaderCacheClean); + } + + if (first) + { + if (commandLine.isShaderCacheTruncateEnabled()) { - deUint32 chunksize = 0; - deUint32 hash = 0; - deUint32 offset = 0; - bool ok = true; - while (ok) + // Open file with "w" access to truncate it + FILE* f = fopen(commandLine.getShaderCacheFilename(), "wb"); + if (f) + fclose(f); + } + else + { + // Parse chunked shader cache file for hashes and offsets + FILE* file = fopen(commandLine.getShaderCacheFilename(), "rb"); + int count = 0; + if (file) { - offset = (deUint32)ftell(file); - if (ok) ok = fread(&chunksize, 1, 4, file) == 4; - if (ok) ok = fread(&hash, 1, 4, file) == 4; - if (ok) cacheInsert(hash, offset); - if (ok) ok = fseek(file, offset + chunksize, SEEK_SET) == 0; - count++; + deUint32 chunksize = 0; + deUint32 hash = 0; + deUint32 offset = 0; + bool ok = true; + while (ok) + { + offset = (deUint32)ftell(file); + if (ok) ok = fread(&chunksize, 1, 4, file) == 4; + if (ok) ok = fread(&hash, 1, 4, file) == 4; + if (ok) cacheInsert(hash, offset); + if (ok) ok = fseek(file, offset + chunksize, SEEK_SET) == 0; + count++; + } + fclose(file); } - fclose(file); } } + cacheFileMutex->unlock(); } - cacheFileMutex->unlock(); } std::string intToString (deUint32 integer) -- 2.7.4