From 0a5eb403adf85b3e04ad90e6b44e0cb1de99ba38 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Fri, 28 Oct 2022 13:13:56 +0100 Subject: [PATCH] Change representation of the SeqSet (#568) This changes the representation of SeqSet to be doubly linked. This is required to enable tracking fully used slabs. --- src/snmalloc/backend/backend.h | 10 +- src/snmalloc/backend_helpers/buddy.h | 8 +- .../backend_helpers/defaultpagemapentry.h | 14 +- .../backend_helpers/smallbuddyrange.h | 1 + src/snmalloc/ds_core/seqset.h | 225 +++++++++--------- src/snmalloc/mem/corealloc.h | 29 +-- src/snmalloc/mem/metadata.h | 34 ++- 7 files changed, 176 insertions(+), 145 deletions(-) diff --git a/src/snmalloc/backend/backend.h b/src/snmalloc/backend/backend.h index 855daaa26..581f67d4e 100644 --- a/src/snmalloc/backend/backend.h +++ b/src/snmalloc/backend/backend.h @@ -25,6 +25,8 @@ namespace snmalloc #ifdef __cpp_concepts static_assert(IsSlabMeta_Arena); #endif + static constexpr size_t SizeofMetadata = + bits::next_pow2_const(sizeof(SlabMetadata)); public: /** @@ -85,8 +87,7 @@ namespace snmalloc SNMALLOC_ASSERT(bits::is_pow2(size)); SNMALLOC_ASSERT(size >= MIN_CHUNK_SIZE); - auto meta_cap = - local_state.get_meta_range().alloc_range(sizeof(SlabMetadata)); + auto meta_cap = local_state.get_meta_range().alloc_range(SizeofMetadata); auto meta = meta_cap.template as_reinterpret().unsafe_ptr(); @@ -103,8 +104,7 @@ namespace snmalloc #endif if (p == nullptr) { - local_state.get_meta_range().dealloc_range( - meta_cap, sizeof(SlabMetadata)); + local_state.get_meta_range().dealloc_range(meta_cap, SizeofMetadata); errno = ENOMEM; #ifdef SNMALLOC_TRACING message<1024>("Out of memory"); @@ -160,7 +160,7 @@ namespace snmalloc capptr::Arena arena = slab_metadata.arena_get(alloc); local_state.get_meta_range().dealloc_range( - capptr::Arena::unsafe_from(&slab_metadata), sizeof(SlabMetadata)); + capptr::Arena::unsafe_from(&slab_metadata), SizeofMetadata); local_state.get_object_range()->dealloc_range(arena, size); } diff --git a/src/snmalloc/backend_helpers/buddy.h b/src/snmalloc/backend_helpers/buddy.h index 1c9b8e4af..770dc3d3c 100644 --- a/src/snmalloc/backend_helpers/buddy.h +++ b/src/snmalloc/backend_helpers/buddy.h @@ -19,9 +19,13 @@ namespace snmalloc size_t to_index(size_t size) { + SNMALLOC_ASSERT(size != 0); + SNMALLOC_ASSERT(bits::is_pow2(size)); auto log = snmalloc::bits::next_pow2_bits(size); - SNMALLOC_ASSERT(log >= MIN_SIZE_BITS); - SNMALLOC_ASSERT(log < MAX_SIZE_BITS); + SNMALLOC_ASSERT_MSG( + log >= MIN_SIZE_BITS, "Size too big: {} log {}.", size, log); + SNMALLOC_ASSERT_MSG( + log < MAX_SIZE_BITS, "Size too small: {} log {}.", size, log); return log - MIN_SIZE_BITS; } diff --git a/src/snmalloc/backend_helpers/defaultpagemapentry.h b/src/snmalloc/backend_helpers/defaultpagemapentry.h index b63f26f90..de5ac306b 100644 --- a/src/snmalloc/backend_helpers/defaultpagemapentry.h +++ b/src/snmalloc/backend_helpers/defaultpagemapentry.h @@ -60,9 +60,19 @@ namespace snmalloc SNMALLOC_FAST_PATH DefaultPagemapEntryT() = default; }; + class StrictProvenanceSlabMetadata + : public StrictProvenanceSlabMetadataMixin< + FrontendSlabMetadata> + {}; + + class LaxProvenanceSlabMetadata + : public LaxProvenanceSlabMetadataMixin< + FrontendSlabMetadata> + {}; + using DefaultPagemapEntry = DefaultPagemapEntryT, - LaxProvenanceSlabMetadataMixin>>; + StrictProvenanceSlabMetadata, + LaxProvenanceSlabMetadata>>; } // namespace snmalloc diff --git a/src/snmalloc/backend_helpers/smallbuddyrange.h b/src/snmalloc/backend_helpers/smallbuddyrange.h index 2a3f3a34c..83796e1ec 100644 --- a/src/snmalloc/backend_helpers/smallbuddyrange.h +++ b/src/snmalloc/backend_helpers/smallbuddyrange.h @@ -244,6 +244,7 @@ namespace snmalloc void dealloc_range(CapPtr base, size_t size) { + SNMALLOC_ASSERT(bits::is_pow2(size)); add_range(base, size); } }; diff --git a/src/snmalloc/ds_core/seqset.h b/src/snmalloc/ds_core/seqset.h index 22a0fcd07..ef854958c 100644 --- a/src/snmalloc/ds_core/seqset.h +++ b/src/snmalloc/ds_core/seqset.h @@ -1,5 +1,6 @@ #pragma once +#include "../aal/aal.h" #include "../ds_core/ds_core.h" #include @@ -10,72 +11,88 @@ namespace snmalloc /** * Simple sequential set of T. * - * Linked using the T::next field. + * Implemented as a doubly linked cyclic list. + * Linked using the T::node field. * * Can be used in either Fifo or Lifo mode, which is - * specified by template parameter. + * specified by template parameter to `pop`. */ - template + template class SeqSet { + public: /** - * This sequence structure is intrusive, in that it requires the use of a - * `next` field in the elements it manages, but, unlike some other intrusive - * designs, it does not require the use of a `container_of`-like construct, - * because its pointers point to the element, not merely the intrusive - * member. - * - * In some cases, the next pointer is provided by a superclass but the list - * is templated over the subclass. The `SeqSet` enforces the invariant that - * only instances of the subclass can be added to the list and so can safely - * down-cast the type of `.next` to `T*`. As such, we require only that the - * `next` field is a pointer to `T` or some superclass of `T`. - * %{ - */ - using NextPtr = decltype(std::declval().next); - static_assert( - std::is_base_of_v, T>, - "T->next must be a queue pointer to T"); - ///@} - - /** - * Field representation for Fifo behaviour. + * The doubly linked Node. */ - struct FieldFifo + class Node { - NextPtr head{nullptr}; + Node* next; + Node* prev; + + friend class SeqSet; + + constexpr Node(Node* next, Node* prev) : next(next), prev(prev) {} + + public: + void invariant() + { + SNMALLOC_ASSERT(next != nullptr); + SNMALLOC_ASSERT(prev != nullptr); + SNMALLOC_ASSERT(next->prev == this); + SNMALLOC_ASSERT(prev->next == this); + } + + void remove() + { + invariant(); + next->invariant(); + prev->invariant(); + next->prev = prev; + prev->next = next; + next->invariant(); + prev->invariant(); + } }; + private: + // Cyclic doubly linked list (initially empty) + Node head{&head, &head}; + /** - * Field representation for Lifo behaviour. + * Returns the containing object. */ - struct FieldLifo + T* containing(Node* n) { - NextPtr head{nullptr}; - NextPtr* end{&head}; - }; + // We could use -static_cast(offsetof(T, node)) here but CHERI + // compiler complains. So we restrict to first entries only. + + static_assert(offsetof(T, node) == 0); + + return pointer_offset(n, 0); + } /** - * Field indirection to actual representation. - * Different numbers of fields are required for the - * two behaviours. + * Gets the doubly linked node for the object. */ - std::conditional_t v; + Node* get_node(T* t) + { +#ifdef __CHERI_PURE_CAPABILITY__ + return &__builtin_no_change_bounds(t->node); +#else + return &(t->node); +#endif + } /** * Check for empty */ SNMALLOC_FAST_PATH bool is_empty() { - if constexpr (Fifo) - { - return v.head == nullptr; - } - else - { - SNMALLOC_ASSERT(v.end != nullptr); - return &(v.head) == v.end; - } + static_assert( + std::is_same_v().node)>, + "T->node must be Node for T"); + head.invariant(); + return head.next == &head; } public: @@ -89,74 +106,60 @@ namespace snmalloc * * Assumes queue is non-empty */ - SNMALLOC_FAST_PATH T* pop() + SNMALLOC_FAST_PATH T* pop_front() { + head.invariant(); SNMALLOC_ASSERT(!this->is_empty()); - auto result = v.head; - if constexpr (Fifo) - { - v.head = result->next; - } + auto node = head.next; + node->remove(); + auto result = containing(node); + head.invariant(); + return result; + } + + /** + * Remove an element from the queue + * + * Assumes queue is non-empty + */ + SNMALLOC_FAST_PATH T* pop_back() + { + head.invariant(); + SNMALLOC_ASSERT(!this->is_empty()); + auto node = head.prev; + node->remove(); + auto result = containing(node); + head.invariant(); + return result; + } + + template + SNMALLOC_FAST_PATH T* pop() + { + head.invariant(); + if constexpr (is_fifo) + return pop_front(); else - { - if (&(v.head->next) == v.end) - v.end = &(v.head); - else - v.head = v.head->next; - } - // This cast is safe if the ->next pointers in all of the objects in the - // list are managed by this class because object types are checked on - // insertion. - return static_cast(result); + return pop_back(); } /** - * Filter + * Applies `f` to all the elements in the set. * - * Removes all elements that f returns true for. - * If f returns true, then filter is not allowed to look at the - * object again, and f is responsible for its lifetime. + * `f` is allowed to remove the element from the set. */ template - SNMALLOC_FAST_PATH void filter(Fn&& f) + SNMALLOC_FAST_PATH void iterate(Fn&& f) { - // Check for empty case. - if (is_empty()) - return; + auto curr = head.next; + curr->invariant(); - NextPtr* prev = &(v.head); - - while (true) + while (curr != &head) { - if constexpr (Fifo) - { - if (*prev == nullptr) - break; - } - - NextPtr curr = *prev; - // Note must read curr->next before calling `f` as `f` is allowed to - // mutate that field. - NextPtr next = curr->next; - if (f(static_cast(curr))) - { - // Remove element; - *prev = next; - } - else - { - // Keep element - prev = &(curr->next); - } - if constexpr (!Fifo) - { - if (&(curr->next) == v.end) - break; - } - } - if constexpr (!Fifo) - { - v.end = prev; + // Read next first, as f may remove curr. + auto next = curr->next; + f(containing(curr)); + curr = next; } } @@ -165,16 +168,16 @@ namespace snmalloc */ SNMALLOC_FAST_PATH void insert(T* item) { - if constexpr (Fifo) - { - item->next = v.head; - v.head = item; - } - else - { - *(v.end) = item; - v.end = &(item->next); - } + auto n = get_node(item); + + n->next = head.next; + head.next->prev = n; + + n->prev = &head; + head.next = n; + + n->invariant(); + head.invariant(); } /** @@ -182,7 +185,7 @@ namespace snmalloc */ SNMALLOC_FAST_PATH const T* peek() { - return static_cast(v.head); + return containing(head.next); } }; } // namespace snmalloc diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index aa6adc5e9..93df53e66 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -55,16 +55,11 @@ namespace snmalloc */ struct SlabMetadataCache { -#ifdef SNMALLOC_CHECK_CLIENT - SeqSet available; -#else - // This is slightly faster in some cases, - // but makes memory reuse more predictable. - SeqSet available; -#endif + SeqSet available{}; + uint16_t unused = 0; uint16_t length = 0; - } alloc_classes[NUM_SMALL_SIZECLASSES]; + } alloc_classes[NUM_SMALL_SIZECLASSES]{}; /** * Local entropy source and current version of keys for @@ -360,7 +355,7 @@ namespace snmalloc SNMALLOC_SLOW_PATH void dealloc_local_slabs(smallsizeclass_t sizeclass) { // Return unused slabs of sizeclass_t back to global allocator - alloc_classes[sizeclass].available.filter([this, sizeclass](auto* meta) { + alloc_classes[sizeclass].available.iterate([this, sizeclass](auto* meta) { auto domesticate = [this](freelist::QueuePtr p) SNMALLOC_FAST_PATH_LAMBDA { auto res = capptr_domesticate(backend_state_ptr(), p); @@ -378,12 +373,16 @@ namespace snmalloc { meta->free_queue.validate(entropy.get_free_list_key(), domesticate); } - return false; + return; } alloc_classes[sizeclass].length--; alloc_classes[sizeclass].unused--; + // Remove from the list. This must be done before dealloc chunk + // as that may corrupt the node. + meta->node.remove(); + // TODO delay the clear to the next user of the slab, or teardown so // don't touch the cache lines at this point in snmalloc_check_client. auto start = clear_slab(meta, sizeclass); @@ -393,8 +392,6 @@ namespace snmalloc *meta, start, sizeclass_to_slab_size(sizeclass)); - - return true; }); } @@ -727,8 +724,9 @@ namespace snmalloc return small_alloc_slow(sizeclass, fast_free_list); } #endif - - auto meta = sl.pop(); + // If CHECK_CLIENT, we use FIFO operations on the list. This reduces + // perf slightly, but increases randomness. + auto meta = sl.template pop(); // Drop length of sl, and empty count if it was empty. alloc_classes[sizeclass].length--; if (meta->needed() == 0) @@ -895,7 +893,7 @@ namespace snmalloc bool debug_is_empty_impl(bool* result) { auto test = [&result](auto& queue, smallsizeclass_t size_class) { - queue.filter([&result, size_class](auto slab_metadata) { + queue.iterate([&result, size_class](auto slab_metadata) { if (slab_metadata->needed() != 0) { if (result != nullptr) @@ -906,7 +904,6 @@ namespace snmalloc sizeclass_to_size(size_class), size_class); } - return false; }); }; diff --git a/src/snmalloc/mem/metadata.h b/src/snmalloc/mem/metadata.h index 391650e6d..fabf85611 100644 --- a/src/snmalloc/mem/metadata.h +++ b/src/snmalloc/mem/metadata.h @@ -360,23 +360,36 @@ namespace snmalloc } }; + /** + * FrontendSlabMetadata_Trait + * + * Used for static checks of inheritance as FrontendSlabMetadata is templated. + */ + class FrontendSlabMetadata_Trait + { + private: + template + friend class FrontendSlabMetadata; + + // Can only be constructed by FrontendSlabMetadata + FrontendSlabMetadata_Trait() = default; + }; + /** * The FrontendSlabMetadata represent the metadata associated with a single * slab. */ - class alignas(CACHELINE_SIZE) FrontendSlabMetadata + template + class FrontendSlabMetadata : public FrontendSlabMetadata_Trait { public: /** * Used to link slab metadata together in various other data-structures. - * This is intended to be used with `SeqSet` and so may actually hold a - * subclass of this class provided by the back end. The `SeqSet` is - * responsible for maintaining that invariant. While an instance of this - * class is in a `SeqSet`, the `next` field should not be assigned to by - * anything that doesn't enforce the invariant that `next` stores a `T*`, - * where `T` is a subclass of `FrontendSlabMetadata`. + * This is used with `SeqSet` and so may actually hold a subclass of this + * class provided by the back end. The `SeqSet` is responsible for + * maintaining that invariant. */ - FrontendSlabMetadata* next{nullptr}; + typename SeqSet::Node node; constexpr FrontendSlabMetadata() = default; @@ -429,6 +442,9 @@ namespace snmalloc */ void initialise(smallsizeclass_t sizeclass) { + static_assert( + std::is_base_of::value, + "Template should be a subclass of FrontendSlabMetadata"); free_queue.init(); // Set up meta data as if the entire slab has been turned into a free // list. This means we don't have to check for special cases where we have @@ -576,7 +592,7 @@ namespace snmalloc * Ensure that the template parameter is valid. */ static_assert( - std::is_convertible_v, + std::is_convertible_v, "The front end requires that the back end provides slab metadata that is " "compatible with the front-end's structure");