From a933e5a711b470cb86c8e7e3f4a8601c9f582149 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Tue, 28 Mar 2023 11:30:33 +0100 Subject: [PATCH 1/3] Move key_global into RemoteAllocator There was a mis-compilation in a Verona configuration that lead to two instances of key_global existing. This change moves it inside a struct that seems to fix the issue. The rest of the changes are limiting the use of key_global as both RemoteCache and RemoteAllocator must use the same configuration, so there is no need to take the key_global as a parameter. --- src/snmalloc/backend/globalconfig.h | 2 +- src/snmalloc/mem/corealloc.h | 14 ++++++------- src/snmalloc/mem/localalloc.h | 4 ++-- src/snmalloc/mem/localcache.h | 2 +- src/snmalloc/mem/remoteallocator.h | 22 +++++++++----------- src/snmalloc/mem/remotecache.h | 19 +++++++++-------- src/test/func/domestication/domestication.cc | 2 +- 7 files changed, 32 insertions(+), 33 deletions(-) diff --git a/src/snmalloc/backend/globalconfig.h b/src/snmalloc/backend/globalconfig.h index 8d106d328..525c77275 100644 --- a/src/snmalloc/backend/globalconfig.h +++ b/src/snmalloc/backend/globalconfig.h @@ -108,7 +108,7 @@ namespace snmalloc LocalEntropy entropy; entropy.init(); // Initialise key for remote deallocation lists - key_global = FreeListKey(entropy.get_free_list_key()); + RemoteAllocator::key_global = FreeListKey(entropy.get_free_list_key()); // Need to randomise pagemap location. If requested and not a // StrictProvenance architecture, randomize its table's location within a diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index 15167f2df..9abd40722 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -462,7 +462,7 @@ namespace snmalloc } }; - return !(message_queue().can_dequeue(key_global, domesticate)); + return !(message_queue().can_dequeue(domesticate)); } /** @@ -502,11 +502,11 @@ namespace snmalloc [](freelist::QueuePtr p) SNMALLOC_FAST_PATH_LAMBDA { return freelist::HeadPtr::unsafe_from(p.unsafe_ptr()); }; - message_queue().dequeue(key_global, domesticate_first, domesticate, cb); + message_queue().dequeue(domesticate_first, domesticate, cb); } else { - message_queue().dequeue(key_global, domesticate, domesticate, cb); + message_queue().dequeue(domesticate, domesticate, cb); } if (need_post) @@ -548,7 +548,7 @@ namespace snmalloc need_post = true; attached_cache->remote_dealloc_cache .template dealloc( - entry.get_remote()->trunc_id(), p.as_void(), key_global); + entry.get_remote()->trunc_id(), p.as_void()); } } @@ -643,7 +643,7 @@ namespace snmalloc bool sent_something = attached_cache->remote_dealloc_cache .post( - backend_state_ptr(), public_state()->trunc_id(), key_global); + backend_state_ptr(), public_state()->trunc_id()); return sent_something; } @@ -838,7 +838,7 @@ namespace snmalloc while (p_tame != nullptr) { bool need_post = true; // Always going to post, so ignore. - auto n_tame = p_tame->atomic_read_next(key_global, domesticate); + auto n_tame = p_tame->atomic_read_next(RemoteAllocator::key_global, domesticate); const PagemapEntry& entry = Config::Backend::get_metaentry(snmalloc::address_cast(p_tame)); handle_dealloc_remote(entry, p_tame.as_void(), need_post); @@ -891,7 +891,7 @@ namespace snmalloc c->remote_allocator = public_state(); // Set up remote cache. - c->remote_dealloc_cache.init(entropy.get_free_list_key()); + c->remote_dealloc_cache.init(); } /** diff --git a/src/snmalloc/mem/localalloc.h b/src/snmalloc/mem/localalloc.h index bd1222ac2..c85d30b2b 100644 --- a/src/snmalloc/mem/localalloc.h +++ b/src/snmalloc/mem/localalloc.h @@ -279,7 +279,7 @@ namespace snmalloc const PagemapEntry& entry = Config::Backend::template get_metaentry(address_cast(p)); local_cache.remote_dealloc_cache.template dealloc( - entry.get_remote()->trunc_id(), p, key_global); + entry.get_remote()->trunc_id(), p); post_remote_cache(); return; } @@ -670,7 +670,7 @@ namespace snmalloc if (local_cache.remote_dealloc_cache.reserve_space(entry)) { local_cache.remote_dealloc_cache.template dealloc( - remote->trunc_id(), p_tame, key_global); + remote->trunc_id(), p_tame); # ifdef SNMALLOC_TRACING message<1024>( "Remote dealloc fast {} ({})", p_raw, alloc_size(p_raw)); diff --git a/src/snmalloc/mem/localcache.h b/src/snmalloc/mem/localcache.h index 68b232e4e..cfbbaa576 100644 --- a/src/snmalloc/mem/localcache.h +++ b/src/snmalloc/mem/localcache.h @@ -86,7 +86,7 @@ namespace snmalloc } return remote_dealloc_cache.post( - local_state, remote_allocator->trunc_id(), key_global); + local_state, remote_allocator->trunc_id()); } template< diff --git a/src/snmalloc/mem/remoteallocator.h b/src/snmalloc/mem/remoteallocator.h index 69fbd4492..32d8e4852 100644 --- a/src/snmalloc/mem/remoteallocator.h +++ b/src/snmalloc/mem/remoteallocator.h @@ -10,11 +10,6 @@ namespace snmalloc { - /** - * Global key for all remote lists. - */ - inline static FreeListKey key_global(0xdeadbeef, 0xbeefdead, 0xdeadbeef); - /** * * A RemoteAllocator is the message queue of freed objects. It exposes a MPSC @@ -44,6 +39,11 @@ namespace snmalloc */ struct alignas(REMOTE_MIN_ALIGN) RemoteAllocator { + /** + * Global key for all remote lists. + */ + inline static FreeListKey key_global{0xdeadbeef, 0xbeefdead, 0xdeadbeef}; + using alloc_id_t = address_t; // Store the message queue on a separate cacheline. It is mutable data that @@ -84,10 +84,10 @@ namespace snmalloc template inline bool - can_dequeue(const FreeListKey& key, Domesticator_head domesticate_head) + can_dequeue(Domesticator_head domesticate_head) { return domesticate_head(front.load()) - ->atomic_read_next(key, domesticate_head) == nullptr; + ->atomic_read_next(key_global, domesticate_head) == nullptr; } /** @@ -101,11 +101,10 @@ namespace snmalloc void enqueue( freelist::HeadPtr first, freelist::HeadPtr last, - const FreeListKey& key, Domesticator_head domesticate_head) { invariant(); - freelist::Object::atomic_store_null(last, key); + freelist::Object::atomic_store_null(last, key_global); // Exchange needs to be acq_rel. // * It needs to be a release, so nullptr in next is visible. @@ -116,7 +115,7 @@ namespace snmalloc if (SNMALLOC_LIKELY(prev != nullptr)) { - freelist::Object::atomic_store_next(domesticate_head(prev), first, key); + freelist::Object::atomic_store_next(domesticate_head(prev), first, key_global); return; } @@ -136,7 +135,6 @@ namespace snmalloc typename Domesticator_queue, typename Cb> void dequeue( - const FreeListKey& key, Domesticator_head domesticate_head, Domesticator_queue domesticate_queue, Cb cb) @@ -150,7 +148,7 @@ namespace snmalloc while (address_cast(curr) != address_cast(b)) { - freelist::HeadPtr next = curr->atomic_read_next(key, domesticate_queue); + freelist::HeadPtr next = curr->atomic_read_next(key_global, domesticate_queue); // We have observed a non-linearisable effect of the queue. // Just go back to allocating normally. if (SNMALLOC_UNLIKELY(next == nullptr)) diff --git a/src/snmalloc/mem/remotecache.h b/src/snmalloc/mem/remotecache.h index 44eef4981..8a982496a 100644 --- a/src/snmalloc/mem/remotecache.h +++ b/src/snmalloc/mem/remotecache.h @@ -68,21 +68,22 @@ namespace snmalloc template SNMALLOC_FAST_PATH void dealloc( RemoteAllocator::alloc_id_t target_id, - capptr::Alloc p, - const FreeListKey& key) + capptr::Alloc p) { SNMALLOC_ASSERT(initialised); auto r = p.template as_reinterpret>(); - list[get_slot(target_id, 0)].add(r, key); + list[get_slot(target_id, 0)].add(r, RemoteAllocator::key_global); } template bool post( typename Config::LocalState* local_state, - RemoteAllocator::alloc_id_t id, - const FreeListKey& key) + RemoteAllocator::alloc_id_t id) { + // Use same key as the remote allocator, so segments can be + // posted to a remote allocator without reencoding. + const auto& key = RemoteAllocator::key_global; SNMALLOC_ASSERT(initialised); size_t post_round = 0; bool sent_something = false; @@ -118,11 +119,11 @@ namespace snmalloc auto domesticate_nop = [](freelist::QueuePtr p) { return freelist::HeadPtr::unsafe_from(p.unsafe_ptr()); }; - remote->enqueue(first, last, key, domesticate_nop); + remote->enqueue(first, last, domesticate_nop); } else { - remote->enqueue(first, last, key, domesticate); + remote->enqueue(first, last, domesticate); } sent_something = true; } @@ -166,7 +167,7 @@ namespace snmalloc * Must be called before anything else to ensure actually initialised * not just zero init. */ - void init(const FreeListKey& key) + void init() { #ifndef NDEBUG initialised = true; @@ -175,7 +176,7 @@ namespace snmalloc { // We do not need to initialise with a particular slab, so pass // a null address. - l.init(0, key); + l.init(0, RemoteAllocator::key_global); } capacity = REMOTE_CACHE; } diff --git a/src/test/func/domestication/domestication.cc b/src/test/func/domestication/domestication.cc index b8697e731..03cc9ba3b 100644 --- a/src/test/func/domestication/domestication.cc +++ b/src/test/func/domestication/domestication.cc @@ -138,7 +138,7 @@ int main() LocalEntropy entropy; entropy.init(); - key_global = FreeListKey(entropy.get_free_list_key()); + RemoteAllocator::key_global = FreeListKey(entropy.get_free_list_key()); auto alloc1 = new Alloc(); From 1f091a15f9202501eab62a3623fd433f078c8be1 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Fri, 31 Mar 2023 08:24:06 +0100 Subject: [PATCH 2/3] Clangformat --- src/snmalloc/mem/corealloc.h | 3 ++- src/snmalloc/mem/remoteallocator.h | 9 +++++---- src/snmalloc/mem/remotecache.h | 11 +++++------ 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/snmalloc/mem/corealloc.h b/src/snmalloc/mem/corealloc.h index 9abd40722..c7fc79b72 100644 --- a/src/snmalloc/mem/corealloc.h +++ b/src/snmalloc/mem/corealloc.h @@ -838,7 +838,8 @@ namespace snmalloc while (p_tame != nullptr) { bool need_post = true; // Always going to post, so ignore. - auto n_tame = p_tame->atomic_read_next(RemoteAllocator::key_global, domesticate); + auto n_tame = + p_tame->atomic_read_next(RemoteAllocator::key_global, domesticate); const PagemapEntry& entry = Config::Backend::get_metaentry(snmalloc::address_cast(p_tame)); handle_dealloc_remote(entry, p_tame.as_void(), need_post); diff --git a/src/snmalloc/mem/remoteallocator.h b/src/snmalloc/mem/remoteallocator.h index 32d8e4852..b6768d6c9 100644 --- a/src/snmalloc/mem/remoteallocator.h +++ b/src/snmalloc/mem/remoteallocator.h @@ -83,8 +83,7 @@ namespace snmalloc } template - inline bool - can_dequeue(Domesticator_head domesticate_head) + inline bool can_dequeue(Domesticator_head domesticate_head) { return domesticate_head(front.load()) ->atomic_read_next(key_global, domesticate_head) == nullptr; @@ -115,7 +114,8 @@ namespace snmalloc if (SNMALLOC_LIKELY(prev != nullptr)) { - freelist::Object::atomic_store_next(domesticate_head(prev), first, key_global); + freelist::Object::atomic_store_next( + domesticate_head(prev), first, key_global); return; } @@ -148,7 +148,8 @@ namespace snmalloc while (address_cast(curr) != address_cast(b)) { - freelist::HeadPtr next = curr->atomic_read_next(key_global, domesticate_queue); + freelist::HeadPtr next = + curr->atomic_read_next(key_global, domesticate_queue); // We have observed a non-linearisable effect of the queue. // Just go back to allocating normally. if (SNMALLOC_UNLIKELY(next == nullptr)) diff --git a/src/snmalloc/mem/remotecache.h b/src/snmalloc/mem/remotecache.h index 8a982496a..96f5e0973 100644 --- a/src/snmalloc/mem/remotecache.h +++ b/src/snmalloc/mem/remotecache.h @@ -66,20 +66,19 @@ namespace snmalloc } template - SNMALLOC_FAST_PATH void dealloc( - RemoteAllocator::alloc_id_t target_id, - capptr::Alloc p) + SNMALLOC_FAST_PATH void + dealloc(RemoteAllocator::alloc_id_t target_id, capptr::Alloc p) { SNMALLOC_ASSERT(initialised); auto r = p.template as_reinterpret>(); - list[get_slot(target_id, 0)].add(r, RemoteAllocator::key_global); + list[get_slot(target_id, 0)].add( + r, RemoteAllocator::key_global); } template bool post( - typename Config::LocalState* local_state, - RemoteAllocator::alloc_id_t id) + typename Config::LocalState* local_state, RemoteAllocator::alloc_id_t id) { // Use same key as the remote allocator, so segments can be // posted to a remote allocator without reencoding. From 711c15e7ce391bb4ac778181052dc05ed52b9427 Mon Sep 17 00:00:00 2001 From: Matthew Parkinson Date: Wed, 26 Apr 2023 15:30:17 +0100 Subject: [PATCH 3/3] CR feedback. --- src/snmalloc/mem/remoteallocator.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/snmalloc/mem/remoteallocator.h b/src/snmalloc/mem/remoteallocator.h index b6768d6c9..5c95e1cab 100644 --- a/src/snmalloc/mem/remoteallocator.h +++ b/src/snmalloc/mem/remoteallocator.h @@ -41,6 +41,11 @@ namespace snmalloc { /** * Global key for all remote lists. + * + * Note that we use a single key for all remote free lists and queues. + * This is so that we do not have to recode next pointers when sending + * segments, and look up specific keys based on destination. This is + * potentially more performant, but could make it easier to guess the key. */ inline static FreeListKey key_global{0xdeadbeef, 0xbeefdead, 0xdeadbeef};