Move key_global into RemoteAllocator (#608)

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.
microsoft · Apr 26, 2023 · 55376aa · 55376aa
1 parent 7b3a2b3
commit 55376aa
Show file tree

Hide file tree

Showing 7 changed files with 42 additions and 37 deletions.
diff --git a/src/snmalloc/backend/globalconfig.h b/src/snmalloc/backend/globalconfig.h
@@ -108,7 +108,7 @@ namespace snmalloc
       LocalEntropy entropy;
       entropy.init<Pal>();
       // 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
@@ -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<sizeof(CoreAllocator)>(
-            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<sizeof(CoreAllocator), Config>(
-            backend_state_ptr(), public_state()->trunc_id(), key_global);
+            backend_state_ptr(), public_state()->trunc_id());
 
       return sent_something;
     }
@@ -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(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 +892,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
@@ -279,7 +279,7 @@ namespace snmalloc
         const PagemapEntry& entry =
           Config::Backend::template get_metaentry(address_cast(p));
         local_cache.remote_dealloc_cache.template dealloc<sizeof(CoreAlloc)>(
-          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<sizeof(CoreAlloc)>(
-            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
@@ -86,7 +86,7 @@ namespace snmalloc
       }
 
       return remote_dealloc_cache.post<allocator_size, Config>(
-        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
@@ -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,16 @@ namespace snmalloc
    */
   struct alignas(REMOTE_MIN_ALIGN) RemoteAllocator
   {
+    /**
+     * 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};
+
     using alloc_id_t = address_t;
 
     // Store the message queue on a separate cacheline. It is mutable data that
@@ -83,11 +88,10 @@ namespace snmalloc
     }
 
     template<typename Domesticator_head>
-    inline bool
-    can_dequeue(const FreeListKey& key, Domesticator_head domesticate_head)
+    inline bool 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 +105,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 +119,8 @@ 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 +140,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 +153,8 @@ 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
@@ -66,23 +66,23 @@ namespace snmalloc
     }
 
     template<size_t allocator_size>
-    SNMALLOC_FAST_PATH void dealloc(
-      RemoteAllocator::alloc_id_t target_id,
-      capptr::Alloc<void> p,
-      const FreeListKey& key)
+    SNMALLOC_FAST_PATH void
+    dealloc(RemoteAllocator::alloc_id_t target_id, capptr::Alloc<void> p)
     {
       SNMALLOC_ASSERT(initialised);
       auto r = p.template as_reinterpret<freelist::Object::T<>>();
 
-      list[get_slot<allocator_size>(target_id, 0)].add(r, key);
+      list[get_slot<allocator_size>(target_id, 0)].add(
+        r, RemoteAllocator::key_global);
     }
 
     template<size_t allocator_size, typename Config>
     bool post(
-      typename Config::LocalState* local_state,
-      RemoteAllocator::alloc_id_t id,
-      const FreeListKey& key)
+      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.
+      const auto& key = RemoteAllocator::key_global;
       SNMALLOC_ASSERT(initialised);
       size_t post_round = 0;
       bool sent_something = false;
@@ -118,11 +118,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 +166,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 +175,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
@@ -138,7 +138,7 @@ int main()
 
   LocalEntropy entropy;
   entropy.init<DefaultPal>();
-  key_global = FreeListKey(entropy.get_free_list_key());
+  RemoteAllocator::key_global = FreeListKey(entropy.get_free_list_key());
 
   auto alloc1 = new Alloc();