From cd5b665d879c09d510912964457a38ccd01f477c Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Mon, 4 Dec 2023 10:45:50 +0000 Subject: [PATCH 1/8] CV test of Reduce use of madvise DODUMP/DONTDUMP * When reserving memory, do not mark as DONTDUMP if the memory will be committed immediately afterwards. * When committing memory that has only just been allocated, do not mark as DODUMP (as this is the default). * When resetting memory, only call madvise once. --- src/coreclr/gc/unix/gcenv.unix.cpp | 105 ++++++++++++++++------------ src/coreclr/pal/src/map/virtual.cpp | 18 +++-- 2 files changed, 74 insertions(+), 49 deletions(-) diff --git a/src/coreclr/gc/unix/gcenv.unix.cpp b/src/coreclr/gc/unix/gcenv.unix.cpp index 3ce1ddec98e3e6..add8bcc8bcec60 100644 --- a/src/coreclr/gc/unix/gcenv.unix.cpp +++ b/src/coreclr/gc/unix/gcenv.unix.cpp @@ -515,12 +515,13 @@ void GCToOSInterface::YieldThread(uint32_t switchCount) // Reserve virtual memory range. // Parameters: -// size - size of the virtual memory range -// alignment - requested memory alignment, 0 means no specific alignment requested -// flags - flags to control special settings like write watching +// size - size of the virtual memory range +// alignment - requested memory alignment, 0 means no specific alignment requested +// flags - flags to control special settings like write watching +// committing - memory will be comitted // Return: // Starting virtual address of the reserved range -static void* VirtualReserveInner(size_t size, size_t alignment, uint32_t flags, uint32_t hugePagesFlag = 0) +static void* VirtualReserveInner(size_t size, size_t alignment, uint32_t flags, uint32_t hugePagesFlag, bool committing) { assert(!(flags & VirtualReserveFlags::WriteWatch) && "WriteWatch not supported on Unix"); if (alignment < OS_PAGE_SIZE) @@ -550,8 +551,11 @@ static void* VirtualReserveInner(size_t size, size_t alignment, uint32_t flags, pRetVal = pAlignedRetVal; #ifdef MADV_DONTDUMP - // Do not include reserved memory in coredump. - madvise(pRetVal, size, MADV_DONTDUMP); + // Do not include reserved uncommitted memory in coredump. + if (!committing) + { + madvise(pRetVal, size, MADV_DONTDUMP); + } #endif return pRetVal; } @@ -569,7 +573,7 @@ static void* VirtualReserveInner(size_t size, size_t alignment, uint32_t flags, // Starting virtual address of the reserved range void* GCToOSInterface::VirtualReserve(size_t size, size_t alignment, uint32_t flags, uint16_t node) { - return VirtualReserveInner(size, alignment, flags); + return VirtualReserveInner(size, alignment, flags, 0, false); } // Release virtual memory range previously reserved using VirtualReserve @@ -585,44 +589,21 @@ bool GCToOSInterface::VirtualRelease(void* address, size_t size) return (ret == 0); } -// Commit virtual memory range. -// Parameters: -// size - size of the virtual memory range -// Return: -// Starting virtual address of the committed range -void* GCToOSInterface::VirtualReserveAndCommitLargePages(size_t size, uint16_t node) -{ -#if HAVE_MAP_HUGETLB - uint32_t largePagesFlag = MAP_HUGETLB; -#elif HAVE_VM_FLAGS_SUPERPAGE_SIZE_ANY - uint32_t largePagesFlag = VM_FLAGS_SUPERPAGE_SIZE_ANY; -#else - uint32_t largePagesFlag = 0; -#endif - - void* pRetVal = VirtualReserveInner(size, OS_PAGE_SIZE, 0, largePagesFlag); - if (VirtualCommit(pRetVal, size, node)) - { - return pRetVal; - } - - return nullptr; -} - // Commit virtual memory range. It must be part of a range reserved using VirtualReserve. // Parameters: -// address - starting virtual address -// size - size of the virtual memory range +// address - starting virtual address +// size - size of the virtual memory range +// newMemory - memory has been newly allocated // Return: // true if it has succeeded, false if it has failed -bool GCToOSInterface::VirtualCommit(void* address, size_t size, uint16_t node) +static bool VirtualCommitInner(void* address, size_t size, uint16_t node, bool newMemory) { bool success = mprotect(address, size, PROT_WRITE | PROT_READ) == 0; #ifdef MADV_DODUMP - if (success) + if (success && !newMemory) { - // Include committed memory in coredump. + // Include committed memory in coredump. New memory is included by default. madvise(address, size, MADV_DODUMP); } #endif @@ -650,6 +631,41 @@ bool GCToOSInterface::VirtualCommit(void* address, size_t size, uint16_t node) return success; } +// Commit virtual memory range. It must be part of a range reserved using VirtualReserve. +// Parameters: +// address - starting virtual address +// size - size of the virtual memory range +// Return: +// true if it has succeeded, false if it has failed +bool GCToOSInterface::VirtualCommit(void* address, size_t size, uint16_t node) +{ + return VirtualCommitInner(address, size, node, false); +} + +// Commit virtual memory range. +// Parameters: +// size - size of the virtual memory range +// Return: +// Starting virtual address of the committed range +void* GCToOSInterface::VirtualReserveAndCommitLargePages(size_t size, uint16_t node) +{ +#if HAVE_MAP_HUGETLB + uint32_t largePagesFlag = MAP_HUGETLB; +#elif HAVE_VM_FLAGS_SUPERPAGE_SIZE_ANY + uint32_t largePagesFlag = VM_FLAGS_SUPERPAGE_SIZE_ANY; +#else + uint32_t largePagesFlag = 0; +#endif + + void* pRetVal = VirtualReserveInner(size, OS_PAGE_SIZE, 0, largePagesFlag, true); + if (VirtualCommitInner(pRetVal, size, node, true)) + { + return pRetVal; + } + + return nullptr; +} + // Decomit virtual memory range. // Parameters: // address - starting virtual address @@ -687,11 +703,18 @@ bool GCToOSInterface::VirtualDecommit(void* address, size_t size) bool GCToOSInterface::VirtualReset(void * address, size_t size, bool unlock) { int st; + bool madviseFlags = MADV_FREE; + +#ifdef MADV_DONTDUMP + // Do not include reset memory in coredump. + madviseFlags |= MADV_DONTDUMP; +#endif + #if HAVE_MADV_FREE // Try to use MADV_FREE if supported. It tells the kernel that the application doesn't // need the pages in the range. Freeing the pages can be delayed until a memory pressure // occurs. - st = madvise(address, size, MADV_FREE); + st = madvise(address, size, madviseFlags); if (st != 0) #endif { @@ -704,14 +727,6 @@ bool GCToOSInterface::VirtualReset(void * address, size_t size, bool unlock) #endif } -#ifdef MADV_DONTDUMP - if (st == 0) - { - // Do not include reset memory in coredump. - madvise(address, size, MADV_DONTDUMP); - } -#endif - return (st == 0); } diff --git a/src/coreclr/pal/src/map/virtual.cpp b/src/coreclr/pal/src/map/virtual.cpp index 56b1e35d61b10a..e58af9de9d9de9 100644 --- a/src/coreclr/pal/src/map/virtual.cpp +++ b/src/coreclr/pal/src/map/virtual.cpp @@ -618,8 +618,11 @@ static LPVOID ReserveVirtualMemory( #endif // MMAP_ANON_IGNORES_PROTECTION #ifdef MADV_DONTDUMP - // Do not include reserved memory in coredump. - madvise(pRetVal, MemSize, MADV_DONTDUMP); + // Do not include reserved uncommitted memory in coredump. + if (!(fAllocationType & MEM_COMMIT)) + { + madvise(pRetVal, MemSize, MADV_DONTDUMP); + } #endif return pRetVal; @@ -646,6 +649,7 @@ VIRTUALCommitMemory( PCMI pInformation = 0; LPVOID pRetVal = NULL; BOOL IsLocallyReserved = FALSE; + BOOL IsNewMemory = FALSE; INT nProtect; if ( lpAddress ) @@ -663,6 +667,9 @@ VIRTUALCommitMemory( if ( !pInformation ) { + /* Set if VIRTUALReserveMemory will allocate new memory from the kernel. */ + IsNewMemory = (((flAllocationType & MEM_RESERVE_EXECUTABLE) != 0) && (lpAddress == NULL)); + /* According to the new MSDN docs, if MEM_COMMIT is specified, and the memory is not reserved, you reserve and then commit. */ @@ -710,8 +717,11 @@ VIRTUALCommitMemory( } #ifdef MADV_DODUMP - // Include committed memory in coredump. - madvise((void *) StartBoundary, MemSize, MADV_DODUMP); + // Include committed memory in coredump. Any newly allocated memory included by default. + if (!IsNewMemory) + { + madvise((void *) StartBoundary, MemSize, MADV_DODUMP); + } #endif pRetVal = (void *) StartBoundary; From 9ecb7095df1087ca12de1d94edd2eaaa0590f58a Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Wed, 6 Dec 2023 09:24:06 +0000 Subject: [PATCH 2/8] Simplify VirtualReset() --- src/coreclr/gc/unix/gcenv.unix.cpp | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/coreclr/gc/unix/gcenv.unix.cpp b/src/coreclr/gc/unix/gcenv.unix.cpp index add8bcc8bcec60..a28d7f9bf1f755 100644 --- a/src/coreclr/gc/unix/gcenv.unix.cpp +++ b/src/coreclr/gc/unix/gcenv.unix.cpp @@ -702,30 +702,33 @@ bool GCToOSInterface::VirtualDecommit(void* address, size_t size) // true if it has succeeded, false if it has failed bool GCToOSInterface::VirtualReset(void * address, size_t size, bool unlock) { - int st; - bool madviseFlags = MADV_FREE; + int st = EINVAL; + int madviseFlags = 0; #ifdef MADV_DONTDUMP // Do not include reset memory in coredump. madviseFlags |= MADV_DONTDUMP; #endif -#if HAVE_MADV_FREE +#ifdef HAVE_MADV_FREE // Try to use MADV_FREE if supported. It tells the kernel that the application doesn't // need the pages in the range. Freeing the pages can be delayed until a memory pressure // occurs. - st = madvise(address, size, madviseFlags); - if (st != 0) + madviseFlags |= MADV_FREE; #endif + + if (madviseFlags != 0) { + st = madvise(address, size, madviseFlags); + } + #if HAVE_POSIX_MADVISE + if (st != 0) + { // In case the MADV_FREE is not supported, use MADV_DONTNEED st = posix_madvise(address, size, MADV_DONTNEED); -#else - // If we don't have posix_madvise, report failure - st = EINVAL; -#endif } +#endif return (st == 0); } From ab47f4ed9a860cbd82a2f32620e7c1475a9c6525 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Wed, 6 Dec 2023 09:49:11 +0000 Subject: [PATCH 3/8] Fix detection of new memory in VIRTUALCommitMemory --- src/coreclr/pal/src/map/virtual.cpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/coreclr/pal/src/map/virtual.cpp b/src/coreclr/pal/src/map/virtual.cpp index e58af9de9d9de9..a471bb9abb2f03 100644 --- a/src/coreclr/pal/src/map/virtual.cpp +++ b/src/coreclr/pal/src/map/virtual.cpp @@ -468,7 +468,8 @@ static LPVOID VIRTUALReserveMemory( IN LPVOID lpAddress, /* Region to reserve or commit */ IN SIZE_T dwSize, /* Size of Region */ IN DWORD flAllocationType, /* Type of allocation */ - IN DWORD flProtect) /* Type of access protection */ + IN DWORD flProtect, /* Type of access protection */ + OUT BOOL *newMemory = NULL) /* Set if new virtual memory is allocated */ { LPVOID pRetVal = NULL; UINT_PTR StartBoundary; @@ -509,6 +510,11 @@ static LPVOID VIRTUALReserveMemory( flAllocationType |= MEM_RESERVE_EXECUTABLE; } pRetVal = ReserveVirtualMemory(pthrCurrent, (LPVOID)StartBoundary, MemSize, flAllocationType); + + if (newMemory != nullptr) + { + *newMemory = true; + } } if (pRetVal != NULL) @@ -667,15 +673,12 @@ VIRTUALCommitMemory( if ( !pInformation ) { - /* Set if VIRTUALReserveMemory will allocate new memory from the kernel. */ - IsNewMemory = (((flAllocationType & MEM_RESERVE_EXECUTABLE) != 0) && (lpAddress == NULL)); - /* According to the new MSDN docs, if MEM_COMMIT is specified, and the memory is not reserved, you reserve and then commit. */ LPVOID pReservedMemory = VIRTUALReserveMemory( pthrCurrent, lpAddress, dwSize, - flAllocationType, flProtect ); + flAllocationType | MEM_COMMIT, flProtect, &IsNewMemory ); TRACE( "Reserve and commit the memory!\n " ); From 2338d642041b6f3ec20d43605329676749f66265 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Thu, 7 Dec 2023 11:00:17 +0000 Subject: [PATCH 4/8] Fix up nits --- src/coreclr/gc/unix/gcenv.unix.cpp | 9 +++++++-- src/coreclr/pal/src/map/virtual.cpp | 4 ++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/coreclr/gc/unix/gcenv.unix.cpp b/src/coreclr/gc/unix/gcenv.unix.cpp index a28d7f9bf1f755..cf3edebdf186b0 100644 --- a/src/coreclr/gc/unix/gcenv.unix.cpp +++ b/src/coreclr/gc/unix/gcenv.unix.cpp @@ -639,7 +639,7 @@ static bool VirtualCommitInner(void* address, size_t size, uint16_t node, bool n // true if it has succeeded, false if it has failed bool GCToOSInterface::VirtualCommit(void* address, size_t size, uint16_t node) { - return VirtualCommitInner(address, size, node, false); + return VirtualCommitInner(address, size, node, /* newMemory */ false); } // Commit virtual memory range. @@ -658,7 +658,7 @@ void* GCToOSInterface::VirtualReserveAndCommitLargePages(size_t size, uint16_t n #endif void* pRetVal = VirtualReserveInner(size, OS_PAGE_SIZE, 0, largePagesFlag, true); - if (VirtualCommitInner(pRetVal, size, node, true)) + if (VirtualCommitInner(pRetVal, size, node, /* newMemory */ true)) { return pRetVal; } @@ -727,6 +727,11 @@ bool GCToOSInterface::VirtualReset(void * address, size_t size, bool unlock) { // In case the MADV_FREE is not supported, use MADV_DONTNEED st = posix_madvise(address, size, MADV_DONTNEED); + +#ifdef MADV_DONTDUMP + // Ensure DONTDUMP is still applied. + madvise(address, size, MADV_DONTDUMP); +#endif } #endif diff --git a/src/coreclr/pal/src/map/virtual.cpp b/src/coreclr/pal/src/map/virtual.cpp index a471bb9abb2f03..4bb4e4ff103cba 100644 --- a/src/coreclr/pal/src/map/virtual.cpp +++ b/src/coreclr/pal/src/map/virtual.cpp @@ -511,7 +511,7 @@ static LPVOID VIRTUALReserveMemory( } pRetVal = ReserveVirtualMemory(pthrCurrent, (LPVOID)StartBoundary, MemSize, flAllocationType); - if (newMemory != nullptr) + if (newMemory != NULL) { *newMemory = true; } @@ -678,7 +678,7 @@ VIRTUALCommitMemory( */ LPVOID pReservedMemory = VIRTUALReserveMemory( pthrCurrent, lpAddress, dwSize, - flAllocationType | MEM_COMMIT, flProtect, &IsNewMemory ); + flAllocationType, flProtect, &IsNewMemory ); TRACE( "Reserve and commit the memory!\n " ); From a17fddc580ed0713e68d8f672042f245aa30b708 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Thu, 7 Dec 2023 11:02:22 +0000 Subject: [PATCH 5/8] More nits --- src/coreclr/gc/unix/gcenv.unix.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/gc/unix/gcenv.unix.cpp b/src/coreclr/gc/unix/gcenv.unix.cpp index cf3edebdf186b0..3a722a2cef2845 100644 --- a/src/coreclr/gc/unix/gcenv.unix.cpp +++ b/src/coreclr/gc/unix/gcenv.unix.cpp @@ -573,7 +573,7 @@ static void* VirtualReserveInner(size_t size, size_t alignment, uint32_t flags, // Starting virtual address of the reserved range void* GCToOSInterface::VirtualReserve(size_t size, size_t alignment, uint32_t flags, uint16_t node) { - return VirtualReserveInner(size, alignment, flags, 0, false); + return VirtualReserveInner(size, alignment, flags, 0, /* committing */ false); } // Release virtual memory range previously reserved using VirtualReserve From 544bea62517a39896db1aac0588c986efa072d27 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Fri, 8 Dec 2023 09:25:26 +0000 Subject: [PATCH 6/8] Use POSIX_MADV_DONTNEED --- src/coreclr/gc/unix/gcenv.unix.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/gc/unix/gcenv.unix.cpp b/src/coreclr/gc/unix/gcenv.unix.cpp index 3a722a2cef2845..16212e307f3b59 100644 --- a/src/coreclr/gc/unix/gcenv.unix.cpp +++ b/src/coreclr/gc/unix/gcenv.unix.cpp @@ -726,7 +726,7 @@ bool GCToOSInterface::VirtualReset(void * address, size_t size, bool unlock) if (st != 0) { // In case the MADV_FREE is not supported, use MADV_DONTNEED - st = posix_madvise(address, size, MADV_DONTNEED); + st = posix_madvise(address, size, POSIX_MADV_DONTNEED); #ifdef MADV_DONTDUMP // Ensure DONTDUMP is still applied. From 52d6b8d76e7c293df43ed9e184c0f1341b6fe3a8 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Fri, 8 Dec 2023 10:30:35 +0000 Subject: [PATCH 7/8] Avoid posix_madvise if madvise fails --- src/coreclr/gc/unix/gcenv.unix.cpp | 31 +++++++++++++----------------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/src/coreclr/gc/unix/gcenv.unix.cpp b/src/coreclr/gc/unix/gcenv.unix.cpp index 16212e307f3b59..fe0cf945633e24 100644 --- a/src/coreclr/gc/unix/gcenv.unix.cpp +++ b/src/coreclr/gc/unix/gcenv.unix.cpp @@ -703,6 +703,9 @@ bool GCToOSInterface::VirtualDecommit(void* address, size_t size) bool GCToOSInterface::VirtualReset(void * address, size_t size, bool unlock) { int st = EINVAL; + +#if defined(MADV_DONTDUMP) || defined(HAVE_MADV_FREE) + int madviseFlags = 0; #ifdef MADV_DONTDUMP @@ -711,29 +714,21 @@ bool GCToOSInterface::VirtualReset(void * address, size_t size, bool unlock) #endif #ifdef HAVE_MADV_FREE - // Try to use MADV_FREE if supported. It tells the kernel that the application doesn't - // need the pages in the range. Freeing the pages can be delayed until a memory pressure - // occurs. + // Tell the kernel that the application doesn't need the pages in the range. + // Freeing the pages can be delayed until a memory pressure occurs. madviseFlags |= MADV_FREE; #endif - if (madviseFlags != 0) - { - st = madvise(address, size, madviseFlags); - } + st = madvise(address, size, madviseFlags); -#if HAVE_POSIX_MADVISE - if (st != 0) - { - // In case the MADV_FREE is not supported, use MADV_DONTNEED - st = posix_madvise(address, size, POSIX_MADV_DONTNEED); +#endif //defined(MADV_DONTDUMP) || defined(HAVE_MADV_FREE) -#ifdef MADV_DONTDUMP - // Ensure DONTDUMP is still applied. - madvise(address, size, MADV_DONTDUMP); -#endif - } -#endif +#if defined(HAVE_POSIX_MADVISE) && !defined(MADV_DONTDUMP) + // DONTNEED is the nearest posix equivalent of FREE. + // Prefer FREE as, since glibc2.6 DONTNEED is a nop. + st = posix_madvise(address, size, POSIX_MADV_DONTNEED); + +#endif //defined(HAVE_POSIX_MADVISE) && !defined(MADV_DONTDUMP) return (st == 0); } From 6ca0e0d8ee407410dafcbc5e3714e98940a23722 Mon Sep 17 00:00:00 2001 From: Alan Hayward Date: Wed, 20 Dec 2023 16:26:15 +0000 Subject: [PATCH 8/8] Better setting of newMemory --- src/coreclr/pal/src/map/virtual.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/coreclr/pal/src/map/virtual.cpp b/src/coreclr/pal/src/map/virtual.cpp index 4bb4e4ff103cba..364f3bba1f0255 100644 --- a/src/coreclr/pal/src/map/virtual.cpp +++ b/src/coreclr/pal/src/map/virtual.cpp @@ -477,6 +477,11 @@ static LPVOID VIRTUALReserveMemory( TRACE( "Reserving the memory now..\n"); + if (newMemory != NULL) + { + *newMemory = false; + } + // First, figure out where we're trying to reserve the memory and // how much we need. On most systems, requests to mmap must be // page-aligned and at multiples of the page size. Unlike on Windows, on @@ -511,7 +516,7 @@ static LPVOID VIRTUALReserveMemory( } pRetVal = ReserveVirtualMemory(pthrCurrent, (LPVOID)StartBoundary, MemSize, flAllocationType); - if (newMemory != NULL) + if (newMemory != NULL && pRetVal != NULL) { *newMemory = true; }