Merge pull request #75 from microsoft/pal_refactor

[NFC] Remove some code duplication in the PALs.

azure-pipelines.yml

@@ -112,7 +112,7 @@ jobs:
       64-bit Release:
         BuildType: Release
         CMakeArgs: '-G"Visual Studio 15 2017 Win64"'
-        JFlag: '-j 4'
+        JFlag: '-j 2'
 
       64-bit Release Windows8Compat:
         BuildType: Release
@@ -127,7 +127,7 @@ jobs:
       32-bit Release:
         BuildType: Release
         CMakeArgs: '-G"Visual Studio 15 2017"'
-        JFlag: '-j 4'
+        JFlag: '-j 2'
 
   steps:
   - task: CMake@1

src/pal/pal.h

@@ -10,9 +10,10 @@ namespace snmalloc
 // If simultating OE, then we need the underlying platform
 #if !defined(OPEN_ENCLAVE) || defined(OPEN_ENCLAVE_SIMULATION)
 #  include "pal_apple.h"
-#  include "pal_free_bsd_kernel.h"
 #  include "pal_freebsd.h"
+#  include "pal_freebsd_kernel.h"
 #  include "pal_linux.h"
+#  include "pal_netbsd.h"
 #  include "pal_openbsd.h"
 #  include "pal_windows.h"
 #endif
@@ -32,9 +33,11 @@ namespace snmalloc
 #  elif defined(FreeBSD_KERNEL)
     PALFreeBSDKernel;
 #  elif defined(__FreeBSD__)
-    PALFBSD;
+    PALFreeBSD;
+#  elif defined(__NetBSD__)
+    PALNetBSD;
 #  elif defined(__OpenBSD__)
-    PALOBSD;
+    PALOpenBSD;
 #  else
 #    error Unsupported platform
 #  endif

src/pal/pal_apple.h

@@ -1,89 +1,27 @@
 #pragma once
 
 #ifdef __APPLE__
-#  include "../ds/bits.h"
+#  include "pal_bsd.h"
-#  include "../mem/allocconfig.h"
-
-#  include <pthread.h>
-#  include <strings.h>
-#  include <sys/mman.h>
-
-extern "C" int puts(const char* str);
 
 namespace snmalloc
 {
   /**
    * PAL implementation for Apple systems (macOS, iOS, watchOS, tvOS...).
+   *
+   * XNU behaves exactly like a generic BSD platform but this class exists
+   * as a place to add XNU-specific behaviour later, if required.
    */
-  class PALApple
+  class PALApple : public PALBSD<PALApple>
   {
   public:
     /**
-     * Bitmap of PalFeatures flags indicating the optional features that this
+     * The features exported by this PAL.
-     * PAL supports.
+     *
+     * Currently, these are identical to the generic BSD PAL.  This field is
+     * declared explicitly to remind anyone who modifies this class that they
+     * should add any required features.
      */
-    static constexpr uint64_t pal_features = LazyCommit;
+    static constexpr uint64_t pal_features = PALBSD::pal_features;
-    static void error(const char* const str)
-    {
-      puts(str);
-      abort();
-    }
-
-    /// Notify platform that we will not be using these pages
-    void notify_not_using(void* p, size_t size) noexcept
-    {
-      assert(bits::is_aligned_block<OS_PAGE_SIZE>(p, size));
-      madvise(p, size, MADV_FREE);
-    }
-
-    /// Notify platform that we will be using these pages
-    template<ZeroMem zero_mem>
-    void notify_using(void* p, size_t size) noexcept
-    {
-      assert(
-        bits::is_aligned_block<OS_PAGE_SIZE>(p, size) || (zero_mem == NoZero));
-      if constexpr (zero_mem == YesZero)
-        zero(p, size);
-    }
-
-    /// OS specific function for zeroing memory
-    template<bool page_aligned = false>
-    void zero(void* p, size_t size) noexcept
-    {
-      if (page_aligned || bits::is_aligned_block<OS_PAGE_SIZE>(p, size))
-      {
-        assert(bits::is_aligned_block<OS_PAGE_SIZE>(p, size));
-        void* r = mmap(
-          p,
-          size,
-          PROT_READ | PROT_WRITE,
-          MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED,
-          -1,
-          0);
-
-        if (r != MAP_FAILED)
-          return;
-      }
-
-      bzero(p, size);
-    }
-
-    template<bool committed>
-    void* reserve(size_t* size) noexcept
-    {
-      void* p = mmap(
-        NULL,
-        *size,
-        PROT_READ | PROT_WRITE,
-        MAP_PRIVATE | MAP_ANONYMOUS,
-        -1,
-        0);
-
-      if (p == MAP_FAILED)
-        error("Out of memory");
-
-      return p;
-    }
   };
-}
+} // namespace snmalloc
 #endif

src/pal/pal_bsd.h

@@ -1,93 +1,40 @@
 #pragma once
 
-#if (defined(__FreeBSD__) || defined(__OpenBSD__)) && !defined(_KERNEL)
+#include "pal_posix.h"
-#  include "../ds/bits.h"
-#  include "../mem/allocconfig.h"
 
-#  include <stdio.h>
+#include <sys/mman.h>
-#  include <strings.h>
-#  include <sys/mman.h>
 
 namespace snmalloc
 {
-  class PALBSD
+  /**
+   * Generic *BSD PAL mixin.  This provides features that are common to the BSD
+   * family.
+   */
+  template<typename OS>
+  class PALBSD : public PALPOSIX<OS>
   {
   public:
     /**
      * Bitmap of PalFeatures flags indicating the optional features that this
      * PAL supports.
+     *
+     * The generic BSD PAL does not add any features that are not supported by
+     * generic POSIX systems, but explicitly declares this variable to remind
+     * anyone who extends this class that they may need to modify this field.
      */
-    static constexpr uint64_t pal_features = 0;
+    static constexpr uint64_t pal_features = PALPOSIX<OS>::pal_features;
-    static void error(const char* const str)
-    {
-      puts(str);
-      abort();
-    }
 
-    /// Notify platform that we will not be using these pages
+    /**
+     * Notify platform that we will not be using these pages.
+     *
+     * BSD systems provide the `MADV_FREE` flag to `madvise`, which allows the
+     * operating system to replace the pages with CoW copies of a zero page at
+     * any point between the call and the next write to that page.
+     */
     void notify_not_using(void* p, size_t size) noexcept
     {
       assert(bits::is_aligned_block<OS_PAGE_SIZE>(p, size));
       madvise(p, size, MADV_FREE);
     }
-
-    /// Notify platform that we will be using these pages
-    template<ZeroMem zero_mem>
-    void notify_using(void* p, size_t size) noexcept
-    {
-      assert(
-        bits::is_aligned_block<OS_PAGE_SIZE>(p, size) || (zero_mem == NoZero));
-      if constexpr (zero_mem == YesZero)
-      {
-        zero(p, size);
-      }
-      else
-      {
-        UNUSED(size);
-        UNUSED(p);
-      }
-    }
-
-    /// OS specific function for zeroing memory
-    template<bool page_aligned = false>
-    void zero(void* p, size_t size) noexcept
-    {
-      if (page_aligned || bits::is_aligned_block<OS_PAGE_SIZE>(p, size))
-      {
-        assert(bits::is_aligned_block<OS_PAGE_SIZE>(p, size));
-        void* r = mmap(
-          p,
-          size,
-          PROT_READ | PROT_WRITE,
-          MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED,
-          -1,
-          0);
-
-        if (r != MAP_FAILED)
-          return;
-      }
-
-      bzero(p, size);
-    }
-
-    template<bool committed>
-    void* reserve(const size_t* size) noexcept
-    {
-      size_t request = *size;
-
-      void* p = mmap(
-        nullptr,
-        request,
-        PROT_READ | PROT_WRITE,
-        MAP_PRIVATE | MAP_ANONYMOUS,
-        -1,
-        0);
-
-      if (p == MAP_FAILED)
-        error("Out of memory");
-
-      return p;
-    }
   };
 } // namespace snmalloc
-#endif

src/pal/pal_bsd_aligned.h

@@ -0,0 +1,55 @@
+#pragma once
+
+#if defined(__FreeBSD__) && !defined(_KERNEL)
+#  include "pal_bsd.h"
+
+namespace snmalloc
+{
+  /**
+   * FreeBSD-specific platform abstraction layer.
+   *
+   * This adds aligned allocation using `MAP_ALIGNED` to the generic BSD
+   * implementation.  This flag is supported by NetBSD and FreeBSD.
+   */
+  template<class OS>
+  class PALBSD_Aligned : public PALBSD<OS>
+  {
+  public:
+    /**
+     * Bitmap of PalFeatures flags indicating the optional features that this
+     * PAL supports.
+     *
+     * This class adds support for aligned allocation.
+     */
+    static constexpr uint64_t pal_features =
+      AlignedAllocation | PALBSD<OS>::pal_features;
+
+    /**
+     * Reserve memory at a specific alignment.
+     */
+    template<bool committed>
+    void* reserve(const size_t* size, size_t align) noexcept
+    {
+      // Alignment must be a power of 2.
+      assert(align == bits::next_pow2(align));
+
+      align = bits::max<size_t>(4096, align);
+
+      size_t log2align = bits::next_pow2_bits(align);
+
+      void* p = mmap(
+        nullptr,
+        *size,
+        PROT_READ | PROT_WRITE,
+        MAP_PRIVATE | MAP_ANONYMOUS | MAP_ALIGNED(log2align),
+        -1,
+        0);
+
+      if (p == MAP_FAILED)
+        error("Out of memory");
+
+      return p;
+    }
+  };
+} // namespace snmalloc
+#endif

src/pal/pal_freebsd.h

@@ -1,49 +1,29 @@
 #pragma once
 
 #if defined(__FreeBSD__) && !defined(_KERNEL)
-#  include "../ds/bits.h"
+#  include "pal_bsd_aligned.h"
-#  include "../mem/allocconfig.h"
-#  include "pal_bsd.h"
-
-#  include <stdio.h>
-#  include <strings.h>
-#  include <sys/mman.h>
 
 namespace snmalloc
 {
-  class PALFBSD : public PALBSD
+  /**
+   * FreeBSD-specific platform abstraction layer.
+   *
+   * This adds FreeBSD-specific aligned allocation to the generic BSD
+   * implementation.
+   */
+  class PALFreeBSD : public PALBSD_Aligned<PALFreeBSD>
   {
   public:
     /**
      * Bitmap of PalFeatures flags indicating the optional features that this
      * PAL supports.
+     *
+     * The FreeBSD PAL does not currently add any features beyond those of a
+     * generic BSD with support for arbitrary alignment from `mmap`.  This
+     * field is declared explicitly to remind anyone modifying this class to
+     * add new features that they should add any required feature flags.
      */
-    static constexpr uint64_t pal_features = AlignedAllocation | LazyCommit;
+    static constexpr uint64_t pal_features = PALBSD_Aligned::pal_features;
-
-    template<bool committed>
-    void* reserve(const size_t* size, size_t align) noexcept
-    {
-      size_t request = *size;
-      // Alignment must be a power of 2.
-      assert(align == bits::next_pow2(align));
-
-      align = bits::max<size_t>(4096, align);
-
-      size_t log2align = bits::next_pow2_bits(align);
-
-      void* p = mmap(
-        nullptr,
-        request,
-        PROT_READ | PROT_WRITE,
-        MAP_PRIVATE | MAP_ANONYMOUS | MAP_ALIGNED(log2align),
-        -1,
-        0);
-
-      if (p == MAP_FAILED)
-        error("Out of memory");
-
-      return p;
-    }
   };
 } // namespace snmalloc
 #endif

src/pal/pal_linux.h

@@ -3,6 +3,7 @@
 #if defined(__linux__)
 #  include "../ds/bits.h"
 #  include "../mem/allocconfig.h"
+#  include "pal_posix.h"
 
 #  include <string.h>
 #  include <sys/mman.h>
@@ -11,47 +12,28 @@ extern "C" int puts(const char* str);
 
 namespace snmalloc
 {
-  class PALLinux
+  class PALLinux : public PALPOSIX<PALLinux>
   {
   public:
     /**
      * Bitmap of PalFeatures flags indicating the optional features that this
      * PAL supports.
+     *
+     * Linux does not support any features other than those in a generic POSIX
+     * platform.  This field is declared explicitly to remind anyone who
+     * extends this PAL that they may need to extend the set of advertised
+     * features.
      */
-    static constexpr uint64_t pal_features = LazyCommit;
+    static constexpr uint64_t pal_features = PALPOSIX::pal_features;
-    static void error(const char* const str)
-    {
-      puts(str);
-      abort();
-    }
 
-    /// Notify platform that we will not be using these pages
+    /**
-    void notify_not_using(void* p, size_t size) noexcept
+     * OS specific function for zeroing memory.
-    {
+     *
-      assert(bits::is_aligned_block<OS_PAGE_SIZE>(p, size));
+     * Linux implements an unusual interpretation of `MADV_DONTNEED`, which
-      // Do nothing. Don't call madvise here, as the system call slows the
+     * immediately resets the pages to the zero state (rather than marking them
-      // allocator down too much.
+     * as sensible ones to swap out in high memory pressure).  We use this to
-      UNUSED(p);
+     * clear the underlying memory range.
-      UNUSED(size);
+     */
-    }
-
-    /// Notify platform that we will be using these pages
-    template<ZeroMem zero_mem>
-    void notify_using(void* p, size_t size) noexcept
-    {
-      assert(
-        bits::is_aligned_block<OS_PAGE_SIZE>(p, size) || (zero_mem == NoZero));
-
-      if constexpr (zero_mem == YesZero)
-        zero<true>(p, size);
-      else
-      {
-        UNUSED(p);
-        UNUSED(size);
-      }
-    }
-
-    /// OS specific function for zeroing memory
     template<bool page_aligned = false>
     void zero(void* p, size_t size) noexcept
     {
@@ -65,23 +47,6 @@ namespace snmalloc
         ::memset(p, 0, size);
       }
     }
-
-    template<bool committed>
-    void* reserve(const size_t* size) noexcept
-    {
-      void* p = mmap(
-        nullptr,
-        *size,
-        PROT_READ | PROT_WRITE,
-        MAP_PRIVATE | MAP_ANONYMOUS,
-        -1,
-        0);
-
-      if (p == MAP_FAILED)
-        error("Out of memory");
-
-      return p;
-    }
   };
 } // namespace snmalloc
 #endif

src/pal/pal_netbsd.h

@@ -0,0 +1,29 @@
+#pragma once
+
+#ifdef __NetBSD__
+#  include "pal_bsd_aligned.h"
+
+namespace snmalloc
+{
+  /**
+   * NetBSD-specific platform abstraction layer.
+   *
+   * This adds NetBSD-specific aligned allocation to the generic BSD
+   * implementation.
+   */
+  class PALNetBSD : public PALBSD_Aligned<PALFBSD>
+  {
+  public:
+    /**
+     * Bitmap of PalFeatures flags indicating the optional features that this
+     * PAL supports.
+     *
+     * The NetBSD PAL does not currently add any features beyond those of a
+     * generic BSD with support for arbitrary alignment from `mmap`.  This
+     * field is declared explicitly to remind anyone modifying this class to
+     * add new features that they should add any required feature flags.
+     */
+    static constexpr uint64_t pal_features = PALBSD_Aligned::pal_features;
+  };
+} // namespace snmalloc
+#endif

src/pal/pal_openbsd.h

@@ -1,20 +1,27 @@
 #pragma once
 
 #if defined(__OpenBSD__) && !defined(_KERNEL)
-#  include "../ds/bits.h"
-#  include "../mem/allocconfig.h"
 #  include "pal_bsd.h"
 
-#  include <stdio.h>
-#  include <string.h>
-#  include <sys/mman.h>
-
 namespace snmalloc
 {
-  class PALOBSD : public PALBSD
+  /**
+   * OpenBSD platform abstraction layer.
+   *
+   * OpenBSD behaves exactly like a generic BSD platform but this class exists
+   * as a place to add OpenBSD-specific behaviour later, if required.
+   */
+  class PALOpenBSD : public PALBSD<PALOpenBSD>
   {
   public:
-    static constexpr uint64_t pal_features = LazyCommit;
+    /**
+     * The features exported by this PAL.
+     *
+     * Currently, these are identical to the generic BSD PAL.  This field is
+     * declared explicitly to remind anyone who modifies this class that they
+     * should add any required features.
+     */
+    static constexpr uint64_t pal_features = PALBSD::pal_features;
   };
 } // namespace snmalloc
 #endif

src/pal/pal_posix.h

@@ -0,0 +1,141 @@
+#pragma once
+
+#include "../ds/bits.h"
+#include "../mem/allocconfig.h"
+
+#include <string.h>
+#include <sys/mman.h>
+
+extern "C" int puts(const char* str);
+
+namespace snmalloc
+{
+  /**
+   * Platform abstraction layer for generic POSIX systems.
+   *
+   * This provides the lowest common denominator for POSIX systems.  It should
+   * work on pretty much any POSIX system, but won't necessarily be the most
+   * efficient implementation.  Subclasses should provide more efficient
+   * implementations using platform-specific functionality.
+   *
+   * The template parameter for this is the subclass and is used for explicit
+   * up casts to allow this class to call non-virtual methods on the templated
+   * version.
+   */
+  template<class OS>
+  class PALPOSIX
+  {
+  public:
+    /**
+     * Bitmap of PalFeatures flags indicating the optional features that this
+     * PAL supports.
+     *
+     * POSIX systems are assumed to support lazy commit.
+     */
+    static constexpr uint64_t pal_features = LazyCommit;
+
+    /**
+     * Report a fatal error an exit.
+     */
+    static void error(const char* const str) noexcept
+    {
+      puts(str);
+      abort();
+    }
+
+    /**
+     * Notify platform that we will not be using these pages.
+     *
+     * This does nothing in a generic POSIX implementation.  Most POSIX systems
+     * provide an `madvise` call that can be used to return pages to the OS in
+     * high memory pressure conditions, though on Linux this seems to impose
+     * too much of a performance penalty.
+     */
+    void notify_not_using(void* p, size_t size) noexcept
+    {
+      assert(bits::is_aligned_block<OS_PAGE_SIZE>(p, size));
+      UNUSED(p);
+      UNUSED(size);
+    }
+
+    /**
+     * Notify platform that we will be using these pages.
+     *
+     * On POSIX platforms, lazy commit means that this is a no-op, unless we
+     * are also zeroing the pages in which case we call the platform's `zero`
+     * function.
+     */
+    template<ZeroMem zero_mem>
+    void notify_using(void* p, size_t size) noexcept
+    {
+      assert(
+        bits::is_aligned_block<OS_PAGE_SIZE>(p, size) || (zero_mem == NoZero));
+
+      if constexpr (zero_mem == YesZero)
+        static_cast<OS*>(this)->template zero<true>(p, size);
+      else
+      {
+        UNUSED(p);
+        UNUSED(size);
+      }
+    }
+
+    /**
+     * OS specific function for zeroing memory.
+     *
+     * The generic POSIX implementation uses mmap to map anonymous memory over
+     * the range for ranges larger than a page.  The underlying OS is assumed
+     * to provide new CoW copies of the zero page.
+     *
+     * Note: On most systems it is faster for a single page to zero the memory
+     * explicitly than do this, we should probably tweak the threshold for
+     * calling bzero at some point.
+     */
+    template<bool page_aligned = false>
+    void zero(void* p, size_t size) noexcept
+    {
+      if (page_aligned || bits::is_aligned_block<OS_PAGE_SIZE>(p, size))
+      {
+        assert(bits::is_aligned_block<OS_PAGE_SIZE>(p, size));
+        void* r = mmap(
+          p,
+          size,
+          PROT_READ | PROT_WRITE,
+          MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED,
+          -1,
+          0);
+
+        if (r != MAP_FAILED)
+          return;
+      }
+
+      bzero(p, size);
+    }
+
+    /**
+     * Reserve memory.
+     *
+     * POSIX platforms support lazy commit, and so this also puts the memory in
+     * the lazy commit state (i.e. pages will be allocated on first use).
+     *
+     * POSIX does not define a portable interface for specifying alignment
+     * greater than a page.
+     */
+    template<bool committed>
+    void* reserve(const size_t* size) noexcept
+    {
+      void* p = mmap(
+        nullptr,
+        *size,
+        PROT_READ | PROT_WRITE,
+        MAP_PRIVATE | MAP_ANONYMOUS,
+        -1,
+        0);
+
+      if (p == MAP_FAILED)
+        OS::error("Out of memory");
+
+      return p;
+    }
+  };
+} // namespace snmalloc