From 23b4230f6af3d548e33e486eb0e3e74523cee5fd Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 14 Nov 2019 12:50:53 +0000 Subject: [PATCH 1/6] Fixed leak in free list. If the first call to alloc uses the minimum size class, then we end up leaking a whole page of allocations. We fill the small_fast_free_list, in a call to build the message_queue. But this means the small_fast_free_list[0] is not empty. But the code was staying on the slow path, and overwriting it. --- src/mem/alloc.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/mem/alloc.h b/src/mem/alloc.h index 09ebf96..4ed76a6 100644 --- a/src/mem/alloc.h +++ b/src/mem/alloc.h @@ -1084,7 +1084,12 @@ namespace snmalloc SNMALLOC_ASSUME(size <= SLAB_SIZE); sizeclass_t sizeclass = size_to_sizeclass(size); + return small_alloc_inner(sizeclass); + } + template + SNMALLOC_FAST_PATH void* small_alloc_inner(sizeclass_t sizeclass) + { assert(sizeclass < NUM_SMALL_CLASSES); auto& fl = small_fast_free_lists[sizeclass]; void* head = fl.value; @@ -1097,7 +1102,7 @@ namespace snmalloc void* p = remove_cache_friendly_offset(head, sizeclass); if constexpr (zero_mem == YesZero) { - large_allocator.memory_provider.zero(p, size); + large_allocator.memory_provider.zero(p, sizeclass_to_size(sizeclass)); } return p; } From c2799f6ec8ed43b87d520d58ff7c52e57a28a2fb Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 14 Nov 2019 12:55:23 +0000 Subject: [PATCH 2/6] Make debug_check_empty not use statistics Debug_check_empty now empties the free lists, and checks it empties all the queues in the allocator. This does not require statistic tracking to work anymore. This additionally can check internal regression that cause leaks that are not the clients fault. --- src/mem/alloc.h | 56 ++++++++++++++++++++++++++++++++++++++++++- src/mem/globalalloc.h | 31 +++++++++--------------- 2 files changed, 66 insertions(+), 21 deletions(-) diff --git a/src/mem/alloc.h b/src/mem/alloc.h index 4ed76a6..cbb1410 100644 --- a/src/mem/alloc.h +++ b/src/mem/alloc.h @@ -875,6 +875,60 @@ namespace snmalloc #endif } + /** + * If result parameter is non-null, then is_empty is passsed into the + * the location pointed to by result. + * + * Otherwise, asserts that it is empty. + **/ + void debug_is_empty(bool* result) + { + auto test = [&result](bool value) { + if (!value) + { + if (result != nullptr) + *result = false; + else + error("debug_is_empty: found non-empty allocator"); + } + }; + + // Destroy the message queue so that it has no stub message. + Remote* p = message_queue().destroy(); + + while (p != nullptr) + { + Remote* n = p->non_atomic_next; + handle_dealloc_remote(p); + p = n; + } + + for (size_t i = 0; i < NUM_SMALL_CLASSES; i++) + { + auto prev = small_fast_free_lists[i].value; + small_fast_free_lists[i].value = nullptr; + while (prev != nullptr) + { + auto n = Metaslab::follow_next(prev); + dealloc(prev); + prev = n; + } + + test(small_classes[i].is_empty()); + } + + for (size_t i = 0; i < NUM_MEDIUM_CLASSES; i++) + { + test(medium_classes[i].is_empty()); + } + + test(super_available.is_empty()); + test(super_only_short_available.is_empty()); + + // Place the static stub message on the queue. + init_message_queue(); + } + template static uintptr_t external_pointer(void* p, sizeclass_t sizeclass, size_t end_point) @@ -1116,7 +1170,7 @@ namespace snmalloc if (void* replacement = Replacement(this)) { return reinterpret_cast(replacement) - ->template small_alloc_slow(sizeclass); + ->template small_alloc_inner(sizeclass); } stats().sizeclass_alloc(sizeclass); diff --git a/src/mem/globalalloc.h b/src/mem/globalalloc.h index 9db7d35..8cb05c2 100644 --- a/src/mem/globalalloc.h +++ b/src/mem/globalalloc.h @@ -120,32 +120,18 @@ namespace snmalloc auto* alloc = Parent::iterate(); bool done = false; + bool okay = true; - size_t non_empty_count = 0; while (!done) { done = true; alloc = Parent::iterate(); - non_empty_count = 0; + okay = true; while (alloc != nullptr) { - // Destroy the message queue so that it has no stub message. - Remote* p = alloc->message_queue().destroy(); - - while (p != nullptr) - { - Remote* next = p->non_atomic_next; - alloc->handle_dealloc_remote(p); - p = next; - } - // Check that the allocator has freed all memory. - if (!alloc->stats().is_empty()) - non_empty_count++; - - // Place the static stub message on the queue. - alloc->init_message_queue(); + alloc->debug_is_empty(&okay); // Post all remotes, including forwarded ones. If any allocator posts, // repeat the loop. @@ -162,13 +148,18 @@ namespace snmalloc if (result != nullptr) { - *result = non_empty_count == 0; + *result = okay; return; } - if (non_empty_count != 0) + if (!okay) { - error("debug_check_empty: found non-empty allocators"); + alloc = Parent::iterate(); + while (alloc != nullptr) + { + alloc->debug_is_empty(nullptr); + alloc = Parent::iterate(alloc); + } } #else UNUSED(result); From 14383614e0b86b84cf8ed0819ee368e70ac54fd2 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 14 Nov 2019 13:04:24 +0000 Subject: [PATCH 3/6] Clang tidy --- src/mem/alloc.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mem/alloc.h b/src/mem/alloc.h index cbb1410..acedaa8 100644 --- a/src/mem/alloc.h +++ b/src/mem/alloc.h @@ -917,9 +917,9 @@ namespace snmalloc test(small_classes[i].is_empty()); } - for (size_t i = 0; i < NUM_MEDIUM_CLASSES; i++) + for (auto & medium_class : medium_classes) { - test(medium_classes[i].is_empty()); + test(medium_class.is_empty()); } test(super_available.is_empty()); From 74cc475787cdcac35186771a3acbe02bec56a06c Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 14 Nov 2019 13:34:57 +0000 Subject: [PATCH 4/6] Code review feedback. --- src/mem/alloc.h | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/mem/alloc.h b/src/mem/alloc.h index acedaa8..53d49cf 100644 --- a/src/mem/alloc.h +++ b/src/mem/alloc.h @@ -876,15 +876,16 @@ namespace snmalloc } /** - * If result parameter is non-null, then is_empty is passsed into the - * the location pointed to by result. + * If result parameter is non-null, then false is assigned into the + * the location pointed to by result if this allocator is non-empty. * - * Otherwise, asserts that it is empty. + * If result pointer is null, then this code raises a Pal::error on the + * particular check that fails, if any do fail. **/ void debug_is_empty(bool* result) { - auto test = [&result](bool value) { - if (!value) + auto test = [&result](auto& queue) { + if (!queue.is_empty()) { if (result != nullptr) *result = false; @@ -914,16 +915,16 @@ namespace snmalloc prev = n; } - test(small_classes[i].is_empty()); + test(small_classes[i]); } for (auto & medium_class : medium_classes) { - test(medium_class.is_empty()); + test(medium_class); } - test(super_available.is_empty()); - test(super_only_short_available.is_empty()); + test(super_available); + test(super_only_short_available); // Place the static stub message on the queue. init_message_queue(); From 4046417f25ee29e3da323b302193085c7fcde733 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 14 Nov 2019 14:02:50 +0000 Subject: [PATCH 5/6] Fix cache friendly offset. --- src/mem/alloc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mem/alloc.h b/src/mem/alloc.h index 53d49cf..45dbd70 100644 --- a/src/mem/alloc.h +++ b/src/mem/alloc.h @@ -911,7 +911,7 @@ namespace snmalloc while (prev != nullptr) { auto n = Metaslab::follow_next(prev); - dealloc(prev); + dealloc(remove_cache_friendly_offset(prev, i)); prev = n; } From 7a3f0eb50b103012fd7983d6e90a96f46d548b26 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Thu, 14 Nov 2019 14:23:56 +0000 Subject: [PATCH 6/6] Clang format --- src/mem/alloc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mem/alloc.h b/src/mem/alloc.h index 45dbd70..6d91ed9 100644 --- a/src/mem/alloc.h +++ b/src/mem/alloc.h @@ -918,7 +918,7 @@ namespace snmalloc test(small_classes[i]); } - for (auto & medium_class : medium_classes) + for (auto& medium_class : medium_classes) { test(medium_class); }