From dbaae4074ba49703f6764b89749b911f72cc2d86 Mon Sep 17 00:00:00 2001 From: Schrodinger ZHU Yifan Date: Wed, 29 Jan 2025 12:58:17 +0000 Subject: [PATCH] address code reviews --- .github/workflows/main.yml | 2 +- src/snmalloc/ds_core/ptrwrap.h | 2 + src/snmalloc/mem/corealloc.h | 12 ++- src/snmalloc/mem/localalloc.h | 74 ++++++++++++------- src/snmalloc/mem/secondary.h | 4 + src/snmalloc/mem/secondary/default.h | 16 ++-- src/snmalloc/mem/secondary/gwp_asan.h | 20 ++--- src/test/func/fixed_region/fixed_region.cc | 11 +-- .../perf/external_pointer/externalpointer.cc | 4 +- 9 files changed, 88 insertions(+), 57 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 59c01a4ec..2df4e8303 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -586,7 +586,7 @@ jobs: - name: Test run: | cd ${{github.workspace}}/build - ctest --parallel + ctest --parallel --output-on-failure all-checks: # Currently FreeBSD and NetBSD CI are not working, so we do not require them to pass. diff --git a/src/snmalloc/ds_core/ptrwrap.h b/src/snmalloc/ds_core/ptrwrap.h index 1d6e41f34..853e5fb4c 100644 --- a/src/snmalloc/ds_core/ptrwrap.h +++ b/src/snmalloc/ds_core/ptrwrap.h @@ -4,6 +4,8 @@ #include "defines.h" #include "snmalloc/stl/atomic.h" +#include + namespace snmalloc { /* diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index 869725ec7..55fd5efe0 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -818,6 +818,14 @@ namespace snmalloc SNMALLOC_SLOW_PATH capptr::Alloc small_alloc(smallsizeclass_t sizeclass, freelist::Iter<>& fast_free_list) { + void* result = SecondaryAllocator::allocate( + [sizeclass]() -> stl::Pair { + auto size = sizeclass_to_size(sizeclass); + return {size, natural_alignment(size)}; + }); + + if (result != nullptr) + return capptr::Alloc::unsafe_from(result); // Look to see if we can grab a free list. auto& sl = alloc_classes[sizeclass].available; if (SNMALLOC_LIKELY(alloc_classes[sizeclass].length > 0)) @@ -886,10 +894,6 @@ namespace snmalloc { size_t rsize = sizeclass_to_size(sizeclass); - void* result = SecondaryAllocator::allocate(rsize); - if (result != nullptr) - return capptr::Alloc::unsafe_from(result); - // No existing free list get a new slab. size_t slab_size = sizeclass_to_slab_size(sizeclass); diff --git a/src/snmalloc/mem/localalloc.h b/src/snmalloc/mem/localalloc.h index 664f2ba2e..c3488b9fd 100644 --- a/src/snmalloc/mem/localalloc.h +++ b/src/snmalloc/mem/localalloc.h @@ -1,6 +1,7 @@ #pragma once #include "snmalloc/aal/address.h" +#include "snmalloc/mem/remoteallocator.h" #include "snmalloc/mem/secondary.h" #if defined(_MSC_VER) # define ALLOCATOR __declspec(allocator) __declspec(restrict) @@ -187,10 +188,6 @@ namespace snmalloc return small_alloc(1); } - void* result = SecondaryAllocator::allocate(size); - if (result != nullptr) - return capptr::Alloc::unsafe_from(result); - return check_init([&](CoreAlloc* core_alloc) { if (size > bits::one_at_bit(bits::BITS - 1)) { @@ -199,6 +196,15 @@ namespace snmalloc errno = ENOMEM; return capptr::Alloc{nullptr}; } + + // Check if secondary allocator wants to offer the memory + void* result = + SecondaryAllocator::allocate([size]() -> stl::Pair { + return {size, natural_alignment(size)}; + }); + if (result != nullptr) + return capptr::Alloc::unsafe_from(result); + // Grab slab of correct size // Set remote as large allocator remote. auto [chunk, meta] = Config::Backend::alloc_chunk( @@ -611,17 +617,13 @@ namespace snmalloc #endif } - SNMALLOC_FAST_PATH void dealloc(void* p_raw) - { -#ifdef SNMALLOC_PASS_THROUGH - external_alloc::free(p_raw); -#else - // Care is needed so that dealloc(nullptr) works before init - // The backend allocator must ensure that a minimal page map exists - // before init, that maps null to a remote_deallocator that will never - // be in thread local state. + // The domestic pointer with its origin allocator + using DomesticInfo = stl::Pair, const PagemapEntry&>; -# ifdef __CHERI_PURE_CAPABILITY__ + // Check whether the raw pointer is owned by snmalloc + SNMALLOC_FAST_PATH_INLINE DomesticInfo get_domestic_info(const void* p_raw) + { +#ifdef __CHERI_PURE_CAPABILITY__ /* * On CHERI platforms, snap the provided pointer to its base, ignoring * any client-provided offset, which may have taken the pointer out of @@ -637,10 +639,29 @@ namespace snmalloc * start of the allocation and so the offset is zero. */ p_raw = __builtin_cheri_offset_set(p_raw, 0); -# endif +#endif + capptr::AllocWild p_wild = + capptr_from_client(const_cast(p_raw)); + auto p_tame = + capptr_domesticate(core_alloc->backend_state_ptr(), p_wild); + const PagemapEntry& entry = + Config::Backend::get_metaentry(address_cast(p_tame)); + return {p_tame, entry}; + } - capptr::AllocWild p_wild = capptr_from_client(p_raw); + // Check if a pointer is domestic to SnMalloc + SNMALLOC_FAST_PATH bool check_domestication(const void* p_raw) + { + auto [_, entry] = get_domestic_info(p_raw); + RemoteAllocator* remote = entry.get_remote(); + return remote != nullptr; + } + SNMALLOC_FAST_PATH void dealloc(void* p_raw) + { +#ifdef SNMALLOC_PASS_THROUGH + external_alloc::free(p_raw); +#else /* * p_tame may be nullptr, even if p_raw/p_wild are not, in the case * where domestication fails. We exclusively use p_tame below so that @@ -653,11 +674,7 @@ namespace snmalloc * well-formedness) of this pointer. The remainder of the logic will * deal with the object's extent. */ - capptr::Alloc p_tame = - capptr_domesticate(core_alloc->backend_state_ptr(), p_wild); - - const PagemapEntry& entry = - Config::Backend::get_metaentry(address_cast(p_tame)); + auto [p_tame, entry] = get_domestic_info(p_raw); if (SNMALLOC_LIKELY(local_cache.remote_allocator == entry.get_remote())) { @@ -702,12 +719,15 @@ namespace snmalloc return; } - SecondaryAllocator::deallocate(p_tame.unsafe_ptr()); - + if (SNMALLOC_LIKELY(p_tame == nullptr)) + { # ifdef SNMALLOC_TRACING - message<1024>("nullptr deallocation"); + message<1024>("nullptr deallocation"); # endif - return; + return; + } + + SecondaryAllocator::deallocate(p_tame.unsafe_ptr()); #endif } @@ -718,7 +738,7 @@ namespace snmalloc #else if constexpr (mitigations(sanity_checks)) { - if (SecondaryAllocator::has_secondary_ownership(p)) + if (!check_domestication(p)) return; size = size == 0 ? 1 : size; auto sc = size_to_sizeclass_full(size); @@ -769,7 +789,7 @@ namespace snmalloc return external_alloc::malloc_usable_size(const_cast(p_raw)); #else - if (SecondaryAllocator::has_secondary_ownership(p_raw)) + if (!SecondaryAllocator::pass_through && !check_domestication(p_raw)) return SecondaryAllocator::alloc_size(p_raw); // TODO What's the domestication policy here? At the moment we just // probe the pagemap with the raw address, without checks. There could diff --git a/src/snmalloc/mem/secondary.h b/src/snmalloc/mem/secondary.h index cde4c1ec8..9f3dab307 100644 --- a/src/snmalloc/mem/secondary.h +++ b/src/snmalloc/mem/secondary.h @@ -1,4 +1,8 @@ #pragma once + +#include "snmalloc/ds_core/defines.h" +#include "snmalloc/ds_core/ptrwrap.h" + #ifdef SNMALLOC_ENABLE_GWP_ASAN_INTEGRATION # include "snmalloc/mem/secondary/gwp_asan.h" diff --git a/src/snmalloc/mem/secondary/default.h b/src/snmalloc/mem/secondary/default.h index 56323f885..ae5d53bfb 100644 --- a/src/snmalloc/mem/secondary/default.h +++ b/src/snmalloc/mem/secondary/default.h @@ -10,11 +10,15 @@ namespace snmalloc class DefaultSecondaryAllocator { public: + // This flag is used to turn off checks on fast paths if the secondary + // allocator does not own the memory at all. + static constexpr inline bool pass_through = true; + SNMALLOC_FAST_PATH static void initialize() {} - SNMALLOC_FAST_PATH - static void* allocate([[maybe_unused]] size_t size) + template + SNMALLOC_FAST_PATH static void* allocate(SizeAlign&&) { return nullptr; } @@ -32,13 +36,7 @@ namespace snmalloc } SNMALLOC_FAST_PATH - static bool has_secondary_ownership([[maybe_unused]] const void* pointer) - { - return false; - } - - SNMALLOC_FAST_PATH - static size_t alloc_size([[maybe_unused]] const void* pointer) + static size_t alloc_size(const void*) { SNMALLOC_ASSERT( false && diff --git a/src/snmalloc/mem/secondary/gwp_asan.h b/src/snmalloc/mem/secondary/gwp_asan.h index 0d50e91d0..a46678321 100644 --- a/src/snmalloc/mem/secondary/gwp_asan.h +++ b/src/snmalloc/mem/secondary/gwp_asan.h @@ -15,6 +15,8 @@ namespace snmalloc static inline size_t max_allocation_size; public: + static constexpr inline bool pass_through = false; + static void initialize() noexcept { // for now, we use default options @@ -31,10 +33,17 @@ namespace snmalloc singleton.getAllocatorState()->maximumAllocationSize(); } - SNMALLOC_FAST_PATH static void* allocate(size_t size) + // Use thunk to avoid extra computation when allocation decision can be made + // before size and alignment are computed. + template + SNMALLOC_FAST_PATH static void* allocate(SizeAlign&& getter) { + // TODO: this `shouldSample` is only triggered on snmalloc's slowpath, + // which may reduce the chance of error detection. We may reconsider + // the logic to improve the precision in future commits. if (SNMALLOC_UNLIKELY(singleton.shouldSample())) { + auto [size, align] = getter(); if (size > max_allocation_size) return nullptr; auto alignment = natural_alignment(size); @@ -46,9 +55,6 @@ namespace snmalloc SNMALLOC_FAST_PATH static void deallocate(void* pointer) { - if (SNMALLOC_LIKELY(pointer == nullptr)) - return; - snmalloc_check_client( mitigations(sanity_checks), singleton.pointerIsMine(pointer), @@ -57,12 +63,6 @@ namespace snmalloc singleton.deallocate(pointer); } - SNMALLOC_FAST_PATH - static bool has_secondary_ownership([[maybe_unused]] const void* pointer) - { - return singleton.pointerIsMine(pointer); - } - SNMALLOC_FAST_PATH static size_t alloc_size(const void* pointer) { diff --git a/src/test/func/fixed_region/fixed_region.cc b/src/test/func/fixed_region/fixed_region.cc index 31b303f82..ff9dc6abf 100644 --- a/src/test/func/fixed_region/fixed_region.cc +++ b/src/test/func/fixed_region/fixed_region.cc @@ -43,7 +43,12 @@ int main() count += object_size; i++; - if (SecondaryAllocator::has_secondary_ownership(r1)) + // Run until we exhaust the fixed region. + // This should return null. + if (r1 == nullptr) + break; + + if (!a.check_domestication(r1)) { a.dealloc(r1); continue; @@ -54,10 +59,6 @@ int main() i = 0; std::cout << "."; } - // Run until we exhaust the fixed region. - // This should return null. - if (r1 == nullptr) - break; if (oe_base > r1) { diff --git a/src/test/perf/external_pointer/externalpointer.cc b/src/test/perf/external_pointer/externalpointer.cc index 1b21e1b4f..1704b2671 100644 --- a/src/test/perf/external_pointer/externalpointer.cc +++ b/src/test/perf/external_pointer/externalpointer.cc @@ -76,14 +76,16 @@ namespace test size_t rand = (size_t)r.next(); size_t oid = rand & (((size_t)1 << count_log) - 1); size_t* external_ptr = objects[oid]; - if (SecondaryAllocator::has_secondary_ownership(external_ptr)) + if (!alloc.check_domestication(external_ptr)) continue; size_t size = *external_ptr; size_t offset = (size >> 4) * (rand & 15); void* interior_ptr = pointer_offset(external_ptr, offset); void* calced_external = alloc.external_pointer(interior_ptr); if (calced_external != external_ptr) + { abort(); + } } }