From 5d0ae71423d0f211bc048a354fdb3155a9e74fe9 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Mon, 19 Jul 2021 11:20:02 +0100 Subject: [PATCH] Remove at_least The Pal was providing policy for overallocating a block of memory to achieve alignment make that part of the backend. The backend should be responsible for layout policy. --- docs/PORTING.md | 6 +-- src/backend/address_space.h | 21 +++++++--- src/backend/backend.h | 2 +- src/backend/pagemap.h | 4 +- src/pal/pal_concept.h | 16 ++++---- src/pal/pal_posix.h | 40 +++++++------------ src/pal/pal_windows.h | 21 +--------- src/test/func/fixed_region/fixed_region.cc | 3 +- src/test/func/pagemap/pagemap.cc | 3 +- .../perf/external_pointer/externalpointer.cc | 2 + 10 files changed, 50 insertions(+), 68 deletions(-) diff --git a/docs/PORTING.md b/docs/PORTING.md index fdf1dec..8e39451 100644 --- a/docs/PORTING.md +++ b/docs/PORTING.md @@ -47,14 +47,14 @@ pages, rather than zeroing them synchronously in this call ```c++ template static void* reserve_aligned(size_t size) noexcept; -static std::pair reserve_at_least(size_t size) noexcept; +static void* reserve(size_t size) noexcept; ``` -All platforms should provide `reserve_at_least` and can optionally provide +All platforms should provide `reserve` and can optionally provide `reserve_aligned` if the underlying system can provide strongly aligned memory regions. If the system guarantees only page alignment, implement only the second. The Pal is free to overallocate based on the platform's desire and snmalloc -will find suitably aligned blocks inside the region. `reserve_at_least` should +will find suitably aligned blocks inside the region. `reserve` should not commit memory as snmalloc will commit the range of memory it requires of what is returned. diff --git a/src/backend/address_space.h b/src/backend/address_space.h index d34b61e..e8d1483 100644 --- a/src/backend/address_space.h +++ b/src/backend/address_space.h @@ -96,12 +96,21 @@ namespace snmalloc else if constexpr (!pal_supports) { // Need at least 2 times the space to guarantee alignment. - // Hold lock here as a race could cause additional requests to - // the PAL, and this could lead to suprious OOM. This is - // particularly bad if the PAL gives all the memory on first call. - auto block_and_size = PAL::reserve_at_least(size * 2); - block = CapPtr(block_and_size.first); - block_size = block_and_size.second; + size_t needed_size = size * 2; + // Magic number (27) for over-allocating a block of memory + // These should be further refined based on experiments. + constexpr size_t min_size = bits::one_at_bit(27); + for (size_t size_request = bits::max(needed_size, min_size); + size_request >= needed_size; + size_request = size_request / 2) + { + block = CapPtr(PAL::reserve(size_request)); + if (block != nullptr) + { + block_size = size_request; + break; + } + } // Ensure block is pointer aligned. if ( diff --git a/src/backend/backend.h b/src/backend/backend.h index 0d4618a..8893a4a 100644 --- a/src/backend/backend.h +++ b/src/backend/backend.h @@ -11,7 +11,7 @@ namespace snmalloc * This class implements the standard backend for handling allocations. * It abstracts page table management and address space management. */ - template + template class BackendAllocator { public: diff --git a/src/backend/pagemap.h b/src/backend/pagemap.h index 4b9d437..f1cda4a 100644 --- a/src/backend/pagemap.h +++ b/src/backend/pagemap.h @@ -119,9 +119,7 @@ namespace snmalloc // TODO request additional space, and move to random offset. - // TODO wasting space if size2 bigger than needed. - auto [new_body_untyped, size2] = - Pal::reserve_at_least(ENTRIES * sizeof(T)); + auto new_body_untyped = Pal::reserve(ENTRIES * sizeof(T)); auto new_body = reinterpret_cast(new_body_untyped); diff --git a/src/pal/pal_concept.h b/src/pal/pal_concept.h index bebd130..42e034b 100644 --- a/src/pal/pal_concept.h +++ b/src/pal/pal_concept.h @@ -52,11 +52,10 @@ namespace snmalloc * Absent any feature flags, the PAL must support a crude primitive allocator */ template - concept ConceptPAL_reserve_at_least = - requires(PAL p, void* vp, std::size_t sz) + concept ConceptPAL_reserve = + requires(PAL p, std::size_t sz) { - { PAL::reserve_at_least(sz) } noexcept - -> ConceptSame>; + { PAL::reserve(sz) } noexcept -> ConceptSame; }; /** @@ -102,10 +101,11 @@ namespace snmalloc (!pal_supports || ConceptPAL_mem_low_notify) && (pal_supports || - (pal_supports && - ConceptPAL_reserve_aligned) || - (!pal_supports && - ConceptPAL_reserve_at_least)); + ( + (!pal_supports || + ConceptPAL_reserve_aligned) && + ConceptPAL_reserve) + ); } // namespace snmalloc #endif diff --git a/src/pal/pal_posix.h b/src/pal/pal_posix.h index 3538487..8f9e2f7 100644 --- a/src/pal/pal_posix.h +++ b/src/pal/pal_posix.h @@ -253,44 +253,32 @@ namespace snmalloc * POSIX does not define a portable interface for specifying alignment * greater than a page. */ - static std::pair reserve_at_least(size_t size) noexcept + static void* reserve(size_t size) noexcept { - SNMALLOC_ASSERT(bits::is_pow2(size)); - - // Magic number for over-allocating chosen by the Pal - // These should be further refined based on experiments. - constexpr size_t min_size = - bits::is64() ? bits::one_at_bit(31) : bits::one_at_bit(27); - #ifdef SNMALLOC_CHECK_CLIENT auto prot = PROT_NONE; #else auto prot = PROT_READ | PROT_WRITE; #endif - for (size_t size_request = bits::max(size, min_size); - size_request >= size; - size_request = size_request / 2) - { - void* p = mmap( - nullptr, - size_request, - prot, - MAP_PRIVATE | MAP_ANONYMOUS | DefaultMMAPFlags::flags, - AnonFD::fd, - 0); + void* p = mmap( + nullptr, + size, + prot, + MAP_PRIVATE | MAP_ANONYMOUS | DefaultMMAPFlags::flags, + AnonFD::fd, + 0); - if (p != MAP_FAILED) - { + if (p != MAP_FAILED) + { #ifdef SNMALLOC_TRACING - std::cout << "Pal_posix reserved: " << p << " (" << size_request - << ")" << std::endl; + std::cout << "Pal_posix reserved: " << p << " (" << size << ")" + << std::endl; #endif - return {p, size_request}; - } + return p; } - OS::error("Out of memory"); + return nullptr; } /** diff --git a/src/pal/pal_windows.h b/src/pal/pal_windows.h index c4f7219..6fe3564 100644 --- a/src/pal/pal_windows.h +++ b/src/pal/pal_windows.h @@ -185,26 +185,9 @@ namespace snmalloc } # endif - static std::pair reserve_at_least(size_t size) noexcept + static void* reserve(size_t size) noexcept { - SNMALLOC_ASSERT(bits::is_pow2(size)); - - // Magic number for over-allocating chosen by the Pal - // These should be further refined based on experiments. - constexpr size_t min_size = - bits::is64() ? bits::one_at_bit(32) : bits::one_at_bit(28); - for (size_t size_request = bits::max(size, min_size); - size_request >= size; - size_request = size_request / 2) - { - void* ret = - VirtualAlloc(nullptr, size_request, MEM_RESERVE, PAGE_READWRITE); - if (ret != nullptr) - { - return std::pair(ret, size_request); - } - } - error("Failed to allocate memory\n"); + return VirtualAlloc(nullptr, size, MEM_RESERVE, PAGE_READWRITE); } /** diff --git a/src/test/func/fixed_region/fixed_region.cc b/src/test/func/fixed_region/fixed_region.cc index a7db57a..b2350a9 100644 --- a/src/test/func/fixed_region/fixed_region.cc +++ b/src/test/func/fixed_region/fixed_region.cc @@ -21,7 +21,8 @@ int main() // 28 is large enough to produce a nested allocator. // It is also large enough for the example to run in. // For 1MiB superslabs, SUPERSLAB_BITS + 4 is not big enough for the example. - auto [oe_base, size] = Pal::reserve_at_least(bits::one_at_bit(28)); + auto size = bits::one_at_bit(28); + auto oe_base = Pal::reserve(size); Pal::notify_using(oe_base, size); auto oe_end = pointer_offset(oe_base, size); std::cout << "Allocated region " << oe_base << " - " diff --git a/src/test/func/pagemap/pagemap.cc b/src/test/func/pagemap/pagemap.cc index dc5d5af..61a23f1 100644 --- a/src/test/func/pagemap/pagemap.cc +++ b/src/test/func/pagemap/pagemap.cc @@ -68,7 +68,8 @@ void test_pagemap(bool bounded) // Initialise the pagemap if (bounded) { - auto [base, size] = Pal::reserve_at_least(bits::one_at_bit(30)); + auto size = bits::one_at_bit(30); + auto base = Pal::reserve(size); Pal::notify_using(base, size); std::cout << "Fixed base: " << base << " (" << size << ") " << " end: " << pointer_offset(base, size) << std::endl; diff --git a/src/test/perf/external_pointer/externalpointer.cc b/src/test/perf/external_pointer/externalpointer.cc index b927daf..ac66a13 100644 --- a/src/test/perf/external_pointer/externalpointer.cc +++ b/src/test/perf/external_pointer/externalpointer.cc @@ -32,6 +32,8 @@ namespace test size = 16; // store object objects[i] = (size_t*)alloc.alloc(size); + if (objects[i] == nullptr) + abort(); // Store allocators size for this object *objects[i] = alloc.alloc_size(objects[i]); }