From b186c277caf08eafba1b4eba2840cac58d5131f1 Mon Sep 17 00:00:00 2001 From: Enrico Giordani Date: Wed, 25 Nov 2015 12:18:54 -0400 Subject: [PATCH] [Fix] FreeHeapBlock should check if the addr is in the redis heap. Since the forked process allocates the memory from the system heap, it must verify if the address is in the system heap or in the redis heap before freeing it. Changed dictRehash to NOOP when called by the forked process to avoid extra processing that is not required when the forked process is saving the dataset. --- src/Win32_Interop/Win32_QFork.cpp | 129 ++++++++++++++++-------------- src/Win32_Interop/Win32_QFork.h | 2 + src/dict.c | 6 ++ 3 files changed, 79 insertions(+), 58 deletions(-) diff --git a/src/Win32_Interop/Win32_QFork.cpp b/src/Win32_Interop/Win32_QFork.cpp index 15b102a8..a04fadb2 100644 --- a/src/Win32_Interop/Win32_QFork.cpp +++ b/src/Win32_Interop/Win32_QFork.cpp @@ -97,7 +97,6 @@ allocate a system paging file that will expand up to about (3.5 * physical). #define QFORK_MAIN_IMPL #include "Win32_QFork.h" - #include "Win32_QFork_impl.h" #include "Win32_SmartHandle.h" #include "Win32_Service.h" @@ -204,6 +203,7 @@ struct heapBlockInfo { struct QForkControl { LPVOID heapStart; + LPVOID heapEnd; int maxAvailableBlocks; int numMappedBlocks; int blockSearchStart; @@ -225,15 +225,16 @@ QForkControl* g_pQForkControl; HANDLE g_hQForkControlFileMap; HANDLE g_hForkedProcess = 0; int g_ChildExitCode = 0; // For child process -BOOL g_isForkedProcess; BOOL g_SentinelMode; - -/* The system heap is used instead of the system paging file heap if - * one of the following case is true: - * - Redis is running as a sentinel - * - the current instance is a forked (child) process - * - the persistence-available configuration flag value is 'no' */ -BOOL g_UseSystemHeap; +BOOL g_PersistenceDisabled; +/* If g_IsForkedProcess || g_PersistenceDisabled || g_SentinelMode is true + * memory is not allocated from the memory map heap, instead the system heap + * is used */ +BOOL g_BypassMemoryMapOnAlloc; +/* g_HasMemoryMappedHeap is true if g_PersistenceDisabled and g_SentinelMode + * are both false, so it is true for the parent process and the child process + * when persistence is available */ +BOOL g_HasMemoryMappedHeap; bool ReportSpecialSystemErrors(int error) { switch (error) @@ -453,7 +454,8 @@ BOOL QForkParentInit() { IFFAILTHROW(VirtualFree(pHigh, 0, MEM_RELEASE), "QForkMasterInit: VirtualFree failed."); // Need to adjust the heap start address to align on allocation granularity offset - g_pQForkControl->heapStart = (LPVOID) (((uint64_t) pHigh + cAllocationGranularity) - ((uint64_t) pHigh % cAllocationGranularity)); + g_pQForkControl->heapStart = (LPVOID) (((uintptr_t) pHigh + cAllocationGranularity) - ((uintptr_t) pHigh % cAllocationGranularity)); + g_pQForkControl->heapEnd = (LPVOID) ((uintptr_t) g_pQForkControl->heapStart + (g_pQForkControl->maxAvailableBlocks + 1) * cAllocationGranularity); // Reserve the heap memory that will be mapped on demand in AllocHeapBlock() for (int i = 0; i < g_pQForkControl->maxAvailableBlocks; i++) { @@ -497,21 +499,6 @@ BOOL QForkParentInit() { } StartupStatus QForkStartup() { - HANDLE QForkControlMemoryMapHandle = NULL; - DWORD PPID = 0; - - g_isForkedProcess = FALSE; - // TODO: consider moving the argument parsing into ParseCommandLineArguments() - - // Child command line looks like: --QFork [QForkControlMemoryMap handle] [parent pid] - if (g_argMap.find(cQFork) != g_argMap.end()) { - g_isForkedProcess = TRUE; - char* endPtr; - QForkControlMemoryMapHandle = (HANDLE) strtoul(g_argMap[cQFork].at(0).at(0).c_str(), &endPtr, 10); - char* end = NULL; - PPID = strtoul(g_argMap[cQFork].at(0).at(1).c_str(), &end, 10); - } - PERFORMANCE_INFORMATION perfinfo; perfinfo.cb = sizeof(PERFORMANCE_INFORMATION); if (FALSE == GetPerformanceInfo(&perfinfo, sizeof(PERFORMANCE_INFORMATION))) { @@ -521,8 +508,10 @@ StartupStatus QForkStartup() { } Globals::pageSize = perfinfo.PageSize; - if (g_isForkedProcess) { - g_UseSystemHeap = TRUE; + if (g_IsForkedProcess) { + // Child command line looks like: --QFork [QForkControlMemoryMap handle] [parent pid] + HANDLE QForkControlMemoryMapHandle = (HANDLE) strtoul(g_argMap[cQFork].at(0).at(0).c_str(), NULL, 10); + DWORD PPID = strtoul(g_argMap[cQFork].at(0).at(1).c_str(), NULL, 10); return QForkChildInit(QForkControlMemoryMapHandle, PPID) ? StartupStatus::ssCHILD_EXIT : StartupStatus::ssFAILED; } else { return QForkParentInit() ? StartupStatus::ssCONTINUE_AS_PARENT : StartupStatus::ssFAILED; @@ -935,7 +924,7 @@ HANDLE CreateBlockMap(int blockIndex) { #ifdef USE_DLMALLOC /* NOTE: The allocateHigh parameter is ignored in this implementation */ LPVOID AllocHeapBlock(size_t size, BOOL allocateHigh) { - if (g_UseSystemHeap) { + if (g_BypassMemoryMapOnAlloc) { return VirtualAlloc(NULL, size, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); } @@ -1001,7 +990,7 @@ LPVOID AllocHeapBlock(size_t size, BOOL allocateHigh) { #elif USE_JEMALLOC LPVOID AllocHeapBlock(LPVOID addr, size_t size, BOOL zero) { - if (g_UseSystemHeap) { + if (g_BypassMemoryMapOnAlloc) { return VirtualAlloc(addr, size, MEM_RESERVE | MEM_COMMIT, PAGE_READWRITE); } @@ -1068,18 +1057,32 @@ LPVOID AllocHeapBlock(LPVOID addr, size_t size, BOOL zero) { #endif BOOL FreeHeapBlock(LPVOID addr, size_t size) { - if (g_UseSystemHeap) { - return VirtualFree(addr, 0, MEM_RELEASE); - } - if (size == 0) { return FALSE; } - // TODO: check addr is in heap + // If g_HasMemoryMappedHeap is FALSE this can only be a system heap address + if (!g_HasMemoryMappedHeap) { + return VirtualFree(addr, 0, MEM_RELEASE); + } + + // Check if the address belongs to the memory map heap or to the system heap + BOOL addressInRedisHeap = (addr >= g_pQForkControl->heapStart && addr < g_pQForkControl->heapEnd); + + // g_BypassMemoryMapOnAlloc is true for the forked process, in this case + // we need to handle the address differently based on the heap that was + // used to allocate it. + if (g_BypassMemoryMapOnAlloc) { + if (!addressInRedisHeap) { + return VirtualFree(addr, 0, MEM_RELEASE); + } else { + redisLog(REDIS_DEBUG, "FreeHeapBlock: address in memory map heap 0x%p", addr); + } + } + // Check the address alignment and that belongs to the memory map heap size_t ptrDiff = reinterpret_cast(addr) - reinterpret_cast(g_pQForkControl->heapStart); - if (ptrDiff < 0 || (ptrDiff % cAllocationGranularity) != 0) { + if ((ptrDiff % cAllocationGranularity) != 0 || !addressInRedisHeap) { return FALSE; } @@ -1104,11 +1107,8 @@ BOOL FreeHeapBlock(LPVOID addr, size_t size) { } BOOL PurgePages(LPVOID addr, size_t length) { - if (g_UseSystemHeap) { - VirtualAlloc(addr, length, MEM_RESET, PAGE_READWRITE); - return TRUE; - } - + // VirtualAlloc is called for all cases regardless the value of + // g_BypassMemoryMapOnAlloc and g_HasMemoryMappedHeap VirtualAlloc(addr, length, MEM_RESET, PAGE_READWRITE); return TRUE; } @@ -1128,16 +1128,35 @@ void SetupLogging() { } } -extern "C" -{ - BOOL IsPersistenceAvailable() { - if (g_argMap.find(cPersistenceAvailable) != g_argMap.end()) { - return (g_argMap[cPersistenceAvailable].at(0).at(0) != cNo); - } else { - return TRUE; - } +BOOL IsPersistenceDisabled() { + if (g_argMap.find(cPersistenceAvailable) != g_argMap.end()) { + return (g_argMap[cPersistenceAvailable].at(0).at(0) == cNo); + } else { + return FALSE; + } +} + +BOOL IsForkedProcess() { + if (g_argMap.find(cQFork) != g_argMap.end()) { + return TRUE; + } else { + return FALSE; } +} + +void SetupQForkGlobals(int argc, char* argv[]) { + // To check sentinel mode we use the antirez code to avoid duplicating code + g_SentinelMode = checkForSentinelMode(argc, argv); + + g_IsForkedProcess = IsForkedProcess(); + g_PersistenceDisabled = IsPersistenceDisabled(); + g_BypassMemoryMapOnAlloc = g_IsForkedProcess || g_PersistenceDisabled || g_SentinelMode; + g_HasMemoryMappedHeap = !g_PersistenceDisabled && !g_SentinelMode; +} + +extern "C" +{ // The external main() is redefined as redis_main() by Win32_QFork.h. // The CRT will call this replacement main() before the previous main() // is invoked so that the QFork allocator can be setup prior to anything @@ -1146,6 +1165,7 @@ extern "C" try { InitTimeFunctions(); ParseCommandLineArguments(argc, argv); + SetupQForkGlobals(argc, argv); SetupLogging(); StackTraceInit(); InitThreadControl(); @@ -1194,13 +1214,6 @@ extern "C" return 0; } - BOOL persistenceAvailable = IsPersistenceAvailable(); - g_SentinelMode = checkForSentinelMode(argc, argv); - - if (g_SentinelMode == TRUE || persistenceAvailable == FALSE) { - g_UseSystemHeap = TRUE; - } - #ifdef USE_DLMALLOC DLMallocInit(); // Setup memory allocation scheme @@ -1220,7 +1233,9 @@ extern "C" #elif USE_JEMALLOC je_init(); #endif - if (persistenceAvailable == TRUE && g_SentinelMode == FALSE) { + if (g_PersistenceDisabled || g_SentinelMode) { + return redis_main(argc, argv); + } else { StartupStatus status = QForkStartup(); if (status == ssCONTINUE_AS_PARENT) { int retval = redis_main(argc, argv); @@ -1237,8 +1252,6 @@ extern "C" // Unexpected status return return 2; } - } else { - return redis_main(argc, argv); } } catch (system_error syserr) { diff --git a/src/Win32_Interop/Win32_QFork.h b/src/Win32_Interop/Win32_QFork.h index 273a8214..4b137671 100644 --- a/src/Win32_Interop/Win32_QFork.h +++ b/src/Win32_Interop/Win32_QFork.h @@ -29,6 +29,8 @@ extern "C" { #endif +BOOL g_IsForkedProcess; + typedef enum operationType { otINVALID = 0, otRDB = 1, diff --git a/src/dict.c b/src/dict.c index 6f620c6c..1f8ff206 100644 --- a/src/dict.c +++ b/src/dict.c @@ -36,6 +36,7 @@ #include "Win32_Interop/Win32_Portability.h" #include "Win32_Interop/Win32_Time.h" #include "Win32_Interop/win32fixes.h" +extern BOOL g_IsForkedProcess; #endif #include "fmacros.h" @@ -250,6 +251,11 @@ int dictExpand(dict *d,PORT_ULONG size) * will visit at max N*10 empty buckets in total, otherwise the amount of * work it does would be unbound and the function may block for a long time. */ int dictRehash(dict *d, int n) { + + // On Windows we choose not to execute the dict rehash since it's not + // necessary and it may have a performance impact. + WIN32_ONLY(if (g_IsForkedProcess) return 0;) + int empty_visits = n*10; /* Max number of empty buckets to visit. */ if (!dictIsRehashing(d)) return 0; -- GitLab